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

Improve import & export of ids/extensions on primitives #1240

Merged
merged 10 commits into from
Mar 23, 2023

Conversation

cmoesel
Copy link
Member

@cmoesel cmoesel commented Mar 20, 2023

In FHIR JSON, ids and extensions on primitives are represented by creating a shadow property prefixed with an underscore. For example, to set an extension on StructureDefinition.baseDefinition, you assign and object containing extension to StructureDefinition._baseDefinition. For more information see JSON representation of primitive elements.

While SUSHI partially supported setting ids and extensions on primitive elements using FSH assignment rules, it dropped all properties starting with _ when it imported an SD using StructureDefinition.fromJSON(...). It also did not propertly consider _ properties when calculating differentials.

This PR adds support for importing _ properties in StructureDefinition.fromJSON and improves support for exporting _ properties via StructureDefinition.toJSON and detecting differentials. Once these improvements were made, however, additional adjustments were required:

  • When ElementDefinition.type.profile or ElementDefinition.type.targetProfile arrays are manipulated, the shadow _profile and _targetProfile arrays must also be adjusted to maintain the relationship between indices. In addition, when a profile or targetProfile is changed, the original extensions may no longer apply, so they are removed.
  • When a SD is used as a parent and that SD has _-prefixed properties, we strip the _-prefixed properties because in many (or most) cases, they don't necessarily apply to the child SD.

I've split up commits by broken tests and fixes if you'd like to go through the wild ride one commit at a time.

I've also run a full regression. It affects two IGs: HL7/fhir-mCODE-ig#master and HL7/fhir-pacio-pfe#master. In both cases, must-support extensions are target profiles are now (properly) inherited from their US Core parents.

@cmoesel cmoesel changed the title Improve for import/export of ids/extensions on primitives Improve import & export of ids/extensions on primitives Mar 21, 2023
@mint-thompson mint-thompson self-assigned this Mar 22, 2023
Copy link
Collaborator

@mint-thompson mint-thompson left a comment

Choose a reason for hiding this comment

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

Hooray for secret extensions being not-so-secret! I found some minor typos, but everything else looks good.

test/fhirtypes/ElementDefinition.test.ts Outdated Show resolved Hide resolved
test/fhirtypes/ElementDefinition.test.ts Outdated Show resolved Hide resolved
Comment on lines +158 to +159
// Because of the way we do differentials, those won't come back the same,
// so remove differentials before comparing.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to make sure I'm understanding correctly: the original observation has all of its elements in the differential, since these are in the FHIR we fished up where they're new compared to Observation's parent DomainResource. But observation.toJSON() actually recalculates the differentials for all the elements, and nothing has changed, so those elements aren't in the differential. Is that correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's basically it. We only calculate differentials for what changed since we populated the SD. Although, TBH, I didn't think that much about it. This test was entirely skipped before because the differentials failed the test -- so I unskipped it, verified that it still fails, and did what you see here so that we'd at least have part of a test.

mint-thompson
mint-thompson previously approved these changes Mar 23, 2023
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.

The commit organization on this PR is ✨

I added one question, that became a few for similar places. I didn't notice any case that wasn't covered, so besides that one question, this looks good to me.

@cmoesel
Copy link
Member Author

cmoesel commented Mar 23, 2023

Mint's approval was technically dismissed because of the fix to the typo in the test description -- but I'll assume they'd be OK w/ that change -- so I'll merge once the tests pass.

@cmoesel cmoesel merged commit 95f25de into master Mar 23, 2023
@cmoesel cmoesel deleted the ids-and-exts-on-primitives branch March 23, 2023 17:21
cmoesel added a commit that referenced this pull request Mar 27, 2023
SUSHI 2.x port of #1240, cherry-picked from its commit.
cmoesel added a commit that referenced this pull request Mar 28, 2023
SUSHI 2.x port of #1240, cherry-picked from its commit.
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.

3 participants