On Thu, Nov 12, 2009 at 8:39 AM, Les Hazlewood <[email protected]> wrote: > You're right, there is a circular dependency and it is mostly a > remnant of the old implementation. Since the default SubjectFactory > implementation needed a a SecurityManager instance to build its > instances, it was made available as an injected property.
First of all, I'm always impressed and appreciative you always taking time to write such comprehensive and well-thought out replies, so thanks for that Les. > But now that you reminded me of it, I think adding the SecurityManager > reference to the context Map right before the > SubjectFactory.createInstance method is called is a great idea - no > more configuration circular dependency and the underlying > implementation can do whatever it wants - either use the one in the > map or use its own if necessary. I think that's the key - whether securityManager is optional or not. If it's not, it might just as well be an additional parameter but if there's even a possibility that some implementation of a SubjectFactory would not need it or would want to instead provide its own, then it's probably better to pass it in as part of the context map. > I personally prefer this to adding a method argument to the > SubjectFactory interface because I personally view the need of the > SecurityManager instance is really an implementation type of detail - > not something that should change the interface contract IMO. Ok, so it sounds like you see use for allowing specific implementations of SubjectFactory to handle SecurityManager as they best see fit - I trust your judgement with that. I'll open an issue for making SubjectFactory part of the context and I can even take care of the refactoring, shouldn't be too difficult. > There is an interface, org.apache.shiro.util.Factory<T>. I want to > change this a bit (change createInstance to getInstance and let the > implementation decide if a new object or cached object is returned). > I was also thinking of adding a new interface: > org.apache.shiro.util.ContextualFactory<T,U> where T is the factory > output type and U is the factory method input type. The > SubjectFactory interface could then just be a sub-interface of the > latter. Sounds good. Kalle
