FI-2233: New validation module for HL7 validator wrapper #401

dehall · 2023-10-25T17:17:52Z

Summary

This PR introduces a new Inferno::DSL::FHIRResourceValidation module which is a clone of Inferno::DSL::FHIRValidation, modified to support the HL7 validator wrapper. The code is largely the same as the proof of concept in #396 but reworked to be a separate module. The main entry point to use the HL7 wrapper is fhir_resource_validator which is the replacement of the validator method.

The key differences are as follows:

The HL7 validator wrapper /validate endpoint request and response use the ValidationRequest and ValidationResponse classes from the org.hl7.fhir.validation library, so we will need to map our input and output appropriately. This is pretty straightforward, see functions wrap_resource_for_hl7_wrapper and operation_outcome_from_hl7_wrapped_response below
The HL7 validator uses sessions to ensure different IGs and combinations of settings don't stomp on each other. Spinning up a new session is slow. To ensure good performance, we need to keep track of sessions so that a test suite will re-use the same session ID. There are some other options we could pursue here, including a global session cache, or various changes to the HL7 session management code, but the bare minimum is to keep track of the session ID the previous call returned in an instance variable and re-use that for subsequent calls.
Test suites can specify the IGs they need via a new DSL method igs, which takes a list of IG identifiers . For example for the US core 3 test suite it could look like this:

      fhir_resource_validator do
        url ENV.fetch('V311_VALIDATOR_URL', 'http://validator_service:4567')

        igs ['hl7.fhir.us.core#3.1.1']

        ...
      end

If the IG isn't published yet, then you can pass in a filename here and it will work just fine. (See https://confluence.hl7.org/display/FHIR/Using+the+FHIR+Validator#UsingtheFHIRValidator-LoadinganimplementationGuide for what's accepted) Filenames appear to be relative to the base directory of the HL7 validator wrapper but we will probably need to test different deployment scenarios to confirm.

Testing Guidance

To have test suites actually use the HL7 validator wrapper instead of the inferno wrapper:

enable the hl7_validator_service in docker-compose.background.yml
In your test suite, use fhir_resource_validator instead of validator and make sure it points to http://localhost:3500 (either as a literal or an env var). An easy place to do this is in the demo suite dev_suites/dev_demo_ig_stu1/demo_suite.rb:

    fhir_resource_validator do
      url ENV.fetch('HL7_VALIDATOR_URL')
      exclude_message { |message| message.type == 'info' }
    end

Resouce validation is invoked in Demo Group Instance 1, 1.1.0.7:

codecov · 2023-10-25T17:24:13Z

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (8a50537) 77.00% compared to head (106656d) 77.05%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #401      +/-   ##
==========================================
+ Coverage   77.00%   77.05%   +0.04%     
==========================================
  Files         214      215       +1     
  Lines       10708    10804      +96     
  Branches      991     1008      +17     
==========================================
+ Hits         8246     8325      +79     
- Misses       1884     1901      +17     
  Partials      578      578

