Repository: FHIR/sushi
Pull request #150 (2020-01-27 01:18:04)

Fix differentials related to choice type slices
1. Remove slicename from renamed choice type differential elements
2. Return root choice element ([x]) from differential when it's only there to introduce the type slice

This addresses #99.  Note that when you build the IG, you will often still see both `value[x]` and the specific choice (e.g., `valueString`) -- but you won't see the weird duplicate slice names anymore.  The reason you will usually still see `value[x]` is because we still must include `value[x]` in the differential if we constrained the choice to a smaller set of choices.  The only time we can exclude `value[x]` from the differential is if we _keep_ all the same types as valid choices.  See: https://blog.fire.ly/2019/09/13/type-slicing-in-fhir-r4/

If you build fsh-mcode, `CancerDiseaseStatus` shows instances of both cases.  

(a) The `EvidenceType` extension in `CancerDiseaseStatus` binds its `valueCodeableConcept` to a value set.  See the before and after in this screenshot.
![image](https://user-images.githubusercontent.com/2278253/73144945-761bad80-4078-11ea-92a2-b341f80dfd01.png)

(b) The `valueCodeableConcept` of `CancerDiseaseObservation` also binds to a value set.  Before and after here:
![image](https://user-images.githubusercontent.com/2278253/73144992-d6125400-4078-11ea-85c1-37a6a84a77de.png)

Note -- the after still looks weird (in a different way), but I believe it it actually technically correct based on the documentation.

ngfreiter: So if there is a line like: `* value[x] only valueCodeableConcept`, we do keep the `value[x]` element around? But if we just directly modify the `valueCodeableConcept` element, we do not keep the `value[x]` element around, since the slicing on it can be inferred? Am I understanding how this works correctly?
cmoesel: @ngfreiter:

> So if there is a line like: * value[x] only valueCodeableConcept, we do keep the value[x] element around?

Yes, and I got that from this:

--- start screenshot ---
<img width="769" alt="image" src="https://user-images.githubusercontent.com/2278253/73186534-e7497800-40ed-11ea-9441-39d5eb6d4d95.png">
--- end screenshot ---

and this:

--- start screenshot ---
![image](https://user-images.githubusercontent.com/2278253/73186632-18c24380-40ee-11ea-9d6d-1b6cca93f7b2.png)
--- end screenshot ---

> But if we just directly modify the valueCodeableConcept element, we do not keep the value[x] element around, since the slicing on it can be inferred?

Yes, and I got that from this:

--- start screenshot ---
<img width="776" alt="image" src="https://user-images.githubusercontent.com/2278253/73186718-3c858980-40ee-11ea-8c20-b63bf1c3ce29.png">
--- end screenshot ---

Patch:
diff --git a/src/fhirtypes/ElementDefinition.ts b/src/fhirtypes/ElementDefinition.ts
index 0f44116..218bce4 100644
--- a/src/fhirtypes/ElementDefinition.ts
+++ b/src/fhirtypes/ElementDefinition.ts
@@ -314,8 +314,14 @@ export class ElementDefinition {
         diff[prop] = cloneDeep(this[prop]);
       }
     }
-    // Path gets set automatically when setting id
+    // Set the diff id, which may be different than snapshot id in the case of choices (e.g., value[x] -> valueString)
+    // NOTE: The path also gets set automatically when setting id
     diff.id = diff.diffId();
+    // If the snapshot is a choice (e.g., value[x]), but the diff is a specific choice (e.g., valueString), then
+    // remove the slicename property from the diff (it is implied and not required in the diff)
+    if (this.path.endsWith('[x]') && !diff.path.endsWith('[x]')) {
+      delete diff.sliceName;
+    }
     return diff;
   }
 
diff --git a/src/fhirtypes/StructureDefinition.ts b/src/fhirtypes/StructureDefinition.ts
index 31eac01..e5b5f88 100644
--- a/src/fhirtypes/StructureDefinition.ts
+++ b/src/fhirtypes/StructureDefinition.ts
@@ -1,6 +1,10 @@
 import upperFirst from 'lodash/upperFirst';
 import cloneDeep from 'lodash/cloneDeep';
-import { ElementDefinition, ElementDefinitionType } from './ElementDefinition';
+import {
+  ElementDefinition,
+  ElementDefinitionType,
+  ElementDefinitionSlicing
+} from './ElementDefinition';
 import { Meta } from './specialTypes';
 import { Identifier, CodeableConcept, Coding, Narrative, Resource, Extension } from './dataTypes';
 import { ContactDetail, UsageContext } from './metaDataTypes';
@@ -314,6 +318,29 @@ export class StructureDefinition {
       element: this.elements.filter(e => e.hasDiff()).map(e => e.calculateDiff().toJSON())
     };
 
+    // Post-process the differential to remove any choice[x] elements if the only thing they do is establish the type
+    // slicing.  In this case, the type slicing can be inferred and does not need to be explicitly added.
+    // See: https://blog.fire.ly/2019/09/13/type-slicing-in-fhir-r4/
+    j.differential.element = j.differential.element.filter(
+      (e: { id: string; path: string; slicing: ElementDefinitionSlicing }) => {
+        return (
+          // not a choice, or
+          !e.id.endsWith('[x]') ||
+          // is a choice but not one whose only diff is the standard type slicing
+          !(
+            Object.keys(e).length === 3 &&
+            e.id &&
+            e.path &&
+            e.slicing?.discriminator?.length === 1 &&
+            e.slicing.discriminator[0].type === 'type' &&
+            e.slicing.discriminator[0].path === '$this' &&
+            e.slicing.rules === 'open' &&
+            (e.slicing.ordered == null || e.slicing.ordered === false)
+          )
+        );
+      }
+    );
+
     // If the StructureDefinition is in progress, we want to persist that in the JSON so that when
     // the Fisher retrieves it from a package and converts to JSON, the inProgress state will be
     // preserved.  But do NOT persist it when it is false.


Test patch:
diff --git a/test/fhirtypes/ElementDefinition.test.ts b/test/fhirtypes/ElementDefinition.test.ts
index 1c7e485..e74e6aa 100644
--- a/test/fhirtypes/ElementDefinition.test.ts
+++ b/test/fhirtypes/ElementDefinition.test.ts
@@ -244,11 +244,14 @@ describe('ElementDefinition', () => {
       valueX.sliceIt('type', '$this', false, 'open');
       const valueString = valueX.addSlice('valueString', new ElementDefinitionType('string'));
       const diff = valueString.calculateDiff();
+      // snapshot should retain formal syntax and slicename
       expect(valueString.id).toBe('Observation.value[x]:valueString');
       expect(valueString.path).toBe('Observation.value[x]');
+      expect(valueString.sliceName).toBe('valueString');
+      // differential should use shortcut syntax and remove slicename
       expect(diff.id).toBe('Observation.valueString');
       expect(diff.path).toBe('Observation.valueString');
-      expect(Object.keys(diff.toJSON()).length).toBe(Object.keys(valueString.toJSON()).length);
+      expect(diff.sliceName).toBeUndefined();
     });
   });
 
