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

X-FHIR-Query support for variable extension #2076

Merged

Conversation

FikriMilano
Copy link
Collaborator

@FikriMilano FikriMilano commented Jul 17, 2023

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

Fixes #2075

Description

  • Allows searching FHIR resources in variable-extension by using X-FHIR-Query
  • When creating X-FHIR-Query from Expression, the code prioritize the pairs from variable-extension instead of from launch-context-extension, this is to prevents a bug mentioned in this comment
  • Use suspend functions for all functions that are using/depends on variable extraction, since doing a variable extraction could cause a database operation that happens because of running the X-FHIR-Query

Alternative(s) considered
N/A

Type
Feature

Screenshots (if applicable)

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).

@santosh-pingle
Copy link
Collaborator

I am reviewing it, thanks!

@FikriMilano
Copy link
Collaborator Author

@santosh-pingle @jingtang10
Can we first merge #2066, before we merge this PR?

I am anticipating merge conflicts from #2066, so I pulled from there and integrate it in this PR.

And I would like to ensure that @MJ1998's contribution is duly noted from #2066.

- Use viewModelScope for calculated-expression
- Move the runBlocking to QuestionnaireItemViewItem for cqf-expression
- Rename questionnaireVariablesMap to questionnaireVariableMap
Notifies the flow which will update the data stream when updateDependentQuestionnaireResponseItems finished running
- updateDependentQuestionnaireResponseItems can evaluate
  X-FHIR-Query expression which can delay the update of UI
  for 1-2 seconds depending on the retrieved resource size.

- In the case of clicking next page after selecting an
  answer that shows the next pages based on skip logic
  (tapping this and that very quickly). We notify the UI
  pages immediately, then once the X-FHIR-Query expression
  is processed (1-2 seconds), we notify the UI again.
  Without this changes, when the user taps next page, the
  next pages that is supposed to be shown by skip logic
  will be shown late enough that the user don't see it,
  but if the user taps previous page button, user will see
  the shown pages decided by skip logic. And that looks
  like a bug.
@FikriMilano FikriMilano force-pushed the 2075-x-fhir-query-support-for-variable branch from c8f8165 to 9d33869 Compare August 8, 2023 09:17
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Aug 11, 2023
- X-FHIR-Query support for variable extension PR google#2076
- Optimize flatten and descendent traversal PR google#2079
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Aug 28, 2023
    - With unmerged PR google#2032
    - With unmerged PR #1
    - With unmerged PR google#1669
    - With unmerged PR google#2132
    - With unmerged PR #9
    - With unmerged PR google#2076
    - With unmerged PR #10
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 7, 2023
        - With unmerged PR google#2032
        - With unmerged PR #1
        - With unmerged PR google#1669
        - With unmerged PR google#2132
        - With unmerged PR #9
        - With unmerged PR google#2076
        - With unmerged PR #10
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 7, 2023
        - With unmerged PR google#2032
        - With unmerged PR #1
        - With unmerged PR google#1669
        - With unmerged PR google#2132
        - With unmerged PR #9
        - With unmerged PR google#2076
        - With unmerged PR #10
@FikriMilano
Copy link
Collaborator Author

FikriMilano commented Sep 12, 2023

Todo:

  1. refactor, think of ways on how the suspend function will work, in the context of enable when expression, because it uses the same evaluate method. Need to do this since enable when expression can access variable #2132 was merged
  2. fix merge conflict from the same other PR enable when expression can access variable #2132

@FikriMilano FikriMilano marked this pull request as draft September 12, 2023 12:11
@FikriMilano FikriMilano force-pushed the 2075-x-fhir-query-support-for-variable branch from 7c8a5c2 to a277d01 Compare November 23, 2023 14:43
@FikriMilano
Copy link
Collaborator Author

@MJ1998 @omarismail94 this is ready for review

@MJ1998
Copy link
Collaborator

MJ1998 commented Nov 27, 2023

