Repository: FHIR/sushi
Pull request #821 (2021-06-01 17:38:25)

Fix issue with too strict checks on array assignment
This fixes a bug that can be seen with this example from FSHOnline: https://fshschool.org/FSHOnline/#/share/2RUORW7.

The bug occurred because we were too strict in our checking of if a value could be assigned to an element that was set by a `pattern[x]` field to an array. In the case that a field is set via `pattern[x]` to an array of values, and we are assigning a value on an instance, we should not be checking if the value being assigned matches the existing values, since FHIR does not prohibit additional elements from being added to an array of values set by `pattern[x]`. This PR loosens that restriction.

cmoesel: The original FSH seems to work correctly now, but I've noticed that if the instance sets a _different_ value at element 0, then it overwrites the supplied fixed value and SUSHI doesn't complain.  E.g., if I don't set `code.coding` at all in the `Instance`, then I get this -- which matches the pattern:
```
  "code": {
    "coding": [
      {
        "code": "8302-2",
        "system": "http://loinc.org"
      }
    ]
  },
```
That's good!  BUT... if I set the coding at index 0 to something else:
```
* code.coding = $loinc#8306-3 "Body height --lying"
```
then it just happily overwrites the value from the pattern and doesn't complain at all:
```
  "code": {
    "coding": [
      {
        "code": "8306-3",
        "system": "http://loinc.org",
        "display": "Body height --lying"
      }
    ]
  },
```

I think the right behavior would probably be to either: (a) add the required value from the pattern to the end of the array, or (b) keep the required value at element 0, but then issue an error if anyone tries to assign a different value to element 0.  I think there are upsides and downsides to both approaches.

I don't know if we should treat this as a separate bug (and approve this PR as-is) or try to fix it in this PR.  Technically speaking, I think this bug was introduced by the fix in this PR.  Before, SUSHI was too strict; now it is too lenient.
ngfreiter: To me this sorta like something that should be handled when we are validating if required elements are present. I think (a) could get confusing, for example, if someone does something like the original bug: https://fshschool.org/FSHOnline/#/share/2RUORW7, do they end up with two copies of the value set by the pattern? I'm not sure what the expected behavior actually is. And then (b) takes ordering out of the users hands when a `pattern[x]` does not indicate order. So my thinking would be (c), after processing, if we find that any array pattern elements are not present, we log an error. Which I think in practice is similar to (b), but would in theory allow someone to mess with order or array elements. Does that make sense? 
cmoesel: Yeah, OK.  So if we just tell them it's missing, they can choose to either repeat it themselves in their instance definition or start assigning other codes at index 1... That makes sense.

But... if we know it is missing, and we know what the pattern is, then couldn't we also fix it by adding it at the end of the array?  Then the author can still control ordering (by specifying the full array), but if they overwrite the required pattern and don't put it somewhere else in the array, then we pop it back on for them at the end.  Or is that too difficult because of where (in the SUSHI process) we do that type of validation?
ngfreiter: > Yeah, OK. So if we just tell them it's missing, they can choose to either repeat it themselves in their instance definition or start assigning other codes at index 1... That makes sense.
> 
> But... if we know it is missing, and we know what the pattern is, then couldn't we also fix it by adding it at the end of the array? Then the author can still control ordering (by specifying the full array), but if they overwrite the required pattern and don't put it somewhere else in the array, then we pop it back on for them at the end. Or is that too difficult because of where (in the SUSHI process) we do that type of validation?

I'm not totally sure, but my guess is that doing the validation will be pretty difficult, but adding the element back once we've done that validation doesn't seem as difficult.
cmoesel: OK.  As discussed, we will merge this PR is and track a separate issue for the required value validation / insertion.
cmoesel: Still needs a second reviewer though!
cmoesel: Yep, we've got a Jira for it!

