FI-930: Support FHIRPath for generating searches #490

jason-crowley · 2020-08-31T13:09:12Z

FI-930: SearchParameters that have complicated FHIRPath expressions (e.g. use descendants() or |) cannot be generically processed by Inferno. Inferno uses the FHIRPath in order to determine the value of the searchParameter to be used in the search and validate that the results returned conform to the value.

This PR creates a FHIRPathEvaluator class that posts elements to the FHIRPath endpoint. It also adds two new config options, fhirpath_evaluator and external_fhirpath_evaluator_url, which are analogous to the resource_validator and external_resource_validator_url config options. When fhirpath_evaluator is set to external, POST requests will be made to the FHIRPath endpoint in order to resolve FHIRPath expressions against a given complex datatype.

The goal was to substitute out the logic that was previously resolve_path and resolve_element_from_path with a more complete FHIRPath solution. This allows us to remove some of the FHIRPath patching (e.g. removing .as() and truncating the path) that was done in the sequences. However, one patch that has not been removed is .where(resolve() ...) because the external FHIRPath evaluator does not know how to resolve references of the form [Resource]/[id].

~~NOTE: This PR requires running wrapper/FI-931-add-fhirpath-endpoint locally to work.~~ UPDATE: this is no longer true; you will just have to re-pull the master branch of f-v-w and rebuild.

Submitter:

This pull request describes why these changes were made
Internal ticket for this PR: https://oncprojectracking.healthit.gov/support/browse/FI-930
Internal ticket links to this PR
Internal ticket is properly labeled (Community/Program)
Internal ticket has a justification for its Community/Program label
Code diff has been reviewed for extraneous/missing code
Tests are included and test edge cases
Tests/code quality metrics have been run locally and pass

Reviewer 1:

Name:

Code is maintainable and reusable, reuses existing code and infrastructure
where appropriate, and accomplishes the task's purpose
The tests appropriately test the new code, including edge cases
You have tried to break the code

Reviewer 2:

Name:

Code is maintainable and reusable, reuses existing code and infrastructure
where appropriate, and accomplishes the task's purpose
The tests appropriately test the new code, including edge cases
You have tried to break the code

Jammjammjamm · 2020-08-31T15:50:26Z

lib/app/sequence_base.rb

