Repository: FHIR/sushi
Pull request #1261 (2023-04-24 12:21:53)

When assigning a value on an Instance, convert primitive to object only if assigned with a slice name
Completes part of task [CIMPL-1091](https://standardhealthrecord.atlassian.net/browse/CIMPL-1091).

When assigning a primitive value on an Instance, if it is being assigned as an element in an array, and it has a slice name, convert it into an object immediately before assigning the value. In the previous implementation, the conversion happened as soon as a slice name was encountered anywhere in the assignment path, which meant that the assigned value would be an object even in cases where it was not assigned as an array element. This led to undesired behavior when a later assignment path did not include a slice name, leading to the primitive assignment value being assigned as if it were an object (that is, using `assignComplexValue`).

As an example, consider the following rule:
```
* identifier[myid].value = "123456"
```
Under the previous implementation, as the rule's path was traversed, the assigned value would turn into an object when using the first part of the path `identifier[myid]`, since it contains a slice name. So, the assigned value would be:
```
{
  assignedValue: "123456",
  _primitive: true
}
```
This led to complications later, when another rule would attempt to write to the same array position:
```
* identifier[0].value = "abc"
```
This rule contains no slice names, and the element at the end of the path is not an array element. But since the existing value at `identifier[0].value` is an object, `assignComplexValue` would be called with a string `assignedValue`! So, `assignComplexValue` dutifully performed its job of assigning each property of `assignedValue` to the existing value, leading to this:
```
{
  assignedValue: "123456"
  _primitive: true
  0: "a",
  1: "b",
  2: "c"
}
```

Under the new implementation, the assigned value at path `identifier[myid].value` is not converted into an object, because the last path part `value` does not include a slice name. So, when the later rule on `identifier[0].value` is processed, the existing value is _not_ an object, and the new `assignedValue` is overwritten with a simple `=` assignment.

As a question about desired behavior: as written in the test case, this should happen when the named slice is optional. But what if it's required? If the test case is changed so that the slice's cardinality is `1..1`, no errors are emitted regarding a missing child element. The functions that check for required child elements check cardinality, but not pattern/fixed properties. The usual warnings about not referring to named slices with a numeric index are emitted. Does this make the current implementation insufficient?

mint-thompson: In the full regression set, one repo showed changes: https://github.com/hl7-eu/unicom-ig. There are two instance definitions ([one](https://github.com/hl7-eu/unicom-ig/blob/e9c68ed61c4cad15f6bc83b4fb9d9a8c0428787a/input/fsh/exampleProductBundles/seCefuroximeStragen1.5gPowder.fsh#L42), [two](https://github.com/hl7-eu/unicom-ig/blob/e9c68ed61c4cad15f6bc83b4fb9d9a8c0428787a/input/fsh/exampleProductBundles/sePanodil500mgPowderInSachet.fsh#L43)) that contains more or less the exact pattern being addressed in this change.
mint-thompson: I've created [a PR on the unicom-ig repo](https://github.com/hl7-eu/unicom-ig/pull/127) that resolves the issue found during regression testing. So, with that, I think this is safe to merge.

Patch:
diff --git a/src/fhirtypes/common.ts b/src/fhirtypes/common.ts
index 20a8ae3..ff1de75 100644
--- a/src/fhirtypes/common.ts
+++ b/src/fhirtypes/common.ts
@@ -554,12 +554,6 @@ export function setPropertyOnInstance(
         if (current[key] == null) current[key] = [];
         sliceName = getSliceName(pathPart);
         if (sliceName) {
-          if (typeof assignedValue !== 'object') {
-            // When an assignedValue is a primitive but also a slice, we convert to an object so that
-            // the sliceName field can be tracked on the object. The _primitive field marks the object
-            // to later be converted back to a primitive by replaceField in cleanResource
-            assignedValue = { assignedValue, _primitive: true };
-          }
           const sliceIndices: number[] = [];
           // Find the indices where slices are placed
           const sliceExtensionUrl = fisher.fishForMetadata(sliceName)?.url;
@@ -614,6 +608,12 @@ export function setPropertyOnInstance(
             current._sliceName = sliceName;
           }
         } else {
+          if (sliceName && typeof assignedValue !== 'object') {
+            // When an assignedValue is a primitive but also a slice, we convert to an object so that
+            // the sliceName field can be tracked on the object. The _primitive field marks the object
+            // to later be converted back to a primitive by replaceField in cleanResource
+            assignedValue = { assignedValue, _primitive: true };
+          }
           if (typeof assignedValue === 'object') {
             Object.assign(current[key][index], assignedValue);
           } else {


Test patch:
diff --git a/test/export/InstanceExporter.test.ts b/test/export/InstanceExporter.test.ts
index 6a092a0..0344e30 100644
--- a/test/export/InstanceExporter.test.ts
+++ b/test/export/InstanceExporter.test.ts
@@ -8173,6 +8173,65 @@ describe('InstanceExporter', () => {
       );
     });
 
+    it('should overwrite optional slice values when a numeric index refers to a slice before the end of a path', () => {
+      // Profile: MyTestPatient
+      // Parent: Patient
+      // * identifier ^slicing.discriminator.type = #pattern
+      // * identifier ^slicing.discriminator.path = "system"
+      // * identifier ^slicing.rules = #open
+      // * identifier contains myid 0..1
+      // * identifier[myid].system = "http://myexample.org/identifier/myid"
+      const profile = new Profile('MyTestPatient');
+      profile.parent = 'Patient';
+      const typeRule = new CaretValueRule('identifier');
+      typeRule.caretPath = 'slicing.discriminator[0].type';
+      typeRule.value = new FshCode('pattern');
+      const pathRule = new CaretValueRule('identifier');
+      pathRule.caretPath = 'slicing.discriminator[0].path';
+      pathRule.value = 'system';
+      const rulesRule = new CaretValueRule('identifier');
+      rulesRule.caretPath = 'slicing.rules';
+      rulesRule.value = new FshCode('open');
+      const containsRule = new ContainsRule('identifier');
+      containsRule.items.push({ name: 'myid' });
+      const assignmentRule = new AssignmentRule('identifier[myid].system');
+      assignmentRule.value = 'http://myexample.org/identifier/myid';
+      profile.rules.push(typeRule, pathRule, rulesRule, containsRule, assignmentRule);
+      doc.profiles.set(profile.name, profile);
+
+      // Instance: MyTestPatientExample
+      // InstanceOf: MyTestPatient
+      // Usage: #example
+      // * identifier[myid].value = "123456"
+      // * identifier[0].system = "http://example.org/mrn"
+      // * identifier[0].value = "abc"
+      // * identifier[1].system = "http://anotherexample.org/mrn"
+      // * identifier[1].value = "123"
+      const instance = new Instance('MyTestPatientExample');
+      instance.instanceOf = 'MyTestPatient';
+      instance.usage = 'Example';
+      const myidValue = new AssignmentRule('identifier[myid].value');
+      myidValue.value = '123456';
+      const firstSystem = new AssignmentRule('identifier[0].system');
+      firstSystem.value = 'http://example.org/mrn';
+      const firstValue = new AssignmentRule('identifier[0].value');
+      firstValue.value = 'abc';
+      const secondSystem = new AssignmentRule('identifier[1].system');
+      secondSystem.value = 'http://anotherexample.org/mrn';
+      const secondValue = new AssignmentRule('identifier[1].value');
+      secondValue.value = '123';
+      instance.rules.push(myidValue, firstSystem, firstValue, secondSystem, secondValue);
+      doc.instances.set(instance.name, instance);
+
+      sdExporter.export();
+      const exported = exporter.export().instances;
+      const exportedInstance = exported.find(i => i._instanceMeta.name === 'MyTestPatientExample');
+      expect(exportedInstance.identifier).toEqual([
+        { system: 'http://example.org/mrn', value: 'abc' },
+        { system: 'http://anotherexample.org/mrn', value: '123' }
+      ]);
+    });
+
     it('should only export an instance once', () => {
       const bundleInstance = new Instance('MyBundle');
       bundleInstance.instanceOf = 'Bundle';