Repository: FHIR/sushi
Pull request #1114 (2022-06-30 14:29:17)

Do not add an element that is defined by an ancestor
Fixes CIMPL-961.

When defining a Resource or Logical, it should not be possible to add an element with the same path as an inherited element. The following is an example of this:

```
Resource: MyResource
* extension 0..1 string "This should not be allowed"
```

This issue addresses this by adding a new error type: `ElementAlreadyDefinedError`. This error is thrown in StructureDefinition where we check if there already exists an element name that is defined by an ancestor.

To test: replicate the above example, if the error `ElementAlreadyDefinedError` is not thrown, then something is breaking.

jafeltra: @guhanthuran I re-reviewed this and agree with Mint's suggested test cases as well. The change to the test from this morning looks good to me! In addition to what Mint mentioned, there is still one outstanding comment from my initial review that should be fixed as well.
cmoesel: @guhanthuran -- Can you add those tests so we can wrap this PR up? It's _so close_!
guhanthuran: @jafeltra Added back the lines in the correct place. 

Patch:
diff --git a/src/errors/ElementAlreadyDefinedError.ts b/src/errors/ElementAlreadyDefinedError.ts
new file mode 100644
index 0000000..abd1260
--- /dev/null
+++ b/src/errors/ElementAlreadyDefinedError.ts
@@ -0,0 +1,7 @@
+export class ElementAlreadyDefinedError extends Error {
+  constructor(public elementId: string, public definitionId: string) {
+    super(
+      `Cannot define element ${elementId} on ${definitionId} because it has already been defined`
+    );
+  }
+}
diff --git a/src/errors/index.ts b/src/errors/index.ts
index 6686d31..6baa70f 100644
--- a/src/errors/index.ts
+++ b/src/errors/index.ts
@@ -56,3 +56,4 @@ export * from './InvalidChoiceTypeRulePathError';
 export * from './MismatchedBindingTypeError';
 export * from './ValidationError';
 export * from './InvalidElementForSlicingError';
+export * from './ElementAlreadyDefinedError';
diff --git a/src/fhirtypes/StructureDefinition.ts b/src/fhirtypes/StructureDefinition.ts
index bba1bad..d2a1c97 100644
--- a/src/fhirtypes/StructureDefinition.ts
+++ b/src/fhirtypes/StructureDefinition.ts
@@ -12,7 +12,8 @@ import {
   MissingSnapshotError,
   InvalidResourceTypeError,
   InvalidTypeAccessError,
-  ValidationError
+  ValidationError,
+  ElementAlreadyDefinedError
 } from '../errors';
 import {
   getArrayIndex,
@@ -366,6 +367,10 @@ export class StructureDefinition {
    * @returns {ElementDefinition} the new ElementDefinition
    */
   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}`)) {
+      throw new ElementAlreadyDefinedError(name, this.id);
+    }
     const el = this.elements[0].newChildElement(name);
     this.addElement(el);
     return el;


Test patch:
diff --git a/test/export/StructureDefinition.ResourceExporter.test.ts b/test/export/StructureDefinition.ResourceExporter.test.ts
index 17bb52f..c1b2aec 100644
--- a/test/export/StructureDefinition.ResourceExporter.test.ts
+++ b/test/export/StructureDefinition.ResourceExporter.test.ts
@@ -13,6 +13,7 @@ import {
   ContainsRule,
   FlagRule
 } from '../../src/fshtypes/rules';
+import { StructureDefinition } from '../../src/fhirtypes';
 
 describe('ResourceExporter', () => {
   let defs: FHIRDefinitions;
@@ -277,6 +278,85 @@ describe('ResourceExporter', () => {
     expect(exported.elements).toHaveLength(12); // 9 AlternateIdentification elements + 3 added elements
   });
 
+  it('should log an error when adding an element with the same path as an inherited element', () => {
+    const resource = new Resource('MyResource');
+    const addElementRule = new AddElementRule('extension');
+    addElementRule.min = 0;
+    addElementRule.max = '1';
+    resource.rules.push(addElementRule);
+    doc.resources.set(resource.name, resource);
+    const exported = exporter.export().resources;
+
+    expect(exported.length).toBe(1);
+    expect(exported[0].elements.filter(r => r.id === 'MyResource.extension')).toHaveLength(1);
+
+    expect(exported[0].name).toBe('MyResource');
+    expect(exported[0].id).toBe('MyResource');
+    expect(exported[0].type).toBe('MyResource');
+    expect(exported[0].baseDefinition).toBe(
+      'http://hl7.org/fhir/StructureDefinition/DomainResource'
+    );
+    expect(exported[0].elements).toHaveLength(9);
+
+    expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
+    expect(loggerSpy.getLastMessage('error')).toMatch(
+      `Cannot define element ${addElementRule.path} on ${resource.name} because it has already been defined`
+    );
+  });
+
+  it('should log an error when two rules add a new element with the same path', () => {
+    const resource = new Resource('MyResource');
+    const addElementRule = new AddElementRule('testElement');
+    addElementRule.min = 0;
+    addElementRule.max = '1';
+    const addElementRule2 = new AddElementRule('testElement');
+    addElementRule2.min = 0;
+    addElementRule2.max = '1';
+    resource.rules.push(addElementRule);
+    resource.rules.push(addElementRule2);
+    doc.resources.set(resource.name, resource);
+    const exported = exporter.export().resources;
+
+    expect(exported.length).toBe(1);
+    expect(exported[0].elements.filter(r => r.id === 'MyResource.testElement')).toHaveLength(1);
+
+    expect(exported[0].name).toBe('MyResource');
+    expect(exported[0].id).toBe('MyResource');
+    expect(exported[0].type).toBe('MyResource');
+    expect(exported[0].baseDefinition).toBe(
+      'http://hl7.org/fhir/StructureDefinition/DomainResource'
+    );
+    expect(exported[0].elements).toHaveLength(10);
+
+    expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
+    expect(loggerSpy.getLastMessage('error')).toMatch(
+      `Cannot define element ${addElementRule.path} on ${resource.name} because it has already been defined`
+    );
+  });
+
+  it('should log an error when a rule with the same path is added by directly calling newElement', () => {
+    doc = new FSHDocument('fileName');
+    const input = new FSHTank([doc], minimalConfig);
+    pkg = new Package(input.config);
+    const fisher = new TestFisher(input, defs, pkg);
+    const alternateIdentification: StructureDefinition = fisher.fishForStructureDefinition(
+      'AlternateIdentification'
+    ) as StructureDefinition;
+
+    const addElementRule = new AddElementRule('elem1');
+    addElementRule.min = 0;
+    addElementRule.max = '1';
+    const addElementRule2 = new AddElementRule('elem1');
+    addElementRule2.min = 0;
+    addElementRule2.max = '1';
+    alternateIdentification.newElement(addElementRule.path);
+    expect(() => {
+      alternateIdentification.newElement(addElementRule2.path);
+    }).toThrow(
+      'Cannot define element elem1 on AlternateIdentification because it has already been defined'
+    );
+  });
+
   it('should not log a warning when exporting a conformant resource', () => {
     const resource = new Resource('Foo');
     const caretRule = new CaretValueRule('');