Test "where as name.family" has incorrect expectations

FHIR / sql-on-fhir-v2

This project provides the source for the SQL on FHIR v2.0 Implementation Guide

https://build.fhir.org/ig/FHIR/sql-on-fhir-v2/

MIT License

105 stars 28 forks source link

Test "where as name.family" has incorrect expectations #198

Closed johngrimes closed 8 months ago

johngrimes commented 9 months ago

The following test is incorrect, as it does not match the definition of where.path in the specification:

A series of zero or more FHIRPath constraints to filter resources for the view. Every constraint must evaluate to true for the resource to be included in the view.

I would expect this to return an empty result, or possibly even an error considering that the type of the name.family element is String and is not possible to be Boolean.

{
  "title": "where as name.family",
  "view": {
    "resource": "Patient",
    "select": [
      {
        "column": [
          {
            "name": "id",
            "path": "id"
          }
        ],
        "type": "column"
      }
    ],
    "where": [
      {
        "path": "name.family"
      }
    ],
    "type": "select"
  },
  "expect": [
    {
      "id": "pt1"
    },
    {
      "id": "pt2"
    }
  ]
}

Yngwarr commented 9 months ago

Yes, you're right, as per FHIRPath documentation.

If the result of evaluating the condition is other than a single boolean value, the evaluation will end and signal an error to the calling environment, consistent with singleton evaluation of collections behavior.

Thanks for pointing out, test fixed in e89eb10, an implementation fix will follow soon.

mput commented 9 months ago

Agreed on this. Additionally, I have corrected all instances where the where.path expression depended on a true or an empty value. Therefore, deceased.ofType(boolean) = true will result in an error, but deceased.ofType(boolean).exists() and deceased.ofType(boolean) = true will not.

johngrimes commented 9 months ago

Hmm, I'm actually not sure if the former example should be an error.

Perhaps this should be an item for discussion.

mput commented 9 months ago

@johngrimes Yes, that's why I highlighted it.

johngrimes commented 9 months ago

The spec currently says:

ViewDefinition.where

A series of zero or more FHIRPath constraints to filter resources for the view. Every constraint must evaluate to true for the resource to be included in the view.

ViewDefinition.where.path

A FHIRPath expression that defines a filter that must evaluate to true for a resource to be included in the output. The input context is the collection of resources of the type specified in the resource element. Constants defined in Reference({constant}) can be referenced as %[name]. The result of the expression must of type Boolean.

Maybe the only change we need is to remove "The result of the expression must of type Boolean."

mput commented 9 months ago