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-2701: Migrate to HL7 validator wrapper #4

Merged
merged 7 commits into from
Jul 2, 2024
Merged

Conversation

dehall
Copy link
Contributor

@dehall dehall commented May 31, 2024

Summary

This PR migrates the test kit from the Inferno validator-wrapper to the HL7 validator-wrapper. This is similar to most of the other test kits, with one significant difference that his test kit used the inferno validator's FHIRPath endpoint, so this now includes the fhirpath-service in the docker-compose files and nginx.background.conf.

Other changes:

  • add the hl7_validator service to docker-compose and nginx. The old validator components are left commented out in case someone really wants the old validator UI
  • Bump the core dependency
  • Migrate validator blocks to fhir_resource_validator and specify the IG
  • Update the generator with ^ and re-generate the suite
  • Update the fhirpath evaluation method to call the appropriate endpoint (based on Migrate to the FHIRPath service #2 )
  • Add a README to the IGs folder explaining when it's necessary for the files to be there
  • Fixes an issue where the custom validation logic tries to validate against native profile urls with the IG's version, in particular http://hl7.org/fhir/StructureDefinition/PractitionerRole|2.0.1 . The validator doesn't have this profile loaded which results in a 500 error and a purple test result
  • Updates the preset and sample bundles to remove relative links, eg <a href="#something"> . A new check in the validator has strict requirements for these and produces "Hyperlink does not resolve" errors. The replacement was <a href=\\\\\\"#.*?">See above \((.*?)\)</a> --> \1, eg, <a href=\\\"#Patient_SubscriberExample\\\">See above (Patient/SubscriberExample)</a> became just Patient/SubscriberExample

Testing Guidance

From a user's perspective there should be no visible difference between this and using the old validator. Run through the tests and make sure everything works

@dehall dehall force-pushed the fi-2701-hl7-validator branch from 0798a3b to f5833d2 Compare June 5, 2024 16:49
@dehall dehall changed the title (WIP) FI-2701: Migrate to HL7 validator wrapper FI-2701: Migrate to HL7 validator wrapper Jun 5, 2024
@dehall dehall requested a review from karlnaden June 5, 2024 16:50
@dehall dehall marked this pull request as ready for review June 5, 2024 16:50
@dehall dehall force-pushed the fi-2701-hl7-validator branch from f5833d2 to 24fbc3f Compare June 7, 2024 14:13
@dehall dehall requested a review from tstrass June 19, 2024 15:50
@dehall dehall force-pushed the fi-2701-hl7-validator branch from d73246d to a35fa86 Compare June 27, 2024 11:32
Copy link
Contributor

@tstrass tstrass left a comment

Choose a reason for hiding this comment

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

Changes look good. I just noticed one difference. With the new validator and preset, the server suite input bundles for approval and denial pass validation, and with the old validator and preset they fail validation with the messages below. Is this expected?

image

@dehall
Copy link
Contributor Author

dehall commented Jun 27, 2024

Failing on the old one and passing on the new one is unexpected. I can look more into that

@dehall
Copy link
Contributor Author

dehall commented Jul 1, 2024

Ignore the previous (now-deleted) message if you saw that. I wonder if the prod validator instance is in an inconsistent state because running that same test locally on main passes for me:
image

@dehall
Copy link
Contributor Author

dehall commented Jul 1, 2024

Ok I see the difference now. That error we're seeing on prod is for a CRD extension http://hl7.org/fhir/us/davinci-crd/STU2/StructureDefinition-ext-coverage-information.html and so the validator will only check that when the CRD IG is loaded, either explicitly or as a dependency of another IG. When you run this test kit by itself on main, you don't get those errors. But on prod the old validator instance is running for a few test kits and one of them may be bringing in a different version of CRD. If you load all the same IGs into the validator locally as are running on prod, the same error happens. (davinci-pdex-2.0.0.tgz, davinci_pas_2.0.1.tgz, uds-plus-external-package.tgz, davinci_dtr_2.0.1.tgz, udsplus-101.tgz)

So, I think this should be good, in terms of matching the old behavior when running in isolation. Let me know if you disagree @tstrass or @karlnaden , otherwise I'll merge this in

@karlnaden
Copy link
Collaborator

your analysis looks sound to me. I'll defer to Tom having not looked closely.

Copy link
Contributor

@tstrass tstrass left a comment

Choose a reason for hiding this comment

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

Agreed, thanks for investigating

@dehall dehall merged commit ca8357b into main Jul 2, 2024
3 checks passed
@dehall dehall deleted the fi-2701-hl7-validator branch July 2, 2024 11:05
@dehall
Copy link
Contributor Author

dehall commented Jul 3, 2024

Just for posterity, turns out that constraint that was failing is bad anyway. https://jira.hl7.org/browse/FHIR-46087 (thanks fhir-at-mitre slack channel)

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.

3 participants