Implement an ensureId method to help with nullability annotation
Description
Environment
Activity
Rod Widdowson June 20, 2023 at 3:03 PM
> I think we can resolve this now
A month on. Resolving it
Rod Widdowson May 25, 2023 at 4:21 PM
Iāve been over everything but OpenSAML using the new method where it would make a difference.
In addition to the two interfaces above Iād add the MetadataResolver, IdPAttribute and ResolverPlugin would all benefit from having a ensureId. But not enough (IMO) to warrant adding the ensure to IdentifiedComponent.
I think we can resolve this now
Rod Widdowson May 4, 2023 at 3:28 PM
A couple of Interfaces which could do with an ensureId are
net.shibboleth.idp.session.IdPSession
net.shibboleth.idp.session.SPSessionThere's a log of defensive asserts in the packages net.shibboleth.idp.session.impl around their getId methods
Rod Widdowson May 4, 2023 at 3:16 PM
Iām working through the IdP making use of this as needed
I am not changing logging - logging can swallow nulls and making something crash needlessly is too much of a risk (plus the logging could help)
I am not changing id usage in equals/hash/toString - there is no guarantee on component state
I am adding it where we currently have defensive code (obvs)
I am adding it in cases where null would break other contracts (e.g. as a key or value into a Map or as a value into a Collection - when these are implied or defined to not contain nulls)
This works pretty well.
Rod Widdowson April 25, 2023 at 2:48 PM
For instance
net.shibboleth.idp.saml.metadata.impl.MetadataResolverServiceGaugeSet.MetadataResolverServiceGaugeSet()
Has an example when it might be useful to have the ensureId on an interface but
It would end up adding a
nullas a key to Map which is probably illegalIām unconvinced that it is a good case (or even a case at all) for adding anything to any Interface
One of the things Iāve hit multiple times when trying to get code null-clean is that in our component system,
getIdis (correctly) labelled as@NonnullAfterInitbut of course is always null in the normal course of execution where you want to pass it to a method which now requires@Nonnull. I've tried several bodges to work round this in various places, but I'm not fond of most of them.My proposal is that we add the following method to
AbstractIdentifiedInitializableComponentalongsidegetId:protected final @Nonnull String ensureId() { final var id = getId(); if (id == null) { throw new IllegalStateException(); } return id; }(Name suggested by Scott)
Note that I am suggesting that we add this to the implementation and not to the interface
IdentifiedComponentwheregetIdis declared. It would then be inherited by (hopefully) most or all of the code where weāre doing null checking without forcing any other places that implementIdentifiedComponentto be changed. I suppose an alternative would be to to add it as a default method to the interface, but Iām a little wary of that because I havenāt used that construct a lot.Iām going to start by implementing this in a couple of places in the abstract class hierarchies in the MDA, but ideally weād put this as high up as it can go, which means JSSH.