FHIR Chat · AssumeValidRestReferences · implementers

Stream: implementers

Topic: AssumeValidRestReferences

Jens Villadsen (Jun 15 2020 at 07:31):

@Grahame Grieve I believe we've found a bug when using assumeValidRestReferences in the version biggybagged with HAPI 5.0.2, but I can't remember where the github repo is, where you prefer samples

Grahame Grieve (Jun 15 2020 at 07:58):

https://github.com/FHIR/fhir-test-cases/pulls

Jens Villadsen (Jun 15 2020 at 08:55):

and test cases for STU3? ... should I just create that folder?

Grahame Grieve (Jun 15 2020 at 16:47):

sure

Grahame Grieve (Jun 15 2020 at 16:47):

oh no - hang on. For validation test cases, just use /validation. you specify the version in manifest.json

Jens Villadsen (Jun 15 2020 at 23:16):

my case is a bit convoluted ... we're experiencing that the validator behaves a bit different depending on the context is it invoked in. When validating resources (in my setup this is done on all invocations automatically), it works like a charm, but when wrapping it in Parameters, like when calling $validate, it seems like the profile is not picked correctly in combination with assumeValidRestReferences.

When validated within a Parameters section (see example below), the error from the InstanceValidator (with assumeValidRestReferences set to true) is the following:

Parameters.parameter[0].resource.ofType(Communication).partOf[0],message=The type "CarePlan" implied by the reference URL http://careplan.fut.trifork.com/CarePlan/42 is not a valid Target for this element (must be one of [Resource])

I believe this error message is wrong as CarePlan is in fact a Resource, hence it should be legal. Also, I've looked into it. I believe profile checks that are declared within the parameters can be checked using the following (replacing this line https://github.com/hapifhir/org.hl7.fhir.core/blob/49c35b1de2bd40f880dfdca7029df5941be69c91/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/InstanceValidator.java#L3577 with this):

  var profiles = element.getChildren("meta")
                        .stream()
                        .map(c -> c.getChildren("profile"))
                        .filter(Objects::nonNull)
                        .flatMap(List::stream)
                        .map(Element.class::cast)
                        .map(Element::getValue)
                        .map(s -> this.context.fetchRawProfile(s))
                        .collect(Collectors.toList());
                if(profiles.isEmpty())
                    validateResource(hc, errors, resource, element, profile, idstatus, stack);
                else
                    // Any of the profiles may do ...?
                    validateResource(hc, errors, resource, element, profiles.get(0), idstatus, stack);

I've tested it manually - seems to work as intended. Let me know if you would like a PR, @Grahame Grieve .

---- example

{
  "resourceType": "Parameters",
  "parameter": [
    {
      "name": "resource",
      "resource": {
        "resourceType": "Communication",
        "meta": {
          "profile": [
            "http://ehealth.sundhed.dk/fhir/StructureDefinition/ehealth-message"
          ]
        },
        "extension": [
          {
            "url": "http://ehealth.sundhed.dk/fhir/StructureDefinition/ehealth-communication-recipientCareTeam",
            "valueReference": {
              "reference": "https://inttest.ehealth.sundhed.dk/organization/fhir/CareTeam/50"
            }
          },
          {
            "url": "http://ehealth.sundhed.dk/fhir/StructureDefinition/ehealth-restriction-category",
            "valueCodeableConcept": {
              "coding": [
                {
                  "system": "http://ehealth.sundhed.dk/cs/restriction-category",
                  "code": "None"
                }
              ]
            }
          }
        ],
        "partOf": [
          {
            "reference": "http://careplan.fut.trifork.com/CarePlan/42"
          }
        ],
        "status": "preparation",
        "category": [
          {
            "coding": [
              {
                "system": "http://ehealth.sundhed.dk/cs/message-category",
                "code": "message"
              }
            ],
            "text": "Besked"
          }
        ],
        "medium": [
          {
            "coding": [
              {
                "system": "http://terminology.hl7.org/CodeSystem/v3-ParticipationMode",
                "code": "WRITTEN"
              }
            ],
            "text": "WRITTEN"
          }
        ],
        "sender": {
          "reference": "http://inttest.ehealth.sundhed.dk/trifork-fhir-server/Patient/100"
        },
        "reasonCode": [
          {
            "coding": [
              {
                "system": "http://ehealth.sundhed.dk/cs/task-category",
                "code": "MissingMeasurementResolving"
              }
            ]
          }
        ],
        "payload": [
          {
            "contentString": "Hello world"
          }
        ]
      }
    }
  ]
}

