Anotating Constructor Parameters (IDP-1047)

[Rod Widdowson]

> We have an explicit bean initialization hook in the class hierarchy and using it solves the problem of guarding required properties. We
> just need to use it. Our classes may not depend on Spring, but our usage does, and that has to take precedence.

I'd agree and even state that more strongly. Our use of the whole initialization feature predates both the hook and my (personal) discovery of Lifecyle#start
I remember stripping out a lot of unneeded cascading initialize code when we realized the overlap.

Anyway we have the paradigm through the stack so we could/should us it.
 
> Existing constructor-based patterns can be used, but the same classes could have default constructors added too if they implement
> Initializeable.

That's an important point.  I've been doing some further investigation as I outlined, and before I even get to our xml config files I have found about 30 classes with constructors in the IdP - these all/mostly being fed by out custom parsing.  Many of them are in Attribute space and most could be made initializable.  Sone,  like IdPAttribute itself probably should not be.

This point, and the number of changes needed so far (and this as I say before I get to any native spring) actually argues that there are a couple of steps needed first.

1) Add property setters to these classes and make them (if not already) initializable[*]
2) Deprecate (with a log message) the old constructors 
3) Change the custom parsers to use the properties not the constructor.

Then we can see what's left.

[*] I do have one question however.  Given that many of these classes are in the API space, is it safe from a versioning policy to change the base class of something ?   I could see that if someone had (by mischance) derived from this stuff with a private doInitialize() call  their code would not longer compile if I had made their base class derive from AbstractInitializable.  I think it's unlikely, but rules are rules.  If I am right I'm not sure what  we can do short of V4.  

> > The code is currently set up to only care about "net.shibboleth.*" and "org.opensaml.*"
> > classes, and it warns if it sees a constructor used for one of them with no annotation.
> 
> Is that an optimization?

Not really, and see below.  The logic goes that if the classes doesn't have that name then we don't own it and so we cannot add the annotation.  Thus adding a warning serves no purpose.  Consider the multiple <bean name=foo class=java.lang.String c:_0= wibble.


> > It will warn when called with numbering parameters (c:_0) (I don't think that this is
> > fixable)
> 
> The second bothers me quite a bit. Otherwise we're going to have to annotate every constructor we have to call, which gets me back
> into avoiding c'tors altogether.

Well if it's in an API we have to and if it's in an impl we have the alternative. 

> Maybe we have to do an enumeration here. I realize this only works if we *greatly* limit calling constructors with names, but maybe
> we just feed this the list of classes to guard.

Yuck,  but maybe.

>> ... detect if we summon up a bean using a deprecated
> > constructor ...
> 
> Seems like it's better done some other way than incompletely with this, but again we have to just understand that we can't go
> changing constructors now, anymore than we can change property method names, without reviewing the whole configuration.

Right.  I like the idea of doing this.  Not in V3 but in a release prior to V4 so people can be proactive.  To be honest the easiest thing is probably just to add a log.warn where appropriate.