Fish With Version #1275

jafeltra · 2023-05-11T15:02:08Z

This PR supports fishing for definitions with a version. This PR adds support for fishing with a specified version in the FSHTank and in the Package. Support for fishing with a specified version in FHIRDefinitions is provided by FPL. There is an FPL PR with this support here.

In addition to supporting fishing with a version, there are a number of places that SUSHI tries to fish for the version specified but is satisfied if any version is found, but logs a warning. To support this, three util functions are included: fishForFHIRBestVersion, fishForMetadataBestVersion, and fishInTankBestVersion. These functions will call either fishForFHIR, fishForMetadata, or fish (in a Tank) first with the item and specified version. If nothing is found, they'll then try again without a version, and if a definition/metadata is found with a differing version, a warning is logged. I replaced the special case handling SUSHI was doing to slice off the version and check for discrepancies between versions with these functions where possible.

To test this, you can use the FPL PR (until a release is ready and available directly in this PR) and link your local FPL to SUSHI. Then you can run the tests and test FSH projects locally.

I ran a full regression and found that two repos had regressions, HL7/vrdr and nightingaleproject/vital_records_fhir_messaging_ig, both with the same issue. Both repos specify a line similar to:

* valueDateTime.extension[partialDateTime].extension[time].valueTime.extension[dataabsent].valueCode = http://hl7.org/fhir/ValueSet/data-absent-reason|4.0.1#unknown

This line is setting a URL with a version as a system in a FSH code. Before, SUSHI was just fishing for this, didn't find a definition so didn't realize it was a ValueSet, checked that it was a valid URI, and successfully used the code system. In this branch, fishing with the version will work correctly and SUSHI will find a ValueSet definition for this definition, which is a problem, and it will throw a MismatchBindingTypeError and not set the value. I think this regression is actually the correct behavior for SUSHI now.

~~Note: this PR is marked as draft until a new version of FPL is released with support for fishing with versions so that the FHIR Definitions fishing works correctly.~~ FPL v.0.3.0 is used now, so this is no longer draft!

- Add support to FSHTank to fish with a version - Add helper function to determine if a version is set on a FSH definition, which can be used by the FSHTank fish() function - Add helper functions for fishing for best version - a version that matches or any version if match is not found - Replace most places where fishing was done without any version that was specified Note: this will need an updated FPL in order to have FHIRDefinitions fish correctly with a version

…nd for FishingUtils

…eck fishing for VS URL with version throws mismatch error

cmoesel

Wow. This turned out to be pretty intense! But... we finally have a more consistent approach to handling versioned canonicals! Thank you!

I found a few things that I think can be improved and/or fixed and/or removed. But... this is looking very good!

src/export/Package.ts

src/export/StructureDefinitionExporter.ts

cmoesel · 2023-05-17T18:06:58Z

src/export/ValueSetExporter.ts