Flag	Coverage Δ
backend	`94.00% <82.29%> (-0.36%)`	⬇️
frontend	`69.75% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
lib/inferno/dsl.rb	`100.00% <100.00%> (ø)`
lib/inferno/entities/test_suite.rb	`98.30% <100.00%> (+0.02%)`	⬆️
lib/inferno/dsl/fhir_resource_validation.rb	`81.91% <81.91%> (ø)`

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

arscan · 2023-10-25T19:26:58Z

I would really like access to the machine-readable contents of the IG from within the tests. That way, I could succinctly do something like this simply based on the published capability statement in the ig (vs duplicating it in the suite).

igs['uscore#3.1.1.'].resources.select{|resource| resource.id == 'ServerCapabilityStatement'}.resources.each |resource|

(not quite exactly like that, but having the DSL actually be able to do things a bit more dynamically based on the content of an IG 'at load time' would make Inferno's value prop much higher in my opinion).

Given that feature request eventually, would it make sense to put this ig definition at the suite level, and not within the 'validation' section? The IG has a lot more information than just 'profile validation' that we could leverage.

Just a thought. I understand the desire to keep the change minimal but also once this gets changed i don't see us having a ton of patience for another iteration.

dehall · 2023-10-25T20:22:03Z

I'm not opposed to the idea of moving the igs block outside the validator block (I think we've talked about making the validator block optional and that would be one step in that direction), the only concern I have is how it would work if there were combinations, for example the G10 test suite uses multiple US core IGs: https://github.com/onc-healthit/onc-certification-g10-test-kit/blob/f20940535c29070c2c543bd98b5283ea90ba52d4/lib/onc_certification_g10_test_kit.rb#L69
and ideally we'd be able to associate each IG 1:1 with the corresponding validator settings.

We have an upcoming task (FI-2236) to implement the other configuration items that get mapped into the cliContext of the validator request (disabling TX server, "display issues are warnings", etc) so can we tackle the best place to put everything as part of that?

Jammjammjamm · 2023-10-26T13:25:26Z

I think that is a level of polish that should wait until later.

Jammjammjamm

With the demo suite it seems to be creating a new validator session every run and it doesn't manage to finish before the call to the validator times out.

Jammjammjamm · 2023-10-30T17:48:29Z

docker-compose.background.yml

+  # hl7_validator_service:
+  #   image: markiantorno/validator-wrapper
+  #   # Update this path to match your directory structure
+  #   volumes:


Does the hl7 wrapper need this volume?

If someone wants to load an IG into the validator from a file instead of from a published version, then this is necessary. To reference the IG you can provide a filename:

fhir_resource_validator do igs ['igs/30-us.core-3.1.1.tgz'] ... end

Jammjammjamm · 2023-10-30T17:53:26Z

docker-compose.background.yml

@@ -28,3 +28,10 @@ services:
    volumes:
      - ./data/redis:/data
    command: redis-server --appendonly yes
+  # hl7_validator_service:


config/nginx.background.conf will need an entry for this as well.

Added an entry plus comments linking the two since it's necessary to enable both at the same time. Note I set the timeout to 600s both in the nginx conf and the actual HTTP call, I know that's way too long but until we get session-startup-magic working it needs to be fairly high.

…Validation

dehall · 2023-11-06T21:27:33Z

When it works, it persist the session ID and re-use it for re-runs. You should see something like

2023-11-06 16:18:33 Cached session exists for session id db79d0cc-3332-4780-9dc4-b5fa617753bd, returning stored validator session id.

in the hl7_validator_service docker logs. But if it times out the session ID won't be returned and can't be persisted so it will try to create a new session each time. (And will probably timeout every time.) Hopefully the 600s timeout I added is enough.

I am seeing more 500s than I remember in my testing so I'll keep looking into that before I mark this ready for re-review.

dehall · 2023-11-08T18:20:33Z

Re-tested this today and I'm not seeing any 500s, and with some extra debug print statements I confirmed the session ID is re-used, so I'm marking this ready for re-review

Jammjammjamm

I notice that the validator service is loading IGs for extensions that it finds within resources. I expect we will want that to default to off to prevent memory usage from exploding.

That isn't something that needs to be done as part of this ticket, but I'm assuming that's part of the cliContext so could be done as part of the work to integrate that.

Jammjammjamm · 2023-11-10T14:06:38Z

lib/inferno/dsl/fhir_resource_validation.rb

+
+        # @private
+        def default_validator_url
+          ENV.fetch('VALIDATOR_URL')


Let's make this FHIR_RESOURCE_VALIDATOR_URL and add it to .env so both services can be used at the same time without any risk of conflicts.

Jammjammjamm · 2023-11-10T14:40:26Z

lib/inferno/dsl/fhir_resource_validation.rb

+            },
+            filesToValidate: [
+              {
+                fileName: 'manually_entered_file.json',


Could we make this something like ResourceType/id.json?

Jammjammjamm · 2023-11-10T14:44:44Z

lib/inferno/dsl/fhir_resource_validation.rb

+        def operation_outcome_from_hl7_wrapped_response(response)
+          res = JSON.parse(response)
+
+          @session_id = res['sessionId']


We need a ticket to handle this across processes. It will need to support multiple worker processes like we have in prod, and I expect the web process will need access too in order to display the validator status in the UI.

Created FI-2311 for this

dehall · 2023-11-10T19:00:34Z

Added a note about loading IGs and making sure we have the right cliContext defaults in FI-2236 (in the current sprint). At a glance the 2 things our validator does that aren't replicated here are set doNative = false (not sure what this even does without digging deep into HL7 code) and anyExtensionsAllowed = true but there doesn't seem to be a cliContext field for that.

Jammjammjamm · 2023-11-10T19:03:17Z

anyExtensionsAllowed = true but there doesn't seem to be a cliContext field for that.

We'll definitely need to investigate that as I think it could be a show stopper.

dehall · 2023-11-10T19:22:41Z

Just so nobody panics on a Friday I did a little more digging, the right settings should be set if we use use these two fields in cliContext. I didn't do a full test to compare results but we can do that as part of FI-2236

    "doNative": false,
    "extensions": ["any"],

dehall requested a review from Jammjammjamm October 25, 2023 17:17

dehall force-pushed the fi-2233-new-validation-module branch from 1d65476 to 2fa3541 Compare October 25, 2023 17:23

dehall requested review from Jammjammjamm and removed request for Jammjammjamm October 27, 2023 15:05

Jammjammjamm reviewed Oct 30, 2023

View reviewed changes

dehall added 5 commits November 6, 2023 16:06

FI-2233: step 1, clone Inferno::DSL::FHIRValidation to ::FHIRResource…

d049f89

…Validation

reapply changed from PoC

1f5b467

update unit tests

1e898f2

wire everything up

431e1ff

added commented-out nginx conf for hl7 validator endpoint

8ff081b

dehall force-pushed the fi-2233-new-validation-module branch from 4bffdf6 to 8ff081b Compare November 6, 2023 21:06

dehall requested a review from Jammjammjamm November 8, 2023 18:19

Jammjammjamm reviewed Nov 10, 2023

View reviewed changes

review feedback

f274aee

update tests with new validator filename format

106656d

Jammjammjamm approved these changes Nov 10, 2023

View reviewed changes

dehall merged commit 913467e into main Nov 13, 2023
9 of 10 checks passed

dehall deleted the fi-2233-new-validation-module branch November 13, 2023 14:55

rpassas mentioned this pull request Dec 5, 2023

Inferno Core v0.4.23 Release #415

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FI-2233: New validation module for HL7 validator wrapper #401

FI-2233: New validation module for HL7 validator wrapper #401

dehall commented Oct 25, 2023 •

edited

Loading

codecov bot commented Oct 25, 2023 •

edited

Loading

arscan commented Oct 25, 2023

dehall commented Oct 25, 2023

Jammjammjamm commented Oct 26, 2023

Jammjammjamm left a comment

Jammjammjamm Oct 30, 2023

dehall Nov 6, 2023

Jammjammjamm Oct 30, 2023

dehall Nov 6, 2023 •

edited

Loading

dehall commented Nov 6, 2023

dehall commented Nov 8, 2023

Jammjammjamm left a comment

Jammjammjamm Nov 10, 2023

Jammjammjamm Nov 10, 2023

Jammjammjamm Nov 10, 2023

dehall Nov 10, 2023

dehall commented Nov 10, 2023

Jammjammjamm commented Nov 10, 2023

dehall commented Nov 10, 2023

FI-2233: New validation module for HL7 validator wrapper #401

FI-2233: New validation module for HL7 validator wrapper #401

Conversation

dehall commented Oct 25, 2023 • edited Loading

Summary

Testing Guidance

codecov bot commented Oct 25, 2023 • edited Loading

Codecov Report

arscan commented Oct 25, 2023

dehall commented Oct 25, 2023

Jammjammjamm commented Oct 26, 2023

Jammjammjamm left a comment

Choose a reason for hiding this comment

Jammjammjamm Oct 30, 2023

Choose a reason for hiding this comment

dehall Nov 6, 2023

Choose a reason for hiding this comment

Jammjammjamm Oct 30, 2023

Choose a reason for hiding this comment

dehall Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

dehall commented Nov 6, 2023

dehall commented Nov 8, 2023

Jammjammjamm left a comment

Choose a reason for hiding this comment

Jammjammjamm Nov 10, 2023

Choose a reason for hiding this comment

Jammjammjamm Nov 10, 2023

Choose a reason for hiding this comment

Jammjammjamm Nov 10, 2023

Choose a reason for hiding this comment

dehall Nov 10, 2023

Choose a reason for hiding this comment

dehall commented Nov 10, 2023

Jammjammjamm commented Nov 10, 2023

dehall commented Nov 10, 2023

dehall commented Oct 25, 2023 •

edited

Loading

codecov bot commented Oct 25, 2023 •

edited

Loading

dehall Nov 6, 2023 •

edited

Loading