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

Task Validation fails in 5.2.0, works in 5.1.0 #2284

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hhund
Copy link

@hhund hhund commented Jan 12, 2021

JUnit test to reproduce Issue #2285:

Validation of Task resource against profile defining multiple input slices discriminated by custom CodeSystem values fails in 5.2.0 but works in 5.1.0.

The TaskValidationTest in branch v5.2.0_task_validation_not_working based on v5.2.0 fails
while the TaskValidationTest in branch v5.1.0_task_validation_working based on v5.1.0 works.

@hhund hhund changed the title task validation test - working in 5.1.0 Task Validation fails in 5.2.0, works in 5.1.0 Jan 12, 2021
@hhund hhund marked this pull request as ready for review January 12, 2021 13:01
@jamesagnew
Copy link
Collaborator

Hi @hhund - I tried the test in this PR, and the validation doesn't actually fail in 5.2.0. It does produce 3 information level messages though, which the test is labelling as a fail:

{
  "resourceType": "OperationOutcome",
  "issue": [ {
    "severity": "information",
    "code": "processing",
    "diagnostics": "Binding for path Task.input[0].type has no source, so can't be checked",
    "location": [ "Task.input[0].type", "Line 1, Col 179" ]
  }, {
    "severity": "information",
    "code": "processing",
    "diagnostics": "Binding for path Task.input[1].type has no source, so can't be checked",
    "location": [ "Task.input[1].type", "Line 1, Col 331" ]
  }, {
    "severity": "information",
    "code": "processing",
    "diagnostics": "Binding for path Task.input[2].type has no source, so can't be checked",
    "location": [ "Task.input[2].type", "Line 1, Col 477" ]
  } ]

Those messages look accurate to me, I don't see any valueset binding for the slices you specify in the profile.

@hhund
Copy link
Author

hhund commented Jan 14, 2021

@jamesagnew thank you for the feedback. With this unit test I was trying to create a minimal example for reproducing a potential bug we encountered while trying to upgrade the HAPI dependency of https://github.com/highmed/highmed-dsf from 5.1.0 to 5.2.0.
In our use case we have a base Task profile and specific Task profiles derived from the base adding for example slices with additional input parameters.
If its OK with you, I will try to fix my example/test or close this PR and Issue #2285 if I can't reproduce my problem.

@jamesagnew
Copy link
Collaborator

@hhund sure thing, sounds good.

I do want to say I really appreciate the good quality test. I wish that every bug came with something of this caliber. :)

@hhund
Copy link
Author

hhund commented Jan 14, 2021

@jamesagnew Can you give me a hint, why the validation using 5.2.0 results in infos about missing bindings?

Validating with 5.1.0 does not give any such infos and at least I thought I specified bindings at the appropriate locations. The generated Resources as JSON below, maybe you can take a look at the StructureDefinition (click to expand):

