Repository: FHIR/sushi
Pull request #1429 (2024-02-21 15:31:28)

Do not allow aliases with unsupported characters
The FSH 3.0 ballot specifies that Alias names can only contain a limited set of characters. Specifically, it says:

> Alias names SHOULD begin with a dollar sign (“$”) and MUST otherwise contain only ASCII letters, numbers, underscores (“_”), hyphens (“-“), and dots (“.”). 

This PR makes it so that SUSHI now follows these rules as well. Any alias that contains unsupported characters now ~logs an error and is not processed fully~ logs a warning and continues processing. ~This does make it so that aliases that use `$` to get the extra error checking do get extra error checking and log errors that an alias couldn't be resolved. Then, because the Alias isn't processed, another error is logged when processing entities that use the unprocessed alias. All that is to say, because SUSHI no longer allows these characters, there will be 2-3 errors logged when using an alias with unsupported characters (only 1 of which is new to this PR). However, the fix is pretty easy and the errors do show the places that the issue impacts, so I think this is a reasonable approach.~ After switching to the warning approach, this cascade of errors no longer happens, yay.

I did spend some time talking to @mint-thompson about the case where we have special handling for when you miss a space or two around the `=` sign when creating an Alias, but there wasn't a great way to know that that handling had happened and skip past this new error. So in the cases where you might have `Alias myalias= http://example.org` or `Alias myalias=http://example.org`, you'll get the error about missing a space and the ~error~ warning that there are unsupported characters. They aren't wrong and since it would be complicated to try to suppress the second one, @mint-thompson agreed with me that we'll just log both.

