Repository: FHIR/sushi
Pull request #657 (2020-10-30 21:26:25)

Merge existing Mappings
This PR adds support for adding rules to existing `Mapping`s on StructureDefinitions ([CIMPL-583](https://standardhealthrecord.atlassian.net/browse/CIMPL-583)).

On master, if you were to make a `Mapping` that has the same `id` as one that is already on the Parent of the Profile you are adding the mapping to, SUSHI will add a duplicate Mapping at the top level of the StructureDefinition and will also add any rules. This PR makes it so that you can add a `Mapping` that has the same `id` as one already on the SD and add rules. As long as any specified metadata matches what is on the existing `Mapping`, no metadata will be added to the SD but any rules on the `Mapping` will be added to the SD. If there is any metadata specified on the `Mapping` that does not match what is on the Mapping on the SD, an error will be logged and no rules will be added.

I had also intended to include fixes for the two things @cmoesel pointed out [here](https://github.com/FHIR/GoFSH/pull/28#pullrequestreview-519770578), but after looking at the original US Core SDs, these changes seem to be caused by the caret value rules GoFSH was creating since we were accessing indexes in arrays in the differential. I believe all the mapping information should be the same as the original US Core SDs.

jafeltra: @cmoesel I've updated this to reflect the changes we talked about this morning. Hopefully the test description is a bit more clear, but all the new test descriptions are long, so I can try to reword them if they're still confusing.

27184ab makes it so that any inherited mapping that has additional or changed comments will add that field to the `mapping` on the structure definition. It also updates the test to check that the value on the `mapping` has been updated. I also added that check for the equality of the `mapping` for the first test to ensure nothing was changed when we don't update the metadata because there was no changed metadata. I can add that check to the other tests if we feel it is necessary.

29b5a6b just re-orders the tests so the test that checks for an error comes last in this group. The diff got confusing when I reordered it, so I just spit it into it's own commit so I could keep better track of the changes.

Patch:
diff --git a/src/export/MappingExporter.ts b/src/export/MappingExporter.ts
index 581d466..551608a 100644
--- a/src/export/MappingExporter.ts
+++ b/src/export/MappingExporter.ts
@@ -2,7 +2,7 @@ import { FSHTank } from '../import';
 import { Package } from '.';
 import { logger, Type, MasterFisher } from '../utils';
 import { Mapping } from '../fshtypes';
-import { StructureDefinition, idRegex } from '../fhirtypes';
+import { StructureDefinition, StructureDefinitionMapping, idRegex } from '../fhirtypes';
 import { InvalidFHIRIdError } from '../errors';
 import { MappingRule } from '../fshtypes/rules';
 import { applyInsertRules } from '../fhirtypes/common';
@@ -72,7 +72,43 @@ export class MappingExporter {
       Type.Extension
     ) as StructureDefinition;
     if (sourceStructDef) {
-      this.setMetadata(sourceStructDef, fshDefinition);
+      const parent = this.fisher.fishForFHIR(
+        sourceStructDef.baseDefinition,
+        Type.Resource,
+        Type.Type,
+        Type.Profile,
+        Type.Extension
+      );
+      const matchingParentMapping = parent?.mapping.find(
+        (m: StructureDefinitionMapping) => m.identity === fshDefinition.id
+      );
+      if (matchingParentMapping != null) {
+        const isMatchingTitle = fshDefinition.title
+          ? fshDefinition.title === matchingParentMapping.name
+          : true;
+        const isMatchingTarget = fshDefinition.target
+          ? fshDefinition.target === matchingParentMapping.uri
+          : true;
+        if (!isMatchingTitle || !isMatchingTarget) {
+          // If the mapping identity matches one on the parent, all other metadata must also match in order to merge MappingRules
+          logger.error(
+            `Unable to add Mapping ${fshDefinition.name} because it conflicts with one already on the parent of ${fshDefinition.source}.`,
+            fshDefinition.sourceInfo
+          );
+          return;
+        } else {
+          // Update parent mapping with additional or changed metadata (comment is the only property this can be the case for)
+          const inheritedMapping = sourceStructDef.mapping.find(
+            m => m.identity === fshDefinition.id
+          );
+          if (fshDefinition.description) {
+            inheritedMapping.comment = fshDefinition.description;
+          }
+        }
+      } else {
+        // Only add metadata if it does not already exist on the parent
+        this.setMetadata(sourceStructDef, fshDefinition);
+      }
       this.setMappingRules(sourceStructDef, fshDefinition);
     } else {
       logger.error(`Unable to find source "${fshDefinition.source}".`, fshDefinition.sourceInfo);


Test patch:
diff --git a/test/export/MappingExporter.test.ts b/test/export/MappingExporter.test.ts
index 9636c14..6e67178 100644
--- a/test/export/MappingExporter.test.ts
+++ b/test/export/MappingExporter.test.ts
@@ -1,9 +1,10 @@
+import path from 'path';
+import { cloneDeep } from 'lodash';
 import { MappingExporter, Package } from '../../src/export';
 import { FSHDocument, FSHTank } from '../../src/import';
 import { TestFisher } from '../testhelpers';
 import { loggerSpy } from '../testhelpers';
 import { FHIRDefinitions, loadFromPath } from '../../src/fhirdefs';
-import path from 'path';
 import { StructureDefinition } from '../../src/fhirtypes';
 import { Mapping, RuleSet } from '../../src/fshtypes';
 import { MappingRule, InsertRule, AssignmentRule } from '../../src/fshtypes/rules';
@@ -194,6 +195,127 @@ describe('MappingExporter', () => {
       exporter.export();
       expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
     });
+
+    it('should not log an error and not add metadata but add rules for a simple Mapping that is inherited from the parent', () => {
+      /**
+       * Mapping: rim
+       * Source: MyObservation
+       * * status -> "Something.new"
+       */
+
+      const mapping = new Mapping('rim');
+      mapping.source = 'MyObservation';
+      const newRule = new MappingRule('status');
+      newRule.map = 'Something.new';
+      mapping.rules.push(newRule);
+      doc.mappings.set(mapping.name, mapping);
+
+      const originalMappingLength = observation.mapping.length;
+      const status = observation.elements.find(e => e.id === 'Observation.status');
+      const originalStatusMappingLength = status.mapping.length;
+      const originalRimMapping = cloneDeep(observation.mapping.find(m => m.identity === 'rim'));
+
+      exporter.export();
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+      expect(observation.mapping.length).toBe(originalMappingLength); // No metadata added
+      expect(status.mapping.length).toBe(originalStatusMappingLength + 1); // New rule added to status element
+
+      // Metadata is same as the parent
+      const rimMapping = observation.mapping.find(m => m.identity === 'rim');
+      expect(rimMapping).toEqual(originalRimMapping);
+    });
+
+    it('should not log an error and not add metadata but add rules for a Mapping that is inherited from the parent with the same metadata', () => {
+      /**
+       * Mapping: rim
+       * Id: rim
+       * Source: MyObservation
+       * Title: "RIM Mapping"
+       * Target: "http://hl7.org/v3"
+       * * status -> "Something.new"
+       */
+
+      const mapping = new Mapping('rim');
+      mapping.source = 'MyObservation';
+      mapping.id = 'rim';
+      mapping.title = 'RIM Mapping';
+      mapping.target = 'http://hl7.org/v3';
+      const newRule = new MappingRule('status');
+      newRule.map = 'Something.new';
+      mapping.rules.push(newRule);
+      doc.mappings.set(mapping.name, mapping);
+
+      const originalMappingLength = observation.mapping.length;
+      const status = observation.elements.find(e => e.id === 'Observation.status');
+      const originalStatusMappingLength = status.mapping.length;
+
+      exporter.export();
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+      expect(observation.mapping.length).toBe(originalMappingLength); // No metadata added
+      expect(status.mapping.length).toBe(originalStatusMappingLength + 1); // New rule added to status element
+    });
+
+    it('should not log an error, should update metadata, and should add rules for a Mapping that is inherited from the parent and has additional metadata not on the parent', () => {
+      /**
+       * Mapping: rim
+       * Source: MyObservation
+       * Description: "A totally new description"
+       * * status -> "Something.new"
+       */
+
+      const mapping = new Mapping('rim');
+      mapping.source = 'MyObservation';
+      mapping.description = 'A totally new description'; // There is no comment on the parent rim mapping
+      const newRule = new MappingRule('status');
+      newRule.map = 'Something.new';
+      mapping.rules.push(newRule);
+      doc.mappings.set(mapping.name, mapping);
+
+      const originalMappingLength = observation.mapping.length;
+      const status = observation.elements.find(e => e.id === 'Observation.status');
+      const originalStatusMappingLength = status.mapping.length;
+      const originalRimMapping = cloneDeep(observation.mapping.find(m => m.identity === 'rim'));
+
+      exporter.export();
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+      expect(observation.mapping.length).toBe(originalMappingLength); // No metadata added
+      expect(status.mapping.length).toBe(originalStatusMappingLength + 1); // New rule added to status element
+
+      // New comment (Description) is added to mapping, all other metadata should be the same
+      const rimMapping = observation.mapping.find(m => m.identity === 'rim');
+      originalRimMapping.comment = 'A totally new description'; // Description is added
+      expect(rimMapping).toEqual(originalRimMapping);
+    });
+
+    it('should log an error and not add mapping or rules when a Mapping has the same identity as one on the parent but name or uri differs', () => {
+      /**
+       * Mapping: rim
+       * Id: rim
+       * Source: MyObservation
+       * Title: "RIM Mapping"
+       * Target: "http://real.org/not"
+       * * status -> "Something.new"
+       */
+
+      const mapping = new Mapping('rim');
+      mapping.source = 'MyObservation';
+      mapping.id = 'rim';
+      mapping.title = 'RIM Mapping';
+      mapping.target = 'http://real.org/not';
+      const newRule = new MappingRule('status');
+      newRule.map = 'Something.new';
+      mapping.rules.push(newRule);
+      doc.mappings.set(mapping.name, mapping);
+
+      const originalMappingLength = observation.mapping.length;
+      const status = observation.elements.find(e => e.id === 'Observation.status');
+      const originalStatusMappingLength = status.mapping.length;
+
+      exporter.export();
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
+      expect(observation.mapping.length).toBe(originalMappingLength); // No metadata added
+      expect(status.mapping.length).toBe(originalStatusMappingLength); // No rule added to status element
+    });
   });
 
   describe('#setMappingRules', () => {