Repository: FHIR/sushi
Pull request #1450 (2024-04-02 13:28:20)

Improve Logical Model type controls to not re-add elements
Fixes [CIMPL-1249](https://standardhealthrecord.atlassian.net/browse/CIMPL-1249).

This PR improves the logic for overwriting the `type` of a Logical Model with a caret value rule to only allow URIs to be set. This seems to line up with the FHIR spec for [type](http://hl7.org/fhir/structuredefinition-definitions.html#StructureDefinition.type). Because you can only overwrite `type` with a URI, I also updated the message of the InvalideTypeAccessError to be a little more specific for Logical Models. This also uses `this.pathType` when adding elements so the element id path is properly calculated so that elements that were in the parent aren't re-added when when the logical model overwrites the `type`.

This issue came up due to changes in the [IntelliSOFT-Consulting/ChanjoKe-FHIR-IG#main](https://github.com/IntelliSOFT-Consulting/ChanjoKe-FHIR-IG/tree/main/) repo after PR #1443. With this branch, there are still changes, but I think the changes are correct. Running a full 1-year regression resulted in 5 repos with changes. Looking at the differences, they all resulted from types that were not full URIs, which is not allowed in FHIR and no longer allowed on this branch. These are the 5 repos:

```
HL7/sdc#master
IHE-Germany/ITI.XDS.VS#main
IHE/pharm-vaccination#main
IntelliSOFT-Consulting/ChanjoKe-FHIR-IG#main
WorldHealthOrganization/ddcc#main
```

jafeltra: [bb0dd4c](https://github.com/FHIR/sushi/pull/1450/commits/bb0dd4ce05259d57c4a1aae76b2be2cfdf1a796b) switches back to overwriting the type of a Logical Model but now logs a warning when that type is not an absolute URL. With this change, three of the repos have no actual differences, but do now log warnings about changing the type.

`IntelliSOFT-Consulting/ChanjoKe-FHIR-IG#main` and `WorldHealthOrganization/ddcc#main` do still have changes where elements from the parent were previously re-added but now they are not added and an error is logged instead. They also have warnings for non-URL types. It looks like the elements that are trying to be re-added are all the same but change cardinality min from 0 to 1. I think both should be able to update to use regular card rules.

Patch:
diff --git a/src/fhirtypes/StructureDefinition.ts b/src/fhirtypes/StructureDefinition.ts
index 35c9bac..d87303a 100644
--- a/src/fhirtypes/StructureDefinition.ts
+++ b/src/fhirtypes/StructureDefinition.ts
@@ -415,7 +415,7 @@ export class StructureDefinition {
     if (path.startsWith('snapshot') || path.startsWith('differential')) {
       throw new InvalidElementAccessError(path);
     }
-    if (path === 'type' && value !== this.pathType) {
+    if (path === 'type' && value !== this.type) {
       throw new InvalidTypeAccessError();
     }
     setPropertyOnDefinitionInstance(this, path, value, fisher);
@@ -428,7 +428,7 @@ export class StructureDefinition {
    */
   newElement(name = '$UNKNOWN'): ElementDefinition {
     // Check if there already exists an element that is defined by an ancestor
-    if (this.elements.find(e => e.id == `${this.id}.${name}`)) {
+    if (this.elements.find(e => e.id == `${this.pathType}.${name}`)) {
       throw new ElementAlreadyDefinedError(name, this.id);
     }
     const el = this.elements[0].newChildElement(name);
diff --git a/src/fhirtypes/common.ts b/src/fhirtypes/common.ts
index c7b73fc..fa32648 100644
--- a/src/fhirtypes/common.ts
+++ b/src/fhirtypes/common.ts
@@ -39,6 +39,7 @@ import { buildSliceTree, calculateSliceTreeCounts } from './sliceTree';
 import { InstanceExporter } from '../export';
 import { MismatchedTypeError } from '../errors';
 import { getValueFromRules } from '../fshtypes/common';
+import { isUri } from 'valid-url';
 
 // List of Conformance and Terminology resources from http://hl7.org/fhir/R4/resourcelist.html
 // and https://hl7.org/fhir/R5/resourcelist.html
@@ -1416,8 +1417,15 @@ export function getTypeFromFshDefinitionOrParent(
     // Select last CaretValueRule with caretPath === 'type' because rules processing
     // ends up applying the last rule in the processing order
     const lastCaretValueRule = caretValueRules[caretValueRules.length - 1];
+    const type = lastCaretValueRule.value.toString();
     // this value should only be a string, but that might change at some point
-    return lastCaretValueRule.value.toString();
+    if (fshDefinition instanceof Logical && !isUri(type)) {
+      logger.warn(
+        `${type} is an invalid type. Logical models require that the type be an absolute URL.`,
+        lastCaretValueRule.sourceInfo
+      );
+    }
+    return type;
   }
 
   // Default type for logical model to the StructureDefinition url;


Test patch:
diff --git a/test/export/StructureDefinition.LogicalExporter.test.ts b/test/export/StructureDefinition.LogicalExporter.test.ts
index ba2c866..68e303d 100644
--- a/test/export/StructureDefinition.LogicalExporter.test.ts
+++ b/test/export/StructureDefinition.LogicalExporter.test.ts
@@ -434,6 +434,96 @@ describe('LogicalExporter', () => {
     expect(barsElement.type[0].code).toBe('http://hl7.org/fhir/us/minimal/StructureDefinition/Bar');
   });
 
+  it('should not re-add elements that are defined on the parent logical model', () => {
+    const fishLogical = new Logical('FishLogical');
+    const addElementRuleFins = new AddElementRule('fins');
+    addElementRuleFins.min = 0;
+    addElementRuleFins.max = '1';
+    addElementRuleFins.types = [{ type: 'boolean' }];
+    addElementRuleFins.short = 'notes about fins';
+    fishLogical.rules.push(addElementRuleFins);
+    doc.logicals.set(fishLogical.name, fishLogical);
+
+    const sharkLogical = new Logical('SharkLogical');
+    sharkLogical.parent = 'FishLogical';
+    const addElementRuleFins2 = new AddElementRule('fins')
+      .withFile('Logicals.fsh')
+      .withLocation([3, 3, 3, 8]);
+    addElementRuleFins2.min = 0;
+    addElementRuleFins2.max = '1';
+    addElementRuleFins2.types = [{ type: 'boolean' }];
+    addElementRuleFins2.short = 'notes about shark fins';
+    const addElementRuleName = new AddElementRule('name');
+    addElementRuleName.min = 0;
+    addElementRuleName.max = '1';
+    addElementRuleName.types = [{ type: 'string' }];
+    addElementRuleName.short = 'some sharks have names';
+    sharkLogical.rules.push(addElementRuleFins2, addElementRuleName);
+    doc.logicals.set(sharkLogical.name, sharkLogical);
+
+    const exported = exporter.export().logicals;
+    expect(exported).toHaveLength(2);
+    const exportedFish = exported[0];
+    const exportedShark = exported[1];
+    expect(exportedFish.name).toBe('FishLogical');
+    expect(exportedShark.name).toBe('SharkLogical');
+
+    expect(exportedFish.elements).toHaveLength(2); // FishLogical, FishLogical.fins
+    expect(exportedShark.elements).toHaveLength(3); // SharkLogical, SharkLogical.fins (only once), SharkLogical.name
+    expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
+    expect(loggerSpy.getLastMessage('error')).toMatch(
+      /Cannot define element fins on SharkLogical because it has already been defined/s
+    );
+    expect(loggerSpy.getLastMessage('error')).toMatch(/File: Logicals\.fsh.*Line: 3/s);
+  });
+
+  it('should not re-add elements that are defined on the parent logical model even when the parent type is overwritten with a caret value rule', () => {
+    const fishLogical = new Logical('FishLogical');
+    const addElementRuleFins = new AddElementRule('fins');
+    addElementRuleFins.min = 0;
+    addElementRuleFins.max = '1';
+    addElementRuleFins.types = [{ type: 'boolean' }];
+    addElementRuleFins.short = 'notes about fins';
+    fishLogical.rules.push(addElementRuleFins);
+    doc.logicals.set(fishLogical.name, fishLogical);
+
+    const sharkLogical = new Logical('SharkLogical');
+    sharkLogical.parent = 'FishLogical';
+    // Note: the caret rule reassigns the type - this is the only difference compared to the test above this one
+    const typeCaretRule = new CaretValueRule('');
+    typeCaretRule.caretPath = 'type';
+    typeCaretRule.value = 'http://hl7.org/fhir/us/minimal/StructureDefinition/FishLogical';
+    const addElementRuleFins2 = new AddElementRule('fins')
+      .withFile('Logicals.fsh')
+      .withLocation([3, 3, 3, 8]);
+    addElementRuleFins2.min = 0;
+    addElementRuleFins2.max = '1';
+    addElementRuleFins2.types = [{ type: 'boolean' }];
+    addElementRuleFins2.short = 'notes about shark fins';
+    const addElementRuleName = new AddElementRule('name');
+    addElementRuleName.min = 0;
+    addElementRuleName.max = '1';
+    addElementRuleName.types = [{ type: 'string' }];
+    addElementRuleName.short = 'some sharks have names';
+    sharkLogical.rules.push(typeCaretRule, addElementRuleFins2, addElementRuleName);
+    doc.logicals.set(sharkLogical.name, sharkLogical);
+
+    const exported = exporter.export().logicals;
+    expect(exported).toHaveLength(2);
+    const exportedFish = exported[0];
+    const exportedShark = exported[1];
+    expect(exportedFish.name).toBe('FishLogical');
+    expect(exportedShark.name).toBe('SharkLogical');
+
+    expect(exportedFish.elements).toHaveLength(2); // FishLogical, FishLogical.fins
+    expect(exportedShark.elements).toHaveLength(3); // SharkLogical, SharkLogical.fins (only once), SharkLogical.name
+    expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
+    expect(loggerSpy.getLastMessage('error')).toMatch(
+      /Cannot define element fins on SharkLogical because it has already been defined/s
+    );
+    expect(loggerSpy.getLastMessage('error')).toMatch(/File: Logicals\.fsh.*Line: 3/s);
+  });
+
   it('should have correct base and types for each nested logical model', () => {
     const logicalOther = new Logical('Other');
     const addElementRuleThing = new AddElementRule('thing');
diff --git a/test/export/StructureDefinitionExporter.test.ts b/test/export/StructureDefinitionExporter.test.ts
index fc4ae04..ba72591 100644
--- a/test/export/StructureDefinitionExporter.test.ts
+++ b/test/export/StructureDefinitionExporter.test.ts
@@ -1615,11 +1615,27 @@ describe('StructureDefinitionExporter R4', () => {
       expect(exported.status).toBe('draft');
     });
 
-    it('should allow type to be overwritten with caret rule', () => {
+    it('should allow type to be overwritten with caret rule with a uri value', () => {
       const logical = new Logical('Foo');
       logical.parent = 'AlternateIdentification';
       const rule = new CaretValueRule('');
       rule.caretPath = 'type';
+      rule.value = 'http://example.org/other/MyType';
+      logical.rules.push(rule);
+      doc.logicals.set(logical.name, logical);
+      exporter.exportStructDef(logical);
+      const exported = pkg.logicals[0];
+      expect(exported.name).toBe('Foo');
+      expect(exported.type).toBe('http://example.org/other/MyType');
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(0);
+    });
+
+    it('should log a warning and allow overwriting type with caret rule with a non-uri value', () => {
+      const logical = new Logical('Foo');
+      logical.parent = 'AlternateIdentification';
+      const rule = new CaretValueRule('').withFile('LogicalModels.fsh').withLocation([3, 3, 3, 25]);
+      rule.caretPath = 'type';
       rule.value = 'MyType';
       logical.rules.push(rule);
       doc.logicals.set(logical.name, logical);
@@ -1627,6 +1643,12 @@ describe('StructureDefinitionExporter R4', () => {
       const exported = pkg.logicals[0];
       expect(exported.name).toBe('Foo');
       expect(exported.type).toBe('MyType');
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(1);
+      expect(loggerSpy.getLastMessage('warn')).toMatch(
+        /MyType is an invalid type\. Logical models require that the type be an absolute URL\./s
+      );
+      expect(loggerSpy.getLastMessage('warn')).toMatch(/File: LogicalModels\.fsh.*Line: 3/s);
     });
 
     it('should log an error when multiple logical models have the same id', () => {