Repository: FHIR/sushi (ts)

Issue:
Warn when users access slices by numeric indices (or soft indexing)
SUSHI auto-populates _required_ slices into instances before processing any rules.  When users then write rules for the same array by number (or soft-index), then they may be accessing these pre-populated slice entries (intentionally or unintentionally).

It is much safer to write rules that call out the slices by name rather than using indices (which are subject to the order in which SUSHI has written the slices into the instance). So... SUSHI should warn users when they do this.

This ultimately helps lead us toward a situation in which we can more safely change how SUSHI handles slices and soft indexing.

See #1028 for one relevant example where this is a problem.

Pull request description:
Warn when a slice element is accessed via numeric index
Fixes #1030, logging a warning when users access a slice using a numeric index. This warning is only logged when an array has it's slicing rules set to `closed` (in which case all elements in the array are slices by definition), or if the numeric index is determined to reference a slice that will be pre-loaded onto an instance as an implied property. A slice will be loaded automatically if it's cardinality indicates that it is required _and_ it contains a `pattern[x]` or `fixed[x]` value on itself or any of its children. 

This [FSH Online link](https://fshschool.org/FSHOnline/#/share/3ryTLXM) includes test cases.

Patch:
diff --git a/src/export/InstanceExporter.ts b/src/export/InstanceExporter.ts
index f80bb82..afd6d6a 100644
--- a/src/export/InstanceExporter.ts
+++ b/src/export/InstanceExporter.ts
@@ -119,7 +119,8 @@ export class InstanceExporter implements Fishable {
           rule.path,
           rule.value,
           this.fisher,
-          inlineResourceTypes
+          inlineResourceTypes,
+          rule.sourceInfo
         );
         // Record each valid rule in a map
         // Choice elements on an instance must use a specific type, so if the path still has an unchosen choice element,
diff --git a/src/fhirtypes/ElementDefinition.ts b/src/fhirtypes/ElementDefinition.ts
index 7472133..fcb8843 100644
--- a/src/fhirtypes/ElementDefinition.ts
+++ b/src/fhirtypes/ElementDefinition.ts
@@ -662,6 +662,34 @@ export class ElementDefinition {
     );
   }
 
