Repository: FHIR/sushi
Pull request #119 (2020-01-22 16:04:09)

Add source information to log messages, make best effort with instance rules
This change adds available source information to some log messages that did not previously have that information. Tests are added to verify that source information is present.

There is one important functional change made in order to make some of the source information tracking slightly more convenient: when an instance has an invalid `FixedValueRule`, the instance will still be exported with any valid rules. This behavior is consistent with other exporters, which similarly make a best effort to export as much as possible. A test is added for this scenario.

This pull request addresses https://standardhealthrecord.atlassian.net/browse/CIMPL-205, which is based on https://github.com/FHIR/sushi/issues/82 and https://github.com/FHIR/sushi/issues/102.


Patch:
diff --git a/src/export/InstanceExporter.ts b/src/export/InstanceExporter.ts
index df1783d..3989033 100644
--- a/src/export/InstanceExporter.ts
+++ b/src/export/InstanceExporter.ts
@@ -22,21 +22,25 @@ export class InstanceExporter {
     // All rules will be FixValueRule
     fshInstanceDef.rules.forEach(rule => {
       rule = replaceReferences(rule, this.tank, this.fisher);
-      const { fixedValue, pathParts } = instanceOfStructureDefinition.validateValueAtPath(
-        rule.path,
-        rule.fixedValue,
-        this.fisher
-      );
+      try {
+        const { fixedValue, pathParts } = instanceOfStructureDefinition.validateValueAtPath(
+          rule.path,
+          rule.fixedValue,
+          this.fisher
+        );
 
-      setPropertyOnInstance(instanceDef, pathParts, fixedValue);
-      // For each part of that path, we add fixed values from the SD
-      let path = '';
-      for (const [i, pathPart] of pathParts.entries()) {
-        path += `${path ? '.' : ''}${pathPart.base}`;
-        // Add back non-numeric (slice) brackets
-        pathPart.brackets?.forEach(b => (path += /^[-+]?\d+$/.test(b) ? '' : `[${b}]`));
-        const element = instanceOfStructureDefinition.findElementByPath(path, this.fisher);
-        this.setFixedValuesForDirectChildren(element, pathParts.slice(0, i + 1), instanceDef);
+        setPropertyOnInstance(instanceDef, pathParts, fixedValue);
+        // For each part of that path, we add fixed values from the SD
+        let path = '';
+        for (const [i, pathPart] of pathParts.entries()) {
+          path += `${path ? '.' : ''}${pathPart.base}`;
+          // Add back non-numeric (slice) brackets
+          pathPart.brackets?.forEach(b => (path += /^[-+]?\d+$/.test(b) ? '' : `[${b}]`));
+          const element = instanceOfStructureDefinition.findElementByPath(path, this.fisher);
+          this.setFixedValuesForDirectChildren(element, pathParts.slice(0, i + 1), instanceDef);
+        }
+      } catch (e) {
+        logger.error(e.message, rule.sourceInfo);
       }
     });
 
diff --git a/src/export/StructureDefinitionExporter.ts b/src/export/StructureDefinitionExporter.ts
index 87ac08d..91f48e2 100644
--- a/src/export/StructureDefinitionExporter.ts
+++ b/src/export/StructureDefinitionExporter.ts
@@ -191,7 +191,8 @@ export class StructureDefinitionExporter implements Fishable {
       logger.warn(
         `The definition of ${fshDefinition.name} may be incomplete because there is a circular ` +
           `dependency with its parent ${parentName} causing the parent to be used before the ` +
-          'parent has been fully processed.'
+          'parent has been fully processed.',
+        fshDefinition.sourceInfo
       );
     }
 
@@ -224,7 +225,7 @@ export class StructureDefinitionExporter implements Fishable {
       try {
         this.exportStructDef(sd);
       } catch (e) {
-        logger.error(e.message, e.sourceInfo);
+        logger.error(e.message, e.sourceInfo || sd.sourceInfo);
       }
     });
     return this.pkg;
diff --git a/src/utils/MasterFisher.ts b/src/utils/MasterFisher.ts
index 7d97e5b..e023cd6 100644
--- a/src/utils/MasterFisher.ts
+++ b/src/utils/MasterFisher.ts
@@ -90,7 +90,10 @@ export class MasterFisher implements Fishable {
             if (fhirMeta) {
               message += `\n  If the parent ${parentResult.name} is intended to refer to the FHIR resource, use its URL: ${fhirMeta.url}`;
             }
-            logger.error(message);
+            logger.error(
+              message,
+              fishable instanceof FSHTank ? fishable.fish(parent)?.sourceInfo : undefined
+            );
             return;
           }
           history.push(parentResult);


Test patch:
diff --git a/test/export/InstanceExporter.test.ts b/test/export/InstanceExporter.test.ts
index 513a9d8..eecc207 100644
--- a/test/export/InstanceExporter.test.ts
+++ b/test/export/InstanceExporter.test.ts
@@ -237,7 +237,9 @@ describe('InstanceExporter', () => {
       const instanceFixedValRule = new FixedValueRule('active');
       instanceFixedValRule.fixedValue = false;
       instance.rules.push(instanceFixedValRule);
-      expect(() => exportInstance(instance)).toThrow(
+      const exported = exportInstance(instance);
+      expect(exported.active).toBe(true);
+      expect(loggerSpy.getLastMessage()).toMatch(
         'Cannot fix false to this element; a different boolean is already fixed: true'
       );
     });
@@ -251,7 +253,12 @@ describe('InstanceExporter', () => {
       const instanceFixedFshCode = new FshCode('bar', 'http://bar.com');
       instanceFixedValRule.fixedValue = instanceFixedFshCode;
       instance.rules.push(instanceFixedValRule);
-      expect(() => exportInstance(instance)).toThrow(
+      const exported = exportInstance(instance);
+      expect(exported.maritalStatus.coding[0]).toEqual({
+        code: 'foo',
+        system: 'http://foo.com'
+      });
+      expect(loggerSpy.getLastMessage()).toMatch(
         'Cannot fix http://bar.com#bar to this element; a different code is already fixed: http://foo.com#foo.'
       );
     });
@@ -507,4 +514,37 @@ describe('InstanceExporter', () => {
       /* Need example of sliced primitive */
     });
   });
+
+  describe('#export', () => {
+    it('should still apply valid rules if one fails', () => {
+      const instance = new Instance('UnmeasurableAttribute');
+      instance.instanceOf = 'Patient';
+      const impossibleRule = new FixedValueRule('impossible');
+      impossibleRule.fixedValue = 'unmeasurable';
+      instance.rules.push(impossibleRule);
+      const possibleRule = new FixedValueRule('identifier.value');
+      possibleRule.fixedValue = 'Pascal';
+      instance.rules.push(possibleRule);
+      doc.instances.set(instance.name, instance);
+
+      const exported = exporter.export().instances;
+      expect(exported.length).toBe(1);
+      expect(exported[0].identifier[0].value).toBe('Pascal');
+    });
+
+    it('should log a message when the path for a fixed value is not found', () => {
+      const instance = new Instance('UnmeasurableAttribute');
+      instance.instanceOf = 'Patient';
+      const impossibleRule = new FixedValueRule('impossible')
+        .withFile('Unmeasurable.fsh')
+        .withLocation([3, 8, 3, 28]);
+      impossibleRule.fixedValue = 'unmeasurable';
+      instance.rules.push(impossibleRule);
+      doc.instances.set(instance.name, instance);
+
+      const exported = exporter.export().instances;
+      expect(exported.length).toBe(1);
+      expect(loggerSpy.getLastMessage()).toMatch(/File: Unmeasurable\.fsh.*Line: 3\D/s);
+    });
+  });
 });
