Fix slicing cardinality validation (#952)

* Remove validation of slicing on arrays and choices * Log error for contains rule on single element * Annoted and clarify error message
FHIR · Nov 12, 2021 · b44cc3d · b44cc3d
1 parent ee9e449
commit b44cc3d
Show file tree

Hide file tree

Showing 6 changed files with 50 additions and 111 deletions.
diff --git a/src/errors/InvalidElementForSlicingError.ts b/src/errors/InvalidElementForSlicingError.ts
@@ -0,0 +1,12 @@
+import { Annotated } from '.';
+
+export class InvalidElementForSlicingError extends Error implements Annotated {
+  specReferences = [
+    'http://hl7.org/fhir/elementdefinition-definitions.html#ElementDefinition.slicing'
+  ];
+  constructor(public path: string) {
+    super(
+      `Cannot slice element '${path}' since FHIR only allows slicing on choice elements (e.g., value[x]) or elements with max > 1`
+    );
+  }
+}
diff --git a/src/errors/index.ts b/src/errors/index.ts
@@ -55,3 +55,4 @@ export * from './InvalidMustSupportError';
 export * from './InvalidChoiceTypeRulePathError';
 export * from './MismatchedBindingTypeError';
 export * from './ValidationError';
+export * from './InvalidElementForSlicingError';
diff --git a/src/export/StructureDefinitionExporter.ts b/src/export/StructureDefinitionExporter.ts
@@ -21,7 +21,8 @@ import {
   ParentNameConflictError,
   ParentNotDefinedError,
   ParentNotProvidedError,
-  MismatchedBindingTypeError
+  MismatchedBindingTypeError,
+  InvalidElementForSlicingError
 } from '../errors';
 import {
   AddElementRule,
@@ -528,6 +529,9 @@ export class StructureDefinitionExporter implements Fishable {
             if (isExtension) {
               this.handleExtensionContainsRule(fshDefinition, rule, structDef, element);
             } else {
+              if (!element.isArrayOrChoice()) {
+                throw new InvalidElementForSlicingError(rule.path);
+              }
               // Not an extension -- just add a slice for each item
               rule.items.forEach(item => {
                 if (item.type) {

diff --git a/src/fhirtypes/ElementDefinition.ts b/src/fhirtypes/ElementDefinition.ts
@@ -306,20 +306,18 @@ export class ElementDefinition {
     return null;
   }
 
+  isArrayOrChoice(): boolean {
+    return (
+      this.max === '*' ||
+      parseInt(this.max) > 1 ||
+      this.base.max === '*' ||
+      parseInt(this.base.max) > 1 ||
+      this.id.endsWith('[x]')
+    );
+  }
+
   private validateSlicing(slicing: ElementDefinitionSlicing): ValidationError[] {
     const validationErrors: ValidationError[] = [];
-    if (
-      this.max !== '*' &&
-      parseInt(this.max) <= 1 &&
-      this.base.max !== '*' &&
-      parseInt(this.base.max) <= 1 &&
-      !this.id.endsWith('[x]')
-    ) {
-      validationErrors.push(
-        new ValidationError('Cannot slice element which is not an array or choice', 'slicing')
-      );
-    }
-
     validationErrors.push(this.validateRequired(slicing.rules, 'slicing.rules'));
     validationErrors.push(
       this.validateIncludes(slicing.rules, ALLOWED_SLICING_RULES, 'slicing.rules')

diff --git a/test/export/StructureDefinitionExporter.test.ts b/test/export/StructureDefinitionExporter.test.ts
@@ -5240,6 +5240,28 @@ describe('StructureDefinitionExporter R4', () => {
         /Cannot create spoon extension; unable to locate extension definition for: IDoNotExist\..*File: BadExt\.fsh.*Line: 6\D*/s
       );
     });
+
+    it('should report an error for a ContainsRule on a single element', () => {
+      const profile = new Profile('Foo');
+      profile.parent = 'Observation';
+
+      const containsRule = new ContainsRule('status')
+        .withFile('SingleElement.fsh')
+        .withLocation([6, 3, 6, 12]);
+      containsRule.items = [{ name: 'test' }];
+      profile.rules.push(containsRule);
+
+      exporter.exportStructDef(profile);
+      const sd = pkg.profiles[0];
+      const baseStructDef = fisher.fishForStructureDefinition('Observation');
+      expect(sd.elements.length).toBe(baseStructDef.elements.length);
+
+      const slice = sd.elements.find(e => e.id === 'Observation.status:test');
+      expect(slice).toBeUndefined();
+      expect(loggerSpy.getLastMessage('error')).toMatch(
+        /Cannot slice element 'status'.*File: SingleElement\.fsh.*Line: 6\D*/s
+      );
+    });
   });
 
   describe('#CaretValueRule', () => {
@@ -6888,70 +6910,6 @@ describe('StructureDefinitionExporter R4', () => {
       expect(labCode.constraint).toContainEqual(expectedWalking);
     });
 
-    it('should apply rules that modify a slice on a choice element', () => {
-      // Profile: PizzaBusiness
-      // Parent: Practitioner
-      // * extension contains TheBusiness named business 1..1
-      // * extension[business].valueString contains badSlice 0..1
-      // * extension[business].valueString[badSlice] = "The Bad Slice"
-
-      // Extension: TheBusiness
-      // * valueString ^slicing.discriminator.type = #value
-      // * valueString contains goodSlice 0..1
-      // * valueString[goodSlice] = "The Good Slice"
-
-      const extension = new Extension('TheBusiness');
-      const slicingType = new CaretValueRule('valueString');
-      slicingType.caretPath = 'slicing.discriminator.type';
-      slicingType.value = new FshCode('value');
-      const valueContains = new ContainsRule('valueString');
-      valueContains.items.push({ name: 'goodSlice' });
-      const valueCard = new CardRule('valueString[goodSlice]');
-      valueCard.min = 0;
-      valueCard.max = '1';
-      const goodValue = new AssignmentRule('valueString[goodSlice]');
-      goodValue.value = 'The Good Slice';
-      extension.rules.push(slicingType, valueContains, valueCard, goodValue);
-
-      const profile = new Profile('PizzaBusiness');
-      profile.parent = 'Practitioner';
-      const profileContains = new ContainsRule('extension');
-      profileContains.items.push({ name: 'business', type: 'TheBusiness' });
-      const profileCard = new CardRule('extension[business]');
-      profileCard.min = 1;
-      profileCard.max = '1';
-      const businessContains = new ContainsRule('extension[business].valueString');
-      businessContains.items.push({ name: 'badSlice' });
-      const businessCard = new CardRule('extension[business].valueString[badSlice]');
-      businessCard.min = 0;
-      businessCard.max = '1';
-      const badSliceValue = new AssignmentRule('extension[business].valueString[badSlice]');
-      badSliceValue.value = 'The Bad Slice';
-      profile.rules.push(
-        profileContains,
-        profileCard,
-        businessContains,
-        businessCard,
-        badSliceValue
-      );
-
-      doc.extensions.set(extension.name, extension);
-      doc.profiles.set(profile.name, profile);
-      exporter.export();
-
-      const businessSd = pkg.extensions[0];
-      expect(businessSd).toBeDefined();
-      const goodSliceElement = businessSd.findElement('Extension.value[x]:valueString/goodSlice');
-      expect(goodSliceElement).toBeDefined();
-
-      const pizzaSd = pkg.profiles[0];
-      expect(pizzaSd).toBeDefined();
-      const badSliceElement = pizzaSd.findElement(
-        'Practitioner.extension:business.value[x]:valueString/badSlice'
-      );
-      expect(badSliceElement).toBeDefined();
-    });
-
     it.todo('should have some tests involving slices and CaretValueRule');
   });
 

diff --git a/test/fhirtypes/ElementDefinition.test.ts b/test/fhirtypes/ElementDefinition.test.ts
@@ -13,14 +13,10 @@ describe('ElementDefinition', () => {
   let jsonObservation: any;
   let jsonValueX: any;
   let jsonValueId: any;
-  let jsonValueComponent: any;
-  let jsonSubject: any;
   let observation: StructureDefinition;
   let resprate: StructureDefinition;
   let valueX: ElementDefinition;
   let valueId: ElementDefinition;
-  let valueComponent: ElementDefinition;
-  let subject: ElementDefinition;
   let fisher: TestFisher;
   beforeAll(() => {
     defs = new FHIRDefinitions();
@@ -36,16 +32,12 @@ describe('ElementDefinition', () => {
     jsonObservation = defs.fishForFHIR('Observation', Type.Resource);
     jsonValueX = jsonObservation.snapshot.element[21];
     jsonValueId = jsonObservation.snapshot.element[1];
-    jsonValueComponent = jsonObservation.snapshot.element[41];
-    jsonSubject = jsonObservation.snapshot.element[15];
   });
   beforeEach(() => {
     observation = StructureDefinition.fromJSON(jsonObservation);
     resprate = fisher.fishForStructureDefinition('resprate');
     valueX = ElementDefinition.fromJSON(jsonValueX);
     valueId = ElementDefinition.fromJSON(jsonValueId);
-    valueComponent = ElementDefinition.fromJSON(jsonValueComponent);
-    subject = ElementDefinition.fromJSON(jsonSubject);
     valueX.structDef = observation;
     valueId.structDef = observation;
   });
@@ -825,31 +817,5 @@ describe('ElementDefinition', () => {
         /slicing.discriminator\[0\].type: Invalid value: #foo. Value must be selected from one of the following: #value, #exists, #pattern, #type, #profile/
       );
     });
-
-    it('should be valid to slice array element', () => {
-      const clone = valueComponent.clone(false);
-      clone.slicing = { rules: 'open' };
-      const validationErrors = clone.validate();
-      expect(validationErrors).toHaveLength(0);
-    });
-
-    it('should be valid to slice constrained array element', () => {
-      const clone = valueComponent.clone(false);
-      clone.max = '2';
-      clone.slicing = { rules: 'open' };
-      const validationErrors = clone.validate();
-      expect(validationErrors).toHaveLength(0);
-    });
-
-    it('should be invalid to slice single element', () => {
-      const clone = subject.clone(false);
-      clone.max = '1';
-      clone.slicing = { rules: 'open' };
-      const validationErrors = clone.validate();
-      expect(validationErrors).toHaveLength(1);
-      expect(validationErrors[0].message).toMatch(
-        /slicing: Cannot slice element which is not an array or choice/
-      );
-    });
   });
 });