Repository: FHIR/sushi
Pull request #325 (2020-04-01 13:50:54)

Fix root element of slice order
Two bugs are addressed by this PR. 

The first bug is that when an element is sliced, the `addElement` function would add children of that slice below the slices of that element. The correct behavior is to add the children of the element above the slices of the element.

The second bug fixed is related to #317. It turns out that a sliced element needs to be present in the differential if its children are changed alongside changes to the slices of that element.

This PR fixes those two bugs. One way to test it is using this snippet of FSH:
```
Extension: MyRace
Parent: http://hl7.org/fhir/us/core/StructureDefinition/us-core-race
* extension.id = "my-root-id"
* extension[ombCategory].id = "my-ombcat-id"
```
When you run SUSHI with this snippet on master, you will exhibits both of these bugs described above, and as a result causes errors in the IG Publisher. When you run SUSHI with this snippet on this branch, the output will be in the correct order, and have the correct elements added, so the IG Publisher will run without error, and produce the expected output.


Patch:
diff --git a/src/fhirtypes/ElementDefinition.ts b/src/fhirtypes/ElementDefinition.ts
index ba83fc9..bf860c1 100644
--- a/src/fhirtypes/ElementDefinition.ts
+++ b/src/fhirtypes/ElementDefinition.ts
@@ -324,10 +324,10 @@ export class ElementDefinition {
         // @ts-ignore
         return prop && !isEqual(this[prop], original[prop]);
       }) ||
-      // When a slice has children that changed, we must treat the slice as if it
-      // differs from the original. The IG Publisher requires slices with changed
+      // When a slice or a sliced element has children that changed, we must treat the slice or sliced element
+      // as if it differs from the original. The IG Publisher requires slices or sliced elements with changed
       // children to be in the differential, or the snapshot is incorrectly generated
-      (this.sliceName && this.children().some(c => c.hasDiff()))
+      ((this.sliceName || this.getSlices().length > 0) && this.children().some(c => c.hasDiff()))
     );
   }
 
