-
Notifications
You must be signed in to change notification settings - Fork 43
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
Allow ValueSet to reference contained inline CodeSystem #1520
Conversation
If a ValueSetComponentRule specifies a system, check if that system is present in the list of contained resources. If so, add the valueset-system extension to the system, using the relative reference as the extension's value. If the system is not present in the list of contained resources, and the system is an inline Instance, log an error and do not add the component to the ValueSet. Fishing in the tank for a CodeSystem will now return inline instances of CodeSystem. This matches the operation of fishing in the tank for a ValueSet. Add _system.extension to the type definition for elements of a ValueSet's include and exclude lists. Use the Extension fhirtype as part of this definition. Replace the Extension fshtype with the Extension fhirtype in other parts of the ValueSet definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works as advertised. I left of could of small comments to address. Thanks!
Add link to zulip thread with information about the contained code system extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I just noticed one issue, that also asks a slightly larger question.
src/export/ValueSetExporter.ts
Outdated
// https://chat.fhir.org/#narrow/stream/215610-shorthand/topic/Contained.20code.20system.20in.20the.20value.20set/near/424938537 | ||
// additionally, if it's not a contained resource, and the system we found is an inline instance, that's a problem | ||
const containedSystem = valueSet.contained?.find((resource: any) => { | ||
return resource?.id === csMetadata.id && resource.resourceType === 'CodeSystem'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of #1402 says
We should also check to see if you can refer to inline value sets from compose rules (and fix it if you can't).
I went to go look to see if this was fixed with this PR, so I tried to add an inline valueset to the contained list and reference it in a compose rule. Maybe it is worth discussing if we want that fix as part of this or if we want to track it as part of a separate issue. Either way, we probably should fix this line because I get an error that says "Cannot read properties of undefined (reading 'id')". So at the very least, I think we want to avoid that. I think updating the line like this should do it:
return resource?.id === csMetadata.id && resource.resourceType === 'CodeSystem'; | |
return resource?.id === csMetadata?.id && resource.resourceType === 'CodeSystem'; |
Add a test showing that an inline example instance of a CodeSystem will not work. Add a currently-skipped test that shows what the inline ValueSet behavior should be, based on currently available information from this zulip thread: https://chat.fhir.org/#narrow/stream/215610-shorthand/topic/Contained.20code.20system.20in.20the.20value.20set/near/475597931
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussion, we decided that the issue that @jafeltra raised is outside the scope of the issue this PR is addressing and will be tracked as a separate issue. That being the case, this all looks good to me. Thank you, @mint-thompson!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Thanks for adding the skipped test!
Description:
If a ValueSetComponentRule specifies a system, check if that system is present in the list of contained resources. If so, add the valueset-system extension to the system, using the relative reference as the extension's value. If the system is not present in the list of contained resources, and the system is an inline Instance, log an error and do not add the component to the ValueSet.
Fishing in the tank for a CodeSystem will now return inline instances of CodeSystem. This matches the operation of fishing in the tank for a ValueSet.
Add
_system.extension
to the type definition for elements of a ValueSet's include and exclude lists. Use the Extension fhirtype as part of this definition. Replace the Extension fshtype with the Extension fhirtype in other parts of the ValueSet definition.Testing Instructions:
Tests are added to ValueSetExporter and FHIRExporter as part of this change set.
As part of this implementation, I chose to make the failure case log an error and skip the component, but not throw anything. This means that processing of the ValueSet will continue. Please let me know if you think that this should instead throw an error and halt processing the ValueSet.
Related Issue:
Fixes #1402. Note that this does not resolve the somewhat related issues #1403 (for allowing fragments as references to contained ValueSets) and #1404 (which would allow an inline CodeSystem to be constructed using caret rules on the ValueSet).
Additional information that led to this issue available in this zulip thread. Additionally, I checked the list of FHIR core extensions for a similar extension to be used when a contained ValueSet is referenced within another ValueSet, and I did not find any such extension.