@@ -57,7 +57,7 @@ export class ValueSetExporter {
        if (component.from.system) {
          const systemParts = component.from.system.split('|');
          const foundSystem = (
-            this.fisher.fishForMetadata(systemParts[0], Type.CodeSystem)?.url ??
+            this.fisher.fishForMetadata(component.from.system, Type.CodeSystem)?.url ??


I see that this change makes it more consistent w/ the approach we take for value sets below -- so that's good.

I also see that if there is a version, we drop the version when there's a match but keep it when there isn't (value sets below does the same thing). I think this is one of those places where we want to keep the version when there is a match. I think we had decided, however, that these kinds of changes aren't necessarily in scope for this PR and can be deferred to another PR if we want.

Now that I have your nifty regex replace in my back pocket, I don't mind doing it here. But I can also hold off for a separate PR if that's preferred.

Actually, wait, now I'm not sure if I agree we should keep the version. The version the author specified gets added to the composeElement.version regardless of if we find a match on line 63. And because the foundSystem variable does a final split('|') and then only foundSystem[0] is used later, the version is always spliced off and never added to composeElement.system. So do we want to include the |version on the system property, as well as on the version property?

Oh, dang. You're right! For whatever reason, ValueSet.compose.include splits the version off into its own element. And the system element is uri not canonical. So, yep, we definitely do not want to attach the |version. Good call!

src/fhirtypes/common.ts

src/utils/FishingUtils.ts

cmoesel · 2023-05-17T18:43:50Z

test/export/InstanceExporter.test.ts

+      const visibleSystem = new FshCodeSystem('Visible');
+      const visibleSystemVersion = new CaretValueRule('');
+      visibleSystemVersion.caretPath = 'url';
+      visibleSystemVersion.value = 'http://hl7.org/fhir/us/minimal/CodeSystem/Visible|1.0.0';


I don't think a CodeSystem definition would declare its url with a version. The url would be the version-less URL and the version element would contain the version. E.g.

const visibleSystemUrl = new CaretValueRule(''); visibleSystemUrl.caretPath = 'url'; visibleSystemUrl.value = 'http://hl7.org/fhir/us/minimal/CodeSystem/Visible'; const visibleSystemVersion = new CaretValueRule(''); visibleSystemVersion.caretPath = 'version'; visibleSystemVersion.value = '1.0.0';

I thought I had added this case because of a regression, but I'm not sure which repo without rerunning the regression. I believe this was also tied to this change, and updating to use the regex replace actually causes this test to fail. I guess it is worth keeping the test with the version caret value rule, so I'll update this!

Included in 70e31fc

Technically, CodeSystem.url is a uri, so you're not supposed to append a version to it. It even says:

URL should not contain | or # - these characters make processing canonical references problematic

So if someone is making a CodeSystem with a URL like that in a real IG, we shouldn't support it anyway.

Oh! Well maybe I just invented that test case. It seems so specific so I thought I must have pulled it from somewhere, but maybe not. Well, it is no longer supported now!

test/import/FSHTank.test.ts

…check on rules

…lify conditional statements

…signing to a primitive code/string/uri

cmoesel

This looks very good! I ran a regression and got the same two regressions as you did. Upon studying them, however, I realized that they're assigning the value to a code -- so while the system in the FSH is incorrect, it isn't actually used because primitive code does not include the system. This means the assignment can still safely happen.

That being the case, I thought it would be more appropriate to detect that situation and warn on it rather than error -- and proceed with the assignment rather than skipping it. So that is what I've done in 3a9f3e9.

Now everything looks good to me and has my approval!

mint-thompson

This looks great, thank you! There's one slightly unusual thing that I noticed, but I don't think it necessarily needs to be changed, so I'm giving my approval.

mint-thompson · 2023-05-19T16:26:25Z

src/fhirtypes/common.ts

+    const codeSystem = fishInTankBestVersion(tank, value.system, Type.CodeSystem);
+    const codeSystemMeta = fishForMetadataBestVersion(fisher, value.system, Type.CodeSystem);


This is not really a problem, but it's possible for each of these functions to separately log a warning for a version mismatch if the CodeSystem is retrieved from the package when fishing for metadata. Here's an example that can make this happen:

CodeSystem: BreakfastCS * ^version = "1.0.0" * #toast "Toast" * #donut "Donut" Instance: MyBreakfast InstanceOf: Observation * status = #final * code = BreakfastCS|2.0.0#toast

Ah, yeah. We technically need to do both fishes -- so the only way I can think of to avoid the double warning is to do the 2nd one based on whatever was actually returned by the first (rather than the original request). E.g.,:

const codeSystemMeta = fishForMetadataBestVersion(fisher, value.system, Type.CodeSystem); if (codeSystemMeta) { clone = cloneDeep(rule); const assignedCode = getRuleValue(clone) as FshCode; assignedCode.system = value.system.replace(/^[^|]+/, codeSystemMeta.url); // Find the code system using the returned metadata to avoid duplicate warnings if version mismatches const matchedCanonical = codeSystemMeta.url ? `${codeSystemMeta.url}${codeSystemMeta.version ? `|${codeSystemMeta.version}` : ''}` : value.system; const codeSystem = fishInTankBestVersion(tank, matchedCanonical, Type.CodeSystem); if (codeSystem && (codeSystem instanceof FshCodeSystem || codeSystem instanceof Instance)) { // if a local system was used, check to make sure the code is actually in that system listUndefinedLocalCodes(codeSystem, [assignedCode.code], tank, rule); } }

@mint-thompson - I have this change locally and it does worth. Do you think I should push it up?

I think so, yes. It's not too complicated, and it makes the output more consistent. Thank you!

OK. Fixed in 858c75d. I also noticed that the warning messages did not log the source info -- so we'd say there was a mismatch, but it would be up to the author to find it in their FSH files. This didn't seem optimal, so I also updated it to pass along the source info and log that. It's in the same commit.

Also updates system lookup code for FSHCodes to avoid double-warning on mismatched version.

cmoesel

OK. I've addressed the double-warning and source info logging. @mint-thompson - can you give it one more review?

cmoesel · 2023-05-19T18:31:11Z

src/fhirtypes/common.ts

+    const codeSystem = fishInTankBestVersion(tank, value.system, Type.CodeSystem);
+    const codeSystemMeta = fishForMetadataBestVersion(fisher, value.system, Type.CodeSystem);


OK. Fixed in 858c75d. I also noticed that the warning messages did not log the source info -- so we'd say there was a mismatch, but it would be up to the author to find it in their FSH files. This didn't seem optimal, so I also updated it to pass along the source info and log that. It's in the same commit.

jafeltra added 6 commits May 8, 2023 15:37

Add tests and minor tweeks around fishing with versions in FSH Tank a…

19ab78e

…nd for FishingUtils

Only append version if not already provided in URL and add test to ch…

4e871d3

…eck fishing for VS URL with version throws mismatch error

Fish with version in Package

ff23abe

Add back version if fished up a value set

4780f60

Use FPL v0.3.0

3e8be48

jafeltra marked this pull request as ready for review May 15, 2023 16:46

jafeltra requested review from cmoesel and mint-thompson as code owners May 15, 2023 16:46

jafeltra added 2 commits May 17, 2023 11:44

Merge branch 'master' into fpl-fishes-with-version

2966e53

Merge branch 'master' into fpl-fishes-with-version

11d0cfa

cmoesel requested changes May 17, 2023

View reviewed changes

jafeltra and others added 5 commits May 17, 2023 17:45

Simplify check for matching version on instances

9668cdc

Simplify places where version needs to be appended to a fished up url

70e31fc

Invariants, Mappings, RuleSets won't have a version so don't need to …

0d0852d

…check on rules

Simplify warning message when incorrect version is fished up and simp…

18b8ba8

…lify conditional statements

Assigning a code with an invalid code system should only warn when as…

3a9f3e9

…signing to a primitive code/string/uri

cmoesel previously approved these changes May 18, 2023

View reviewed changes

mint-thompson previously approved these changes May 19, 2023

View reviewed changes

Log source info for mismatched version when fishing for best version

858c75d

Also updates system lookup code for FSHCodes to avoid double-warning on mismatched version.

cmoesel dismissed stale reviews from mint-thompson and themself via 858c75d May 19, 2023 18:28

Remove unnecessary import

0b7ae4d

cmoesel reviewed May 19, 2023

View reviewed changes

mint-thompson approved these changes May 19, 2023

View reviewed changes

cmoesel merged commit 2c2e4c4 into master May 19, 2023

cmoesel deleted the fpl-fishes-with-version branch May 19, 2023 20:12

jafeltra mentioned this pull request Sep 12, 2024

Improve error when using a value set in a primitive code assignment #1239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fish With Version #1275

Fish With Version #1275

jafeltra commented May 11, 2023 •

edited

Loading

cmoesel left a comment

cmoesel May 17, 2023

jafeltra May 17, 2023

jafeltra May 17, 2023

cmoesel May 17, 2023

cmoesel May 17, 2023

jafeltra May 17, 2023

jafeltra May 17, 2023

cmoesel May 17, 2023

jafeltra May 17, 2023

cmoesel left a comment

mint-thompson left a comment

mint-thompson May 19, 2023

cmoesel May 19, 2023

mint-thompson May 19, 2023

cmoesel May 19, 2023

cmoesel left a comment

cmoesel May 19, 2023

		const codeSystem = fishInTankBestVersion(tank, value.system, Type.CodeSystem);
		const codeSystemMeta = fishForMetadataBestVersion(fisher, value.system, Type.CodeSystem);

Fish With Version #1275

Fish With Version #1275

Conversation

jafeltra commented May 11, 2023 • edited Loading

cmoesel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmoesel left a comment

Choose a reason for hiding this comment

mint-thompson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmoesel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafeltra commented May 11, 2023 •

edited

Loading