Patch:
diff --git a/src/fhirtypes/ElementDefinition.ts b/src/fhirtypes/ElementDefinition.ts
index 3204198..1980438 100644
--- a/src/fhirtypes/ElementDefinition.ts
+++ b/src/fhirtypes/ElementDefinition.ts
@@ -1525,7 +1525,11 @@ export class ElementDefinition {
     const currentElementValue = this[fixedX] ?? this[patternX] ?? this.assignedByAnyParent();
     // For complex types, use isMatch to check if they are a subset, otherwise use isEqual
     const compareFn = typeof fhirValue === 'object' ? isMatch : isEqual;
-    if (currentElementValue != null && !compareFn(fhirValue, currentElementValue)) {
+    if (
+      currentElementValue != null &&
+      !Array.isArray(currentElementValue) &&
+      !compareFn(fhirValue, currentElementValue)
+    ) {
       // It's a different value and not a compatible subset (e.g., the new value doesn't contain the old)
       throw new ValueAlreadyAssignedError(fshValue, type, JSON.stringify(currentElementValue));
     }


Test patch:
diff --git a/test/export/InstanceExporter.test.ts b/test/export/InstanceExporter.test.ts
index 832c1cd..9e65af8 100644
--- a/test/export/InstanceExporter.test.ts
+++ b/test/export/InstanceExporter.test.ts
@@ -924,6 +924,30 @@ describe('InstanceExporter', () => {
       );
     });
 
+    it('should assign an element to a value different than the pattern value on the Structure Definition on an array', () => {
+      const assignedValRule = new AssignmentRule('maritalStatus');
+      const assignedFshCode = new FshCode('foo', 'http://foo.com');
+      assignedValRule.value = assignedFshCode;
+      patient.rules.push(assignedValRule);
+      const cardRule = new CardRule('maritalStatus');
+      cardRule.min = 1;
+      patient.rules.push(cardRule);
+      const instanceAssignedValRule = new AssignmentRule('maritalStatus.coding[1]');
+      const instanceAssignedFshCode = new FshCode('bar', 'http://bar.com');
+      instanceAssignedValRule.value = instanceAssignedFshCode;
+      patientInstance.rules.push(instanceAssignedValRule);
+      const exported = exportInstance(patientInstance);
+      expect(exported.maritalStatus.coding[0]).toEqual({
+        code: 'foo',
+        system: 'http://foo.com'
+      });
+      expect(exported.maritalStatus.coding[1]).toEqual({
+        code: 'bar',
+        system: 'http://bar.com'
+      });
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+    });
+
     // Nested elements
     it('should assign a nested element that has parents defined in the instance and is assigned on the Structure Definition', () => {
       const cardRule = new CardRule('communication.preferred');
diff --git a/test/fhirtypes/ElementDefinition.assignValue.test.ts b/test/fhirtypes/ElementDefinition.assignValue.test.ts
index 0181469..fb033ee 100644
--- a/test/fhirtypes/ElementDefinition.assignValue.test.ts
+++ b/test/fhirtypes/ElementDefinition.assignValue.test.ts
@@ -56,22 +56,6 @@ describe('ElementDefinition', () => {
       );
     });
 
-    it('should throw ValueAlreadyAssignedError when assigning a value assigned via parent pattern to a conflicting array', () => {
-      const medicationForm = medication.elements.find(e => e.id === 'Medication.form');
-      medicationForm.patternCodeableConcept = { coding: [{ system: 'foo' }, { system: 'bar' }] };
-      const medicationFormCodingSystem = medication.findElementByPath('form.coding.system', fisher);
-      expect(() => {
-        medicationFormCodingSystem.assignValue('baz');
-      }).toThrow(
-        'Cannot assign "baz" to this element; a different uri is already assigned: ["foo","bar"].'
-      );
-      expect(() => {
-        medicationFormCodingSystem.assignValue('baz', true);
-      }).toThrow(
-        'Cannot assign "baz" to this element; a different uri is already assigned: ["foo","bar"].'
-      );
-    });
-
     it('should ensure that minimum cardinality is 1 when assigning a value mentioned in a parent slice discriminator', () => {
       const cat = medicationRequest.elements.find(e => e.id === 'MedicationRequest.category');
       cat.slicing = { discriminator: [{ type: 'value', path: 'coding.code' }], rules: 'open' };