CodeSystem
{
  "resourceType": "CodeSystem",
  "url": "http:https://test.com/fhir/CodeSystem/test",
  "version": "1.0.0",
  "name": "Test_CodeSystem",
  "status": "active",
  "experimental": true,
  "date": "2021-01-14T21:01:35+01:00",
  "caseSensitive": true,
  "hierarchyMeaning": "grouped-by",
  "versionNeeded": false,
  "content": "complete",
  "concept": [ {
    "code": "foo",
    "display": "Foo Display",
    "definition": "Foo Definition"
  }, {
    "code": "bar",
    "display": "Bar Display",
    "definition": "Bar Definition"
  }, {
    "code": "baz",
    "display": "Baz Display",
    "definition": "Baz Definition"
  } ]
}
ValueSet
{
  "resourceType": "ValueSet",
  "url": "http:https://test.com/fhir/ValueSet/test",
  "version": "1.0.0",
  "name": "Test_ValueSet",
  "status": "active",
  "experimental": true,
  "date": "2021-01-14T21:01:36+01:00",
  "immutable": true,
  "compose": {
    "include": [ {
      "system": "http:https://test.com/fhir/CodeSystem/test"
    } ]
  }
}
StructureDefinition
{
  "resourceType": "StructureDefinition",
  "url": "http:https://test.com/fhir/StructureDefinition/TestTask",
  "name": "TestTask",
  "status": "active",
  "experimental": true,
  "fhirVersion": "4.0.1",
  "kind": "resource",
  "abstract": false,
  "type": "Task",
  "baseDefinition": "http:https://hl7.org/fhir/StructureDefinition/Task",
  "derivation": "constraint",
  "differential": {
    "element": [ {
      "id": "Task.input",
      "path": "Task.input",
      "slicing": {
        "discriminator": [ {
          "type": "value",
          "path": "type.coding.system"
        }, {
          "type": "value",
          "path": "type.coding.code"
        } ],
        "rules": "closed"
      },
      "min": 3,
      "max": "3"
    }, {
      "id": "Task.input:stringSlice1",
      "path": "Task.input",
      "sliceName": "stringSlice1",
      "min": 1,
      "max": "1"
    }, {
      "id": "Task.input:stringSlice1.type",
      "path": "Task.input.type",
      "binding": {
        "strength": "required",
        "valueSet": "http:https://test.com/fhir/ValueSet/test"
      }
    }, {
      "id": "Task.input:stringSlice1.type.coding",
      "path": "Task.input.type.coding",
      "min": 1,
      "max": "1"
    }, {
      "id": "Task.input:stringSlice1.type.coding.system",
      "path": "Task.input.type.coding.system",
      "min": 1,
      "fixedUri": "http:https://test.com/fhir/CodeSystem/test"
    }, {
      "id": "Task.input:stringSlice1.type.coding.code",
      "path": "Task.input.type.coding.code",
      "min": 1,
      "fixedCode": "foo"
    }, {
      "id": "Task.input:stringSlice1.value[x]",
      "path": "Task.input.value[x]",
      "fixedString": "foo-bar-baz"
    }, {
      "id": "Task.input:booleanSlice",
      "path": "Task.input",
      "sliceName": "booleanSlice",
      "min": 1,
      "max": "1"
    }, {
      "id": "Task.input:booleanSlice.type",
      "path": "Task.input.type",
      "binding": {
        "strength": "required",
        "valueSet": "http:https://test.com/fhir/ValueSet/test"
      }
    }, {
      "id": "Task.input:booleanSlice.type.coding",
      "path": "Task.input.type.coding",
      "min": 1,
      "max": "1"
    }, {
      "id": "Task.input:booleanSlice.type.coding.system",
      "path": "Task.input.type.coding.system",
      "min": 1,
      "fixedUri": "http:https://test.com/fhir/CodeSystem/test"
    }, {
      "id": "Task.input:booleanSlice.type.coding.code",
      "path": "Task.input.type.coding.code",
      "min": 1,
      "fixedCode": "bar"
    }, {
      "id": "Task.input:booleanSlice.value[x]",
      "path": "Task.input.value[x]",
      "type": [ {
        "code": "boolean"
      } ]
    }, {
      "id": "Task.input:stringSlice2",
      "path": "Task.input",
      "sliceName": "stringSlice2",
      "min": 1,
      "max": "1"
    }, {
      "id": "Task.input:stringSlice2.type",
      "path": "Task.input.type",
      "binding": {
        "strength": "required",
        "valueSet": "http:https://test.com/fhir/ValueSet/test"
      }
    }, {
      "id": "Task.input:stringSlice2.type.coding",
      "path": "Task.input.type.coding",
      "min": 1,
      "max": "1"
    }, {
      "id": "Task.input:stringSlice2.type.coding.system",
      "path": "Task.input.type.coding.system",
      "min": 1,
      "fixedUri": "http:https://test.com/fhir/CodeSystem/test"
    }, {
      "id": "Task.input:stringSlice2.type.coding.code",
      "path": "Task.input.type.coding.code",
      "min": 1,
      "fixedCode": "baz"
    }, {
      "id": "Task.input:stringSlice2.value[x]",
      "path": "Task.input.value[x]",
      "type": [ {
        "code": "string"
      } ]
    } ]
  }
}
Task
{
  "resourceType": "Task",
  "meta": {
    "profile": [ "http:https://test.com/fhir/StructureDefinition/TestTask" ]
  },
  "status": "requested",
  "intent": "order",
  "input": [ {
    "type": {
      "coding": [ {
        "system": "http:https://test.com/fhir/CodeSystem/test",
        "code": "foo"
      } ]
    },
    "valueString": "foo-bar-baz"
  }, {
    "type": {
      "coding": [ {
        "system": "http:https://test.com/fhir/CodeSystem/test",
        "code": "bar"
      } ]
    },
    "valueBoolean": true
  }, {
    "type": {
      "coding": [ {
        "system": "http:https://test.com/fhir/CodeSystem/test",
        "code": "baz"
      } ]
    },
    "valueString": "310fc43d-f387-4915-8ad0-9c757a943923"
  } ]
}

Using a code not part of the CodeSystem ('code-does-not-exist'), results in an validation error using 5.1.0 which suggests to me, that the binding is being picked up.

