Home → Suggestions →

SwiftSuspenders: warning when overwriting mappings?

• Edit

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

   

   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

   • Edit

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

   

   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

   • Edit

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

   

   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.

   • Edit

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

   

   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.

   • Edit

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

   

   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.

   • Edit

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