diff --git a/test/fhirtypes/StructureDefinition.test.ts b/test/fhirtypes/StructureDefinition.test.ts
index 905a683..3d2fcef 100644
--- a/test/fhirtypes/StructureDefinition.test.ts
+++ b/test/fhirtypes/StructureDefinition.test.ts
@@ -121,6 +121,177 @@ describe('StructureDefinition', () => {
         min: 1
       });
     });
+
+    it('should properly serialize snapshot and differential for constrained choice type with constraints on specific choices', () => {
+      // constrain value[x] to only a Quantity or string and give each its own short
+      const valueX = observation.elements.find(e => e.id === 'Observation.value[x]');
+      valueX.sliceIt('type', '$this', false, 'open');
+      valueX.constrainType([{ type: 'Quantity' }, { type: 'string' }], fisher);
+      const valueQuantity = valueX.addSlice('valueQuantity', new ElementDefinitionType('Quantity'));
+      valueQuantity.short = 'the quantity choice';
+      const valueString = valueX.addSlice('valueString', new ElementDefinitionType('string'));
+      valueString.short = 'the string choice';
+
+      const json = observation.toJSON();
+      // first check the snapshot value[x], value[x]:valueQuantity, and value[x]:valueString for formal correctness
+      const valueXSnapshot = json.snapshot.element[21];
+      expect(valueXSnapshot.id).toBe('Observation.value[x]');
+      expect(valueXSnapshot.path).toBe('Observation.value[x]');
+      expect(valueXSnapshot.type).toEqual([{ code: 'Quantity' }, { code: 'string' }]);
+      expect(valueXSnapshot.slicing).toEqual({
+        discriminator: [{ type: 'type', path: '$this' }],
+        ordered: false,
+        rules: 'open'
+      });
+      const valueQuantitySnapshot = json.snapshot.element[22];
+      expect(valueQuantitySnapshot.id).toBe('Observation.value[x]:valueQuantity');
+      expect(valueQuantitySnapshot.path).toBe('Observation.value[x]');
+      expect(valueQuantitySnapshot.type).toEqual([{ code: 'Quantity' }]);
+      expect(valueQuantitySnapshot.sliceName).toEqual('valueQuantity');
+      expect(valueQuantitySnapshot.short).toBe('the quantity choice');
+      const valueStringSnapshot = json.snapshot.element[23];
+      expect(valueStringSnapshot.id).toBe('Observation.value[x]:valueString');
+      expect(valueStringSnapshot.path).toBe('Observation.value[x]');
+      expect(valueStringSnapshot.type).toEqual([{ code: 'string' }]);
+      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(3);
+      const valueXDiff = json.differential.element[0];
+      expect(valueXDiff).toEqual({
+        id: 'Observation.value[x]',
+        path: 'Observation.value[x]',
+        type: [{ code: 'Quantity' }, { code: 'string' }],
+        slicing: {
+          discriminator: [{ type: 'type', path: '$this' }],
+          ordered: false,
+          rules: 'open'
+        }
+      });
+      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[2];
+      expect(valueStringDiff.id).toBe('Observation.valueString');
+      expect(valueStringDiff.path).toBe('Observation.valueString');
+      expect(valueStringDiff.type).toEqual([{ code: 'string' }]);
+      expect(valueStringDiff.sliceName).toBeUndefined();
+      expect(valueStringSnapshot.short).toBe('the string choice');
+    });
+
+    it('should properly serialize snapshot and differential for unconstrained choice type with constraints on specific choices', () => {
+      const valueX = observation.elements.find(e => e.id === 'Observation.value[x]');
+      valueX.sliceIt('type', '$this', false, 'open');
+      // note: NOT constraining value[x] types.  Just adding specific elements for valueQuantity and valueString.
+      const valueQuantity = valueX.addSlice('valueQuantity', new ElementDefinitionType('Quantity'));
+      valueQuantity.short = 'the quantity choice';
+      const valueString = valueX.addSlice('valueString', new ElementDefinitionType('string'));
+      valueString.short = 'the string choice';
+
+      const json = observation.toJSON();
+      // first check the snapshot value[x], value[x]:valueQuantity, and value[x]:valueString for formal correctness
+      const valueXSnapshot = json.snapshot.element[21];
+      expect(valueXSnapshot.id).toBe('Observation.value[x]');
+      expect(valueXSnapshot.path).toBe('Observation.value[x]');
+      expect(valueXSnapshot.type).toHaveLength(11);
+      expect(valueXSnapshot.slicing).toEqual({
+        discriminator: [{ type: 'type', path: '$this' }],
+        ordered: false,
+        rules: 'open'
+      });
+      const valueQuantitySnapshot = json.snapshot.element[22];
+      expect(valueQuantitySnapshot.id).toBe('Observation.value[x]:valueQuantity');
+      expect(valueQuantitySnapshot.path).toBe('Observation.value[x]');
+      expect(valueQuantitySnapshot.type).toEqual([{ code: 'Quantity' }]);
+      expect(valueQuantitySnapshot.sliceName).toEqual('valueQuantity');
+      expect(valueQuantitySnapshot.short).toBe('the quantity choice');
+      const valueStringSnapshot = json.snapshot.element[23];
+      expect(valueStringSnapshot.id).toBe('Observation.value[x]:valueString');
+      expect(valueStringSnapshot.path).toBe('Observation.value[x]');
+      expect(valueStringSnapshot.type).toEqual([{ code: 'string' }]);
+      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(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[1];
+      expect(valueStringDiff.id).toBe('Observation.valueString');
+      expect(valueStringDiff.path).toBe('Observation.valueString');
+      expect(valueStringDiff.type).toEqual([{ code: 'string' }]);
+      expect(valueStringDiff.sliceName).toBeUndefined();
+      expect(valueStringSnapshot.short).toBe('the string choice');
+    });
+
+    it('should properly serialize snapshot and differential for choice type with non-type constraint and with constraints on specific choices', () => {
+      // This is the same test as above, but we add a non-type constraint on value[x] to make sure it doesn't get
+      // blindly thrown away just because it doesn't constrain the types
+      const valueX = observation.elements.find(e => e.id === 'Observation.value[x]');
+      valueX.short = 'a choice of many things';
+      valueX.sliceIt('type', '$this', false, 'open');
+      // note: NOT constraining value[x] types.  Just adding specific elements for valueQuantity and valueString.
+      const valueQuantity = valueX.addSlice('valueQuantity', new ElementDefinitionType('Quantity'));
+      valueQuantity.short = 'the quantity choice';
+      const valueString = valueX.addSlice('valueString', new ElementDefinitionType('string'));
+      valueString.short = 'the string choice';
+
+      const json = observation.toJSON();
+      // first check the snapshot value[x], value[x]:valueQuantity, and value[x]:valueString for formal correctness
+      const valueXSnapshot = json.snapshot.element[21];
+      expect(valueXSnapshot.id).toBe('Observation.value[x]');
+      expect(valueXSnapshot.path).toBe('Observation.value[x]');
+      expect(valueXSnapshot.type).toHaveLength(11);
+      expect(valueXSnapshot.short).toBe('a choice of many things');
+      expect(valueXSnapshot.slicing).toEqual({
+        discriminator: [{ type: 'type', path: '$this' }],
+        ordered: false,
+        rules: 'open'
+      });
+      const valueQuantitySnapshot = json.snapshot.element[22];
+      expect(valueQuantitySnapshot.id).toBe('Observation.value[x]:valueQuantity');
+      expect(valueQuantitySnapshot.path).toBe('Observation.value[x]');
+      expect(valueQuantitySnapshot.type).toEqual([{ code: 'Quantity' }]);
+      expect(valueQuantitySnapshot.sliceName).toEqual('valueQuantity');
+      expect(valueQuantitySnapshot.short).toBe('the quantity choice');
+      const valueStringSnapshot = json.snapshot.element[23];
+      expect(valueStringSnapshot.id).toBe('Observation.value[x]:valueString');
+      expect(valueStringSnapshot.path).toBe('Observation.value[x]');
+      expect(valueStringSnapshot.type).toEqual([{ code: 'string' }]);
+      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 valueXDiff = json.differential.element[0];
+      expect(valueXDiff).toEqual({
+        id: 'Observation.value[x]',
+        path: 'Observation.value[x]',
+        slicing: {
+          discriminator: [{ type: 'type', path: '$this' }],
+          ordered: false,
+          rules: 'open'
+        },
+        short: 'a choice of many things'
+      });
+      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[2];
+      expect(valueStringDiff.id).toBe('Observation.valueString');
+      expect(valueStringDiff.path).toBe('Observation.valueString');
+      expect(valueStringDiff.type).toEqual([{ code: 'string' }]);
+      expect(valueStringDiff.sliceName).toBeUndefined();
+      expect(valueStringSnapshot.short).toBe('the string choice');
+    });
   });
 
   describe('#newElement', () => {