Thanks @FikriMilano

A few requests as this is difficult to follow:-

  1. Could you resolve the comments.
  2. Also could you describe your solution a little more in this PR's descripiton

@FikriMilano FikriMilano marked this pull request as draft November 30, 2023 09:41
@FikriMilano
Copy link
Collaborator Author

hold up, let me try to use _count, and see whether it can prevent the resolver to search everything, when there are no parameter values supplied

Better alternative is to limit the received
resource by using the _count common
parameter.

Example:
Patient?name=&_count=2
@FikriMilano
Copy link
Collaborator Author

ok, we don't need the hasMissingParamValue.
adding the query with the _count param would prevent the resolver to search everything.

Patient?name=&_count=2

@FikriMilano FikriMilano marked this pull request as ready for review December 3, 2023 04:38
@FikriMilano
Copy link
Collaborator Author

@MJ1998 @omarismail94
I've resolved the comments and updated the PR description

@jingtang10
Copy link
Collaborator

@omarismail94

there is a bug in createXFhirQueryFromExpression method.

where if we try to combine both fhirPathsEvaluatedPairs and variablesEvaluatedPairs, then fold the expression to replace the string template into the right evaluated value, the resulting xFhirQuery string gives the wrong result.

that happens because of the same variable name from both fhirPathsEvaluatedPairs and variablesEvaluatedPairs, and the variable from fhirPathsEvaluatedPairs are given first priority over the one from variablesEvaluatedPairs.

the one from fhirPathsEvaluatedPairs are empty, while what we need was from variablesEvaluatedPairs.

the resulting xFhirQuery has a search parameter, but that search parameter value is empty because of this. This will cause the fhirEngine resolver to search all resources of that type mentioned in the xFhirQuery. This can be prevented by using the _count param.

Example:

fhirPathsEvaluatedPairs contains: [{a}, “”] variablesEvaluatedPairs contains: [{a}, “47626”]

Resulting xFhirQuery: Patient?_id= The expected and correct xFhirQuery: Patient?_id=47626

Prevention 1, xFhirQuery: Patient?_count =1&_id= Prevention 2, xFhirQuery: Patient?_id=NOTHING

@FikriMilano can you create an issue here?

@FikriMilano
Copy link
Collaborator Author

@omarismail94
there is a bug in createXFhirQueryFromExpression method.
where if we try to combine both fhirPathsEvaluatedPairs and variablesEvaluatedPairs, then fold the expression to replace the string template into the right evaluated value, the resulting xFhirQuery string gives the wrong result.
that happens because of the same variable name from both fhirPathsEvaluatedPairs and variablesEvaluatedPairs, and the variable from fhirPathsEvaluatedPairs are given first priority over the one from variablesEvaluatedPairs.
the one from fhirPathsEvaluatedPairs are empty, while what we need was from variablesEvaluatedPairs.
the resulting xFhirQuery has a search parameter, but that search parameter value is empty because of this. This will cause the fhirEngine resolver to search all resources of that type mentioned in the xFhirQuery. This can be prevented by using the _count param.
Example:
fhirPathsEvaluatedPairs contains: [{a}, “”] variablesEvaluatedPairs contains: [{a}, “47626”]
Resulting xFhirQuery: Patient?_id= The expected and correct xFhirQuery: Patient?_id=47626
Prevention 1, xFhirQuery: Patient?_count =1&_id= Prevention 2, xFhirQuery: Patient?_id=NOTHING

@FikriMilano can you create an issue here?

here's the ticket #2451

@FikriMilano
Copy link
Collaborator Author

@jingtang10 this PR is ready to merge

@jingtang10 jingtang10 merged commit 6964271 into google:master Feb 27, 2024
3 checks passed
@jingtang10 jingtang10 deleted the 2075-x-fhir-query-support-for-variable branch February 27, 2024 14:13
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.

X-FHIR-Query support for variable extension
5 participants