diff --git a/test/export/StructureDefinitionExporter.test.ts b/test/export/StructureDefinitionExporter.test.ts
index d0960a0..6110d5c 100644
--- a/test/export/StructureDefinitionExporter.test.ts
+++ b/test/export/StructureDefinitionExporter.test.ts
@@ -807,7 +807,9 @@ describe('StructureDefinitionExporter', () => {
   });
 
   it('should log a warning message when we detect a circular dependency that causes an incomplete parent', () => {
-    const profile1 = new Profile('FooQuantity');
+    const profile1 = new Profile('FooQuantity')
+      .withFile('FooQuantity.fsh')
+      .withLocation([6, 7, 11, 33]);
     profile1.parent = 'BarQuantity';
     doc.profiles.set(profile1.name, profile1);
 
@@ -835,6 +837,7 @@ describe('StructureDefinitionExporter', () => {
     expect(lastLog.message).toMatch(
       /The definition of FooQuantity may be incomplete .* BarQuantity/
     );
+    expect(lastLog.message).toMatch(/File: FooQuantity\.fsh.*Line: 6 - 11\D/s);
   });
 
   it('should not apply an incorrect OnlyRule', () => {
@@ -1196,6 +1199,18 @@ describe('StructureDefinitionExporter', () => {
     );
   });
 
+  it('should log a message when exporting a package containing a sliced extension without a url', () => {
+    const profile = new Profile('Mystery').withFile('NoURL.fsh').withLocation([13, 1, 23, 28]);
+    profile.parent = 'Observation';
+
+    const rule = new ContainsRule('extension');
+    rule.items = ['mysterious'];
+    profile.rules.push(rule);
+    doc.profiles.set('Mystery', profile);
+    exporter.export();
+    expect(loggerSpy.getLastMessage()).toMatch(/File: NoURL\.fsh.*Line: 13 - 23\D/s);
+  });
+
   // toJSON
   it('should correctly generate a diff containing only changed elements', () => {
     // We already have separate tests for the differentials, so this just ensures that the
diff --git a/test/utils/MasterFisher.test.ts b/test/utils/MasterFisher.test.ts
index e48d7ca..6a44919 100644
--- a/test/utils/MasterFisher.test.ts
+++ b/test/utils/MasterFisher.test.ts
@@ -29,7 +29,10 @@ describe('MasterFisher', () => {
     doc1.profiles.get('Organization').id = 'my-org';
     doc1.profiles.get('Organization').parent =
       'http://hl7.org/fhir/StructureDefinition/Organization';
-    doc1.profiles.set('Practitioner', new Profile('Practitioner'));
+    doc1.profiles.set(
+      'Practitioner',
+      new Profile('Practitioner').withFile('Practitioner.fsh').withLocation([2, 8, 4, 28])
+    );
     doc1.profiles.get('Practitioner').id = 'my-dr';
     doc1.profiles.get('Practitioner').parent = 'Practitioner';
     const tank = new FSHTank([doc1], config);
@@ -99,6 +102,7 @@ describe('MasterFisher', () => {
     expect(loggerSpy.getLastMessage()).toMatch(
       /Circular dependency .* Practitioner < Practitioner/
     );
+    expect(loggerSpy.getLastMessage()).toMatch(/File: Practitioner\.fsh.*Line: 2 - 4\D/s);
     expect(resultMD).toEqual({
       id: 'my-dr',
       name: 'Practitioner',