Repository: FHIR/sushi
Pull request #1355 (2023-09-21 13:27:30)

Don't return instance rules when looking for id
Completes task [CIMPL-1169](https://standardhealthrecord.atlassian.net/browse/CIMPL-1169).

When searching for a rule that sets an id, check the rule's isInstance flag. Only return the rule if the flag is not set.

To explain how this was going infinite: an Instance had a rule that looked like this:
```
* id = procedure-death-certification-ut-example1
```
Because the value is unquoted, the rule's `isInstance` flag is set. So, when applying this rule during export, the exporter attempts to fish up an Instance with a matching name or id. When checking the same instance, `findIdAssignmentRule` would find this rule, meaning that it now wants to assign this Instance. But, the Instance must be exported before it can be assigned. So we start exporting the Instance, attempt to apply this rule, and thus we are stuck until the call stack fills up and the computer drops it all on the ground.

This FSH still results in an error, but a much more helpful one: the fisher reports that it can't find an Instance called `procedure-death-certification-ut-example1` to assign for this rule.

mint-thompson: @cmoesel I'm glad you asked about `url`, since that got me more curious about other related scenarios. As it turns out, _any_ situation where an Instance refers to itself leads to a stack overflow. For example, both of these Instances cause stack overflows:
```
Instance: MyQuestionnaire
InstanceOf: Questionnaire
* url = MyQuestionnaire
* status = #draft

Instance: MyObs
InstanceOf: Observation
Usage: #example
* status = #final
* code = #123
* valueString = MyObs
```

So, it looks like we may need to do a better job handling this in the more general case of AssignmentRules on Instances.

But to answer your original question: when checking for a `url`, we use `getValueFromFshRules`, which doesn't check if the rule has `isInstance` set, but also just returns the value as a string when a rule with the correct path is found. This implementation is safe as long as `isInstance` is not set, but either way, it does not cause a stack overflow. Also, it's not until processing the actual CaretValueRule that we get into trouble. For example, using this FSH:
```
Profile: ObsProfile
Parent: Observation
* ^url = http://hl7.org/some/example/StructureDefinition/ObsProfile
```
The call to `getUrlFromFshDefinition` at `StructureDefinitionExporter.ts#L309` returns the string `http://hl7.org/some/example/StructureDefinition/ObsProfile` without any trouble. But when processing the CaretValueRule, we get down to `StructureDefinitionExporter.ts#L1000`, with this line:
```ts
sd.setInstancePropertyByPath(rule.caretPath, fishedValue, this);
```
We get an error, since `fishedValue` is an InstanceDefinition that represents ObsProfile, which is not the right type here:
```
Cannot assign StructureDefinition value: ObsProfile. Value does not match element type: uri
```
This error is much better than filling up the stack, of course.

Patch:
diff --git a/src/fhirtypes/common.ts b/src/fhirtypes/common.ts
index bd432d7..f9038b1 100644
--- a/src/fhirtypes/common.ts
+++ b/src/fhirtypes/common.ts
@@ -880,7 +880,7 @@ export function replaceReferences<T extends AssignmentRule | CaretValueRule>(
   fisher: Fishable
 ): T {
   let clone: T;
-  const value = getRuleValue(rule);
+  const value = rule.value;
   if (value instanceof FshReference) {
     let type: string, id: string, instanceMeta: Metadata;
     // Prefer resolving to instances, so look them up first
@@ -945,7 +945,7 @@ export function replaceReferences<T extends AssignmentRule | CaretValueRule>(
     }
     if (type != null && id != null) {
       clone = cloneDeep(rule);
-      const assignedReference = getRuleValue(clone) as FshReference;
+      const assignedReference = clone.value as FshReference;
       // Set the sdType which will be used for type-checking during assignment
       assignedReference.sdType = type;
       // Only replace references that are specified using name or id. Absolute URLs or
@@ -982,7 +982,7 @@ export function replaceReferences<T extends AssignmentRule | CaretValueRule>(
     const codeSystemMeta = fisher.fishForMetadata(baseSystem, Type.CodeSystem);
     if (codeSystemMeta) {
       clone = cloneDeep(rule);
-      const assignedCode = getRuleValue(clone) as FshCode;
+      const assignedCode = clone.value as FshCode;
       assignedCode.system = value.system.replace(/^[^|]+/, codeSystemMeta.url);
 
       // Find the code system using the returned metadata to avoid duplicate warnings if version mismatches
@@ -1004,19 +1004,6 @@ export function replaceReferences<T extends AssignmentRule | CaretValueRule>(
   return clone ?? rule;
 }
 
-/**
- * Function to get a value from a rule that has a value (AssignedValue or CaretValue)
- * @param rule - The rule to get a value from
- * @returns - The value on the rule
- */
-function getRuleValue(rule: AssignmentRule | CaretValueRule): AssignmentValueType {
-  if (rule instanceof AssignmentRule) {
-    return rule.value;
-  } else if (rule instanceof CaretValueRule) {
-    return rule.value;
-  }
-}
-
 export function listUndefinedLocalCodes(
   codeSystem: FshCodeSystem | Instance,
   codes: string[],
@@ -1346,9 +1333,8 @@ export function isInheritedResource(
  * @param assignmentRulePath the path of the assignment rule whose value we want
  * @param caretRulePath the path of the caret value rule whose value we want
  * @param caretRuleCaretPath the caret path of the caret value rule
- * @returns the value set by either the assignment rule or the caret value rule or undefined if neither rule is set on the definition
- * NOTE: this function assumes the value being assigned is a string.
- * If other assigned types are needed, additional logic is needed before returning the value.
+ * @returns an object with the value set by either the assignment rule or the caret value rule, and whether or not the value represents an instance,
+ * or undefined if neither rule is set on the definition
  */
 function getValueFromFshRules(
   fshDefinition:
@@ -1364,18 +1350,15 @@ function getValueFromFshRules(
   assignmentRulePath: string,
   caretRulePath: string,
   caretRuleCaretPath: string
-): string | undefined {
+): { value: AssignmentValueType; isInstance: boolean } | undefined {
   const fshRules: Rule[] = fshDefinition.rules;
   if (fshDefinition instanceof Instance) {
     const assignmentRules = fshRules.filter(
-      rule =>
-        rule instanceof AssignmentRule &&
-        rule.path === assignmentRulePath &&
-        typeof rule.value === 'string'
+      rule => rule instanceof AssignmentRule && rule.path === assignmentRulePath
     ) as AssignmentRule[];
     if (assignmentRules.length > 0) {
       const lastAssignmentRule = assignmentRules[assignmentRules.length - 1];
-      return lastAssignmentRule.value.toString();
+      return { value: lastAssignmentRule.value, isInstance: lastAssignmentRule.isInstance };
     }
   } else {
     const caretValueRules = fshRules.filter(
@@ -1388,8 +1371,7 @@ function getValueFromFshRules(
       // Select last CaretValueRule with caretPath === caretRuleCaretPath because rules processing
       // ends up applying the last rule in the processing order
       const lastCaretValueRule = caretValueRules[caretValueRules.length - 1];
-      // For now, this value is assumed to be a string, but additional types may be supported in the future.
-      return lastCaretValueRule.value.toString();
+      return { value: lastCaretValueRule.value, isInstance: lastCaretValueRule.isInstance };
     }
   }
   return;
@@ -1399,6 +1381,8 @@ function getValueFromFshRules(
  * Determines the formal FHIR URL to use to refer to this entity (for example when fishing).
  * If a caret value rule has been applied to the entity's url, use the value specified in that
  * rule. Otherwise, use the default url based on the configured canonical url.
+ * Since a string should always be assigned here, only return the rule value if it is a string
+ * and not an Instance.
  *
  * @param fshDefinition - The FSH definition that the returned URL refers to
  * @param canonical - The canonical URL for the FSH project
@@ -1409,8 +1393,8 @@ export function getUrlFromFshDefinition(
   canonical: string
 ): string {
   const urlFromFshRules = getValueFromFshRules(fshDefinition, 'url', '', 'url');
-  if (urlFromFshRules != null) {
-    return urlFromFshRules;
+  if (typeof urlFromFshRules?.value === 'string' && !urlFromFshRules?.isInstance) {
+    return urlFromFshRules.value.toString();
   }
 
   let fhirType: string;
@@ -1428,6 +1412,8 @@ export function getUrlFromFshDefinition(
  * Determines the version of this entity.
  * If a caret value rule has been applied to the entity's version, use the value specified in that
  * rule. Otherwise, use the default version based on the configured version from the tank.
+ * Since a string should always be assigned here, only return the rule value if it is a string
+ * and not an Instance.
  *
  * @param fshDefinition - The FSH definition whose version is being determined
  * @param canonical - The version for the FSH project
@@ -1438,8 +1424,8 @@ export function getVersionFromFshDefinition(
   version: string
 ): string {
   const versionFromFshRules = getValueFromFshRules(fshDefinition, 'version', '', 'version');
-  if (versionFromFshRules != null) {
-    return versionFromFshRules;
+  if (typeof versionFromFshRules?.value === 'string' && !versionFromFshRules.isInstance) {
+    return versionFromFshRules.value.toString();
   }
 
   return version;
diff --git a/src/fshtypes/common.ts b/src/fshtypes/common.ts
index fdc82aa..238623c 100644
--- a/src/fshtypes/common.ts
+++ b/src/fshtypes/common.ts
@@ -49,13 +49,18 @@ export function findIdCaretRule(rules: Rule[]): CaretValueRule | undefined {
       rule instanceof CaretValueRule &&
       rule.path === '' &&
       rule.caretPath === 'id' &&
-      typeof rule.value === 'string'
+      typeof rule.value === 'string' &&
+      !rule.isInstance
   ) as CaretValueRule;
 }
 
 export function findIdAssignmentRule(rules: Rule[]): AssignmentRule | undefined {
   return findLast(
     rules,
-    rule => rule instanceof AssignmentRule && rule.path === 'id' && typeof rule.value === 'string'
+    rule =>
+      rule instanceof AssignmentRule &&
+      rule.path === 'id' &&
+      typeof rule.value === 'string' &&
+      !rule.isInstance
   ) as AssignmentRule;
 }


Test patch:
diff --git a/test/export/FHIRExporter.test.ts b/test/export/FHIRExporter.test.ts
index 1566bed..dc92de8 100644
--- a/test/export/FHIRExporter.test.ts
+++ b/test/export/FHIRExporter.test.ts
@@ -280,5 +280,29 @@ describe('FHIRExporter', () => {
         /Cannot assign number value: 1234\. Value does not match element type: Identifier.*File: Profiles\.fsh.*Line: 10\D*/s
       );
     });
+
+    it('should log a message and not change the URL when trying to assign an instance to a URL and the instance is not the correct type', () => {
+      // this represents the case where the author does not quote the url value, such as:
+      // * ^url = http://example.org/some/url
+      const profile = new Profile('MyObservation');
+      profile.parent = 'Observation';
+
+      const rule = new CaretValueRule('').withFile('UnquotedUrl.fsh').withLocation([4, 3, 4, 12]);
+      rule.caretPath = 'url';
+      rule.value = 'http://example.org/some/url';
+      rule.isInstance = true;
+      profile.rules.push(rule);
+      doc.profiles.set(profile.name, profile);
+      const result = exporter.export();
+
+      expect(result.profiles.length).toBe(1);
+      expect(result.profiles[0].url).toBe(
+        'http://hl7.org/fhir/us/minimal/StructureDefinition/MyObservation'
+      );
+      expect(loggerSpy.getLastMessage('error')).toMatch(
+        'Could not find a resource named http://example.org/some/url'
+      );
+      expect(loggerSpy.getLastMessage('error')).toMatch(/File: UnquotedUrl\.fsh.*Line: 4\D*/s);
+    });
   });
 });
diff --git a/test/fshtypes/Instance.test.ts b/test/fshtypes/Instance.test.ts
index 7a64d7a..d256c2d 100644
--- a/test/fshtypes/Instance.test.ts
+++ b/test/fshtypes/Instance.test.ts
@@ -15,6 +15,26 @@ describe('Instance', () => {
     });
   });
 
+  describe('#id', () => {
+    it('should return an id set by a string assignment rule', () => {
+      const p = new Instance('MyInstance');
+      const idRule = new AssignmentRule('id');
+      idRule.value = 'different-id';
+      p.rules.push(idRule);
+      expect(p.id).toBe('different-id');
+    });
+
+    it('should not return an id set by an instance assignment rule', () => {
+      const p = new Instance('MyInstance');
+      const idRule = new AssignmentRule('id');
+      idRule.value = 'different-id';
+      idRule.isInstance = true;
+      p.rules.push(idRule);
+      // the id is still the default
+      expect(p.id).toBe('MyInstance');
+    });
+  });
+
   describe('#toFSH', () => {
     it('should produce FSH for the simplest Instance', () => {
       const i = new Instance('MyInstance');
diff --git a/test/fshtypes/Profile.test.ts b/test/fshtypes/Profile.test.ts
index 4870a0e..86473a7 100644
--- a/test/fshtypes/Profile.test.ts
+++ b/test/fshtypes/Profile.test.ts
@@ -1,7 +1,13 @@
 import { EOL } from 'os';
 import 'jest-extended';
 import { Profile } from '../../src/fshtypes/Profile';
-import { CardRule, FlagRule, BindingRule, ObeysRule } from '../../src/fshtypes/rules';
+import {
+  CardRule,
+  FlagRule,
+  BindingRule,
+  ObeysRule,
+  CaretValueRule
+} from '../../src/fshtypes/rules';
 describe('Profile', () => {
   describe('#constructor', () => {
     it('should set the properties correctly', () => {
@@ -13,6 +19,27 @@ describe('Profile', () => {
     });
   });
 
+  describe('#id', () => {
+    it('should return an id set by a string caret rule', () => {
+      const p = new Profile('MyObservation');
+      const idRule = new CaretValueRule('');
+      idRule.caretPath = 'id';
+      idRule.value = 'different-id';
+      p.rules.push(idRule);
+      expect(p.id).toBe('different-id');
+    });
+
+    it('should not return an id set by an instance caret rule', () => {
+      const p = new Profile('MyObservation');
+      const idRule = new CaretValueRule('');
+      idRule.caretPath = 'id';
+      idRule.value = 'different-id';
+      idRule.isInstance = true;
+      p.rules.push(idRule);
+      expect(p.id).toBe('MyObservation');
+    });
+  });
+
   describe('#toFSH', () => {
     it('should produce FSH for the simplest profile', () => {
       const input = new Profile('SimpleProfile');