+  /**
+   * Returns an array of slices that will be pre-loaded.
+   * A slice is pre-loaded if if has a min of 1 and contains a fixed or pattern value on itself or it's descendents
+   * @returns {ElementDefinition[]} - Array of slices to be pre-loaded
+   */
+  getPreloadedSlices(): ElementDefinition[] {
+    return this.getSlices().filter(
+      slice =>
+        slice.min > 0 &&
+        (Object.keys(slice).find(k => k.startsWith('fixed') || k.startsWith('pattern')) ||
+          slice
+            .getAssignableDescendents()
+            .some((element: ElementDefinition) =>
+              Object.keys(element).find(k => k.startsWith('fixed') || k.startsWith('pattern'))
+            ))
+    );
+  }
+
+  /**
+   * Determines if an array index references a slice that will be preloaded.
+   * A slice is pre-loaded if if has a min of 1 and contains a fixed or pattern value on itself or it's descendents
+   * @param {number} sliceIndex - The index
+   * @returns {boolean}
+   */
+  isPreloadedSlice(sliceIndex: number): boolean {
+    return sliceIndex <= this.getPreloadedSlices().length - 1;
+  }
+
   /**
    * Constrains the cardinality of this element.  Cardinality constraints can only narrow
    * cardinality.  Attempts to constrain to a wider cardinality will throw.
diff --git a/src/fhirtypes/StructureDefinition.ts b/src/fhirtypes/StructureDefinition.ts
index 047ff9b..f75b7af 100644
--- a/src/fhirtypes/StructureDefinition.ts
+++ b/src/fhirtypes/StructureDefinition.ts
@@ -16,6 +16,7 @@ import {
 } from '../errors';
 import {
   getArrayIndex,
+  getSliceName,
   setPropertyOnDefinitionInstance,
   isInheritedResource,
   isExtension
@@ -26,6 +27,8 @@ import { applyMixins } from '../utils/Mixin';
 import { parseFSHPath, assembleFSHPath } from '../utils/PathUtils';
 import { InstanceDefinition } from './InstanceDefinition';
 import { isUri } from 'valid-url';
+import { logger } from '../utils';
+import { SourceInfo } from '../fshtypes';
 
 /**
  * A class representing a FHIR R4 StructureDefinition.  For the most part, each allowable property in a StructureDefinition
@@ -499,6 +502,7 @@ export class StructureDefinition {
    * @param {any} value - The value to assign; use null to validate just the path when you know the value is valid
    * @param {Fishable} fisher - A fishable implementation for finding definitions and metadata
    * @param {string[]} inlineResourceTypes - Types that will be used to replace Resource elements
+   * @param {SourceInfo} sourceInfo - Source info of the rule being validated
    * @throws {CannotResolvePathError} when the path cannot be resolved to an element
    * @throws {InvalidResourceTypeError} when setting resourceType to an invalid value
    * @returns {any} - The object or value to assign
@@ -507,7 +511,8 @@ export class StructureDefinition {
     path: string,
     value: any,
     fisher: Fishable,
-    inlineResourceTypes: string[] = []
+    inlineResourceTypes: string[] = [],
+    sourceInfo: SourceInfo = null
   ): { assignedValue: any; pathParts: PathPart[] } {
     const pathParts = parseFSHPath(path);
     let currentPath = '';
@@ -520,6 +525,7 @@ export class StructureDefinition {
       currentPath += `${currentPath ? '.' : ''}${pathPart.base}`;
       // If we are indexing into an array, the last bracket should be numeric
       const arrayIndex = getArrayIndex(pathPart);
+      const sliceName = arrayIndex !== null ? getSliceName(pathPart) : null;
       if (arrayIndex != null) {
         // If it is a number, add all bracket info besides it back to path
         pathPart.brackets.slice(0, -1).forEach(p => (currentPath += `[${p}]`));
@@ -528,6 +534,22 @@ export class StructureDefinition {
         pathPart.brackets?.forEach(p => (currentPath += `[${p}]`));
       }
       currentElement = this.findElementByPath(currentPath, fisher);
+      // If the current element is sliced and the element is being accesed by numeric
+      // indices, warn to use the sliceName in the following cases:
+      // 1. The slicing is closed in which case a slice is certainly being accessed numerically
+      // 2. The numeric index references a slice that will be preloaded
+      if (
+        currentElement &&
+        currentElement.slicing &&
+        !sliceName &&
+        (currentElement.slicing.rules === 'closed' ||
+          currentElement.isPreloadedSlice(arrayIndex || 0))
+      ) {
+        logger.warn(
+          `Sliced element ${currentElement.id} is being accessed via numeric index. Use slice names in rule paths when possible.`,
+          sourceInfo
+        );
+      }
       // Allow for adding extension elements to the instance that are not on the SD
       if (!currentElement && isExtension(pathPart.base)) {
         // Get extension element (if currentPath is A.B.extension[C], get A.B.extension)


Test patch:
diff --git a/test/export/InstanceExporter.test.ts b/test/export/InstanceExporter.test.ts
index 56c5d2a..e511cc0 100644
--- a/test/export/InstanceExporter.test.ts
+++ b/test/export/InstanceExporter.test.ts
@@ -3432,6 +3432,149 @@ describe('InstanceExporter', () => {
       expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
     });
 
+    it('should log a warning when a pre-loaded element in a sliced array is accessed with a numeric index', () => {
+      const caretRule = new CaretValueRule('code');
+      caretRule.caretPath = 'slicing.discriminator.path';
+      caretRule.value = 'type';
+      const dTypeRule = new CaretValueRule('code');
+      dTypeRule.caretPath = 'slicing.discriminator.type';
+      dTypeRule.value = new FshCode('value');
+      const rulesRule = new CaretValueRule('code');
+      rulesRule.caretPath = 'slicing.rules';
+      rulesRule.value = new FshCode('open');
+      const containsRule = new ContainsRule('code');
+      containsRule.items.push({ name: 'boo' });
+      const cardRule = new CardRule('code[boo]');
+      cardRule.min = 1;
+      cardRule.max = '1';
+      const codeRule = new AssignmentRule('code[boo]');
+      codeRule.value = new FshCode('1-8', 'http://loinc.org/', 'Acyclovir [Susceptibility]');
+      // * code ^slicing.discriminator[0].path = "type"
+      // * code ^slicing.discriminator[0].type = #value
+      // * code ^slicing.rules = #open
+      // * code contains boo 1..1
+      // * code[boo] = http://loinc.org#1-8 "Acyclovir [Susceptibility]"
+      questionnaire.rules.push(caretRule, dTypeRule, rulesRule, containsRule, cardRule, codeRule);
+      const linkIdRule = new AssignmentRule('item[0].linkId');
+      linkIdRule.value = 'bar';
+      const typeRule = new AssignmentRule('item[0].type');
+      typeRule.value = new FshCode('group');
+      const statusRule = new AssignmentRule('status');
+      statusRule.value = new FshCode('active');
+      const codeAssignmentRule = new AssignmentRule('code[0]');
+      codeAssignmentRule.value = new FshCode('otherCode', 'http://loinc.org/', 'OtherDisplay');
+      // * item[0].linkId = "bar"
+      // * item[0].type = #group
+      // * status = #active
+      // * code[0] = http://loinc.org#otherCode "OtherDisplay"
+      const questionnaireInstance = new Instance('Test');
+      questionnaireInstance.instanceOf = 'TestQuestionnaire';
+      questionnaireInstance.rules.push(statusRule, linkIdRule, typeRule, codeAssignmentRule);
+      exportInstance(questionnaireInstance);
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(1);
+      expect(loggerSpy.getLastMessage('warn')).toBe(
+        'Sliced element Questionnaire.code is being accessed via numeric index. Use slice names in rule paths when possible.'
+      );
+    });
+
+    it('should log a warning when the child of a pre-loaded element in a sliced array is accessed with a numeric index', () => {
+      const caretRule = new CaretValueRule('item');
+      caretRule.caretPath = 'slicing.discriminator.path';
+      caretRule.value = 'type';
+      const dTypeRule = new CaretValueRule('item');
+      dTypeRule.caretPath = 'slicing.discriminator.type';
+      dTypeRule.value = new FshCode('value');
+      const rulesRule = new CaretValueRule('item');
+      rulesRule.caretPath = 'slicing.rules';
+      rulesRule.value = new FshCode('open');
+      const containsRule = new ContainsRule('item');
+      containsRule.items.push({ name: 'boo' });
+      const cardRule = new CardRule('item[boo]');
+      cardRule.min = 1;
+      cardRule.max = '1';
+      const textCardRule = new CardRule('item[boo].text');
+      textCardRule.min = 1;
+      textCardRule.max = '1';
+      const textAssignmentRule = new AssignmentRule('item[boo].text');
+      textAssignmentRule.value = 'boo!';
+      // * item ^slicing.discriminator[0].path = "type"
+      // * item ^slicing.discriminator[0].type = #value
+      // * item ^slicing.rules = #open
+      // * item contains boo 1..1
+      // * item[boo].text 1..1
+      // * item[boo].text = "boo!"
+      questionnaire.rules.push(
+        caretRule,
+        dTypeRule,
+        rulesRule,
+        containsRule,
+        cardRule,
+        textCardRule,
+        textAssignmentRule
+      );
+      const linkIdRule = new AssignmentRule('item[0].linkId');
+      linkIdRule.value = 'bar';
+      const typeRule = new AssignmentRule('item[0].type');
+      typeRule.value = new FshCode('group');
+      const statusRule = new AssignmentRule('status');
+      statusRule.value = new FshCode('active');
+      // * item[0].linkId = "bar"
+      // * item[0].type = #group
+      // * status = #active
+      const questionnaireInstance = new Instance('Test');
+      questionnaireInstance.instanceOf = 'TestQuestionnaire';
+      questionnaireInstance.rules.push(statusRule, linkIdRule, typeRule);
+      exportInstance(questionnaireInstance);
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(2);
+      expect(loggerSpy.getLastMessage('warn')).toBe(
+        'Sliced element Questionnaire.item is being accessed via numeric index. Use slice names in rule paths when possible.'
+      );
+    });
+
+    it('should log a warning when any element in a closed sliced array is accessed with a numeric index', () => {
+      const caretRule = new CaretValueRule('item');
+      caretRule.caretPath = 'slicing.discriminator.path';
+      caretRule.value = 'type';
+      const dTypeRule = new CaretValueRule('item');
+      dTypeRule.caretPath = 'slicing.discriminator.type';
+      dTypeRule.value = new FshCode('value');
+      const rulesRule = new CaretValueRule('item');
+      rulesRule.caretPath = 'slicing.rules';
+      rulesRule.value = new FshCode('closed');
+      const containsRule = new ContainsRule('item');
+      containsRule.items.push({ name: 'boo' });
+      const cardRule = new CardRule('item[boo]');
+      cardRule.min = 0;
+      cardRule.max = '1';
+      // * item ^slicing.discriminator[0].path = "type"
+      // * item ^slicing.discriminator[0].type = #value
+      // * item ^slicing.rules = #closed
+      // * item contains boo 0..1
+      questionnaire.rules.push(caretRule, dTypeRule, rulesRule, containsRule, cardRule);
+      const textAssignmentRule = new AssignmentRule('item[boo].text');
+      textAssignmentRule.value = 'boo!';
+      const linkIdRule = new AssignmentRule('item[0].linkId');
+      linkIdRule.value = 'bar';
+      const typeRule = new AssignmentRule('item[0].type');
+      typeRule.value = new FshCode('group');
+      const statusRule = new AssignmentRule('status');
+      statusRule.value = new FshCode('active');
+      // * item[0].linkId = "bar"
+      // * item[0].type = #group
+      // * status = #active
+      const questionnaireInstance = new Instance('Test');
+      questionnaireInstance.instanceOf = 'TestQuestionnaire';
+      questionnaireInstance.rules.push(textAssignmentRule, statusRule, typeRule, linkIdRule);
+      exportInstance(questionnaireInstance);
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(2);
+      expect(loggerSpy.getLastMessage('warn')).toBe(
+        'Sliced element Questionnaire.item is being accessed via numeric index. Use slice names in rule paths when possible.'
+      );
+    });
+
     it('should log a warning when a choice element has its cardinality satisfied, but an ancestor of the choice element is a named slice that is referenced numerically', () => {
       // Making an assignment rule on a required element inside the named slice forces the slice to be created when setting implied properties
       // see https://github.com/FHIR/sushi/issues/1028
@@ -3486,7 +3629,7 @@ describe('InstanceExporter', () => {
       questionnaireInstance.rules.push(answerRule, linkIdRule, typeRule, statusRule);
       exportInstance(questionnaireInstance);
       expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
-      expect(loggerSpy.getAllMessages('warn')).toHaveLength(1);
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(4);
       expect(loggerSpy.getLastMessage('warn')).toBe(
         'Element Questionnaire.item:boo.answerOption.value[x] has its cardinality satisfied by a rule that does not include the slice name. Use slice names in rule paths when possible.'
       );