Grahame Grieve (Jun 16 2020 at 00:31):

I don't think we're up to a PR yet. My interpretation of the error is that assumeValidRestReferences is true, but CarePlan is not a known resource - the StructureDefinition is not loaded?

Grahame Grieve (Jun 16 2020 at 00:32):

Also, I won't accept a PR to use streaming - the code looks nice but I typically find it impossible to debug

Jens Villadsen (Jun 16 2020 at 07:02):

Grahame Grieve said:

I don't think we're up to a PR yet. My interpretation of the error is that assumeValidRestReferences is true, but CarePlan is not a known resource - the StructureDefinition is not loaded?

If that was the case, I would have stumbled across the problem when I created the resource as I also validate it during the creation flow which is not the case. The problem is that the profile stated in the resource (in meta) in the parameter is not loaded in validateContains. My code fixes that - for loop or not.

Grahame Grieve said:

Also, I won't accept a PR to use streaming - the code looks nice but I typically find it impossible to debug

1) that is a matter of (your) taste. I am willing to convert it to a eg. 'for loop' but it would be at the cost of readability (you said "it looks nice". Readability is a quality of code that should never be underestimated).
2) if you were to accept it in its current state, debugging streams has been improved in IntelliJ (https://www.baeldung.com/intellij-debugging-java-streams)

Jens Villadsen (Jun 16 2020 at 07:09):

as for loop:

for (Element c : element.getChildren("meta")) {
                    List<Element> children = c.getChildren("profile");
                    if (children != null) {
                        for (Element element1 : children) {
                            Element element2 = element1;
                            String s = element2.getValue();
                            StructureDefinition structureDefinition = this.context.fetchRawProfile(s);
                            profiles.add(structureDefinition);
                        }
                    }
                }

Jens Villadsen (Jun 16 2020 at 11:04):

@Grahame Grieve comments?

Jens Villadsen (Jun 17 2020 at 09:21):

Do you follow me on the argument:

If that was the case, I would have stumbled across the problem when I created the resource as I also validate it during the creation flow which is not the case. The problem is that the profile stated in the resource (in meta) in the parameter is not loaded in validateContains. My code fixes that - for loop or not.

Grahame Grieve (Jun 17 2020 at 21:35):

I'll tell you where I will accept a PR: if you add a test case that demonstrates the problem by failing to validate properly to https://github.com/FHIR/fhir-test-cases

Jens Villadsen (Jun 18 2020 at 13:46):

@Grahame Grieve sure - I would gladly do so, but that repo looks like a bunch of test files - I can't see any unit tests there - and AFAIK, I can't toggle the assumeValidRestReferences from commandline which would be needed, right?

Jens Villadsen (Jun 18 2020 at 13:49):

I think it would be easier for me to show the bug if I make a github project myself and write it as unit tests. Would you be willing to have a look at that?

Jens Villadsen (Jun 18 2020 at 13:49):

(I promise to keep it simple)

Grahame Grieve (Jun 18 2020 at 17:43):

no, use the test cases. The file validation\manifest.json drives the unit tests - the Java class org.hl7.fhir.validation.tests.ValidationTests executes them. "assumeValidRestReferences" : true is the equivalent

Jens Villadsen (Jun 18 2020 at 21:17):

Got it

Jens Villadsen (Jun 25 2020 at 12:46):

hmmm .... this issue is not reproducible using the fhir-test-cases repo

Grahame Grieve (Jun 25 2020 at 12:49):

that indicates that the validator is working properly and the problem is somewhere else?

Jens Villadsen (Jun 25 2020 at 13:12):

maybe the context that the validator is started in - .... I'll keep digging ... or maybe just right in front of the screen ... I'll keep you posted

Jens Villadsen (Jul 08 2020 at 11:21):

ok @Grahame Grieve - I'm pretty sure I've found something now. It's not directly related, but it is a bug. When validating a Communication resource with eg. a Communication.topic, where the topic is not constrained in the profile, the type returned on https://github.com/hapifhir/org.hl7.fhir.core/blob/f40f012dfb42d6579e8d3eb02c1b9a170642eb0e/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/InstanceValidator.java#L2463 when using "assumeValidRestReferences", will be a reference to a "Resource". When an instance of a Communication that eg. refers to an EpisodeOfCare gets validated, it will end up failing with eg.

The type "EpisodeOfCare" implied by the reference URL https://careplan.inttest.ehealth.sundhed.dk/fhir/EpisodeOfCare/26869 is not a valid Target for this element (must be one of [Resource])

Jens Villadsen (Jul 08 2020 at 11:23):

while the github link points to latest, this is also present when using HAPI 5.0.2 as it uses the 5.0.0 version

Grahame Grieve (Jul 08 2020 at 11:53):

yes that looks like a bug. do you have time to turn it into a test case for me? - that is, a profile and an instance that passes and one that fails

Jens Villadsen (Jul 08 2020 at 12:00):

would you like it as a unit test or as a part of https://github.com/FHIR/fhir-test-cases

Grahame Grieve (Jul 08 2020 at 12:03):

I will put it in https://github.com/FHIR/fhir-test-cases but I can put it in there if you just create the relevant resources. But if you want to make it as a PR, you put it it /validator and add it to /validator/manifest.xml

Jens Villadsen (Jul 08 2020 at 12:11):

I'll give it a go later today

Grahame Grieve (Jul 08 2020 at 12:11):

thx.

Jens Villadsen (Jul 08 2020 at 12:23):

(I'm guessing I'm the only person currently using this feature then :) )

