FHIR Chat · Potential bug · ibm

Stream: ibm

Topic: Potential bug

Lee Surprenant (Nov 06 2019 at 16:57):

Changing to "strict" mode by default has highlighted how bad our e2e search tests are. Most of them are just checking for >=1 result, so the fact we were returning all the results for unexpected issues was masking some problems.

Lee Surprenant (Nov 06 2019 at 16:58):

I think our JDBC search tests are pretty good, so I'm not sure how important it is to improve all these e2e tests

Lee Surprenant (Nov 06 2019 at 16:58):

but there is a related issue i'm looking at right now which I believe is either a bug in our fhirpath engine or in the spec-defined search criteria

Lee Surprenant (Nov 06 2019 at 16:59):

the search parameter in question is Observation-value-quantity and the expression is (Observation.component.value as Quantity) | (Observation.component.value as SampledData)

Lee Surprenant (Nov 06 2019 at 16:59):

i would expect that to extract the values from the following sample Observation (Observation1.json):

{
        "resourceType" : "Observation",
        "status" : "final",
        "category" : [{
            "coding" : [ {
                "system" : "http://terminology.hl7.org/CodeSystem/observation-category",
                "code" : "vital-signs",
                "display" : "Vital Signs"
            } ]
        }],
        "code" : {
            "coding" : [ {
                "system" : "http://loinc.org",
                "code" : "55284-4",
                "display" : "Blood pressure systolic and diastolic"
            } ]
        },
        "component" : [ {
            "code" : {
                "coding" : [ {
                    "system" : "http://loinc.org",
                    "code" : "8459-0",
                    "display" : "Systolic blood pressure--sitting"
                } ]
            },
            "valueQuantity" : {
                "value" : 125.0,
                "unit" : "mmHg"
            }
        }, {
            "code" : {
                "coding" : [ {
                    "system" : "http://loinc.org",
                    "code" : "8453-3",
                    "display" : "BP dias--sitting"
                } ]
            },
            "valueQuantity" : {
                "value" : 80.0,
                "unit" : "mmHg"
            }
        } ]
}

Lee Surprenant (Nov 06 2019 at 17:00):

but its not

Lee Surprenant (Nov 06 2019 at 17:02):

@John Timm does the fhirpath look right to you? component is an array, but FHIRPath handles that automatically, right?

Lee Surprenant (Nov 06 2019 at 17:20):

OK, I think I figured this one out. Its failing to select these because of the isSingleton(nodes) check in FHIRPathEvaluator.visitTypeExpression
pasted image

Lee Surprenant (Nov 06 2019 at 17:37):

is it possible just to have that method call the is and as methods instead of duplicating the logic?

Lee Surprenant (Nov 06 2019 at 17:49):

Although I think the FHIRPath spec says "as" should have a single left-hand operand, I'm gonna treat this one like a bug on our side (since the X as Y pattern is so common in search expressions)

Lee Surprenant (Nov 06 2019 at 18:02):

opened https://github.com/IBM/FHIR/issues/370

John Timm (Nov 06 2019 at 18:04):

Per the FHIRPath Specification (Section 6.3):
http://hl7.org/fhirpath/2018Sep/index.html#types
"If the input collections contains more than one item, the evaluator will throw an error. In all other cases this function returns the empty collection."

Lee Surprenant (Nov 06 2019 at 18:06):

yes, i included that in the "additional context" on the bug report

Lee Surprenant (Nov 06 2019 at 19:12):

included the "fix" in https://github.com/IBM/FHIR/pull/369

Lee Surprenant (Nov 06 2019 at 21:03):

@John Timm interesting build error after making that change to ensure is always operates on a singleton:

[ERROR] Tests run: 13, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 9.016 s <<< FAILURE! - in TestSuite
[ERROR] testInvalidBundleContainedInDomainResource(com.ibm.fhir.validation.test.BundleValidationTest)  Time elapsed: 0.057 s  <<< FAILURE!
java.lang.Error: An error occurred while validating constraint: bdl-11 with location: (base) and expression: type = 'document' implies entry.first().resource.is(Composition) at path: Bundle
    at com.ibm.fhir.validation.test.BundleValidationTest.testInvalidBundleContainedInDomainResource(BundleValidationTest.java:237)