@@ -743,6 +743,8 @@ def check_resource_against_profile(resource, resource_type, specified_profile =
      end

      def resolve_path(elements, path)
+        return Inferno::FHIRPATH_EVALUATOR.evaluate(elements, path) if Inferno::FHIRPATH_EVALUATOR


The validator example avoids these if statements by having two versions of the validator. I think the remaining logic in this method should be moved to an internal fhirpath evaluator so that these methods can just call a method on whichever evaluator was instantiated at boot.

Do you know if resolve_element_from_path is the same as calling resolve_path and returning the first element that satisfies the block predicate (if it exists)? I think that resolve_element_from_path does some short-circuiting to skip unnecessary computation, but other than that, should the result be the same?

I wanted to ask before I make a common interface between the internal and external FHIRPath evaluator because I want to know if there's anything wrong with defining resolve_element_from_path in terms of resolve_path.

I'm not 100% sure. @czh-orz ?

lib/app/utils/fhirpath_evaluator.rb

Jammjammjamm · 2020-08-31T15:54:57Z

lib/app/sequence_base.rb

@@ -754,6 +756,11 @@ def resolve_path(elements, path)
      end

      def resolve_element_from_path(element, path)
+        if Inferno::FHIRPATH_EVALUATOR
+          elements = Inferno::FHIRPATH_EVALUATOR.evaluate(element, path)
+          return block_given? ? elements.find { |el| yield(el) } : elements.first


If this is behavior we need the evaluator to perform, I think it belongs there as a separate method rather than here.

lib/app/utils/fhirpath_evaluator.rb

Jammjammjamm · 2020-08-31T16:02:38Z

lib/app/utils/fhirpath_evaluator.rb

+    end
+
+    # @param fhirpath_url [String] the base url for the FHIRPath /evaluate endpoint
+    def initialize(fhirpath_url)


Looking over this branch is exposing some issues that are also present in the validator implementation. This class (as well as both of the validator classes) has no non-global state, so it could just be a module instead. This isn't necessarily something that needs fixing, just something that jumped out at me.

Jammjammjamm

This is looking pretty good. Still need to do some testing, which I won't be able to get to until tomorrow.

lib/app/utils/basic_fhirpath_evaluator.rb

Jammjammjamm · 2020-09-04T13:25:58Z

I noticed this difference when running against our demo server:

dev:

FI-930:

jason-crowley · 2020-09-04T13:30:01Z

I noticed this difference when running against our demo server:

dev:

FI-930:

Yes, I came across that issue as well. It seems to be non-deterministic which outcome happens, and I was able to reproduce that skip result on development by re-running the test multiple times.

EDIT: Just now I got this screenshot on development:

Jammjammjamm reviewed Aug 31, 2020

View reviewed changes

jason-crowley force-pushed the FI-930-fhirpath-for-searches branch from 3ef696e to 6ff4b5b Compare August 31, 2020 17:12

jason-crowley added the WIP Work in progress label Aug 31, 2020

jason-crowley force-pushed the FI-930-fhirpath-for-searches branch from bf1ccf4 to 02a34e3 Compare September 3, 2020 13:20

jason-crowley removed the WIP Work in progress label Sep 3, 2020

jason-crowley requested review from Jammjammjamm, okeefm, radamson and a user September 3, 2020 13:21

Jammjammjamm reviewed Sep 3, 2020

View reviewed changes

lib/app/utils/basic_fhirpath_evaluator.rb Outdated Show resolved Hide resolved

Jammjammjamm approved these changes Sep 4, 2020

View reviewed changes

okeefm removed request for okeefm and radamson September 8, 2020 17:17

ghost approved these changes Sep 16, 2020

View reviewed changes

Jammjammjamm added 2 commits October 2, 2020 14:19

support fhirpath for search generation

def7daf

regenerate us core sequences

a40c0fd

Jammjammjamm force-pushed the FI-930-fhirpath-for-searches branch from a771fb3 to a40c0fd Compare October 2, 2020 18:51

Jammjammjamm merged commit 9e2e2f7 into development Oct 2, 2020

Jammjammjamm deleted the FI-930-fhirpath-for-searches branch October 2, 2020 19:30

radamson mentioned this pull request Dec 15, 2020

Version 2.11.0 Release #514

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FI-930: Support FHIRPath for generating searches #490

FI-930: Support FHIRPath for generating searches #490

jason-crowley commented Aug 31, 2020 •

edited by okeefm

Loading

Jammjammjamm Aug 31, 2020

jason-crowley Sep 2, 2020

Jammjammjamm Sep 2, 2020

Jammjammjamm Aug 31, 2020

Jammjammjamm Aug 31, 2020

Jammjammjamm left a comment

Jammjammjamm commented Sep 4, 2020

jason-crowley commented Sep 4, 2020 •

edited

Loading

FI-930: Support FHIRPath for generating searches #490

FI-930: Support FHIRPath for generating searches #490

Conversation

jason-crowley commented Aug 31, 2020 • edited by okeefm Loading

Jammjammjamm Aug 31, 2020

Choose a reason for hiding this comment

jason-crowley Sep 2, 2020

Choose a reason for hiding this comment

Jammjammjamm Sep 2, 2020

Choose a reason for hiding this comment

Jammjammjamm Aug 31, 2020

Choose a reason for hiding this comment

Jammjammjamm Aug 31, 2020

Choose a reason for hiding this comment

Jammjammjamm left a comment

Choose a reason for hiding this comment

Jammjammjamm commented Sep 4, 2020

jason-crowley commented Sep 4, 2020 • edited Loading

jason-crowley commented Aug 31, 2020 •

edited by okeefm

Loading

jason-crowley commented Sep 4, 2020 •

edited

Loading