Repository: FHIR/sushi (ts)

Issue:
Error on applying type and binding constraints on CodeableReference sub-elements
[The spec says](https://hl7.org/fhir/R5/references.html#CodeableReference):
> The targetProfile and binding constraints on the CodeableReference apply to the CodeableReference.reference or the CodeableReference.concept respectively as appropriate and they SHALL NOT be specified directly on the .reference or .concept elements. 

It would be nice of SUSHI to error when people try to do these invalid things. The error should suggest changing the binding to the parent.

Pull request:
Log an error when applying type and binding constraints on CodeableReference subelements
Fixes #1264 

When constraining the `reference` portion of the CodeableReference or applying a binding to the `concept` portion of the CodeableReference, the constraints should be applied to the top level CodeableReference element. If an author tries to add the to the `reference` or `concept` portion of the CodeableReference element directly, log an error.

As it is implemented now, SUSHI will still apply the constraint or binding to the element as the author specified, but this is not correct according the the spec. If we think SUSHI should not even apply the element at all, I can adjust the implementation to throw an error, which will not apply the constraint or binding.

Patch:
diff --git a/src/export/StructureDefinitionExporter.ts b/src/export/StructureDefinitionExporter.ts
index 1284364..59ce931 100644
--- a/src/export/StructureDefinitionExporter.ts
+++ b/src/export/StructureDefinitionExporter.ts
@@ -738,7 +738,11 @@ export class StructureDefinitionExporter implements Fishable {
             if (csURI && !isUri(vsURI)) {
               throw new MismatchedBindingTypeError(rule.valueSet, rule.path, 'ValueSet');
             }
-            element.bindToVS(vsURI, rule.strength as ElementDefinitionBindingStrength);
+            element.bindToVS(
+              vsURI,
+              rule.strength as ElementDefinitionBindingStrength,
+              rule.sourceInfo
+            );
           } else if (rule instanceof ContainsRule) {
             const isExtension =
               element.type?.length === 1 &&
diff --git a/src/fhirtypes/ElementDefinition.ts b/src/fhirtypes/ElementDefinition.ts
index 4ed4095..e76c9ba 100644
--- a/src/fhirtypes/ElementDefinition.ts
+++ b/src/fhirtypes/ElementDefinition.ts
@@ -998,6 +998,19 @@ export class ElementDefinition {
       );
     }
 
+    // Check if a CodeableReference is attempting to constraint directly to the reference element
+    if (
+      this.type.length === 1 &&
+      this.type[0].code === 'Reference' &&
+      this.path.endsWith('.reference') &&
+      this.parent()?.type?.[0]?.code === 'CodeableReference'
+    ) {
+      logger.error(
+        "Constraining references on a CodeableReference element's underlying .reference path is not allowed. Instead, constrain the references directly on the CodeableReference element.",
+        rule.sourceInfo
+      );
+    }
+
     // Setup a map to store how each existing element type maps to the input types
     const typeMatches: Map<string, ElementTypeMatchInfo[]> = new Map();
     targetTypes.forEach(t => typeMatches.set(t.code, []));
@@ -1574,11 +1587,16 @@ export class ElementDefinition {
    * @see {@link http://hl7.org/fhir/R4/terminologies.html#strength}
    * @param {string} vsURI - the value set URI to bind
    * @param {string} strength - the strength of the binding (e.g., 'required')
+   * @param {SourceInfo} - optionally include rule.sourceInfo if the binding is coming from a rule
    * @throws {BindingStrengthError} when the binding can't be applied because it is looser than the existing binding
    * @throws {CodedTypeNotFoundError} - when the binding can't be applied because the element is the wrong type
    * @throws {InvalidUriError} when the value set uri is not valid
    */
-  bindToVS(vsURI: string, strength: ElementDefinitionBindingStrength): void {
+  bindToVS(
+    vsURI: string,
+    strength: ElementDefinitionBindingStrength,
+    ruleSourceInfo?: SourceInfo
+  ): void {
     // Check if this is a valid type to be bound against
     const validTypes = this.findTypesByCode(
       'code',
@@ -1592,6 +1610,18 @@ export class ElementDefinition {
     if (isEmpty(validTypes)) {
       throw new CodedTypeNotFoundError(this.type ? this.type.map(t => t.code) : []);
     }
+    // Check if a CodeableReference is attempting to bind directly to the concept element
+    if (
+      this.type.length === 1 &&
+      this.type[0].code === 'CodeableConcept' &&
+      this.path.endsWith('.concept') &&
+      this.parent()?.type?.[0]?.code === 'CodeableReference'
+    ) {
+      logger.error(
+        "Applying value set bindings to a CodeableReference element's underlying .concept path is not allowed. Instead, apply the binding directly to the CodeableReference element.",
+        ruleSourceInfo
+      );
+    }
     const strengths = ['example', 'preferred', 'extensible', 'required'];
     // Check if this is a valid strength (if the binding.strength already exists)
     if (this.binding && this.binding.strength) {


Test patch:
diff --git a/test/fhirtypes/ElementDefinition.bindToVS.test.ts b/test/fhirtypes/ElementDefinition.bindToVS.test.ts
index c7192e0..e104650 100644
--- a/test/fhirtypes/ElementDefinition.bindToVS.test.ts
+++ b/test/fhirtypes/ElementDefinition.bindToVS.test.ts
@@ -1,7 +1,7 @@
 import { loadFromPath } from 'fhir-package-loader';
 import { FHIRDefinitions } from '../../src/fhirdefs/FHIRDefinitions';
 import { StructureDefinition } from '../../src/fhirtypes/StructureDefinition';
-import { TestFisher } from '../testhelpers';
+import { TestFisher, loggerSpy } from '../testhelpers';
 import cloneDeep from 'lodash/cloneDeep';
 import path from 'path';
 
@@ -193,6 +193,7 @@ describe('ElementDefinition R5', () => {
   });
   beforeEach(() => {
     r5CarePlan = fisher.fishForStructureDefinition('CarePlan');
+    loggerSpy.reset();
   });
 
   describe('#bindToVS()', () => {
@@ -202,5 +203,29 @@ describe('ElementDefinition R5', () => {
       expect(addresses.binding.valueSet).toBe('http://myvaluesets.org/myvs');
       expect(addresses.binding.strength).toBe('required');
     });
+
+    it('should log an error when trying to bind the concept portion of a CodeableReference element directly', () => {
+      const addresses = r5CarePlan.elements.find(e => e.id === 'CarePlan.addresses');
+      addresses.unfold(fisher);
+      const concept = r5CarePlan.elements.find(e => e.id === 'CarePlan.addresses.concept');
+      const ruleSourceInfo = {
+        file: 'fishy.fsh',
+        location: {
+          startLine: 6,
+          startColumn: 1,
+          endLine: 6,
+          endColumn: 10
+        }
+      };
+      concept.bindToVS('http://myvaluesets.org/myvs', 'required', ruleSourceInfo);
+      expect(concept.binding.valueSet).toBe('http://myvaluesets.org/myvs');
+      expect(concept.binding.strength).toBe('required');
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
+      expect(loggerSpy.getLastMessage('error')).toMatch(
+        /Applying value set bindings to a CodeableReference element's underlying \.concept path is not allowed.* directly to the CodeableReference element/is
+      );
+      expect(loggerSpy.getLastMessage('error')).toMatch(/File: fishy\.fsh.*Line: 6\D*/s);
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(0);
+    });
   });
 });
diff --git a/test/fhirtypes/ElementDefinition.constrainType.test.ts b/test/fhirtypes/ElementDefinition.constrainType.test.ts
index e88b909..8aa33a4 100644
--- a/test/fhirtypes/ElementDefinition.constrainType.test.ts
+++ b/test/fhirtypes/ElementDefinition.constrainType.test.ts
@@ -1490,5 +1490,34 @@ describe('ElementDefinition R5', () => {
         }).toThrow(/"CodeableReference\(Patient\).*CodeableReference\(.*Condition\)/);
       });
     });
+
+    it('should log an error when trying to constrain the reference portion of a CodeableReference element directly', () => {
+      const performedActivity = r5CarePlan.elements.find(
+        e => e.id === 'CarePlan.activity.performedActivity'
+      );
+      performedActivity.unfold(fisher);
+      const reference = r5CarePlan.elements.find(
+        e => e.id === 'CarePlan.activity.performedActivity.reference'
+      );
+      const onlyRule = new OnlyRule('activity.performedActivity.reference')
+        .withFile('fishy.fsh')
+        .withLocation([6, 1, 6, 20]);
+      onlyRule.types = [{ type: 'Practitioner', isReference: true }];
+      reference.constrainType(onlyRule, fisher);
+      // applies the constraint author specified, even though the spec says this should not be done
+      expect(reference.type).toHaveLength(1);
+      expect(reference.type[0]).toEqual(
+        new ElementDefinitionType('Reference').withTargetProfiles(
+          'http://hl7.org/fhir/StructureDefinition/Practitioner'
+        )
+      );
+      // log an error because the author should not have constrained reference directly
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
+      expect(loggerSpy.getLastMessage('error')).toMatch(
+        /Constraining references on a CodeableReference element's underlying \.reference path is not allowed.* directly on the CodeableReference element/is
+      );
+      expect(loggerSpy.getLastMessage('error')).toMatch(/File: fishy\.fsh.*Line: 6\D*/s);
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(0);
+    });
   });
 });