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

EnableWhenExpression context literal fhirpath supplement implementation #1957

Merged
merged 14 commits into from
May 4, 2023

Conversation

maimoonak
Copy link
Collaborator

@maimoonak maimoonak commented Apr 6, 2023

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #1958

Description
To make question fhirpath dynamic and have current questionnaire item we need to support fhirpath supplement context literal. This allow to get current questionnaire item as %context into fhirpath expressions on questionnaire items. Details for spec defined here https://build.fhir.org/ig/HL7/sdc/expressions.html#fhirpath-supplements .
This is a bug fix and the changes MAY affect existing use of enableWhenExpression where anyone tried to write expression for questionnaireResponse as $this. $this refers to current item under consideration aka %context as well. Old implementation might have entertained $this as questionnaireResponse. Hence anyone using expression wrongfully can have erratic behavior for their current enableWhenExpressions

Alternative(s) considered
Bug fix

Type
Choose one: Bug fix

Checklist

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

Thanks @maimoonak for this PR.

I'm having some trouble understanding what this PR is doing beyond wrapping fhir path engine functions in wrappers.

To support fhirpath supplements, we need to use FHIRPathEngineHostServices so that it can resolve %resource, %context etc. It is the service FHIRPathEngine uses when it sees a variable with % that it cannot resolve.

@jingtang10 jingtang10 assigned maimoonak and unassigned jingtang10 Apr 24, 2023
@jingtang10
Copy link
Collaborator

@omarismail94 fyi - we discussed this before.

@maimoonak
Copy link
Collaborator Author

Thanks @maimoonak for this PR.

I'm having some trouble understanding what this PR is doing beyond wrapping fhir path engine functions in wrappers.

There is a bug where we are not passing focusResource (which is recognized as %resource in expression) and baseResource (which is recognized as %context or $this in expression) separately into evaluate function when evaluating enableWhen-expression. This leads to a missing feature i.e. use of %context to get current questionnaireResponseItem in expression as defined here https://build.fhir.org/ig/HL7/sdc/expressions.html#fhirpath-supplements and a bug where user can use %resource, %context, $this all constants for questionnaireResponse which is wrong.

To support fhirpath supplements, we need to use FHIRPathEngineHostServices so that it can resolve %resource, %context etc. It is the service FHIRPathEngine uses when it sees a variable with % that it cannot resolve.

As per implementation here org.hl7.fhir.r4.utils.FHIRPathEngine#resolveConstant(ExecutionContext, String, boolean, ExpressionNode), the constants which are part of default fhir spec (https://hl7.org/fhir/fhirpath.html#variables and https://hl7.org/fhir/fhirpath.html#vars) i.e. %constant_name are resolved automatically by engine. We can not provide implementation for those. If any variable is not reserved one and is provided by specific IG, then we need to provide it via HostServices. Some of the examples of such cases are StructureMap extraction and SDC variable resolution.

See attached image to see the code flow.

Screenshot from 2023-04-26 16-59-36

Co-authored-by: Jing Tang <jingtang@google.com>
catalog/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@jingtang10 jingtang10 enabled auto-merge (squash) May 4, 2023 21:41
@jingtang10 jingtang10 requested a review from a team as a code owner May 4, 2023 21:44
@jingtang10 jingtang10 merged commit 70277d3 into google:master May 4, 2023
1 of 2 checks passed
@jingtang10 jingtang10 deleted the enablewhen-context branch May 4, 2023 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Support fhirpath supplement context literal
4 participants