Grahame Grieve (Jul 08 2020 at 12:28):

no you're not. You're probably the only one using that combination

Jens Villadsen (Jul 08 2020 at 19:12):

awesome

Jens Villadsen (Jul 08 2020 at 19:56):

Hmmm ... before I add my examples - I'm seeing other stuff as well: image.png

Grahame Grieve (Jul 08 2020 at 19:59):

well, I'm not surprised. how can you fix the value of a Reference?

Jens Villadsen (Jul 08 2020 at 20:00):

call it a monkey test in forge ;)

Grahame Grieve (Jul 08 2020 at 20:01):

well, you found a corner case i had not implemented because my time is limited

Jens Villadsen (Jul 08 2020 at 20:01):

its mostly the error message here: the IG is v4 - so how come there's a message saying something about an r5 model

Jens Villadsen (Jul 08 2020 at 20:01):

(i know internally its v5)

Grahame Grieve (Jul 08 2020 at 20:01):

because internally it's all r5 and you've managed to produce an internal error message

Jens Villadsen (Jul 08 2020 at 20:21):

https://github.com/FHIR/fhir-test-cases/pull/63 - I'm not entirely sure on the syntax in manifest file regarding the errors. There should be no errors in my profile - only in the invalid example

Jens Villadsen (Jul 08 2020 at 20:27):

And FWIW - I hope your fix is not limited to r4, as I need it to work in STU3

Grahame Grieve (Jul 08 2020 at 20:46):

so your PR says taht mycommunication.invalid.json is invalid against the base spec, but valid against the profile. That's a super rare combination, so I assume you meant the opposite

Jens Villadsen (Jul 08 2020 at 20:47):

yep

Jens Villadsen (Jul 08 2020 at 20:48):

while it may be super rare, i guess it would also be super useless ;)

Grahame Grieve (Jul 08 2020 at 20:51):

I get these 4 errors validating against the profile:

ERROR: Communication.basedOn[0].reference: The element reference is present in the instance but not allowed in the applicable fixed value specified in profile
ERROR: Communication.basedOn[0].type: Missing element "type" - required by fixed value assigned in profile http://hl7.dk/fhir/core/StructureDefinition/MyCommunication
ERROR: Communication.basedOn[0]: The type "Patient" implied by the reference URL Patient/pat-good is not a valid Target for this element (must be one of [Resource])
ERROR: Communication.about[0]: The type "Patient" implied by the reference URL Patient/pat-good is not a valid Target for this element (must be one of [Resource])

