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

Find entities by id, even when their id is set by a rule #1198

Merged
merged 6 commits into from
Jan 26, 2023

Conversation

mint-thompson
Copy link
Collaborator

Fixes #1183 and completes task CIMPL-1052.

When fishing for most FSH entities, one of the conditions for a match is that the provided string matches the entity's id. However, an entity's id may be set by one of its rules. To make this more reliable, this PR introduces two main changes:

  • All insert rules are applied before any resources are exported.
  • When getting the id from a FSH definition, check the rules on the definition for one that sets the id.
    These two changes together help to make sure that references to resources defined in FSH that have a rule for setting the id will resolve correctly.

In addition to the test cases added as part of the PR, the sample FSH in the linked issue will now resolve all references correctly.

This change applies only to FSH entities that can be fished up by id. This is why Mapping is not changed, even though it has an id.

When fishing for an Instance, the id of the Instance may be set by a
rule. The rule could be directly on the Instance, or it might be
contained within a RuleSet. Examine these rules to find potential
matches.
When exporting FHIR, apply insert rules on all FSH definitions before
exporting any resources. Each exporter provides a method to apply insert
rules on all of the definitions for the resource type it exports. This
allows functions that are called during export to assume that all insert
rules have already been applied, which can help detect rules that
override metadata.

FSH entities that can be fished by id now have a get method for their id
that accounts for the presence of id-setting rules.
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.

Overall, this looks good and the solution seems like it may help us down the road too!

I made one suggestion and I am currently running a full regression to see if there is any unintended impact.

src/fshtypes/FshCodeSystem.ts Outdated Show resolved Hide resolved
@cmoesel
Copy link
Member

cmoesel commented Jan 24, 2023

Full regression showed two IGs with differences. Both are good.

In exhibit A, we see some canonical URLs changed to more properly reflect the correct ID:
image

In exhibit B, we see some relative references changed to properly include the resourceType. Honestly, I'm not 100% sure why this was affected, but I'll take it:
image

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.

This looks really good. I didn't notice anything else beyond Chris's suggestion.

src/export/InstanceExporter.ts Show resolved Hide resolved
src/fshtypes/FshCodeSystem.ts Outdated Show resolved Hide resolved
jafeltra
jafeltra previously approved these changes Jan 25, 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.

Thanks, Mint! I think this makes sense. I was going back and forth on if it would help to have a findIdAssignmentRule function next to the findIdCaretRule function with a note that says to change the two together, but they probably are distinct enough that I think it's okay as is. But if you or Chris like the idea, I'm certainly not against it.

I didn't notice anything else!

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.

Julia suggested creating a findIdAssignmentRule. I think that's probably a good idea, because I just found at least one other place that looks for an id assignment rule (and it actually isn't as good as yours, @mint-thompson).

So I suggest creating findIdAssignmentRule and using it in the relevant location in this PR as well as the other place I found similar code:

const idRule = instance.rules.find(
r => r.path === 'id' && r instanceof AssignmentRule
) as AssignmentRule;

Similarly, I also found a place where findIdCaretRule can be used:

const idRule = _.findLast(
fshDefinition.rules,
rule => rule instanceof CaretValueRule && rule.caretPath === 'id' && rule.path === ''
) as CaretValueRule;

Also note the use of _.findLast above, which seems pretty nice.

Instances can have an id set by an AssignmentRule, so they get a
slightly different helper function.
Refactor replaceReferences function and Instance class to use this new
helper function.
HasId.validateId now looks for an id AssignmentRule when operating on an
Instance, which helps it give more accurate line numbers in error logs.
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.

I like it! Well done!

Comment on lines -432 to -436
// If the instance has a rule setting id, that overrides instance.id
const idRule = instance.rules.find(
r => r.path === 'id' && r instanceof AssignmentRule
) as AssignmentRule;
const id = idRule?.value ?? instance.id;
Copy link
Member

Choose a reason for hiding this comment

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

Oh... right! We don't even need this anymore. How exciting!

Comment on lines +66 to +67
if (fshDefinition instanceof Instance) {
idRule = findIdAssignmentRule(fshDefinition.rules);
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

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.

Looks good to me!

@mint-thompson mint-thompson merged commit e776505 into master Jan 26, 2023
@mint-thompson mint-thompson deleted the cimpl-1052-resolve-reference-from-assignment branch January 26, 2023 17:37
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 should look at assigned values when resolving References
3 participants