Repository: FHIR/sushi
Pull request #478 (2020-06-04 19:19:14)

Bring back sparse differentials
Fixes https://github.com/FHIR/sushi/issues/463.

In https://github.com/FHIR/sushi/pull/453 we got rid of sparse differentials. Now sparse differentials are back! I mostly just removed the code and tests that were added in #453. So if you compare the contents of StructureDefinition.ts on this branch to the contents on the commit before #453 (https://github.com/FHIR/sushi/commit/b9bd6c78f377a83ff69833ed334b95022ea3e589), things should look very similar.

cmoesel: I realize this is a rather unconventional thing to do, but in reviewing your PR, I realized that there are *some* things about the approach I took for sparse differentials that I kind of like -- and personally prefer over the old way I wrote that code.  So... I issued a PR against your PR with the newish approach minus the special code for sparse differentials.  All the tests still pass.  Let me know what you think.  If you don't like it better, that's OK.

See: #479 

Patch:
diff --git a/src/fhirtypes/StructureDefinition.ts b/src/fhirtypes/StructureDefinition.ts
index ba13597..3b6fb94 100644
--- a/src/fhirtypes/StructureDefinition.ts
+++ b/src/fhirtypes/StructureDefinition.ts
@@ -337,20 +337,10 @@ export class StructureDefinition {
       j.snapshot = { element: this.elements.map(e => e.toJSON()) };
     }
 
