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

add support for inheritance for FHIRPath is and as operators #3974

Closed
PrasannaHegde1 opened this issue Sep 14, 2022 · 5 comments
Closed

add support for inheritance for FHIRPath is and as operators #3974

PrasannaHegde1 opened this issue Sep 14, 2022 · 5 comments
Assignees
Labels
bug Something isn't working P3 Priority 3 - Nice To Have

Comments

@PrasannaHegde1
Copy link
Collaborator

Describe the bug
add support for inheritance for FHIRPath is and as operators
For example: code data type is a sub type of string, and the validation of expression "code.is(string)" should be successful.

For more information see - https://chat.fhir.org/#narrow/stream/179266-fhirpath/topic/FHIRPath.20.60is.60.20and.20.60as.60.20with.20FHIR.20types

Environment
Which version of LinuxForHealth FHIR Server? 5.0

To Reproduce
Steps to reproduce the behavior:

  1. Go to 'FHIR/fhir-path/src/test/java/org/linuxforhealth/fhir/path/test/FHIRPathIsTest.java'
  2. Create a code data type variable and validate the expression code.is(string)" An example is given below
     Code code = Code.of("100");
     FHIRPathEvaluator evaluator = FHIRPathEvaluator.evaluator();
     Collection<FHIRPathNode> result = evaluator.evaluate(code, "is(string)");
     assertTrue(getSingleton(result, FHIRPathBooleanValue.class)._boolean());
  1. See error - The assertion is not successful

Expected behavior
evaluator.evaluate(code, "is(string)") should return true

@PrasannaHegde1 PrasannaHegde1 added the bug Something isn't working label Sep 14, 2022
@lmsurpre
Copy link
Member

For testing, we should add positive and negative test cases to the "official" test suite (tests-fhir-r4.xml under fhir-path/src/test/resources). Then, if no one else has don it yet, we should offer to contribute these back upstream.

@lmsurpre
Copy link
Member

lmsurpre commented Sep 15, 2022

Per https://chat.fhir.org/#narrow/stream/179166-implementers/topic/parsing.20polymorphism we'll need to change our ConstraintGenerator in order to make this change without breaking the existing (correct) behavior of our profile validator.

That is because we want code.is(FHIR.string) to return true, yet we also want a way to say things like "this element must be a FHIR string, and cannot be a string subtype)".

@lmsurpre
Copy link
Member

lmsurpre commented Sep 16, 2022

Its the difference between

  • a.getClass() == String.class; vs
  • a instanceof String

Two options:

A. instead of using a.is(FHIR.string), generate code like this from ConstraintGenerator: a.type().namespace = 'FHIR' and a.type().name = 'string'

B. define a new function, maybe isTypeEqual(typeSpecifier), and use that in our generated constraints instead of is(typeSpecifier)

the advantage of of B is that its a bit more concise

@lmsurpre lmsurpre added the P3 Priority 3 - Nice To Have label Sep 16, 2022
@lmsurpre
Copy link
Member

lmsurpre commented Oct 12, 2022

to QA: manually run a handful of fhirpath expressions using is and as

maybe also confirm the new isTypeEqual and asTypeEqual functions

@lmsurpre
Copy link
Member

The task was implemented as requested, but after looking at this again I'm wondering if we should also introduce ofTypeEqual and use that in our generated expressions instead of asTypeEqual.

The background on this one is pretty interesting:
many of the fhirpath expressions in the base spec (and a few of the constraints) use the fhirpath as function to convert a list of input items to a filtered list of output items of a given type (or subtypes).

However, the official as documentation says this about it:

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

Furthermore, the spec lists the as function as deprecated, although I think its more to favor the as operator instead (which is not retired).

Anyway, I think most of those expressions and FHIRPath constraints should probably be using the ofType function instead of as.

I think its too much of a breaking change to make our as function spec compliant at this point, however I do think that we could introduce ofTypeEqual and use that instead of asTypeEqual in our generated constraints. I will open a separate issue for that and consider this one closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P3 Priority 3 - Nice To Have
Projects
None yet
Development

No branches or pull requests

2 participants