[stskjisc]

Create new bean 'StatusMetadataCheckStrategy' to tests whether the supplied metadata item contains a StatusMetadata of the configured type with a matching status message.

[philsmart]

The code changes look good right now. I think we should test this out based on this branch on ukf-mda in case we need to make further changes before merging this back in.

[philsmart]

Some minor improvments

[philsmart]

Small changes in the check class. Looking good though

[philsmart]

I think we should use anonymous Entity Descriptor XML for tests, see the comments.

[philsmart]

Approve, but it would be great to have Ian review as well.

[iay]

I think this looks like a pretty good start. There are some things I think I'd probably want to think about if it were to come in as a proposed contribution to the MDA, though.

• I don't see the benefit of the STATUS_TYPE within StatusMetadataCheckStrategy. It only seems to exist to wrap a restricted class, but those are things you can pass in as first-class objects in Spring configuration so that seems like a layer you could remove. The enum part is also redundant, I think, as the parent type is sealed so there's an implicit restriction on the subtypes.
• I'd have used Checking rather than Check in the name, to fit in better with the way other classes in the MDA are named.
• StatusMetadataFilterStrategy is not a strategy, it does something in isolation. In general, anything which implements the Stage interface should have a name which ends with Stage. Something like StatusMetadataFilteringStage would probably be more appropriate.
• The MDA, and Shibboleth code in general, doesn't use constructor arguments. This is annoying, as very often you'd want to do what you do here and just make all the fields final from the constructor. However, there's a reason for this which I don't recall in detail but has to do with Spring configuration. I think it's fine to go this route for now but be aware that we'd want to reconsider if and when you want to upstream this, as we'd probably want to match the rest of the MDA components even if it isn't what we'd prefer in isolation.
• In general if you see a hard down-cast in modern Java, as in final StatusMetadata statusMetadata = (StatusMetadata) itemMetadata; then there's probably something smelly about your types. In this case, I think if you applied a more appropriate second type bound to the AbstractItemMetadataSelectionStage parent class of StatusMetadataFilterStrategy you wouldn't need this because your types would be correct. Probably changing ItemMetadata to StatusMetadata is what you're looking for there. I haven't tried this ;-)

[stskjisc]

Thanks @iay ! I have made some changes based on your comment. All are very useful, especially the last one regarding the StatusMetadata class.

For the suggestion regarding the use of constructor injection. I understand the preference for setter injection, as it fits better with the Spring/Shibboleth component model and generally makes components more configurable.
In my original implementation I used constructor injection because all three properties are mandatory, so it guarantees the object is always in a valid state.
I've updated StatusMetadataFilteringStage to follow the setter + doInitialize() pattern, since it participates in the Shibboleth component lifecycle.
However, StatusMetadataCheckingStrategy is just a Predicate and doesn't extend an initializable component, so it has no doInitialize() phase in which to validate mandatory properties. Because of that, I'm not sure how best to follow the same pattern there while still ensuring the required properties are set.
I also noticed that the other Predicate implementations in the MDA project use constructor injection rather than setters, which made me think that might be the intended pattern for this type of helper class.
Would you still recommend using setters for StatusMetadataCheckingStrategy, or is constructor injection acceptable for this kind of helper class?

I've pushed an updated version of the code and run the tests locally. If you have some time, could you also take a look at the latest changes and let me know if they look OK? Thanks a lot!

[iay]

It looks like I was a little unclear. The comment I was making about constructor initialisation vs. property setting for initialisation was aimed at the Stage, which is part of the Shibboleth component system. There, even if everything could be final and initialised by a constructor, there would still be properties inherited from parent classes (ultimately, for example, the id property) and mixing constructor and property initialisation is just confusing. (Plus there's that thing about Spring which I still don't exactly recall the details of; I'd have to check in with Rod to be sure about that part).

For the thing that's really just a Predicate, I agree that constructor initialisation makes sense. In a major sense, this object is just a wrapper around some final fields and a method or two and that's exactly where this kind of initialisation works well. So I have no problem with that.

I actually think we went a bit far with this in the Shibboleth codebase in general, probably due to that Spring thing I keep mentioning. There are a few places where we've ended up having objects with non-final fields where it would have been possible to go final and get all the benefits of that (compiler optimisations, better nullability checking, etc.). That's probably not fixable but for something like your Strategy class I think what you've done is fine.

One thing in the MDA specifically which pushes some classes away from constructor initialisation towards properties is where a class needs to have an identifier used in status metadata. That has usually meant that they need to be part of the component system, and the id property in particular is implemented by a setter (and in fact there's some background stuff which means it can be altered dynamically after object creation). So there are some classes that look initially as if they could be constructor-based which aren't. Most of the time, though, as you've noticed if something is just a wrapper around some fields it ends up being constructor-based. There are a couple of cases which even ended up as records.

[iay]

Some additional thoughts:

• StatusMetadataCheckingStrategy implements Predicate<DOMElementItem> but you should probably not use the concrete type here but Predicate<Item<Element>> instead.
• In general, setting and getting primitives such as booleans are atomic operations, so they don't need to be @GuardedBy or synchronized. I'm not sure if I've been rigorous about that in the main codebase.
• If you implement setters, it makes sense to implement getters as well. Normally I'd actually use the getters for properties inside the code, too, and not the bare fields.

Only the first of these items actually matters at all at this stage.

[stskjisc]

Fixed, please check again. Thanks

[philsmart]

Just reviewing this from the ukf context (due to internal changes). We can then ask Ian, in a Shibboleth MDA context, whether we want to try to have the StatusChecker upstreamed (which is a separate conversation and does not have to affect the release of these changes in ukf-mda). So far:

• STATUS_TYPE was removed; looks good.
• StatusMetadataCheckStrategy was changed to StatusMetadataCheckingStrategy, looks good.
• StatusMetadataFilterStrategy is now StatusMetadataFilteringStage, looks good.
• StatusMetadataFilteringStage now uses setter-based property initialisation, and StatusMetadataCheckingStrategy uses constructor initialisation. Looks good and fits with the discussion.
• StatusMetadataCheckingStrategy implements Predicate<Item<Element>> as discussed, looks good.
• It is a good point Ian made about using synchronized setters and getters for primitives. I get confused with thread visibility and atomic read/writes. Anyway, the code now has an inconsistency in that the setter for ExactMatch is synchronised but the getter is not, so I guess remove the one from the setter. (I'll add a comment in the code).

[philsmart]

My fault; we can only increment this value once we have tagged 0.10.1. That happens when you follow the Release Process. So that will need to go back to 0.10.1-SNAPSHOT for now.