-    // Create a cached hasDiff function since hasDiff can be expensive and may be called multiple times
-    // per element when generating the differential.
-    const hasDiffCache: Map<string, boolean> = new Map();
-    const cachedHasDiff = (element: ElementDefinition): boolean => {
-      if (!hasDiffCache.has(element.id)) {
-        hasDiffCache.set(element.id, element.hasDiff());
-      }
-      return hasDiffCache.get(element.id);
-    };
-
-    // Populate the differential (including intermediate elements to avoid a "sparse differential")
+    // Populate the differential
     j.differential = { element: [] };
     this.elements.forEach(e => {
-      if (cachedHasDiff(e)) {
+      if (e.hasDiff()) {
         const diff = e.calculateDiff().toJSON();
         const isTypeSlicingChoiceDiff =
           diff.id.endsWith('[x]') &&
@@ -367,19 +357,7 @@ export class StructureDefinition {
         // differential, as they are inferred. See: https://blog.fire.ly/2019/09/13/type-slicing-in-fhir-r4/
         if (!isTypeSlicingChoiceDiff) {
           j.differential.element.push(diff);
-        } else if (e.children().some(c => cachedHasDiff(c))) {
-          // Even for a type-slicing choice diff, we want at least a simple diff when it has children with a diff
-          j.differential.element.push({
-            id: e.id,
-            path: e.path
-          });
         }
-      } else if (e.children().some(c => cachedHasDiff(c))) {
-        // Since it has children with a diff, at least add a simple diff element to avoid "sparse differentials"
-        j.differential.element.push({
-          id: e.id,
-          path: e.path
-        });
       }
     });
 


Test patch:
diff --git a/test/export/StructureDefinitionExporter.test.ts b/test/export/StructureDefinitionExporter.test.ts
index c89301f..9298e88 100644
--- a/test/export/StructureDefinitionExporter.test.ts
+++ b/test/export/StructureDefinitionExporter.test.ts
@@ -3691,9 +3691,8 @@ describe('StructureDefinitionExporter', () => {
     const sd = pkg.profiles[0];
     const json = sd.toJSON();
 
-    expect(json.differential.element).toHaveLength(2);
+    expect(json.differential.element).toHaveLength(1);
     expect(json.differential.element).toEqual([
-      { id: 'Observation', path: 'Observation' },
       { id: 'Observation.subject', path: 'Observation.subject', min: 1 }
     ]);
   });
@@ -3720,16 +3719,8 @@ describe('StructureDefinitionExporter', () => {
     const sd = pkg.profiles[0];
     const json = sd.toJSON();
 
-    expect(json.differential.element).toHaveLength(4);
+    expect(json.differential.element).toHaveLength(2);
     expect(json.differential.element).toEqual([
-      {
-        id: 'Observation',
-        path: 'Observation'
-      },
-      {
-        id: 'Observation.code',
-        path: 'Observation.code'
-      },
       {
         id: 'Observation.code.coding',
         path: 'Observation.code.coding',
@@ -3792,12 +3783,8 @@ describe('StructureDefinitionExporter', () => {
     const sd = pkg.profiles[0];
     const json = sd.toJSON();
 
-    expect(json.differential.element).toHaveLength(8);
+    expect(json.differential.element).toHaveLength(7);
     expect(json.differential.element).toEqual([
-      {
-        id: 'Observation',
-        path: 'Observation'
-      },
       {
         id: 'Observation.component',
         path: 'Observation.component',
@@ -3857,16 +3844,8 @@ describe('StructureDefinitionExporter', () => {
     exporter.exportStructDef(profile);
     const sd = pkg.profiles[0];
     const json = sd.toJSON();
-    expect(json.differential.element).toHaveLength(3);
+    expect(json.differential.element).toHaveLength(1);
     expect(json.differential.element).toEqual([
-      {
-        id: 'Observation',
-        path: 'Observation'
-      },
-      {
-        id: 'Observation.code',
-        path: 'Observation.code'
-      },
       {
         id: 'Observation.code.coding:RespRateCode',
         path: 'Observation.code.coding',
diff --git a/test/fhirtypes/StructureDefinition.test.ts b/test/fhirtypes/StructureDefinition.test.ts
index 72745b9..a1a53e4 100644
--- a/test/fhirtypes/StructureDefinition.test.ts
+++ b/test/fhirtypes/StructureDefinition.test.ts
@@ -122,17 +122,13 @@ describe('StructureDefinition', () => {
       valueX.min = 1;
 
       const json = observation.toJSON();
-      expect(json.differential.element).toHaveLength(3);
+      expect(json.differential.element).toHaveLength(2);
       expect(json.differential.element[0]).toEqual({
-        id: 'Observation',
-        path: 'Observation'
-      });
-      expect(json.differential.element[1]).toEqual({
         id: 'Observation.code',
         path: 'Observation.code',
         short: 'Special observation code'
       });
-      expect(json.differential.element[2]).toEqual({
+      expect(json.differential.element[1]).toEqual({
         id: 'Observation.value[x]',
         path: 'Observation.value[x]',
         min: 1
@@ -146,17 +142,13 @@ describe('StructureDefinition', () => {
       valueX.min = 1;
 
       const json = observation.toJSON(false);
-      expect(json.differential.element).toHaveLength(3);
+      expect(json.differential.element).toHaveLength(2);
       expect(json.differential.element[0]).toEqual({
-        id: 'Observation',
-        path: 'Observation'
-      });
-      expect(json.differential.element[1]).toEqual({
         id: 'Observation.code',
         path: 'Observation.code',
         short: 'Special observation code'
       });
-      expect(json.differential.element[2]).toEqual({
+      expect(json.differential.element[1]).toEqual({
         id: 'Observation.value[x]',
         path: 'Observation.value[x]',
         min: 1
@@ -164,91 +156,19 @@ describe('StructureDefinition', () => {
       expect(json.snapshot).toBeUndefined();
     });
 
-    it('should contain intermediate elements in differential to avoid sparse differentials', () => {
+    it('should generate sparse differentials', () => {
       const componentCode = observation.elements.find(e => e.id === 'Observation.component.code');
       componentCode.short = 'Special component code';
 
       const json = observation.toJSON();
-      expect(json.differential.element).toHaveLength(3);
+      expect(json.differential.element).toHaveLength(1);
       expect(json.differential.element[0]).toEqual({
-        id: 'Observation',
-        path: 'Observation'
-      });
-      expect(json.differential.element[1]).toEqual({
-        id: 'Observation.component',
-        path: 'Observation.component'
-      });
-      expect(json.differential.element[2]).toEqual({
         id: 'Observation.component.code',
         path: 'Observation.component.code',
         short: 'Special component code'
       });
     });
 
-    it('should contain intermediate elements in differential to avoid sparse differentials without generating snapshot', () => {
-      const componentCode = observation.elements.find(e => e.id === 'Observation.component.code');
-      componentCode.short = 'Special component code';
-
-      const json = observation.toJSON(false);
-      expect(json.differential.element).toHaveLength(3);
-      expect(json.differential.element[0]).toEqual({
-        id: 'Observation',
-        path: 'Observation'
-      });
-      expect(json.differential.element[1]).toEqual({
-        id: 'Observation.component',
-        path: 'Observation.component'
-      });
-      expect(json.differential.element[2]).toEqual({
-        id: 'Observation.component.code',
-        path: 'Observation.component.code',
-        short: 'Special component code'
-      });
-    });
-
-    it('should contain intermediate elements in differential for children of choices', () => {
-      // If we have this:
-      //   * valueQuantity ^short = "value[x] Quantity choice"
-      // then value[x] should not appear because IG Publisher infers it.
-      // But if we have this:
-      //   * value[x].extension ^short = "Extension on value[x]"
-      //   * valueQuantity ^short = "the quantity choice"
-      // then it should appear since it has a direct child.
-      const valueX = observation.elements.find(e => e.id === 'Observation.value[x]');
-      valueX.sliceIt('type', '$this', false, 'open');
-      valueX.unfoldChoiceElementTypes(fisher);
-      const valueXExtension = observation.elements.find(
-        e => e.id === 'Observation.value[x].extension'
-      );
-      valueXExtension.short = 'Extension on value[x]';
-      const valueQuantity = valueX.addSlice('valueQuantity', new ElementDefinitionType('Quantity'));
-      valueQuantity.short = 'the quantity choice';
-
-      const json = observation.toJSON();
-      expect(json.differential.element).toHaveLength(4);
-      expect(json.differential.element[0]).toEqual({
-        id: 'Observation',
-        path: 'Observation'
-      });
-      expect(json.differential.element[1]).toEqual({
-        id: 'Observation.value[x]',
-        path: 'Observation.value[x]'
-      });
-      expect(json.differential.element[2]).toEqual({
-        id: 'Observation.value[x].extension',
-        path: 'Observation.value[x].extension',
-        short: 'Extension on value[x]'
-      });
-      expect(json.differential.element[3]).toEqual({
-        id: 'Observation.valueQuantity',
-        path: 'Observation.valueQuantity',
-        short: 'the quantity choice',
-        type: [{ code: 'Quantity' }],
-        min: 0,
-        max: '1'
-      });
-    });
-
     it('should reflect basic differential for structure definitions with no changes', () => {
       const json = observation.toJSON();
       expect(json.differential).toEqual({ element: [{ id: 'Observation', path: 'Observation' }] });
@@ -296,13 +216,8 @@ describe('StructureDefinition', () => {
       expect(valueStringSnapshot.sliceName).toEqual('valueString');
       expect(valueStringSnapshot.short).toBe('the string choice');
       // then check that differential has value[x] and shortcut syntax valueQuantity and valueString
-      expect(json.differential.element).toHaveLength(4);
-      const rootDiff = json.differential.element[0];
-      expect(rootDiff).toEqual({
-        id: 'Observation',
-        path: 'Observation'
-      });
-      const valueXDiff = json.differential.element[1];
+      expect(json.differential.element).toHaveLength(3);
+      const valueXDiff = json.differential.element[0];
       expect(valueXDiff).toEqual({
         id: 'Observation.value[x]',
         path: 'Observation.value[x]',
@@ -313,13 +228,13 @@ describe('StructureDefinition', () => {
           rules: 'open'
         }
       });
-      const valueQuantityDiff = json.differential.element[2];
+      const valueQuantityDiff = json.differential.element[1];
       expect(valueQuantityDiff.id).toBe('Observation.valueQuantity');
       expect(valueQuantityDiff.path).toBe('Observation.valueQuantity');
       expect(valueQuantityDiff.type).toEqual([{ code: 'Quantity' }]);
       expect(valueQuantityDiff.sliceName).toBeUndefined();
       expect(valueQuantitySnapshot.short).toBe('the quantity choice');
-      const valueStringDiff = json.differential.element[3];
+      const valueStringDiff = json.differential.element[2];
       expect(valueStringDiff.id).toBe('Observation.valueString');
       expect(valueStringDiff.path).toBe('Observation.valueString');
       expect(valueStringDiff.type).toEqual([{ code: 'string' }]);
@@ -360,19 +275,14 @@ describe('StructureDefinition', () => {
       expect(valueStringSnapshot.sliceName).toEqual('valueString');
       expect(valueStringSnapshot.short).toBe('the string choice');
       // then check that differential does NOT have value[x] but has shortcut syntax for valueQuantity and valueString
-      expect(json.differential.element).toHaveLength(3);
-      const rootDiff = json.differential.element[0];
-      expect(rootDiff).toEqual({
-        id: 'Observation',
-        path: 'Observation'
-      });
-      const valueQuantityDiff = json.differential.element[1];
+      expect(json.differential.element).toHaveLength(2);
+      const valueQuantityDiff = json.differential.element[0];
       expect(valueQuantityDiff.id).toBe('Observation.valueQuantity');
       expect(valueQuantityDiff.path).toBe('Observation.valueQuantity');
       expect(valueQuantityDiff.type).toEqual([{ code: 'Quantity' }]);
       expect(valueQuantityDiff.sliceName).toBeUndefined();
       expect(valueQuantitySnapshot.short).toBe('the quantity choice');
-      const valueStringDiff = json.differential.element[2];
+      const valueStringDiff = json.differential.element[1];
       expect(valueStringDiff.id).toBe('Observation.valueString');
       expect(valueStringDiff.path).toBe('Observation.valueString');
       expect(valueStringDiff.type).toEqual([{ code: 'string' }]);
@@ -417,13 +327,8 @@ describe('StructureDefinition', () => {
       expect(valueStringSnapshot.sliceName).toEqual('valueString');
       expect(valueStringSnapshot.short).toBe('the string choice');
       // then check that differential does NOT have value[x] but has shortcut syntax for valueQuantity and valueString
-      expect(json.differential.element).toHaveLength(4);
-      const rootDiff = json.differential.element[0];
-      expect(rootDiff).toEqual({
-        id: 'Observation',
-        path: 'Observation'
-      });
-      const valueXDiff = json.differential.element[1];
+      expect(json.differential.element).toHaveLength(3);
+      const valueXDiff = json.differential.element[0];
       expect(valueXDiff).toEqual({
         id: 'Observation.value[x]',
         path: 'Observation.value[x]',
@@ -434,13 +339,13 @@ describe('StructureDefinition', () => {
         },
         short: 'a choice of many things'
       });
-      const valueQuantityDiff = json.differential.element[2];
+      const valueQuantityDiff = json.differential.element[1];
       expect(valueQuantityDiff.id).toBe('Observation.valueQuantity');
       expect(valueQuantityDiff.path).toBe('Observation.valueQuantity');
       expect(valueQuantityDiff.type).toEqual([{ code: 'Quantity' }]);
       expect(valueQuantityDiff.sliceName).toBeUndefined();
       expect(valueQuantitySnapshot.short).toBe('the quantity choice');
-      const valueStringDiff = json.differential.element[3];
+      const valueStringDiff = json.differential.element[2];
       expect(valueStringDiff.id).toBe('Observation.valueString');
       expect(valueStringDiff.path).toBe('Observation.valueString');
       expect(valueStringDiff.type).toEqual([{ code: 'string' }]);
@@ -483,8 +388,8 @@ describe('StructureDefinition', () => {
       expect(catCoding).toEqual(catCodingOriginal);
       expect(cat).toEqual(catOriginal);
 
-      const vsCatCodingDiff = json.differential.element[4];
-      const vsCatDiff = json.differential.element[3];
+      const vsCatCodingDiff = json.differential.element[3];
+      const vsCatDiff = json.differential.element[2];
       expect(vsCatCodingDiff).toEqual({
         id: 'Observation.category:VSCat.coding',
         path: 'Observation.category.coding',
@@ -496,8 +401,8 @@ describe('StructureDefinition', () => {
         sliceName: 'VSCat'
       });
 
-      const catCodingDiff = json.differential.element[2];
-      const catDiff = json.differential.element[1];
+      const catCodingDiff = json.differential.element[1];
+      const catDiff = json.differential.element[0];
       expect(catCodingDiff).toEqual({
         id: 'Observation.category.coding',
         path: 'Observation.category.coding',
@@ -507,12 +412,6 @@ describe('StructureDefinition', () => {
         id: 'Observation.category',
         path: 'Observation.category'
       });
-
-      const rootDiff = json.differential.element[0];
-      expect(rootDiff).toEqual({
-        id: 'Observation',
-        path: 'Observation'
-      });
     });
   });