google / android-fhir

The Android FHIR SDK is a set of Kotlin libraries for building offline-capable, mobile-first healthcare applications using the HL7® FHIR® standard on Android.
https://google.github.io/android-fhir/
Apache License 2.0
463 stars 241 forks source link

X-FHIR-Query support for variable extension #2076

Closed FikriMilano closed 3 months ago

FikriMilano commented 10 months ago

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

Fixes #2075

Description

Alternative(s) considered N/A

Type Feature

Screenshots (if applicable)

Checklist

santosh-pingle commented 10 months ago

I am reviewing it, thanks!

FikriMilano commented 10 months ago

@santosh-pingle @jingtang10 Can we first merge https://github.com/google/android-fhir/pull/2066, before we merge this PR?

I am anticipating merge conflicts from https://github.com/google/android-fhir/pull/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 https://github.com/google/android-fhir/pull/2066.

FikriMilano commented 8 months ago

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 https://github.com/google/android-fhir/pull/2132 was merged
  2. fix merge conflict from the same other PR https://github.com/google/android-fhir/pull/2132
omarismail94 commented 7 months ago

@FikriMilano any update on this PR?

FikriMilano commented 7 months ago

@omarismail94 let me address that tomorrow. Thanks for the follow-up.

FikriMilano commented 6 months ago

@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

MJ1998 commented 6 months ago

@FikriMilano Or what if we replace the value where it exists. The case where different value exists in both fhirPathsEvaluatedPairs and variablesEvaluatedPairs should ideally be considered as incorrect questionnaire authoring ? Update :- Case with different values will never arise. We can prioritize variablesEvaluatedPairs first.

FikriMilano commented 6 months ago

@MJ1998 will also add the filtering as discussed

EDIT: I take that back, this will break the feature where we supposed to replace the variable template with empty string, assuming the evaulated value is an empty string, that happens when the variable we need is from the fhirPathsEvaluatedPairs instead.

FikriMilano commented 6 months ago

@MJ1998 @omarismail94 this is ready for review

MJ1998 commented 6 months ago

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 commented 6 months ago

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

FikriMilano commented 6 months ago

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 commented 6 months ago

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

jingtang10 commented 3 months ago

@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 commented 3 months ago

@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 https://github.com/google/android-fhir/issues/2451

FikriMilano commented 3 months ago

@jingtang10 this PR is ready to merge