Anotating Constructor Parameters (IDP-1047)

[Cantor, Scott]

On 9/23/16, 8:32 AM, "dev on behalf of Rod Widdowson" <dev-bounces at shibboleth.net on behalf of rdw at steadingsoftware.com> wrote:

> I'd like feedback as to whether what is there looks reasonable.  This is a pretty busy
> class and so the constructor declaration is getting pretty noisy;  I don't think that we
> have a choice but before I make further changes I want to check what other think.

Self-inflicted obviously, but I don't see much choice. I guess it comes back to me believing we should limit this as much as possible and get away from needing it.

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.

Existing constructor-based patterns can be used, but the same classes could have default constructors added too if they implement Initializeable.

> 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?

> It will warn on implementation classes as well as api classes (yes we could add a filter
> on the name but that’s getting icky)
>    
> 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.

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.

> We could add extra fields to the annotation to mention where (in idp-conf) we use this
> constructor; then if we chose to rename a parameter in an impl class we could be directed
> to the configuration changes needed.  I don't like this idea much since the information
> will rot.

Yes, I think that's a Spring tooling issue, we can find these things when we need to.

> We could hijack the use of this class to detect if we summon up a bean using a deprecated
> constructor - this might be a useful service.  It would only work for constructors with
> parameters, which means that the coverage would be minimal.  It is also an abuse
> of the API which makes me feel vaguely nauseous.  If we wanted to do this (and it is an
> interesting idea) I would imagine that there is a better way of hooking into Spring (maybe
> the InstantationStrategy?)

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.

-- Scott