Introduce new validator cliContext option disableDefaultResourceFetcher

[dehall]

This PR is the result of this discussion on Zulip: https://chat.fhir.org/#narrow/stream/291844-FHIR-Validator/topic/CLI.20option.20to.20not.20fetch.20unknown.20profiles.3F

Description:

This PR introduces a new option which disables the default resource fetcher in the validation engine. The resource fetcher is used to fetch IGs when an unknown profile is referenced. The default behavior of the validator is unchanged. The new setting is called disableDefaultResourceFetcher or if using the CLI jar -disable-default-resource-fetcher
There are three end-user scenarios to consider:

1. The default behavior, validating a resource with a profile
   • Expected behavior: the appropriate IG is fetched and the resource is validated against the given profile
2. Setting disableDefaultResourceFetcher = true
   • Expected behavior: the IG is not fetched, the resource is not validated against the given profile, and a warning message is reported
3. Setting disableDefaultResourceFetcher = true, and adding the appropriate IG to cliContext.igs/-ig
   • Expected behavior: the IG is already loaded and does not need to be fetched, the resource is validated against the given profile

History/Context:

The Inferno team is continuing to look into replacing our validator wrapper service with the HL7 validator-wrapper, and we notice that our service will respond with a warning for unknown profiles in meta.profile (e.g. "Profile reference 'http://hl7.org/fhir/us/core/StructureDefinition/us-core-patient' has not been checked because it is unknown, and the validator is set to not fetch unknown profiles") but the validator-wrapper will fetch the appropriate IG and validate the resource against the profile. For our use case of certification, I'm not sure it's relevant if the resource validates against any profiles other than the one under test, and we're also concerned that fetching arbitrary IGs could pollute the validator session.
This is because we directly instantiate a ValidatorEngine and don't set a "fetcher" whereas the validator-wrapper uses a ValidationService which does set a "fetcher" on the ValidatorEngine s it spins up. Per David's suggestion on the Zulip thread, I've added a new CLI option which, if true, does not set that fetcher.

Original text below for reference only

Description:

This PR introduces a new option which prevents the validator from fetching (and validating against) unknown profiles. The default behavior of the validator is unchanged. The new setting is called doNotFetchUnknownProfiles or if using the CLI jar -do-not-fetch-unknown-profiles
There are three scenarios to consider:

1. The default behavior, validating a resource with a profile
   • Expected behavior: the appropriate IG is fetched and the resource is validated against the given profile
2. Setting doNotFetchUnknownProfiles = true
   • Expected behavior: the IG is not fetched, the resource is not validated against the given profile, and a warning message is reported
3. Setting doNotFetchUnknownProfiles = true, and adding the appropriate IG to cliContext.igs/-ig
   • Expected behavior: the IG is already loaded and does not need to be fetched, the resource is validated against the given profile

Notes on testing:

I added a unit test to check the three scenarios as mentioned above:

1. The default behavior, validating a resource with a profile
2. Setting doNotFetchUnknownProfiles = true
3. Setting doNotFetchUnknownProfiles = true, and adding the appropriate IG to cliContext.igs

I don't really like the test though - it's much too slow. Any suggestions on a better test approach are very welcome. (I've also selected a test resource that passes validation with the profile. A better test might use a test resource that fails profile validation but succeeds with the "not checked" warning when doNotFetchUnknownProfiles is set, but I didn't see any examples of those in the existing test set. Wasn't sure if adding a whole new test resource here was appropriate)

[dotasek]

@dehall

I think you're right; that test does a lot that's outside of the very limited scope of testing this function. In fact, it errors locally for me after some commits to unrelated code.

I think they way to make this more efficient and less prone to breakage would be to mock parts of the ValidationService instead of trying to use a full instance. There are examples in ValidationTests of how this can be done. I will have a look and see if I can start you on this path with some suggestions.

[dotasek]

I've had a chance to look at this more in depth now.

@dehall I've made a commit that fixes this test, and adds a mocked version here: https://github.com/hapifhir/org.hl7.fhir.core/tree/do-20240115-unknown-profile-fetch I think the mocked test should stay where it is, but would like the validation test moved.

@grahamegrieve I think this test Dylan wrote actually belongs in the validation tests in fhir-test-cases, but with an additional parameter that specifies whether or not to use the standalone fetcher. But before that takes place, we should address my concern below.

Now that I've had a chance to look at this more deeply, I'm concerned that the intent implied by most of the methods and arguments looks very confusing.

doNotFetchUnknownProfiles is essentially a description of the warning we want to see, but in actuality, it's disabling a default implementation of the resource fetcher altogether. If this was simply 'useDefaultResourceFetcher' and -use-default-resource-fetcher in the code, and in the CLI, and this was set to true by default, I think it would make much more sense. Does that make sense to you @dehall and @grahamegrieve ?

[dehall]

Thanks so much @dotasek - I really didn't intend for this to require so much time on your part. I defer to whatever you want to do, definitely agree that framing it as "using/disabling the resource fetcher" makes it easier to test.

My only thought is that since the parameter name doesn't reflect the reason that the feature was added, how can we ensure it doesn't evolve in a way that breaks the original intent? Is the unit test I wrote enough to do that? (Or am I overthinking it because profile fetching is unlikely to change much going forward?)

[dehall]

I've pushed an update that includes your new test cases and renames the parameter to -disable-default-resource-fetcher. It's a little more consistent with the other "flag"-style parameters this way (don't need to specify an additional value on the command-line, and the default value is false) but if you feel strongly I can change it to use-default... and make the default value true.

[dehall]

@dotasek Are the unit tests flaky on CI? I keep rebasing to get the latest from master and each time there's 1 or 2 different failures. This time it's in the FHIRPathTests, last time I think it was in the ExternalTerminologyServiceTests. Neither one seems like it should be affected by a change to the ValidationEngine class. (And I hate saying this but they run successfully on my machine)

[dotasek]

See the comment I left regarding fluent setters. Otherwise, this looks good to me, and I will investigate the failing tests and follow up.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dd3770d) 11.73% compared to head (74aa970) 11.73%.
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #1526   +/-   ##
=========================================
  Coverage     11.73%   11.73%           
- Complexity    30374    30378    +4     
=========================================
  Files          2162     2162           
  Lines        667558   667565    +7     
  Branches     197134   197136    +2     
=========================================
+ Hits          78344    78349    +5     
  Misses       560809   560809           
- Partials      28405    28407    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dotasek]

Merging after discussion with @grahamegrieve