diff --git a/src/fhirtypes/StructureDefinition.ts b/src/fhirtypes/StructureDefinition.ts
index 151d224..a6bd9da 100644
--- a/src/fhirtypes/StructureDefinition.ts
+++ b/src/fhirtypes/StructureDefinition.ts
@@ -133,8 +133,9 @@ export class StructureDefinition {
       if (element.id.startsWith(`${currentId}.`) || element.id.startsWith(`${currentId}:`)) {
         lastMatchId = currentId;
       } else if (
-        !currentId.startsWith(`${lastMatchId}.`) &&
-        !currentId.startsWith(`${lastMatchId}:`)
+        (!currentId.startsWith(`${lastMatchId}.`) && !currentId.startsWith(`${lastMatchId}:`)) ||
+        // If element is not a slice at this level, and the currentId is a slice, break to add children before slices
+        (element.id.startsWith(`${lastMatchId}.`) && currentId.startsWith(`${lastMatchId}:`))
       ) {
         break;
       }


Test patch:
diff --git a/test/fhirtypes/ElementDefinition.test.ts b/test/fhirtypes/ElementDefinition.test.ts
index 48c3b75..21d196f 100644
--- a/test/fhirtypes/ElementDefinition.test.ts
+++ b/test/fhirtypes/ElementDefinition.test.ts
@@ -306,6 +306,37 @@ describe('ElementDefinition', () => {
       expect(vsCatCoding.hasDiff()).toBeFalsy();
       expect(vsCat.hasDiff()).toBeFalsy();
     });
+
+    it('should detect a diff on a sliced element with changed children', () => {
+      const catCoding = resprate.findElementByPath('category.coding', fisher);
+      const cat = resprate.elements.find(e => e.id === 'Observation.category');
+      expect(catCoding.hasDiff()).toBeFalsy();
+      expect(cat.hasDiff()).toBeFalsy();
+      catCoding.min = 2;
+      expect(catCoding.hasDiff()).toBeTruthy();
+      expect(cat.hasDiff()).toBeTruthy();
+    });
+
+    it('should detect a diff on a sliced element with changed grandchildren', () => {
+      const catCodingId = resprate.findElementByPath('category.coding.id', fisher);
+      const cat = resprate.elements.find(e => e.id === 'Observation.category');
+      expect(catCodingId.hasDiff()).toBeFalsy();
+      expect(cat.hasDiff()).toBeFalsy();
+      catCodingId.short = 'id';
+      expect(catCodingId.hasDiff()).toBeTruthy();
+      expect(cat.hasDiff()).toBeTruthy();
+    });
+
+    it('should not detect a diff on a sliced element with no changed descendents', () => {
+      const vsCatCoding = resprate.elements.find(e => e.id === 'Observation.category:VSCat.coding');
+      const cat = resprate.elements.find(e => e.id === 'Observation.category');
+      expect(vsCatCoding.hasDiff()).toBeFalsy();
+      expect(cat.hasDiff()).toBeFalsy();
+      vsCatCoding.min = 2;
+      expect(vsCatCoding.hasDiff()).toBeTruthy();
+      // Changed child of slice, not of sliced element, should have no change
+      expect(cat.hasDiff()).toBeFalsy();
+    });
   });
 
   describe('#calculateDiff', () => {
diff --git a/test/fhirtypes/StructureDefinition.test.ts b/test/fhirtypes/StructureDefinition.test.ts
index 0dac1f1..0727eca 100644
--- a/test/fhirtypes/StructureDefinition.test.ts
+++ b/test/fhirtypes/StructureDefinition.test.ts
@@ -341,22 +341,27 @@ describe('StructureDefinition', () => {
     });
 
     it('should properly serialize snapshot and differential for constraints on children of slices', () => {
+      // element doesnt yet exist, must be added
+      resprate.findElementByPath('category.coding', fisher);
       let json = resprate.toJSON();
-      const vsCatCodingOriginal = json.snapshot.element.find(
-        (e: any) => e.id === 'Observation.category:VSCat.coding'
-      );
-      const vsCatOriginal = json.snapshot.element.find(
-        (e: any) => e.id === 'Observation.category:VSCat'
-      );
-      let vsCatCoding = resprate.elements.find(
-        (e: any) => e.id === 'Observation.category:VSCat.coding'
+      const vsCatOriginal = json.snapshot.element[18];
+      const vsCatCodingOriginal = json.snapshot.element[21];
+      const catOriginal = json.snapshot.element[13];
+      const catCodingOriginal = json.snapshot.element[16];
+
+      const vsCatCodingSD = resprate.elements.find(
+        e => e.id === 'Observation.category:VSCat.coding'
       );
-      vsCatCoding.short = 'Hello I am a change';
+      vsCatCodingSD.short = 'Hello I am a change';
+      const catCodingSD = resprate.elements.find(e => e.id === 'Observation.category.coding');
+      vsCatCodingSD.short = 'Hello I am a change';
+      catCodingSD.short = 'Hello I am also a change';
+
       json = resprate.toJSON();
-      vsCatCoding = json.snapshot.element.find(
-        (e: any) => e.id === 'Observation.category:VSCat.coding'
-      );
-      const vsCat = json.snapshot.element.find((e: any) => e.id === 'Observation.category:VSCat');
+      const vsCat = json.snapshot.element[18];
+      const vsCatCoding = json.snapshot.element[21];
+      const cat = json.snapshot.element[13];
+      const catCoding = json.snapshot.element[16];
 
       // Check Observation.category:VSCat and Observation.category:VSCat.coding elements on the snapshot
       expect(vsCatCoding.short).toBe('Hello I am a change');
@@ -364,13 +369,14 @@ describe('StructureDefinition', () => {
       delete vsCatCodingOriginal.short;
       expect(vsCatCoding).toEqual(vsCatCodingOriginal);
       expect(vsCat).toEqual(vsCatOriginal);
-
-      const vsCatCodingDiff = json.differential.element.find(
-        (e: any) => e.id === 'Observation.category:VSCat.coding'
-      );
-      const vsCatDiff = json.differential.element.find(
-        (e: any) => e.id === 'Observation.category:VSCat'
-      );
+      expect(catCoding.short).toBe('Hello I am also a change');
+      delete catCoding.short;
+      delete catCodingOriginal.short;
+      expect(catCoding).toEqual(catCodingOriginal);
+      expect(cat).toEqual(catOriginal);
+
+      const vsCatCodingDiff = json.differential.element[3];
+      const vsCatDiff = json.differential.element[2];
       expect(vsCatCodingDiff).toEqual({
         id: 'Observation.category:VSCat.coding',
         path: 'Observation.category.coding',
@@ -381,6 +387,18 @@ describe('StructureDefinition', () => {
         path: 'Observation.category',
         sliceName: 'VSCat'
       });
+
+      const catCodingDiff = json.differential.element[1];
+      const catDiff = json.differential.element[0];
+      expect(catCodingDiff).toEqual({
+        id: 'Observation.category.coding',
+        path: 'Observation.category.coding',
+        short: 'Hello I am also a change'
+      });
+      expect(catDiff).toEqual({
+        id: 'Observation.category',
+        path: 'Observation.category'
+      });
     });
   });
 
@@ -426,6 +444,13 @@ describe('StructureDefinition', () => {
       expect(observation.elements).toHaveLength(51);
       expect(observation.elements[22].id).toBe('Observation.value[x]:valueQuantity');
     });
+
+    it('should add children of sliced elements in the right place', () => {
+      const originalLength = resprate.elements.length;
+      resprate.addElement(new ElementDefinition('Observation.category.coding'));
+      expect(resprate.elements).toHaveLength(originalLength + 1);
+      expect(resprate.elements[14].id).toBe('Observation.category.coding');
+    });
   });
 
   describe('#findElement', () => {