Repository: FHIR/sushi (ts)

Issue:
Warning about string-typed filter values seems problematic
There are a number of standard CodeSystem filters defined in the FHIR spec, such as https://hl7.org/fhir/snomedct.html#4.3.1.0.8.3, but when I use these in my FSH files to define ValueSets, I'm getting the very concerning warning:

> The match value of filter operator "=" must be a code. For string valued properties, use the regex filter operator with a regular expression value. Support for using strings as filter values has been deprecated and will be removed in a future release.

Do I need to worry that these standard filters are going to be unusable in some upcoming version of Sushi?

Pull request description:
Update allowed VS Filter Operator values
This PR fixes #936 by updating the types we allow for each VS filter operation. [This comment](https://github.com/FHIR/sushi/issues/936#issuecomment-947826886) describes what we now support. The changes to the code include:

- Support for string values on every operator, which also removes the deprecation warning since we now support it 😁 
- Special checking that the string value provided to the `exist` operator is either `"true"` or `"false"`
- Updating the `ValueSetFilterValueTypeError` to support allowing multiple types of values and specific string values
- Tests for the new error cases and new allowed cases

Patch:
diff --git a/src/errors/ValueSetFilterValueTypeError.ts b/src/errors/ValueSetFilterValueTypeError.ts
index 5bb3025..762607e 100644
--- a/src/errors/ValueSetFilterValueTypeError.ts
+++ b/src/errors/ValueSetFilterValueTypeError.ts
@@ -1,12 +1,25 @@
 import { VsOperator } from '../fshtypes';
 
+type expectedType = 'string' | 'regex' | 'boolean' | 'code';
 export class ValueSetFilterValueTypeError extends Error {
   constructor(
     public operator: VsOperator,
-    public expectedType: 'string' | 'regex' | 'boolean' | 'code'
+    public expectedTypes: expectedType[],
+    public allowedStrings: string[] = []
   ) {
     super(
-      `Not applying value set filter: operator "${operator}" requires a ${expectedType} value.`
+      `Not applying value set filter: operator "${operator}" requires ${getExpectedTypes(
+        expectedTypes,
+        allowedStrings
+      )}.`
     );
   }
 }
+
+function getExpectedTypes(types: expectedType[], allowedStrings: string[]): string {
+  const formattedAllowedStrings = allowedStrings.map(s => `"${s}"`).join(' or ');
+  const allowedStringsClause = formattedAllowedStrings
+    ? ` or a string value of ${formattedAllowedStrings}`
+    : '';
+  return `a ${types.join(' or ')} value${allowedStringsClause}`;
+}
diff --git a/src/import/FSHImporter.ts b/src/import/FSHImporter.ts
index 74889ad..9030c99 100644
--- a/src/import/FSHImporter.ts
+++ b/src/import/FSHImporter.ts
@@ -1969,42 +1969,35 @@ export class FSHImporter extends FSHVisitor {
       throw new ValueSetFilterMissingValueError(operator);
     }
     const value = ctx.vsFilterValue() ? this.visitVsFilterValue(ctx.vsFilterValue()) : true;
