Default setting of isEnabledValidationForCodingsLogicalAnd() is wrong way round

[stueynz]

Way back in Jan-2023 @lukedegruchy opened an issue Inconsistent validation of language codes. The resulting pull request included a commit that added a new validation configuration variable IValidationContext..isEnabledValidationForCodingsLogicalAnd()

The effect of the new config variable was:

• false - validation will return OK if there is at least one valid coding that matched the binding.
• true - validation will return OK only if all codings match the binding.

The default value for the new config variable was false which meant that validation would return OK if there was at least one valid coding that matched the binding. This meant that default 'out of the box' codeset validation behaviour continued as before.

Then in until early June-2024 when Core Library Bump 6.3.11 included a commit pull request that re-worked the codeset validation problem.

The re-worked code for implementing the desired effect of the IValidationContext..isEnabledValidationForCodingsLogicalAnd() reversed the meaning of the validation variable:

• true - validation will return OK if there is at least one valid coding that matched the binding.
• false - validation will return OK only if all codings match the binding.

The Core Library Bump 6.3.11 pull request included a comment from @dotasek suggesting that the variable be renamed. With the reversal of the meaning of true and false values, it probably should be.

The Actual Problem: The re-working of this code in Core Library Bump 6.3.11 reversed the behaviour of the config variable, without reversing the default setting, to ensure default behaviour that was consistent with historical behaviour. ie: This code should set the default value to true

Why I care: In my IG we have made extensive use of additional bindings which means we have numerous codeable concepts with multiple bindings; which are now failing validation in the HAPI-FHIR validator because the default codeset validation behaviour has changed.

Is it unreasonable to expect the default out-of-the box behaviour to remain unchanged when new functionality is added?

[dotasek]

@stueynz there were several test cases that specified this behaviour: https://github.com/hapifhir/hapi-fhir/blame/724339c8861d61c3e6c62cb27024653d01b58335/hapi-fhir-validation/src/test/java/org/hl7/fhir/r4/validation/FhirInstanceValidatorR4Test.java#L1780-1838

The only one of these that changed was https://github.com/hapifhir/hapi-fhir/blame/724339c8861d61c3e6c62cb27024653d01b58335/hapi-fhir-validation/src/test/java/org/hl7/fhir/r4/validation/FhirInstanceValidatorR4Test.java#L1797

In that case, I believe the original test case was expecting the incorrect result. Regardless of the ...LogicalAndSupport setting, there are no valid codes present in this case (only an invalid en_US code) and this should never be valid.

None of the other test cases covering this setting have changed. The behaviour, at least where defined by this setting, should otherwise be as it was.

Is this actually what is impacting your validation? Is there a replicable example of what was previously valid that is no longer?

[stueynz]

I'm not questioning the test-cases or the functionality of the validation code. It all works as expected, except for the default setting in the IValidationSupport interface which is false, meaning when validating multiple codings all of them must match the bound ValueSet.

I've got some fixes to ValidationCLI fixes in my fork; just checking if there's anything I've missed before doing a PullRequest.

What I'm really asking: Is my logic about default settings preserving historical behaviour coherent? Should I just change the default setting in IValidationSupport or only in the validationCLI implementation of the interface?

[stueynz]

I went back to v6.2.2 -- before all these changes started; and also checked v6.10.5 - and it turns out I was wrong, in my memory of what constitutes historical behaviour. Oh well.

[dotasek]

@stueynz Thank you for checking. I did a similar exploration myself before my initial reply.

@lukedegruchy and myself had a discussion about this yesterday, and believe we should revise the documentation to make this setting clearer.