Caused by: com.ibm.fhir.model.path.exception.FHIRPathException: java.lang.IllegalArgumentException: Input collection has 0 items, but only 1 is allowed
    at com.ibm.fhir.validation.test.BundleValidationTest.testInvalidBundleContainedInDomainResource(BundleValidationTest.java:237)
Caused by: java.lang.IllegalArgumentException: Input collection has 0 items, but only 1 is allowed
    at com.ibm.fhir.validation.test.BundleValidationTest.testInvalidBundleContainedInDomainResource(BundleValidationTest.java:237)

Lee Surprenant (Nov 06 2019 at 21:04):

i guess we should just return empty() if the input collection is empty()?

Lee Surprenant (Nov 06 2019 at 21:04):

instead of throwing

John Timm (Nov 06 2019 at 21:14):

Agreed with the empty() for empty() comment

Lee Surprenant (Nov 06 2019 at 21:14):

i don't think fhirpath spec even covers that

Lee Surprenant (Nov 06 2019 at 21:15):

If there is more than one item in the input collection, the evaluator will throw an error.

Lee Surprenant (Nov 06 2019 at 21:15):

so i guess it should fall through to the next comment:

Otherwise, this operator returns the empty collection.

Lee Surprenant (Nov 06 2019 at 21:16):

but its weird to think that a left-hand operator can be empty collection

John Timm (Nov 06 2019 at 21:17):

If it's not required, then it could be not present / empty. Not all expressions have the element.exists() implies guard.

Lee Surprenant (Nov 06 2019 at 21:18):

proposed fix:
pasted image

Lee Surprenant (Nov 06 2019 at 21:18):

so we'd exit early for both as and is

Lee Surprenant (Nov 06 2019 at 21:19):

I'm hoping this takes care of the build failures

Lee Surprenant (Nov 06 2019 at 21:23):

actually, this messes up the indentLevel

Lee Surprenant (Nov 06 2019 at 21:23):

i assume thats just used for debug purposes?

John Timm (Nov 06 2019 at 21:24):

Please make sure to do a indentLevel-- before your early exit.

John Timm (Nov 06 2019 at 21:24):

Or alter the code to maintain a single exit.

Lee Surprenant (Nov 06 2019 at 21:27):

will do

Lee Surprenant (Nov 06 2019 at 21:27):

but now i'm having second-thoughts on returning empty list of is

Lee Surprenant (Nov 06 2019 at 21:27):

for as it seems right

Lee Surprenant (Nov 06 2019 at 21:27):

but maybe is should return SINGLETON_FALSE ?

Lee Surprenant (Nov 06 2019 at 21:28):

thats what seems to match the prior behavior

John Timm (Nov 06 2019 at 21:28):

There are other examples of functions that return empty if the input is empty. I think we should follow that. returning false implies something was actually checked. an empty input doesn't have anything that can be checked. i think returning
empty makes more sense.

Lee Surprenant (Nov 06 2019 at 21:30):

ok, thanks for the confirm. trying it out now

Lee Surprenant (Nov 06 2019 at 21:36):

on the indent thing, does the indent level matter when we visit the expression? assuming yes, I just popped a indentLevel--; in before the early exit return

Lee Surprenant (Nov 06 2019 at 21:36):

if indentLevel didn't matter for the visit, i would have just moved the early exit block above the indentLevel++

Lee Surprenant (Nov 06 2019 at 21:38):

https://github.com/IBM/FHIR/pull/369 is updated

John Timm (Nov 06 2019 at 21:52):

we call visit a couple of times after the indentLevel++ statement so I think the best solution is to put indentLevel-- right before return empty()

Lee Surprenant (Nov 06 2019 at 21:58):

ok, cool, thats what i did

Last updated: Apr 12 2022 at 19:14 UTC