Jens Villadsen (Jul 08 2020 at 20:52):

nr.3 and 4 are the ones that hit me in production

Grahame Grieve (Jul 08 2020 at 20:53):

well, ok. but this can never be valid if the reference is prohibited

Jens Villadsen (Jul 08 2020 at 20:54):

sry - no ... nr 4 hit me in production

Jens Villadsen (Jul 08 2020 at 20:55):

which errors were present with mycommunication.invalid.json

Jens Villadsen (Jul 08 2020 at 20:55):

?

Grahame Grieve (Jul 08 2020 at 20:56):

I'm looking at the valid one here

Jens Villadsen (Jul 08 2020 at 20:56):

oh

Jens Villadsen (Jul 08 2020 at 20:57):

the 'basedOn' stuff was made up spontaneously

Jens Villadsen (Jul 08 2020 at 20:58):

disregard those ... even though nr. 3 seems like its a valid bug

Jens Villadsen (Jul 08 2020 at 21:02):

and nr.1 ... what are the rules again on fixed stuff? it seems weird that you can specify that the value is fixed without stating the fixed value itself - is that intentional?

Jens Villadsen (Jul 08 2020 at 21:08):

nr. 2 seems redundant/void in the case where type is computed due to assumeValidRestReferences

Jens Villadsen (Jul 08 2020 at 21:09):

so - if my understanding of fixed is correct, I would say that nr. 1 is an error in the profile AND the resource

Grahame Grieve (Jul 08 2020 at 21:11):

well, here's quoting from your profile:

     <fixedReference>
        <type value="Patient" />
      </fixedReference>

Jens Villadsen (Jul 08 2020 at 21:11):

yep

Grahame Grieve (Jul 08 2020 at 21:11):

you're saying that the vaule is fixed to not have a reference; it can only have a type, and the value of the type is "Patient"

Jens Villadsen (Jul 08 2020 at 21:11):

oh my bad

Jens Villadsen (Jul 08 2020 at 21:11):

misread it

Jens Villadsen (Jul 08 2020 at 21:13):

and nr. 2 then ...

Jens Villadsen (Jul 08 2020 at 21:13):

it says the resource should state a type in its reference ...

Grahame Grieve (Jul 08 2020 at 21:14):

that's what your profile says

Jens Villadsen (Jul 08 2020 at 21:14):

nop

Grahame Grieve (Jul 08 2020 at 21:14):

indeed it does

Jens Villadsen (Jul 08 2020 at 21:15):

wait ...

Jens Villadsen (Jul 08 2020 at 21:15):

you're right

Grahame Grieve (Jul 08 2020 at 21:16):

check out my fixed up tests - just committed

Jens Villadsen (Jul 08 2020 at 21:19):

ok ... seems sane, now what about the invalid one

Jens Villadsen (Jul 08 2020 at 21:19):

wait ...
your commit shows that

Jens Villadsen (Jul 08 2020 at 21:26):

the question from is now) when will the fix be available and released? - and) does the release contain breaking changes towards HAPI 5.0.2?

Jens Villadsen (Jul 08 2020 at 21:26):

if the release contains breaking changes, I'll do the fix myself and do class shadowing

Grahame Grieve (Jul 08 2020 at 21:31):

https://github.com/hapifhir/org.hl7.fhir.core/pull/270

no change to the interface but I can't comment on the HAPI release timeline. you'll have to ask on #hapi

Jens Villadsen (Jul 08 2020 at 21:34):

I can overwrite the validator dependency - so If you haven't changed the interface contract of how HAPI uses the validation framework, it should be safe for me to import it I suppose

Jens Villadsen (Jul 08 2020 at 21:34):

(I like you call the branch 'work')

Jens Villadsen (Jul 08 2020 at 21:34):

image.png

Jens Villadsen (Jul 08 2020 at 21:35):

:grinning:

Jens Villadsen (Jul 08 2020 at 21:37):

but thx for the fix

Last updated: Apr 12 2022 at 19:14 UTC