+
+    // NOTE: We support string value for every operator, in addition to the specific typed values
+    // for some operators based on the filter value documentation:
+    // http://hl7.org/fhir/R4/valueset-definitions.html#ValueSet.compose.include.filter.value
+    // and the discussion on https://github.com/FHIR/sushi/issues/936
     switch (operator) {
       case VsOperator.EQUALS:
       case VsOperator.IN:
       case VsOperator.NOT_IN:
-        // NOTE: We believe that =, in, and not-in operators should ONLY support code values
-        // based on the filter value documentation:
-        // http://hl7.org/fhir/R4/valueset-definitions.html#ValueSet.compose.include.filter.value
-        // Both string and code are supported for now in order to maintain backwards compatibility.
-        if (typeof value === 'string') {
-          logger.warn(
-            `The match value of filter operator "${operator}" must be a code. ` +
-              'For string valued properties, use the regex filter operator with a regular expression value. ' +
-              'Support for using strings as filter values has been deprecated and will be removed in a future release.',
-            { location: this.extractStartStop(ctx), file: this.currentFile }
-          );
-        }
-        if (typeof value !== 'string' && !(value instanceof FshCode)) {
-          throw new ValueSetFilterValueTypeError(operator, 'code');
-        }
-        break;
       case VsOperator.IS_A:
       case VsOperator.DESCENDENT_OF:
       case VsOperator.IS_NOT_A:
       case VsOperator.GENERALIZES:
-        if (!(value instanceof FshCode)) {
-          throw new ValueSetFilterValueTypeError(operator, 'code');
+        if (!(value instanceof FshCode) && typeof value !== 'string') {
+          throw new ValueSetFilterValueTypeError(operator, ['code', 'string']);
         }
         break;
       case VsOperator.REGEX:
-        if (!(value instanceof RegExp)) {
-          throw new ValueSetFilterValueTypeError(operator, 'regex');
+        if (!(value instanceof RegExp) && typeof value !== 'string') {
+          throw new ValueSetFilterValueTypeError(operator, ['regex', 'string']);
         }
         break;
       case VsOperator.EXISTS:
-        if (typeof value !== 'boolean') {
-          throw new ValueSetFilterValueTypeError(operator, 'boolean');
+        const allowedStrings = ['true', 'false'];
+        if (
+          typeof value !== 'boolean' &&
+          !(typeof value === 'string' && allowedStrings.includes(value))
+        ) {
+          throw new ValueSetFilterValueTypeError(operator, ['boolean'], allowedStrings);
         }
         break;
       default:


Test patch:
diff --git a/test/import/FSHImporter.ValueSet.test.ts b/test/import/FSHImporter.ValueSet.test.ts
index 4d0921e..2f85745 100644
--- a/test/import/FSHImporter.ValueSet.test.ts
+++ b/test/import/FSHImporter.ValueSet.test.ts
@@ -558,10 +558,6 @@ describe('FSHImporter', () => {
             value: '2.0'
           }
         ]);
-        expect(loggerSpy.getLastMessage('warn')).toMatch(
-          /match value of filter operator "=" must be a code/
-        );
-        expect(loggerSpy.getLastMessage('warn')).toMatch(/File: Zoo\.fsh.*Line: 3\D*/s);
       });
 
       it('should parse a value set that uses filter operator = with code value', () => {
@@ -610,11 +606,11 @@ describe('FSHImporter', () => {
         const valueSet = result.valueSets.get('ZooVS');
         expect(valueSet.rules.length).toBe(1);
         assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, []);
-        expect(loggerSpy.getLastMessage('error')).toMatch(/"=".*code/);
+        expect(loggerSpy.getLastMessage('error')).toMatch(/"=".*code or string/);
         expect(loggerSpy.getLastMessage('error')).toMatch(/File: Zoo\.fsh.*Line: 3\D*/s);
       });
 
-      it('should parse a value set that uses filter operator is-a', () => {
+      it('should parse a value set that uses filter operator is-a with a code value', () => {
         const input = leftAlign(`
         ValueSet: AllUrsinesVS
         * codes from system ZOO where concept is-a #bear "Bear"
@@ -634,7 +630,7 @@ describe('FSHImporter', () => {
         ]);
       });
 
-      it('should log an error when the is-a filter has a non-code value', () => {
+      it('should parse a value set that uses filter operator is-a with a string value', () => {
         const input = leftAlign(`
         ValueSet: AllUrsinesVS
         * codes from system ZOO where concept is-a "Bear"
@@ -643,12 +639,30 @@ describe('FSHImporter', () => {
         expect(result.valueSets.size).toBe(1);
         const valueSet = result.valueSets.get('AllUrsinesVS');
         expect(valueSet.rules.length).toBe(1);
+        assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, [
+          {
+            property: 'concept',
+            operator: VsOperator.IS_A,
+            value: 'Bear'
+          }
+        ]);
+      });
+
+      it('should log an error when the is-a filter has a non-code and non-string value', () => {
+        const input = leftAlign(`
+        ValueSet: AllUrsinesVS
+        * codes from system ZOO where concept is-a /([Bb]ear)/
+        `);
+        const result = importSingleText(input, 'Ursines.fsh');
+        expect(result.valueSets.size).toBe(1);
+        const valueSet = result.valueSets.get('AllUrsinesVS');
+        expect(valueSet.rules.length).toBe(1);
         assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, []);
-        expect(loggerSpy.getLastMessage('error')).toMatch(/"is-a".*code/);
+        expect(loggerSpy.getLastMessage('error')).toMatch(/"is-a".*code or string/);
         expect(loggerSpy.getLastMessage('error')).toMatch(/File: Ursines\.fsh.*Line: 3\D*/s);
       });
 
-      it('should parse a value set that uses filter operator descendent-of', () => {
+      it('should parse a value set that uses filter operator descendent-of with a code value', () => {
         const input = leftAlign(`
         ValueSet: AllFelinesVS
         * codes from system ZOO where concept descendent-of ZOO#cat
@@ -688,7 +702,7 @@ describe('FSHImporter', () => {
         ]);
       });
 
-      it('should log an error when the descendent-of filter has a non-code value', () => {
+      it('should parse a value set that uses filter operator descendent-of with a string value', () => {
         const input = leftAlign(`
         ValueSet: AllFelinesVS
         * codes from system ZOO where concept descendent-of "Cat"
@@ -697,12 +711,30 @@ describe('FSHImporter', () => {
         expect(result.valueSets.size).toBe(1);
         const valueSet = result.valueSets.get('AllFelinesVS');
         expect(valueSet.rules.length).toBe(1);
+        assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, [
+          {
+            property: 'concept',
+            operator: VsOperator.DESCENDENT_OF,
+            value: 'Cat'
+          }
+        ]);
+      });
+
+      it('should log an error when the descendent-of filter has a non-code and non-string value', () => {
+        const input = leftAlign(`
+        ValueSet: AllFelinesVS
+        * codes from system ZOO where concept descendent-of /([Cc]at)/
+        `);
+        const result = importSingleText(input, 'Felines.fsh');
+        expect(result.valueSets.size).toBe(1);
+        const valueSet = result.valueSets.get('AllFelinesVS');
+        expect(valueSet.rules.length).toBe(1);
         assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, []);
-        expect(loggerSpy.getLastMessage('error')).toMatch(/"descendent-of".*code/);
+        expect(loggerSpy.getLastMessage('error')).toMatch(/"descendent-of".*code or string/);
         expect(loggerSpy.getLastMessage('error')).toMatch(/File: Felines\.fsh.*Line: 3\D*/s);
       });
 
-      it('should parse a value set that uses filter operator is-not-a', () => {
+      it('should parse a value set that uses filter operator is-not-a with a code value', () => {
         const input = leftAlign(`
         ValueSet: NonCanineVS
         * codes from system ZOO where concept is-not-a #dog
@@ -722,7 +754,7 @@ describe('FSHImporter', () => {
         ]);
       });
 
-      it('should log an error when the is-not-a filter has a non-code value', () => {
+      it('should parse a value set that uses filter operator is-not-a with a string value', () => {
         const input = leftAlign(`
         ValueSet: NonCanineVS
         * codes from system ZOO where concept is-not-a "dog"
@@ -731,12 +763,30 @@ describe('FSHImporter', () => {
         expect(result.valueSets.size).toBe(1);
         const valueSet = result.valueSets.get('NonCanineVS');
         expect(valueSet.rules.length).toBe(1);
+        assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, [
+          {
+            property: 'concept',
+            operator: VsOperator.IS_NOT_A,
+            value: 'dog'
+          }
+        ]);
+      });
+
+      it('should log an error when the is-not-a filter has a non-code and non-string value', () => {
+        const input = leftAlign(`
+        ValueSet: NonCanineVS
+        * codes from system ZOO where concept is-not-a /([Dd]og)/
+        `);
+        const result = importSingleText(input, 'NonCanine.fsh');
+        expect(result.valueSets.size).toBe(1);
+        const valueSet = result.valueSets.get('NonCanineVS');
+        expect(valueSet.rules.length).toBe(1);
         assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, []);
-        expect(loggerSpy.getLastMessage('error')).toMatch(/"is-not-a".*code/);
+        expect(loggerSpy.getLastMessage('error')).toMatch(/"is-not-a".*code or string/);
         expect(loggerSpy.getLastMessage('error')).toMatch(/File: NonCanine\.fsh.*Line: 3\D*/s);
       });
 
-      it('should parse a value set that uses filter operator regex', () => {
+      it('should parse a value set that uses filter operator regex with a regex value', () => {
         const input = leftAlign(`
         ValueSet: ProbablyDogsVS
         * codes from system ZOO where display regex /([Dd]og)|([Cc]anine)/
@@ -754,7 +804,7 @@ describe('FSHImporter', () => {
         ]);
       });
 
-      it('should log an error when the regex filter has a non-regex value', () => {
+      it('should parse a value set that uses filter operator regex with a string value', () => {
         const input = leftAlign(`
         ValueSet: ProbablyDogsVS
         * codes from system ZOO where display regex "Dog|Canine"
@@ -763,8 +813,26 @@ describe('FSHImporter', () => {
         expect(result.valueSets.size).toBe(1);
         const valueSet = result.valueSets.get('ProbablyDogsVS');
         expect(valueSet.rules.length).toBe(1);
+        assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, [
+          {
+            property: 'display',
+            operator: VsOperator.REGEX,
+            value: 'Dog|Canine'
+          }
+        ]);
+      });
+
+      it('should log an error when the regex filter has a non-regex and non-string value', () => {
+        const input = leftAlign(`
+        ValueSet: ProbablyDogsVS
+        * codes from system ZOO where display regex #Dog|Canine
+        `);
+        const result = importSingleText(input, 'MostlyDogs.fsh');
+        expect(result.valueSets.size).toBe(1);
+        const valueSet = result.valueSets.get('ProbablyDogsVS');
+        expect(valueSet.rules.length).toBe(1);
         assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, []);
-        expect(loggerSpy.getLastMessage('error')).toMatch(/"regex".*regex/);
+        expect(loggerSpy.getLastMessage('error')).toMatch(/"regex".*regex or string/);
         expect(loggerSpy.getLastMessage('error')).toMatch(/File: MostlyDogs\.fsh.*Line: 3\D*/s);
       });
 
@@ -784,10 +852,6 @@ describe('FSHImporter', () => {
             value: '#cat, #dog'
           }
         ]);
-        expect(loggerSpy.getLastMessage('warn')).toMatch(
-          /match value of filter operator "in" must be a code/
-        );
-        expect(loggerSpy.getLastMessage('warn')).toMatch(/File: CatDog\.fsh.*Line: 3\D*/s);
       });
 
       it('should parse a value set that use filter operator in with a code value', () => {
@@ -818,7 +882,7 @@ describe('FSHImporter', () => {
         const valueSet = result.valueSets.get('CatAndDogVS');
         expect(valueSet.rules.length).toBe(1);
         assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, []);
-        expect(loggerSpy.getLastMessage('error')).toMatch(/"in".*code/);
+        expect(loggerSpy.getLastMessage('error')).toMatch(/"in".*code or string/);
         expect(loggerSpy.getLastMessage('error')).toMatch(/File: CatDog\.fsh.*Line: 3\D*/s);
       });
 
@@ -838,10 +902,6 @@ describe('FSHImporter', () => {
             value: '#goose'
           }
         ]);
-        expect(loggerSpy.getLastMessage('warn')).toMatch(
-          /match value of filter operator "not-in" must be a code/
-        );
-        expect(loggerSpy.getLastMessage('warn')).toMatch(/File: NoGoose\.fsh.*Line: 3\D*/s);
       });
 
       it('should parse a value set that uses filter operator not-in with a code value', () => {
@@ -872,11 +932,11 @@ describe('FSHImporter', () => {
         const valueSet = result.valueSets.get('NoGooseVS');
         expect(valueSet.rules.length).toBe(1);
         assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, []);
-        expect(loggerSpy.getLastMessage('error')).toMatch(/"not-in".*code/);
+        expect(loggerSpy.getLastMessage('error')).toMatch(/"not-in".*code or string/);
         expect(loggerSpy.getLastMessage('error')).toMatch(/File: NoGoose\.fsh.*Line: 3\D*/s);
       });
 
-      it('should parse a value set that uses filter operator generalizes', () => {
+      it('should parse a value set that uses filter operator generalizes with a code value', () => {
         const input = leftAlign(`
         ValueSet: MustelidVS
         * codes from system ZOO where concept generalizes #mustela-nivalis "least weasel"
@@ -896,7 +956,7 @@ describe('FSHImporter', () => {
         ]);
       });
 
-      it('should log an error when the generalizes filter has a non-code value', () => {
+      it('should parse a value set that uses filter operator generalizes with a string value', () => {
         const input = leftAlign(`
         ValueSet: MustelidVS
         * codes from system ZOO where concept generalizes "least weasel"
@@ -905,12 +965,30 @@ describe('FSHImporter', () => {
         expect(result.valueSets.size).toBe(1);
         const valueSet = result.valueSets.get('MustelidVS');
         expect(valueSet.rules.length).toBe(1);
+        assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, [
+          {
+            property: 'concept',
+            operator: VsOperator.GENERALIZES,
+            value: 'least weasel'
+          }
+        ]);
+      });
+
+      it('should log an error when the generalizes filter has a non-code value', () => {
+        const input = leftAlign(`
+        ValueSet: MustelidVS
+        * codes from system ZOO where concept generalizes /least weasel/
+        `);
+        const result = importSingleText(input, 'Mustelids.fsh');
+        expect(result.valueSets.size).toBe(1);
+        const valueSet = result.valueSets.get('MustelidVS');
+        expect(valueSet.rules.length).toBe(1);
         assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, []);
-        expect(loggerSpy.getLastMessage('error')).toMatch(/"generalizes".*code/);
+        expect(loggerSpy.getLastMessage('error')).toMatch(/"generalizes".*code or string/);
         expect(loggerSpy.getLastMessage('error')).toMatch(/File: Mustelids\.fsh.*Line: 3\D*/s);
       });
 
-      it('should parse a value set that uses filter operator exists', () => {
+      it('should parse a value set that uses filter operator exists with a boolean value', () => {
         const input = leftAlign(`
         ValueSet: ZooVS
         * codes from system ZOO where display exists true
@@ -967,17 +1045,73 @@ describe('FSHImporter', () => {
         ]);
       });
 
-      it('should log an error when the exists filter has a non-boolean value', () => {
+      it('should parse a value set that uses filter operator exists with a string value that looks like a boolean', () => {
+        const input = leftAlign(`
+        ValueSet: ZooVS
+        * codes from system ZOO where display exists "true"
+        * codes from system ZOO where version exists "false"
+        `);
+        const result = importSingleText(input, 'Zoo.fsh');
+        expect(result.valueSets.size).toBe(1);
+        const valueSet = result.valueSets.get('ZooVS');
+        expect(valueSet.rules.length).toBe(2);
+        assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, [
+          {
+            property: 'display',
+            operator: VsOperator.EXISTS,
+            value: 'true'
+          }
+        ]);
+        assertValueSetFilterComponent(valueSet.rules[1], 'ZOO', undefined, [
+          {
+            property: 'version',
+            operator: VsOperator.EXISTS,
+            value: 'false'
+          }
+        ]);
+      });
+
+      it('should log an error when the exists filter has a string value that does not look like a boolean', () => {
         const input = leftAlign(`
         ValueSet: ZooVS
         * codes from system ZOO where display exists "display"
+        * codes from system ZOO where version exists "True"
+        * codes from system ZOO where variant exists "FALSE"
+        `);
+        const result = importSingleText(input, 'Zoo.fsh');
+        expect(result.valueSets.size).toBe(1);
+        const valueSet = result.valueSets.get('ZooVS');
+        // All three of these string values cause errors
+        expect(valueSet.rules.length).toBe(3);
+        assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, []);
+        expect(loggerSpy.getAllMessages('error')).toHaveLength(3);
+        expect(loggerSpy.getMessageAtIndex(0, 'error')).toMatch(
+          /"exists".*boolean value or a string value of "true" or "false"/
+        );
+        expect(loggerSpy.getMessageAtIndex(0, 'error')).toMatch(/File: Zoo\.fsh.*Line: 3\D*/s);
+        expect(loggerSpy.getMessageAtIndex(1, 'error')).toMatch(
+          /"exists".*boolean value or a string value of "true" or "false"/
+        );
+        expect(loggerSpy.getMessageAtIndex(1, 'error')).toMatch(/File: Zoo\.fsh.*Line: 4\D*/s);
+        expect(loggerSpy.getMessageAtIndex(2, 'error')).toMatch(
+          /"exists".*boolean value or a string value of "true" or "false"/
+        );
+        expect(loggerSpy.getMessageAtIndex(2, 'error')).toMatch(/File: Zoo\.fsh.*Line: 5\D*/s);
+      });
+
+      it('should log an error when the exists filter has a non-boolean and non-string value', () => {
+        const input = leftAlign(`
+        ValueSet: ZooVS
+        * codes from system ZOO where display exists /true/
         `);
         const result = importSingleText(input, 'Zoo.fsh');
         expect(result.valueSets.size).toBe(1);
         const valueSet = result.valueSets.get('ZooVS');
         expect(valueSet.rules.length).toBe(1);
         assertValueSetFilterComponent(valueSet.rules[0], 'ZOO', undefined, []);
-        expect(loggerSpy.getLastMessage('error')).toMatch(/"exists".*boolean/);
+        expect(loggerSpy.getLastMessage('error')).toMatch(
+          /"exists".*boolean value or a string value of "true" or "false"/
+        );
         expect(loggerSpy.getLastMessage('error')).toMatch(/File: Zoo\.fsh.*Line: 3\D*/s);
       });