Warn when a slice element is accessed via numeric index (#1066)

* Warning loged on numeric indices for slices * I cannot spell * Robust to preloaded slices that appear after non-preloaded ones * issue warning when sliced element itself contains a fixed value * new unit test
FHIR · May 16, 2022 · 84c0d5e · 84c0d5e
1 parent c821184
commit 84c0d5e
Show file tree

Hide file tree

Showing 4 changed files with 197 additions and 3 deletions.
diff --git 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
@@ -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
@@ -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)

diff --git 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.'
       );