Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FI-2460: Support for HL7 validator wrapper #488

Merged
merged 9 commits into from
Apr 8, 2024
Merged

Conversation

dehall
Copy link
Contributor

@dehall dehall commented Feb 6, 2024

This PR enables support for the the g10-test-kit to use the original Inferno validator wrapper or the HL7 validator wrapper, toggled by the env var USE_HL7_RESOURCE_VALIDATOR. From the user perspective, there should be no visible change. If I've done this right then it should be plug & play.

Summary of changes:

  • Add HL7 validator service to docker-compose and nginx, commented out by default
  • Change validator block in test suite to a conditional that selects validator or fhir_resource_validator based on a new env var USE_HL7_RESOURCE_VALIDATOR. Add igs setting to this block if using the HL7 validator.
  • Add a cli_context block to specify validator settings:
    • txServer nil disables use of a TX server. Aligns with DISABLE_TX env var in the inferno validator
    • displayWarnings true sets any "display issue" messages to be warnings instead of errors. Aligns with DISPLAY_ISSUES_ARE_WARNINGS env var in the inferno validator
    • disableDefaultResourceFetcher true disables fetching and validating against any unknown profiles found in a resource. (This is now a default in inferno-core, not yet released though. No harm in keeping this setting here temporarily or forever)
  • Change version endpoint and expected version. (HL7 validator wrapper reports version as the the https://github.com/hapifhir/org.hl7.fhir.core release number)
  • Ignore "URL does not resolve" errors - these were ignored natively in the Inferno validator but the mechanism to do that isn't exposed in the HL7 wrapper
  • Ignore "[No server available]" errors - these were introduced somewhere in the core validator between versions 6.2.8 and 6.3.0 and occur when txServer is disabled. See screenshot below.
  • Update spec tests; see notes on FI-2532: Add feature flag to toggle using HL7 validator wrapper #497

New error messages that need to be ignored -- there are two messages that occur per usage of a mime type (CapabilityStatement, DocumentReference, some uses of DiagnosticReport)
image (12)

Copy link
Contributor

@Jammjammjamm Jammjammjamm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be working well for me, but I expect your FHIR core PR will need to get in before we could use this in production. We should see what @arscan thinks as well. I think the next steps are something like the following:

  • Make a subclass of the g10 suite, change its id, and have one use the current validator and one use the new validator so that we'll be able to support both during a transition period. I'm not sure whether we'll want the "real" g10 suite to use the new validator to ensure it gets broad testing or the current one to minimize any potential disruption. I guess this would need two version of the configuration checker as well.
  • Update the US Core and SMART App Launch test suites to use the new validator so that we'll be ready to completely cut over and remove the current validator from this repo.

case (us_core_version_requirement[:us_core_version])
when G10Options::US_CORE_3
igs('hl7.fhir.us.core#3.1.1')
us_core_message_filters = USCoreTestKit::USCoreV311::USCoreTestSuite::VALIDATION_MESSAGE_FILTERS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this assignment was changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just felt strange to have the igs line in the middle of the case when the result of that case is assigning to a separate variable. I'm probably going to revert this anyway because I suspect there's a better way to define the IG for each option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the igs lines there wouldn't interfere with the original assignment style.

@dehall
Copy link
Contributor Author

dehall commented Feb 16, 2024

How will the "two suites" approach look from a user perspective? Will we show the two suites on the start page and the user has to pick one?
In the meantime I'll press forward with cleanup here and then adding hl7 validator support to the other two test kits.

@Jammjammjamm
Copy link
Contributor

How will the "two suites" approach look from a user perspective? Will we show the two suites on the start page and the user has to pick one?

We'll want to discuss that with @arscan

@dehall dehall changed the title (WIP) FI-2460: Support for HL7 validator wrapper FI-2460: Support for HL7 validator wrapper Mar 7, 2024
@dehall dehall marked this pull request as ready for review March 7, 2024 16:29
@Jammjammjamm Jammjammjamm merged commit 785e875 into main Apr 8, 2024
3 checks passed
@Jammjammjamm Jammjammjamm deleted the fi-2460-hl7-validator branch April 8, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants