SwiftSuspenders: warning when overwriting mappings?

Abel de Beer's Avatar

Abel de Beer

19 Jan, 2011 02:19 AM

This post is probably mainly aimed at Till, although interesting enough for other Robotlegs users as well.

While examining the Swiftsuspenders' Injector class code today I was surprised to find how easily mappings are overwritten, without any warning. Here's my test example, if I do:

injector.mapClass(IMyInterface, MyClass);
injector.mapSingletonOf(IMyInterface, MyClass)

The mapping is overwritten by the latter function call, because the m_mappings Dictionary entry is reused in the getMapping() method:

public function getMapping(whenAskedFor : Class, named : String = "") : InjectionConfig
{
    var requestName : String = getQualifiedClassName(whenAskedFor);
    var config : InjectionConfig = m_mappings[requestName + '#' + named];
    if (!config)
    {
        config = m_mappings[requestName + '#' + named] =
            new InjectionConfig(whenAskedFor, named);
    }
    return config;
}

I understand it is necessary to overwrite the previous mapping, since it is not possible to define multiple rules for the action of retrieving an instance. But in my opinion this scenario is common enough - although it probably occurs by mistake most of the time - for the user to be notified of this behavior. I even think throwing an Error would be in place, because if the user actually wanted to overwrite the mapping, he should unmap() first.

Thoughts? :)

PS: The library is great, I can honestly say I love it!!

  1. Support Staff 1 Posted by Till Schneidere... on 19 Jan, 2011 04:13 PM

    Till Schneidereit's Avatar

    Hey Abel,

    thanks for taking the time to write this up - excellent feedback!

    I have to say that I never even considered this to be something one
    might cause problems with. Thinking about what you're saying makes me
    realize that it definitely has real potential to create subtle and
    hard to debug bugs.

    I have added a ticket to SwiftSuspenders' issue list on github
    (https://github.com/tschneidereit/SwiftSuspenders/issues#issue/38).
    Obviously, this would be a breaking change so it would only happen in
    a major release, i.e. Swift-Suspenders 2.

    cheers,
    till

  2. 2 Posted by Stray on 19 Jan, 2011 04:25 PM

    Stray's Avatar

    As a non-breaking change, can I suggest a loud and unmissable trace - if that won't impact on performance?

    Along the lines of

    /************ SWIFT SUSPENDERS INJECTION OVERWRITE WARNING ***********

    The mapping ISomeInjection has been previously mapped.
    If you have overwritten this mapping intentionally you can use
    injector.unmap(ISomeInjection)
    prior to your replacement mapping in order to avoid seeing this message.

    *******************************************************************************************/

    If the check isn't already built in and it does impact on performance that might be a no go.

    A random thought:

    Is there any conditional compiling rolled in for debug vs non-debug builds?

    Obviously we don't want to add a specific compiler arg, but I've often wondered whether there's something there that can be leveraged so that the check would only get compiled in to debug builds.

    Stray

  3. Support Staff 3 Posted by Till Schneidere... on 19 Jan, 2011 04:55 PM

    Till Schneidereit's Avatar

    That's a good idea.

    As for the performance: I don't think that mapping is a
    performance-critical part of any conceivable use of a generic IoC
    container. Meaning that in all cases where that would in fact be a
    problem, you probably shouldn't use such a generic solution anyway.

  4. 4 Posted by Stray on 19 Jan, 2011 05:18 PM

    Stray's Avatar

    Ah, of course - the mappings should be infrequent, it's just accessing the mappings that might happen frequently.

    Well - hopefully that'll be of use to a few people in avoiding long debugging sessions until the new API in 2.0.

  5. Support Staff 5 Posted by Till Schneidere... on 19 Jan, 2011 05:24 PM

    Till Schneidereit's Avatar

    Exactly: Changing a mapping really shouldn't happen frequently.

    I'm still wondering how often the problem we're talking about here
    really occurs, but since I don't see any harm in adding the trace, I
    just did so and will release a new version contain this and a few
    other small changes over the next couple of days.

  6. Stray closed this discussion on 16 Feb, 2011 09:26 PM.

Comments are currently closed for this discussion. You can start a new one.

Keyboard shortcuts

Generic

? Show this help
ESC Blurs the current field

Comment Form

r Focus the comment reply box
^ + ↩ Submit the comment

You can use Command ⌘ instead of Control ^ on Mac