I ran a full regression ~and there are two repos that this causes issues for: [joofio/ufis-ig#main](https://github.com/joofio/ufis-ig/tree/main/) and [joofio/ufis-swe-ig#main](https://github.com/joofio/ufis-swe-ig/tree/main/). For these two repos, this PR causes ~300 new errors due to 1 alias with `~` in it and ~100 uses of that alias. Since it is just those two repos with issues, the issue is pretty easy to resolve, and the `~` is not allowed according to the FSH spec, I think this is okay, despite being a little (maybe a lot) annoying. I was thinking of putting up two PRs for the fixes for these repos.~ After switching to the warning approach, the only changes in these repos is one new warning, as expected.

jafeltra: I agree with you, especially since names already log a warning where these characters are not supported. If that is the case, then I think aliases can be even more flexible since they don't represent an element within FHIR. I switched to a warning and continue processing the alias when unsupported characters are encountered. Let me know what you think.

Patch:
diff --git a/src/import/FSHImporter.ts b/src/import/FSHImporter.ts
index fbcf3ee..7cce75f 100644
--- a/src/import/FSHImporter.ts
+++ b/src/import/FSHImporter.ts
@@ -132,6 +132,7 @@ enum Flag {
 const FLAGS = ['MS', 'SU', '?!', 'TU', 'N', 'D'];
 const INDENT_WIDTH = 2;
 const DEFAULT_START_COLUMN = 1;
+const aliasRegex = /^\$?[a-zA-z0-9_\-\.]*$/;
 
 function unescapeUnicode(match: string, p1: string): string {
   return JSON.parse(`"\\${p1}"`);
@@ -197,6 +198,11 @@ export class FSHImporter extends FSHVisitor {
               { file: doc.file ?? '', location: this.extractStartStop(e.alias()) }
             );
             return;
+          } else if (!aliasRegex.test(name)) {
+            logger.warn(
+              `Alias ${name} includes unsupported characters. Alias names can only contain letters, numbers, underscores ("_"), hyphens ("-"), and dots (".").`,
+              { file: doc.file ?? '', location: this.extractStartStop(e.alias()) }
+            );
           }
           if (this.allAliases.has(name) && this.allAliases.get(name) !== value) {
             logger.error(


Test patch:
diff --git a/test/import/FSHErrorListener.test.ts b/test/import/FSHErrorListener.test.ts
index 70a6dcb..1432d71 100644
--- a/test/import/FSHErrorListener.test.ts
+++ b/test/import/FSHErrorListener.test.ts
@@ -42,9 +42,15 @@ describe('FSHErrorListener', () => {
     Alias: $OrgType= http://terminology.hl7.org/CodeSystem/organization-type
     `);
     importSingleText(input, 'Invalid.fsh');
+    expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
     expect(loggerSpy.getLastMessage('error')).toMatch(
       /Alias declarations must include at least one space both before and after the '=' sign.*File: Invalid\.fsh.*Line: 2\D*/s
     );
+    // = is an unsupported character so we also get this error
+    expect(loggerSpy.getAllMessages('warn')).toHaveLength(1);
+    expect(loggerSpy.getLastMessage('warn')).toMatch(
+      /Alias \$OrgType= includes unsupported characters/
+    );
   });
 
   it('should make sense of errors due to no space after = in an Alias definition', () => {
@@ -62,9 +68,15 @@ describe('FSHErrorListener', () => {
     Alias: $OrgType=http://terminology.hl7.org/CodeSystem/organization-type
     `);
     importSingleText(input, 'Invalid.fsh');
+    expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
     expect(loggerSpy.getLastMessage('error')).toMatch(
       /Alias declarations must include at least one space both before and after the '=' sign.*File: Invalid\.fsh.*Line: 2\D*/s
     );
+    // = is an unsupported character so we also get this warning
+    expect(loggerSpy.getAllMessages('warn')).toHaveLength(1);
+    expect(loggerSpy.getLastMessage('warn')).toMatch(
+      /Alias \$OrgType=http:\/\/terminology\.hl7\.org\/CodeSystem\/organization-type includes unsupported characters/
+    );
   });
 
   it('should make sense of errors due to no space before = in an assignment rule', () => {
diff --git a/test/import/FSHImporter.Alias.test.ts b/test/import/FSHImporter.Alias.test.ts
index 452349f..55af69e 100644
--- a/test/import/FSHImporter.Alias.test.ts
+++ b/test/import/FSHImporter.Alias.test.ts
@@ -34,6 +34,7 @@ describe('FSHImporter', () => {
       expect(result.aliases.get('SCT')).toBe('http://snomed.info/sct');
       expect(result.aliases.get('RXNORM')).toBe('http://www.nlm.nih.gov/research/umls/rxnorm');
       expect(result.aliases.get('UCUM')).toBe('http://unitsofmeasure.org');
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
     });
 
     it('should parse aliases that replicate the syntax of a code', () => {
@@ -44,6 +45,7 @@ describe('FSHImporter', () => {
       const result = importSingleText(input);
       expect(result.aliases.size).toBe(1);
       expect(result.aliases.get('LOINC')).toBe('http://loinc.org#1234');
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
     });
 
     it('should report when the same alias is defined twice w/ different values in the same file', () => {
@@ -57,6 +59,7 @@ describe('FSHImporter', () => {
       expect(result.aliases.get('USCoreRace')).toBe(
         'http://hl7.org/fhir/us/core/ValueSet/omb-race-category'
       );
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
       expect(loggerSpy.getLastMessage('error')).toMatch(
         /Alias USCoreRace cannot be redefined to http:\/\/hl7.org\/fhir\/us\/core\/StructureDefinition\/us-core-race; it is already defined as http:\/\/hl7.org\/fhir\/us\/core\/ValueSet\/omb-race-category\..*Line: 3\D*/s
       );
@@ -80,6 +83,7 @@ describe('FSHImporter', () => {
         'http://hl7.org/fhir/us/core/ValueSet/omb-race-category'
       );
       expect(results[1].aliases.size).toBe(0);
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
       expect(loggerSpy.getLastMessage('error')).toMatch(
         /Alias USCoreRace cannot be redefined to http:\/\/hl7.org\/fhir\/us\/core\/StructureDefinition\/us-core-race; it is already defined as http:\/\/hl7.org\/fhir\/us\/core\/ValueSet\/omb-race-category\..*File: Alias2\.fsh.*Line: 2\D*/s
       );
@@ -122,6 +126,7 @@ describe('FSHImporter', () => {
       const result = importSingleText(input);
       const rule = result.profiles.get('ObservationProfile').rules[0] as BindingRule;
       expect(rule.valueSet).toBe('http://loinc.org');
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
     });
 
     it('should translate an alias when the alias is defined after its use', () => {
@@ -136,6 +141,7 @@ describe('FSHImporter', () => {
       const result = importSingleText(input);
       const rule = result.profiles.get('ObservationProfile').rules[0] as BindingRule;
       expect(rule.valueSet).toBe('http://loinc.org');
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
     });
 
     it('should not translate an alias when the alias does not match', () => {
@@ -150,6 +156,7 @@ describe('FSHImporter', () => {
       const result = importSingleText(input);
       const rule = result.profiles.get('ObservationProfile').rules[0] as BindingRule;
       expect(rule.valueSet).toBe('LAINC');
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
     });
 
     it('should translate an alias from any input file', () => {
@@ -166,6 +173,7 @@ describe('FSHImporter', () => {
       expect(results.length).toBe(2);
       const rule = results[0].profiles.get('ObservationProfile').rules[0] as BindingRule;
       expect(rule.valueSet).toBe('http://loinc.org');
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
     });
 
     it('should log an error when an aliased code prefixed with $ does not resolve', () => {
@@ -179,6 +187,7 @@ describe('FSHImporter', () => {
 
       const results = importText([new RawFSH(input, 'Loinc.fsh')]);
       expect(results.length).toBe(1);
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
       expect(loggerSpy.getLastMessage('error')).toMatch(/\$LOINCZ.*\$/);
       expect(loggerSpy.getLastMessage('error')).toMatch(/File: Loinc.fsh.*Line: 6\D*/s);
     });
@@ -194,6 +203,7 @@ describe('FSHImporter', () => {
 
       const results = importText([new RawFSH(input, 'Loinc.fsh')]);
       expect(results.length).toBe(1);
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
       expect(loggerSpy.getLastMessage('error')).toMatch(/\$LOINCZ.*\$/);
       expect(loggerSpy.getLastMessage('error')).toMatch(/File: Loinc.fsh.*Line: 6\D*/s);
     });
@@ -209,6 +219,7 @@ describe('FSHImporter', () => {
 
       const results = importText([new RawFSH(input, 'MyPatient.fsh')]);
       expect(results.length).toBe(1);
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
       expect(loggerSpy.getLastMessage('error')).toMatch(/\$MYPATIENT.*\$/);
       expect(loggerSpy.getLastMessage('error')).toMatch(/File: MyPatient.fsh.*Line: 6\D*/s);
     });
@@ -224,6 +235,7 @@ describe('FSHImporter', () => {
 
       const results = importText([new RawFSH(input, 'MyPatient.fsh')]);
       expect(results.length).toBe(1);
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
       expect(loggerSpy.getLastMessage('error')).toMatch(/\$MYPATIENTZ.*\$/);
       expect(loggerSpy.getLastMessage('error')).toMatch(/File: MyPatient.fsh.*Line: 6\D*/s);
     });
@@ -239,6 +251,7 @@ describe('FSHImporter', () => {
 
       const results = importText([new RawFSH(input, 'MyPatient.fsh')]);
       expect(results.length).toBe(1);
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
       expect(loggerSpy.getLastMessage('error')).toMatch(/\$MYPATIENTZ.*\$/);
       expect(loggerSpy.getLastMessage('error')).toMatch(/File: MyPatient.fsh.*Line: 6\D*/s);
     });
@@ -264,11 +277,12 @@ describe('FSHImporter', () => {
 
       Profile: MyObservation
       Parent: Observation
-      * extension contains $EXTZ named ext
+      * extension contains $EXTZ named ext 1..1
       `);
 
       const results = importText([new RawFSH(input, 'Ext.fsh')]);
       expect(results.length).toBe(1);
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
       expect(loggerSpy.getLastMessage('error')).toMatch(/\$EXTZ.*\$/);
       expect(loggerSpy.getLastMessage('error')).toMatch(/File: Ext.fsh.*Line: 6\D*/s);
     });
@@ -283,6 +297,7 @@ describe('FSHImporter', () => {
 
       const results = importText([new RawFSH(input, 'Loinc.fsh')]);
       expect(results.length).toBe(1);
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
       expect(loggerSpy.getLastMessage('error')).toMatch(/\$LOINCZ.*\$/);
       expect(loggerSpy.getLastMessage('error')).toMatch(/File: Loinc.fsh.*Line: 5\D*/s);
     });
@@ -297,6 +312,7 @@ describe('FSHImporter', () => {
 
       const results = importText([new RawFSH(input, 'Loinc.fsh')]);
       expect(results.length).toBe(1);
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
       expect(loggerSpy.getLastMessage('error')).toMatch(/\$LOINCZ.*\$/);
       expect(loggerSpy.getLastMessage('error')).toMatch(/File: Loinc.fsh.*Line: 5\D*/s);
     });
@@ -355,7 +371,73 @@ describe('FSHImporter', () => {
       expect(rule.value).toEqual(
         new FshCode('foo', 'BAD|LOINC').withFile('Loinc.fsh').withLocation([6, 10, 6, 22])
       );
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
       expect(loggerSpy.getLastMessage('error')).toMatch(/BAD\|LOINC cannot include "\|"/);
     });
+
+    it('should resolve an alias with all supported characters', () => {
+      // The only supported characters in Aliases are ASCII letters, numbers, _, -, .
+      const input = leftAlign(`
+      Alias: Foo_McBar-Baz_Jr.3 = http://example.org
+
+      Profile: ObservationProfile
+      Parent: Observation
+      * code = Foo_McBar-Baz_Jr.3#foo
+      `);
+
+      const results = importText([new RawFSH(input, 'Alias.fsh')]);
+      expect(results.length).toBe(1);
+      const rule = results[0].profiles.get('ObservationProfile').rules[0] as AssignmentRule;
+      expect(rule.value).toEqual(
+        new FshCode('foo', 'http://example.org').withFile('Alias.fsh').withLocation([6, 10, 6, 31])
+      );
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+    });
+
+    it('should resolve but log a warning when an alias contains unsupported characters', () => {
+      // The only supported characters in Aliases are ASCII letters, numbers, _, -, .
+      const input = leftAlign(`
+      Alias: B@dAlias = http://example.org
+
+      Profile: ObservationProfile
+      Parent: Observation
+      * code = B@dAlias#foo
+      `);
+
+      const results = importText([new RawFSH(input, 'Alias.fsh')]);
+      expect(results.length).toBe(1);
+      const rule = results[0].profiles.get('ObservationProfile').rules[0] as AssignmentRule;
+      // The B@dAlias alias does still resolve because only a warning is logged
+      expect(rule.value).toEqual(
+        new FshCode('foo', 'http://example.org').withFile('Alias.fsh').withLocation([6, 10, 6, 21])
+      );
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(1);
+      expect(loggerSpy.getLastMessage('warn')).toMatch(/B@dAlias includes unsupported characters/);
+    });
+
+    it('should resolve but log a warning when an alias contains unsupported characters and starts with $', () => {
+      // The only supported characters in Aliases are ASCII letters, numbers, _, -, .
+      const input = leftAlign(`
+      Alias: $N*tAll*wed = http://example.org
+
+      Profile: ObservationProfile
+      Parent: Observation
+      * code = $N*tAll*wed#foo
+      `);
+
+      const results = importText([new RawFSH(input, 'Alias.fsh')]);
+      expect(results.length).toBe(1);
+      const rule = results[0].profiles.get('ObservationProfile').rules[0] as AssignmentRule;
+      // The $N*tAll*wed alias does still resolve because only a warning is logged
+      expect(rule.value).toEqual(
+        new FshCode('foo', 'http://example.org').withFile('Alias.fsh').withLocation([6, 10, 6, 24])
+      );
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(1);
+      expect(loggerSpy.getLastMessage('warn')).toMatch(
+        /\$N\*tAll\*wed includes unsupported characters/
+      );
+    });
   });
 });