Repository: FHIR/sushi (ts)

Issue:
SUSHI beta does not check for duplicate resources between input/* and input/fsh
If there is a FSH definition that has the same canonical URL as a JSON/XML resource already in one of the other `input/*`, SUSHI should issue a warning and ignore the FSH definition. Currently it still generates the FSH definition, causing duplicates in the source to the IG Publisher.

Reported here: https://chat.fhir.org/#narrow/stream/215610-shorthand/topic/SUSHI.201.2E0.2E0.20Beta.20discussion/near/212163679

Pull request description:
Avoid Duplicate Resources in fsh-generated
Fixes #623 by avoiding writing a duplicate resource to the `fsh-generated` folder. Before any resource is written, the list of predefined resources (those coming from `input`) is checked to see if any of those resources match the `url` of the resource about to be written. If they do match, a warning is logged, and the FSH resource is not generated. This means that the pre-defined resource is preferred, and this matches how things work in previous versions of SUSHI, pre-defined resources have been preferred to FSH defined resources.

Patch:
diff --git a/src/app.ts b/src/app.ts
index 07078f4..cb8f790 100644
--- a/src/app.ts
+++ b/src/app.ts
@@ -146,7 +146,7 @@ async function app() {
 
   logger.info('Converting FSH to FHIR resources...');
   const outPackage = exportFHIR(tank, defs);
-  writeFHIRResources(outDir, outPackage, program.snapshot, isIgPubContext);
+  writeFHIRResources(outDir, outPackage, defs, program.snapshot, isIgPubContext);
 
   // If FSHOnly is true in the config, do not generate IG content, otherwise, generate IG content
   if (config.FSHOnly) {
diff --git a/src/fhirdefs/FHIRDefinitions.ts b/src/fhirdefs/FHIRDefinitions.ts
index a7a4b22..3c8bff7 100644
--- a/src/fhirdefs/FHIRDefinitions.ts
+++ b/src/fhirdefs/FHIRDefinitions.ts
@@ -66,6 +66,10 @@ export class FHIRDefinitions implements Fishable {
     return cloneJsonMapValues(this.implementationGuides);
   }
 
+  allPredefinedResources(): any[] {
+    return cloneJsonMapValues(this.predefinedResources);
+  }
+
   add(definition: any): void {
     if (definition.resourceType === 'StructureDefinition') {
       if (
@@ -103,6 +107,39 @@ export class FHIRDefinitions implements Fishable {
     return this.predefinedResources.get(file);
   }
 
+  resetPredefinedResources() {
+    this.predefinedResources = new Map();
+  }
+
+  fishForPredefinedResource(item: string, ...types: Type[]): any | undefined {
+    const resource = this.fishForFHIR(item, ...types);
+    if (
+      resource &&
+      this.allPredefinedResources().find(
+        predefResource =>
+          predefResource.id === resource.id &&
+          predefResource.resourceType === resource.resourceType &&
+          predefResource.url === resource.url
+      )
+    ) {
+      return resource;
+    }
+  }
+
+  fishForPredefinedResourceMetadata(item: string, ...types: Type[]): any | undefined {
+    const resource = this.fishForPredefinedResource(item, ...types);
+    if (resource) {
+      return {
+        id: resource.id as string,
+        name: resource.name as string,
+        sdType: resource.type as string,
+        url: resource.url as string,
+        parent: resource.baseDefinition as string,
+        abstract: resource.abstract as boolean
+      };
+    }
+  }
+
   fishForFHIR(item: string, ...types: Type[]): any | undefined {
     // No types passed in means to search ALL supported types
     if (types.length === 0) {
diff --git a/src/utils/MasterFisher.ts b/src/utils/MasterFisher.ts
index e023cd6..cc20ff0 100644
--- a/src/utils/MasterFisher.ts
+++ b/src/utils/MasterFisher.ts
@@ -29,8 +29,11 @@ export class MasterFisher implements Fishable {
     // Resolve the alias if necessary
     item = this.tank?.resolveAlias(item) ?? item;
 
+    let result = this.fhir.fishForPredefinedResource(item, ...types);
+    if (result != null) return result;
+
     // First check for it in the package
-    let result = this.pkg?.fishForFHIR(item, ...types);
+    result = this.pkg?.fishForFHIR(item, ...types);
     if (result == null) {
       // If it is in the tank, return undefined. We don't want to return the external FHIR
       // definition, even if it exists -- because it won't match what is in the tank.  This
@@ -56,9 +59,12 @@ export class MasterFisher implements Fishable {
     // Resolve the alias if necessary
     item = this.tank?.resolveAlias(item) ?? item;
 
+    let result = this.fhir.fishForPredefinedResourceMetadata(item, ...types);
+    if (result != null) return result;
+
     const fishables = [this.pkg, this.tank, this.fhir].filter(f => f != null);
     for (const fishable of fishables) {
-      const result = fishable.fishForMetadata(item, ...types);
+      result = fishable.fishForMetadata(item, ...types);
       if (result) {
         // If it came from the tank, we need to get the sdType because the tank doesn't know.
         if (fishable instanceof FSHTank) {
diff --git a/src/utils/Processing.ts b/src/utils/Processing.ts
index 7b8b9ff..b264c6a 100644
--- a/src/utils/Processing.ts
+++ b/src/utils/Processing.ts
@@ -210,23 +210,47 @@ export function fillTank(rawFSHes: RawFSH[], config: Configuration): FSHTank {
 export function writeFHIRResources(
   outDir: string,
   outPackage: Package,
+  defs: FHIRDefinitions,
   snapshot: boolean,
   isIgPubContext: boolean
 ) {
   logger.info('Exporting FHIR resources as JSON...');
   let count = 0;
+  const predefinedResources = defs.allPredefinedResources();
   const writeResources = (
     folder: string,
-    resources: { getFileName: () => string; toJSON: (snapshot: boolean) => any }[]
+    resources: {
+      getFileName: () => string;
+      toJSON: (snapshot: boolean) => any;
+      url?: string;
+      id?: string;
+      resourceType?: string;
+    }[]
   ) => {
     const exportDir = isIgPubContext
       ? path.join(outDir, 'fsh-generated', 'resources')
       : path.join(outDir, 'input', folder);
     resources.forEach(resource => {
-      fs.outputJSONSync(path.join(exportDir, resource.getFileName()), resource.toJSON(snapshot), {
-        spaces: 2
-      });
-      count++;
+      if (
+        !predefinedResources.find(
+          predef =>
+            predef.url === resource.url &&
+            predef.resourceType === resource.resourceType &&
+            predef.id === resource.id
+        )
+      ) {
+        fs.outputJSONSync(path.join(exportDir, resource.getFileName()), resource.toJSON(snapshot), {
+          spaces: 2
+        });
+        count++;
+      } else {
+        logger.error(
+          `Ignoring FSH definition for ${resource.url} since it duplicates existing pre-defined resource. ` +
+            'To use the FSH definition, remove the conflicting file from "input". ' +
+            'If you do want the FSH definition to be ignored, please comment the definition out ' +
+            'to remove this error.'
+        );
+      }
     });
   };
   writeResources('profiles', outPackage.profiles);


Test patch:
diff --git a/test/fhirdefs/FHIRDefinitions.test.ts b/test/fhirdefs/FHIRDefinitions.test.ts
index 83aec43..da2df04 100644
--- a/test/fhirdefs/FHIRDefinitions.test.ts
+++ b/test/fhirdefs/FHIRDefinitions.test.ts
@@ -25,6 +25,10 @@ describe('FHIRDefinitions', () => {
     fisher.fishForFHIR('w3c-provenance-activity-type');
   });
 
+  beforeEach(() => {
+    defs.resetPredefinedResources();
+  });
+
   describe('#fishForFHIR()', () => {
     it('should find base FHIR resources', () => {
       const conditionByID = defs.fishForFHIR('Condition', Type.Resource);
@@ -610,4 +614,118 @@ describe('FHIRDefinitions', () => {
       );
     });
   });
+
+  describe('#fishForPredefinedResource', () => {
+    it('should not find resources that are not predefined', () => {
+      const condition = defs.fishForFHIR('Condition');
+      expect(condition.id).toBe('Condition');
+      const predefinedCondition = defs.fishForPredefinedResource('Condition');
+      expect(predefinedCondition).toBeUndefined();
+    });
+
+    it('should not find resources that are predefined with different resourceTypes', () => {
+      const condition = defs.fishForFHIR('Condition');
+      expect(condition.id).toBe('Condition');
+      defs.addPredefinedResource('', {
+        resourceType: 'foo',
+        id: condition.id,
+        url: condition.url
+      });
+      const predefinedCondition = defs.fishForPredefinedResource('Condition');
+      expect(predefinedCondition).toBeUndefined();
+    });
+
+    it('should not find resources that are predefined with different ids', () => {
+      const condition = defs.fishForFHIR('Condition');
+      expect(condition.id).toBe('Condition');
+      defs.addPredefinedResource('', {
+        resourceType: condition.resourceType,
+        id: 'foo',
+        url: condition.url
+      });
+      const predefinedCondition = defs.fishForPredefinedResource('Condition');
+      expect(predefinedCondition).toBeUndefined();
+    });
+
+    it('should not find resources that are predefined with different urls', () => {
+      const condition = defs.fishForFHIR('Condition');
+      expect(condition.id).toBe('Condition');
+      defs.addPredefinedResource('', {
+        resourceType: condition.resourceType,
+        id: condition.id,
+        url: 'foo'
+      });
+      const predefinedCondition = defs.fishForPredefinedResource('Condition');
+      expect(predefinedCondition).toBeUndefined();
+    });
+
+    it('should find resources that are predefined', () => {
+      const condition = defs.fishForFHIR('Condition');
+      expect(condition.id).toBe('Condition');
+      defs.addPredefinedResource('', {
+        resourceType: condition.resourceType,
+        id: condition.id,
+        url: condition.url
+      });
+      const predefinedCondition = defs.fishForPredefinedResource('Condition');
+      expect(predefinedCondition.id).toBe('Condition');
+    });
+  });
+
+  describe('#fishForPredefinedResourceMetadata', () => {
+    it('should not find resources that are not predefined', () => {
+      const condition = defs.fishForFHIR('Condition');
+      expect(condition.id).toBe('Condition');
+      const predefinedCondition = defs.fishForPredefinedResourceMetadata('Condition');
+      expect(predefinedCondition).toBeUndefined();
+    });
+
+    it('should not find resources that are predefined with different resourceTypes', () => {
+      const condition = defs.fishForFHIR('Condition');
+      expect(condition.id).toBe('Condition');
+      defs.addPredefinedResource('', {
+        resourceType: 'foo',
+        id: condition.id,
+        url: condition.url
+      });
+      const predefinedCondition = defs.fishForPredefinedResourceMetadata('Condition');
+      expect(predefinedCondition).toBeUndefined();
+    });
+
+    it('should not find resources that are predefined with different ids', () => {
+      const condition = defs.fishForFHIR('Condition');
+      expect(condition.id).toBe('Condition');
+      defs.addPredefinedResource('', {
+        resourceType: condition.resourceType,
+        id: 'foo',
+        url: condition.url
+      });
+      const predefinedCondition = defs.fishForPredefinedResourceMetadata('Condition');
+      expect(predefinedCondition).toBeUndefined();
+    });
+
+    it('should not find resources that are predefined with different urls', () => {
+      const condition = defs.fishForFHIR('Condition');
+      expect(condition.id).toBe('Condition');
+      defs.addPredefinedResource('', {
+        resourceType: condition.resourceType,
+        id: condition.id,
+        url: 'foo'
+      });
+      const predefinedCondition = defs.fishForPredefinedResourceMetadata('Condition');
+      expect(predefinedCondition).toBeUndefined();
+    });
+
+    it('should find resources that are predefined', () => {
+      const condition = defs.fishForFHIR('Condition');
+      expect(condition.id).toBe('Condition');
+      defs.addPredefinedResource('', {
+        resourceType: condition.resourceType,
+        id: condition.id,
+        url: condition.url
+      });
+      const predefinedCondition = defs.fishForPredefinedResourceMetadata('Condition');
+      expect(predefinedCondition.id).toBe('Condition');
+    });
+  });
 });
diff --git a/test/utils/MasterFisher.test.ts b/test/utils/MasterFisher.test.ts
index aa9c669..2d4abd2 100644
--- a/test/utils/MasterFisher.test.ts
+++ b/test/utils/MasterFisher.test.ts
@@ -10,6 +10,7 @@ import { minimalConfig } from './minimalConfig';
 
 describe('MasterFisher', () => {
   let fisher: MasterFisher;
+  let defs: FHIRDefinitions;
   beforeAll(() => {
     const doc1 = new FSHDocument('doc.fsh');
     doc1.aliases.set('TankProfile1', 'http://hl7.org/fhir/us/minimal/StructureDefinition/prf1');
@@ -49,7 +50,7 @@ describe('MasterFisher', () => {
     profile4.fhirVersion = '4.0.1';
     pkg.profiles.push(profile3, profile4);
 
-    const defs = new FHIRDefinitions();
+    defs = new FHIRDefinitions();
     loadFromPath(
       path.join(__dirname, '..', 'testhelpers', 'testdefs', 'package'),
       'test#1.1.1',
@@ -153,6 +154,27 @@ describe('MasterFisher', () => {
     });
   });
 
+  it('should return a profile that is predefined when it also exists in the package', () => {
+    // Mark vital signs as predefined
+    const fhirDefinedVitalSigns = defs.fishForFHIR('vitalsigns');
+    defs.addPredefinedResource('', defs.fishForFHIR('vitalsigns'));
+
+    // Result should match the fhir defined version, not our defined version
+    const result = fisher.fishForFHIR('vitalsigns');
+    expect(result.name).toBe(fhirDefinedVitalSigns.name);
+
+    const resultMD = fisher.fishForMetadata('vitalsigns');
+    expect(resultMD).toEqual({
+      abstract: false,
+      id: fhirDefinedVitalSigns.id,
+      name: fhirDefinedVitalSigns.name,
+      sdType: fhirDefinedVitalSigns.type,
+      url: fhirDefinedVitalSigns.url,
+      parent: fhirDefinedVitalSigns.baseDefinition
+    });
+    defs.resetPredefinedResources();
+  });
+
   it('should not return the FHIR def for a resource if there is a profile w/ the same name in the tank', () => {
     const result = fisher.fishForFHIR('Organization');
     expect(result).toBeUndefined();
diff --git a/test/utils/Processing.test.ts b/test/utils/Processing.test.ts
index 927612b..374d1af 100644
--- a/test/utils/Processing.test.ts
+++ b/test/utils/Processing.test.ts
@@ -339,6 +339,7 @@ describe('Processing', () => {
     let tempRoot: string;
     let tempIGPubRoot: string;
     let outPackage: Package;
+    let defs: FHIRDefinitions;
 
     beforeAll(() => {
       tempRoot = temp.mkdirSync('output-dir');
@@ -346,6 +347,7 @@ describe('Processing', () => {
       const input = path.join(__dirname, 'fixtures', 'valid-yaml');
       const config = readConfig(input, false);
       outPackage = new Package(config);
+      defs = new FHIRDefinitions();
 
       const myProfile = new StructureDefinition();
       myProfile.id = 'my-profile';
@@ -390,7 +392,18 @@ describe('Processing', () => {
       myOtherInstance.id = 'my-other-instance';
       myOtherInstance.resourceType = 'Observation';
 
-      outPackage.profiles.push(myProfile);
+      const myPredefinedProfile = new StructureDefinition();
+      myPredefinedProfile.id = 'my-duplicate-profile';
+      myPredefinedProfile.url = 'http://example.com/StructureDefinition/my-duplicate-profile';
+      defs.addPredefinedResource(
+        'StructureDefinition-my-duplicate-profile.json',
+        myPredefinedProfile
+      );
+      const myFSHDefinedProfile = new StructureDefinition();
+      myFSHDefinedProfile.id = 'my-duplicate-profile';
+      myFSHDefinedProfile.url = 'http://example.com/StructureDefinition/my-duplicate-profile';
+
+      outPackage.profiles.push(myProfile, myFSHDefinedProfile);
       outPackage.extensions.push(myExtension);
       outPackage.valueSets.push(myValueSet);
       outPackage.codeSystems.push(myCodeSystem);
@@ -413,7 +426,7 @@ describe('Processing', () => {
 
     describe('IG Publisher mode', () => {
       beforeAll(() => {
-        writeFHIRResources(tempIGPubRoot, outPackage, false, true);
+        writeFHIRResources(tempIGPubRoot, outPackage, defs, false, true);
       });
 
       afterAll(() => {
@@ -438,11 +451,27 @@ describe('Processing', () => {
         expect(allGeneratedFiles).toContain('OperationDefinition-my-operation.json');
         expect(allGeneratedFiles).toContain('Observation-my-other-instance.json');
       });
+
+      it('should not write a resource if that resource already exists in the "input" folder', () => {
+        expect(
+          fs.existsSync(
+            path.join(
+              tempIGPubRoot,
+              'fsh-generated',
+              'resources',
+              'StructureDefinition-my-duplicate-profile.json'
+            )
+          )
+        ).toBeFalsy();
+        expect(loggerSpy.getLastMessage('error')).toMatch(
+          /Ignoring FSH definition for .*my-duplicate-profile/
+        );
+      });
     });
 
     describe('legacy IG Publisher mode and legacy flat tank', () => {
       beforeAll(() => {
-        writeFHIRResources(tempRoot, outPackage, false, false);
+        writeFHIRResources(tempRoot, outPackage, defs, false, false);
       });
 
       afterAll(() => {