OperationOutcome for unknown code with 5.1.0
{
  "resourceType": "OperationOutcome",
  "issue": [ {
    "severity": "error",
    "code": "processing",
    "diagnostics": "This element does not match any known slice  defined in the profile http:https://test.com/fhir/StructureDefinition/TestTask and slicing is CLOSED: Task.input[2]: Does not match slice 'stringSlice1', Task.input[2]: Does not match slice 'booleanSlice', Task.input[2]: Does not match slice 'stringSlice1'",
    "location": [ "Task.input[2]", "Line 1, Col 551" ]
  }, {
    "severity": "error",
    "code": "processing",
    "diagnostics": "Profile http:https://test.com/fhir/StructureDefinition/TestTask, Element 'Task.input[stringSlice1]': minimum required = 1, but only found 0",
    "location": [ "Task", "Line 1, Col 35" ]
  }, {
    "severity": "information",
    "code": "processing",
    "diagnostics": "Binding for path Task.input[2].type has no source, so can't be checked",
    "location": [ "Task.input[2].type", "Line 1, Col 557" ]
  }, {
    "severity": "error",
    "code": "processing",
    "diagnostics": "Unknown code 'http:https://test.com/fhir/CodeSystem/test#code-does-not-exist' for 'http:https://test.com/fhir/CodeSystem/test#code-does-not-exist'",
    "location": [ "Task.input[2].type.coding[0]", "Line 1, Col 565" ]
  } ]
}

If I remove the bindings from the profile, validating with 5.1.0 gives the same info messages as 5.2.0 with bindings.

OperationOutcome bindings remove from profile with 5.1.0
{
  "resourceType": "OperationOutcome",
  "issue": [ {
    "severity": "information",
    "code": "processing",
    "diagnostics": "Binding for path Task.input[0].type has no source, so can't be checked",
    "location": [ "Task.input[0].type", "Line 1, Col 204" ]
  }, {
    "severity": "information",
    "code": "processing",
    "diagnostics": "Binding for path Task.input[1].type has no source, so can't be checked",
    "location": [ "Task.input[1].type", "Line 1, Col 383" ]
  }, {
    "severity": "information",
    "code": "processing",
    "diagnostics": "Binding for path Task.input[2].type has no source, so can't be checked",
    "location": [ "Task.input[2].type", "Line 1, Col 557" ]
  } ]
}

As mentioned in #2285 I think there is an Issue starting from HAPI 5.2.0: org.hl7.fhir.validation.instance.InstanceValidator L4158ff specifically the fact, that the validation is done twice:

public void checkChild(ValidatorHostContext hostContext, List<ValidationMessage> errors, StructureDefinition profile, ElementDefinition definition,
  Element resource, Element element, String actualType, NodeStack stack, boolean inCodeableConcept, boolean checkDisplayInContext, ElementInfo ei, String extensionUrl)
  throws FHIRException, DefinitionException {

  if (debug && ei.definition != null && ei.slice != null) {
    System.out.println(Utilities.padLeft("", ' ', stack.depth())+ "Check "+ei.getPath()+" against both "+ei.definition.getId()+" and "+ei.slice.getId());
  }
  if (ei.definition != null) {
    if (debug) {
      System.out.println(Utilities.padLeft("", ' ', stack.depth())+ "Check "+ei.getPath()+" against defn "+ei.definition.getId());
    }
    checkChildByDefinition(hostContext, errors, profile, definition, resource, element, actualType, stack, inCodeableConcept, checkDisplayInContext, ei, extensionUrl, ei.definition, false);
  }
  if (ei.slice != null) {
    if (debug) {
      System.out.println(Utilities.padLeft("", ' ', stack.depth())+ "Check "+ei.getPath()+" against slice "+ei.slice.getId());
    }
    checkChildByDefinition(hostContext, errors, profile, definition, resource, element, actualType, stack, inCodeableConcept, checkDisplayInContext, ei, extensionUrl, ei.slice, true);
  }
}

The first ei.definition case has a binding with an associated ValueSet and the second ei.slice case has no ValueSet associated resulting in the info messages.

Validating against our more complex HiGHmed DSF Task profiles results in very weird validation errors for 5.2.0, again with some of the elements being validated twice (once without error, once with).

Update: Fixed non unique slice name in StructuredDefinition (see hhund/hapi-fhir@c201a17) no change to validation behaviour

@jamesagnew
Copy link
Collaborator

@hhund this is definitely straining my personal understanding of profiling if I'm honest.

This PR that has just merged looks like it adds a flag to suppress this warning. Does this address your concern?

#2054

@hhund
Copy link
Author

hhund commented Jan 21, 2021

@jamesagnew looking at the changes merged, PR #2054 will help in suppressing the info messages. But I think it does not fix the underlying issue. In the profile above bindings are defined for all slices and the info messages should not be generated.

To determine if #2285 is a bug, we should confirm that the profile used in this PR is valid. If so, I would argue that a bug was introduced into InstanceValidator in version 5.2.0.

Could we maybe have @grahamegrieve take a look at this PR?

@jamesagnew
Copy link
Collaborator

If you're confident this is an actual bug in the validator itself, my recommendation would be to get it into a PR against the actual reference validator test cases, which live here: https://github.com/FHIR/fhir-test-cases/blob/master/validator/manifest.json

This is the runner that executes these if you wanted to try it out: https://github.com/hapifhir/org.hl7.fhir.core/blob/master/org.hl7.fhir.validation/src/test/java/org/hl7/fhir/validation/tests/ValidationTests.java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants