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

Assignment on primitive satisfies cardinality of value element #1284

Merged
merged 8 commits into from
Jun 15, 2023

Conversation

mint-thompson
Copy link
Collaborator

Fixes #1281 and completes task CIMPL-1109.

All elements with primitive types have a child element named "value". When this child element is required, an assignment to the primitive satisfies this cardinality requirement.

Because the primitive's "value" element will only be checked if the parent primitive element is not null, it is already known that a value has been assigned to the parent primitive when the "value" element's cardinality is being checked.

All elements with primitive types have a child element named "value".
When this child element is required, an assignment to the primitive
satisfies this cardinality requirement.
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solves the false negative quite elegantly.

Unfortunately, it introduces a false positive. If a primitive's .value is 1..1, then it must have a value; setting just an extension on the primitive should not satisfy it. Prior to this PR, SUSHI would emit an error, but now it lets it pass with no error.

E.g., the following should cause an error if target.reference.value is 1..1, but it no longer does in this PR:

* target.reference.extension[http://hl7.org/fhir/StructureDefinition/data-absent-reason].valueCode = #unsupported

Full example on FSH Online

There is an existing problem regarding primitive arrays that is outside
the scope of this change. Test cases are added and skipped that show
these cases.
@mint-thompson
Copy link
Collaborator Author

There are some existing problems with extensions on primitive arrays that are not fixed in this PR. Two test cases are added that give examples of these problems, but they are currently skipped. I've added CIMPL-1116 for this to be addressed in the future.

Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code makes sense, but when I went to run a regression, the repo HL7/davinci-epdx#master has differences that I don't quite understand. Also, there are errors being logged saying Cannot read properties of undefined (reading 'length'), which shouldn't be happening. I haven't looked into why that's happening yet and if the differences and that unexpected error are related, but I can if that is helpful.

An element with a content reference may fail to unfold if there are
problems in the element's StructureDefinition or the referenced
element's StructureDefinition, or if the content reference does not
resolve to an element. In these cases, assume the element is not a
primitive for the purpose of validating an instance.
@mint-thompson
Copy link
Collaborator Author

@jafeltra I've made a small update that should give a better error message for that repo. I've also created CIMPL-1120 to address the underlying problem more generally.

Unfolding the full content reference doesn't provide any benefit, since
the type is all that is needed to determine whether an element is a
primitive or not.
Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense. Thanks for making those changes to avoid logging any issues found during the check for a primitive type. I haven't re-run the full regression, but I can if that would be helpful!

@cmoesel
Copy link
Member

cmoesel commented Jun 15, 2023

@jafeltra - I'm in the middle of running a regression on this one -- so I'll report back.

Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good and regression passes w/ no changes. Thank you!

@cmoesel cmoesel merged commit e2b155a into master Jun 15, 2023
@cmoesel cmoesel deleted the cimpl-1109-value-element-cardinality branch June 15, 2023 16:24
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.

SUSHI does not validate cardinality of primitive .value element correctly
3 participants