Repository: FHIR/sushi (ts)

Issue:
FHIR rules for name and id should be enforced
Currently SUSHI allows `name` and `id` to be any sequence of non-whitespace characters.  FHIR, however, is more specific:

* name: `[A-Z]([A-Za-z0-9_]){0,254}` (http://hl7.org/fhir/R4/structuredefinition-definitions.html#StructureDefinition.name)
* id: `[A-Za-z0-9\-\.]{1,64}`  (http://hl7.org/fhir/R4/datatypes.html#id)

We should probably catch names/ids that violate these rules and report them as errors in SUSHI.  While we could update the ANTLR grammar to reflect these requirements, it might be more user-friendly to keep the grammar as-is and do the check in code.  Grammar violations make ugly and sometimes-confusing error messages.

Pull request:
Show message on invalid FHIR id or name
Fixes #139 

The id and name elements in code systems, structure definitions,
value sets, and instances are checked against the invariants from
the FHIR specification. Both invariants are regular expressions.
When an id or name is not valid, it is still used in the output,
but a message indicates to the user that it is invalid.

The validation of these elements occurs when exporting to FHIR.
Although it would be possible to check some of this earlier on
during FSH import, the validation step is based on specifications
from FHIR. Therefore, I think it makes the most sense to enforce
these rules at the time of creating the FHIR resources.

This pull request addresses https://standardhealthrecord.atlassian.net/browse/CIMPL-243.

Patch:
diff --git a/src/export/CodeSystemExporter.ts b/src/export/CodeSystemExporter.ts
index f1d3e62..1a7b657 100644
--- a/src/export/CodeSystemExporter.ts
+++ b/src/export/CodeSystemExporter.ts
@@ -8,8 +8,8 @@ export class CodeSystemExporter {
   constructor(private readonly tank: FSHTank, private readonly pkg: Package) {}
 
   private setMetadata(codeSystem: CodeSystem, fshDefinition: FshCodeSystem): void {
-    codeSystem.name = fshDefinition.name;
-    codeSystem.id = fshDefinition.id;
+    codeSystem.setName(fshDefinition.name, fshDefinition.sourceInfo);
+    codeSystem.setId(fshDefinition.id, fshDefinition.sourceInfo);
     if (fshDefinition.title) codeSystem.title = fshDefinition.title;
     if (fshDefinition.description) codeSystem.description = fshDefinition.description;
     // Version is set to value provided in config, will be overriden if reset by rules
diff --git a/src/export/InstanceExporter.ts b/src/export/InstanceExporter.ts
index 2c41184..70130d6 100644
--- a/src/export/InstanceExporter.ts
+++ b/src/export/InstanceExporter.ts
@@ -154,7 +154,7 @@ export class InstanceExporter {
     }
     // Set Fixed values based on the FSH rules and the Structure Definition
     instanceDef = this.setFixedValues(fshDefinition, instanceDef, instanceOfStructureDefinition);
-
+    instanceDef.validateId(fshDefinition.sourceInfo);
     return instanceDef;
   }
 
diff --git a/src/export/StructureDefinitionExporter.ts b/src/export/StructureDefinitionExporter.ts
index 32b3982..bcf3701 100644
--- a/src/export/StructureDefinitionExporter.ts
+++ b/src/export/StructureDefinitionExporter.ts
@@ -32,8 +32,8 @@ export class StructureDefinitionExporter implements Fishable {
    * @param {Profile | Extension} fshDefinition - The Profile or Extension we are exporting
    */
   private setMetadata(structDef: StructureDefinition, fshDefinition: Profile | Extension): void {
-    structDef.name = fshDefinition.name;
-    structDef.id = fshDefinition.id;
+    structDef.setName(fshDefinition.name, fshDefinition.sourceInfo);
+    structDef.setId(fshDefinition.id, fshDefinition.sourceInfo);
     if (fshDefinition.title) structDef.title = fshDefinition.title;
     if (fshDefinition.description) structDef.description = fshDefinition.description;
     // Version is set to value provided in config, will be overriden if reset by rules
diff --git a/src/export/ValueSetExporter.ts b/src/export/ValueSetExporter.ts
index fc072db..8616e58 100644
--- a/src/export/ValueSetExporter.ts
+++ b/src/export/ValueSetExporter.ts
@@ -17,8 +17,8 @@ export class ValueSetExporter {
   constructor(private readonly tank: FSHTank, private pkg: Package, private fisher: MasterFisher) {}
 
   private setMetadata(valueSet: ValueSet, fshDefinition: FshValueSet): void {
-    valueSet.name = fshDefinition.name;
-    valueSet.id = fshDefinition.id;
+    valueSet.setName(fshDefinition.name, fshDefinition.sourceInfo);
+    valueSet.setId(fshDefinition.id, fshDefinition.sourceInfo);
     if (fshDefinition.title) {
       valueSet.title = fshDefinition.title;
     }
diff --git a/src/fhirtypes/CodeSystem.ts b/src/fhirtypes/CodeSystem.ts
index 6c0ac7a..d344276 100644
--- a/src/fhirtypes/CodeSystem.ts
+++ b/src/fhirtypes/CodeSystem.ts
@@ -2,13 +2,15 @@ import { Meta } from './specialTypes';
 import { Extension } from '../fshtypes';
 import { Narrative, Resource, Identifier, CodeableConcept, Coding } from './dataTypes';
 import { ContactDetail, UsageContext } from './metaDataTypes';
+import { HasName, HasId } from './common';
+import { applyMixins } from '../utils';
 
 /**
  * Class representing a FHIR R4 CodeSystem
  * @see {@link https://www.hl7.org/fhir/codesystem.html}
  */
 export class CodeSystem {
-  id?: string;
+  // id?: FHIRId; // provided by HasId mixin
   meta?: Meta;
   implicitRules?: string;
   language?: string;
@@ -19,7 +21,7 @@ export class CodeSystem {
   url?: string;
   identifier?: Identifier[];
   version?: string;
-  name?: string;
+  // name?: string; // provided by HasName mixin
   title?: string;
   status = 'active';
   experimental?: boolean;
@@ -63,6 +65,9 @@ export class CodeSystem {
   }
 }
 
+export interface CodeSystem extends HasId, HasName {}
+applyMixins(CodeSystem, [HasId, HasName]);
+
 export type CodeSystemFilter = {
   code: string;
   description?: string;
diff --git a/src/fhirtypes/InstanceDefinition.ts b/src/fhirtypes/InstanceDefinition.ts
index 7904e45..5d799b4 100644
--- a/src/fhirtypes/InstanceDefinition.ts
+++ b/src/fhirtypes/InstanceDefinition.ts
@@ -1,5 +1,7 @@
 import cloneDeep = require('lodash/cloneDeep');
 import { Meta } from './specialTypes';
+import { HasId } from './common';
+import { applyMixins } from '../utils';
 
 /**
  * A class representing a FHIR Instance.
@@ -10,7 +12,7 @@ import { Meta } from './specialTypes';
 export class InstanceDefinition {
   resourceType: string;
   instanceName: string;
-  id?: string;
+  // id?: FHIRId; // provided by HasId mixin
   meta?: Meta;
   [key: string]: any; // Allow any key value pair on InstanceDefinition due to the high number of potential properties that can be set on a FHIR instance
 
@@ -36,3 +38,7 @@ export class InstanceDefinition {
     return instanceDefinition;
   }
 }
+
+// eslint-disable-next-line @typescript-eslint/no-empty-interface
+export interface InstanceDefinition extends HasId {}
+applyMixins(InstanceDefinition, [HasId]);
diff --git a/src/fhirtypes/StructureDefinition.ts b/src/fhirtypes/StructureDefinition.ts
index e5b5f88..6e8a413 100644
--- a/src/fhirtypes/StructureDefinition.ts
+++ b/src/fhirtypes/StructureDefinition.ts
@@ -9,8 +9,9 @@ import { Meta } from './specialTypes';
 import { Identifier, CodeableConcept, Coding, Narrative, Resource, Extension } from './dataTypes';
 import { ContactDetail, UsageContext } from './metaDataTypes';
 import { CannotResolvePathError, InvalidElementAccessError } from '../errors';
-import { getArrayIndex, setPropertyOnDefinitionInstance } from './common';
+import { getArrayIndex, setPropertyOnDefinitionInstance, HasName, HasId } from './common';
 import { Fishable, Type } from '../utils/Fishable';
+import { applyMixins } from '../utils';
 
 /**
  * A class representing a FHIR R4 StructureDefinition.  For the most part, each allowable property in a StructureDefinition
@@ -23,7 +24,7 @@ import { Fishable, Type } from '../utils/Fishable';
  * @see {@link http://hl7.org/fhir/R4/structuredefinition.html|FHIR StructureDefinition}
  */
 export class StructureDefinition {
-  id: string;
+  // id: FHIRId; // provided by HasId mixin
   meta: Meta;
   implicitRules: string;
   language: string;
@@ -34,7 +35,7 @@ export class StructureDefinition {
   url: string;
   identifier: Identifier[];
   version: string;
-  name: string;
+  // name: string; // provided by HasName mixin
   title: string;
   status: string;
   experimental: boolean;
@@ -610,6 +611,9 @@ export class StructureDefinition {
   }
 }
 
+export interface StructureDefinition extends HasName, HasId {}
+applyMixins(StructureDefinition, [HasName, HasId]);
+
 export type StructureDefinitionMapping = {
   identity: string;
   uri?: string;
diff --git a/src/fhirtypes/ValueSet.ts b/src/fhirtypes/ValueSet.ts
index 938a920..8768e21 100644
--- a/src/fhirtypes/ValueSet.ts
+++ b/src/fhirtypes/ValueSet.ts
@@ -4,6 +4,8 @@ import { Extension } from '../fshtypes';
 import { Narrative, Resource, Identifier, CodeableConcept, Coding } from './dataTypes';
 import { ContactDetail, UsageContext } from './metaDataTypes';
 import { cloneDeep } from 'lodash';
+import { HasName, HasId } from './common';
+import { applyMixins } from '../utils';
 
 /**
  * Class representing a FHIR R4 ValueSet.
@@ -12,7 +14,7 @@ import { cloneDeep } from 'lodash';
  */
 
 export class ValueSet {
-  id: string;
+  // id: FHIRId; // provided by HasId mixin
   meta: Meta;
   implicitRules: string;
   language: string;
@@ -23,7 +25,7 @@ export class ValueSet {
   url: string;
   identifier: Identifier[];
   version: string;
-  name: string;
+  // name: string; // provided by HasName mixin
   title: string;
   status = 'active';
   experimental: boolean;
@@ -58,6 +60,9 @@ export class ValueSet {
   }
 }
 
+export interface ValueSet extends HasName, HasId {}
+applyMixins(ValueSet, [HasName, HasId]);
+
 export type ValueSetCompose = {
   lockedDate?: string;
   inactive?: boolean;
diff --git a/src/fhirtypes/common.ts b/src/fhirtypes/common.ts
index 6482772..3cba033 100644
--- a/src/fhirtypes/common.ts
+++ b/src/fhirtypes/common.ts
@@ -1,9 +1,11 @@
 import { StructureDefinition, PathPart, ElementDefinition, InstanceDefinition } from '.';
 import { FixedValueRule } from '../fshtypes/rules';
-import { FshReference, Instance } from '../fshtypes';
+import { FshReference, Instance, SourceInfo } from '../fshtypes';
 import { FSHTank } from '../import';
 import { Type, Fishable } from '../utils/Fishable';
 import cloneDeep = require('lodash/cloneDeep');
+import { logger } from '../utils';
+import { FHIRId, idRegex } from './primitiveTypes';
 
 /**
  * This function sets an instance property of an SD or ED if possible
@@ -146,7 +148,8 @@ export function replaceReferences(
   return clone ?? rule;
 }
 
-/* Returns the sliceName for a set of pathParts
+/**
+ * Returns the sliceName for a set of pathParts
  * @param {PathPart} pathPart - The part of the path to get a sliceName for
  * @returns {string} The slicenName for the path part
  */
@@ -176,3 +179,63 @@ export function replaceField(
     }
   }
 }
+
+const nameRegex = /^[A-Z]([A-Za-z0-9_]){0,254}$/;
+
+export class HasName {
+  name?: string;
+  /**
+   * Set the name and check if it matches the regular expression specified
+   * in the invariant for "name" properties. A name must be between 1 and 255 characters long,
+   * begin with an uppercase letter, and contain only uppercase letter, lowercase letter,
+   * numeral, and '_' characters.
+   * If the string does not match, log an error.
+   *
+   * @see {@link http://hl7.org/fhir/R4/structuredefinition-definitions.html#StructureDefinition.name}
+   * @see {@link http://hl7.org/fhir/R4/valueset-definitions.html#ValueSet.name}
+   * @see {@link http://hl7.org/fhir/R4/codesystem-definitions.html#CodeSystem.name}
+   * @param {string} name - The name to check against the name invariant
+   * @param {SourceInfo} sourceInfo - The FSH file and location that specified the name
+   */
+  setName(name: string, sourceInfo: SourceInfo) {
+    this.name = name;
+    if (!nameRegex.test(name)) {
+      logger.error(
+        `The string "${name}" does not represent a valid FHIR name. Valid names start with an upper-case ASCII letter ('A'..'Z') followed by any combination of of upper- or lower-case ASCII letters ('A'..'Z', and 'a'..'z'), numerals ('0'..'9') and '_', with a length limit of 255 characters.`,
+        sourceInfo
+      );
+    }
+  }
+}
+
+export class HasId {
+  id?: FHIRId;
+  /**
+   * Set the id and check if it matches the regular expression specified
+   * in the definition of the "id" type.
+   * If the FHIRId does not match, log an error.
+   *
+   * @param id - The new id to set
+   * @param sourceInfo - The FSH file and location that specified the id
+   */
+  setId(id: FHIRId, sourceInfo: SourceInfo) {
+    this.id = id;
+    this.validateId(sourceInfo);
+  }
+
+  /**
+   * Check if the current id matches the regular expression specified
+   * in the definition of the "id" type.
+   * If the FHIRId does not match, log an error.
+   *
+   * @param sourceInfo - The FSH file and location that specified the id
+   */
+  validateId(sourceInfo: SourceInfo) {
+    if (!idRegex.test(this.id)) {
+      logger.error(
+        `The string "${this.id}" does not represent a valid FHIR id. FHIR ids may contain any combination of upper- or lower-case ASCII letters ('A'..'Z', and 'a'..'z'), numerals ('0'..'9'), '-' and '.', with a length limit of 64 characters.`,
+        sourceInfo
+      );
+    }
+  }
+}
diff --git a/src/fhirtypes/primitiveTypes.ts b/src/fhirtypes/primitiveTypes.ts
index ba859eb..e162a52 100644
--- a/src/fhirtypes/primitiveTypes.ts
+++ b/src/fhirtypes/primitiveTypes.ts
@@ -1,4 +1,4 @@
-import { InvalidDateTimeError } from '../errors/InvalidDateTimeError';
+import { InvalidDateTimeError } from '../errors';
 
 /**
  * Represents the FHIR primitive type dateTime.
@@ -15,3 +15,14 @@ export function validateFHIRDateTime(dateTime: FHIRDateTime): void {
     throw new InvalidDateTimeError(dateTime);
   }
 }
+
+/**
+ * Represents the FHIR primitive type id.
+ * An id is a string between 1 and 64 characters long and contains only uppercase letter,
+ * lowercase letter, numeral, '-', and '.' characters.
+ *
+ * @see {@link http://hl7.org/fhir/R4/datatypes.html#id}
+ */
+export type FHIRId = string;
+
+export const idRegex = /^[A-Za-z0-9\-\.]{1,64}$/;
diff --git a/src/utils/Mixin.ts b/src/utils/Mixin.ts
new file mode 100644
index 0000000..693d0e7
--- /dev/null
+++ b/src/utils/Mixin.ts
@@ -0,0 +1,16 @@
+/**
+ * Helper function for applying mixins.
+ * @see {@link https://www.typescriptlang.org/docs/handbook/mixins.html#mixin-sample}
+ */
+
+export function applyMixins(derivedCtor: any, baseCtors: any[]) {
+  baseCtors.forEach(baseCtor => {
+    Object.getOwnPropertyNames(baseCtor.prototype).forEach(name => {
+      Object.defineProperty(
+        derivedCtor.prototype,
+        name,
+        Object.getOwnPropertyDescriptor(baseCtor.prototype, name)
+      );
+    });
+  });
+}
diff --git a/src/utils/index.ts b/src/utils/index.ts
index 41bd7b5..1dc5ef4 100644
--- a/src/utils/index.ts
+++ b/src/utils/index.ts
@@ -1,3 +1,4 @@
 export * from './Fishable';
 export * from './FSHLogger';
 export * from './MasterFisher';
+export * from './Mixin';


Test patch:
diff --git a/test/export/CodeSystemExporter.test.ts b/test/export/CodeSystemExporter.test.ts
index 78b5a1f..4adbd1a 100644
--- a/test/export/CodeSystemExporter.test.ts
+++ b/test/export/CodeSystemExporter.test.ts
@@ -2,6 +2,7 @@ import { CodeSystemExporter, Package } from '../../src/export';
 import { FSHDocument, FSHTank } from '../../src/import';
 import { FshCodeSystem } from '../../src/fshtypes';
 import { FshConcept } from '../../src/fshtypes/FshConcept';
+import { loggerSpy } from '../testhelpers';
 
 describe('CodeSystemExporter', () => {
   let doc: FSHDocument;
@@ -119,4 +120,29 @@ describe('CodeSystemExporter', () => {
       ]
     });
   });
+
+  it('should log a message when the code system has an invalid id', () => {
+    const codeSystem = new FshCodeSystem('StrangeSystem')
+      .withFile('Strange.fsh')
+      .withLocation([2, 4, 6, 23]);
+    codeSystem.id = 'Is this allowed?';
+    doc.codeSystems.set(codeSystem.name, codeSystem);
+    const exported = exporter.export().codeSystems;
+    expect(exported.length).toBe(1);
+    expect(exported[0].id).toBe('Is this allowed?');
+    expect(loggerSpy.getLastMessage()).toMatch(/does not represent a valid FHIR id/s);
+    expect(loggerSpy.getLastMessage()).toMatch(/File: Strange\.fsh.*Line: 2 - 6\D/s);
+  });
+
+  it('should log a message when the code system has an invalid name', () => {
+    const codeSystem = new FshCodeSystem('Strange.Code.System')
+      .withFile('Strange.fsh')
+      .withLocation([3, 4, 8, 24]);
+    doc.codeSystems.set(codeSystem.name, codeSystem);
+    const exported = exporter.export().codeSystems;
+    expect(exported.length).toBe(1);
+    expect(exported[0].name).toBe('Strange.Code.System');
+    expect(loggerSpy.getLastMessage()).toMatch(/does not represent a valid FHIR name/s);
+    expect(loggerSpy.getLastMessage()).toMatch(/File: Strange\.fsh.*Line: 3 - 8\D/s);
+  });
 });
diff --git a/test/export/InstanceExporter.test.ts b/test/export/InstanceExporter.test.ts
index 98bf443..145c955 100644
--- a/test/export/InstanceExporter.test.ts
+++ b/test/export/InstanceExporter.test.ts
@@ -176,6 +176,24 @@ describe('InstanceExporter', () => {
       expect(exported.toJSON()).toEqual(expectedInstanceJSON);
     });
 
+    it('should log a message when the instance has an invalid id', () => {
+      const myExamplePatient = new Instance('MyExample')
+        .withFile('Some.fsh')
+        .withLocation([3, 6, 6, 45]);
+      myExamplePatient.instanceOf = 'Patient';
+      const fixedValRule = new FixedValueRule('id');
+      fixedValRule.fixedValue = 'Some Patient';
+      myExamplePatient.rules.push(fixedValRule);
+      const exported = exportInstance(myExamplePatient);
+      const expectedInstanceJSON = {
+        resourceType: 'Patient',
+        id: 'Some Patient'
+      };
+      expect(exported.toJSON()).toEqual(expectedInstanceJSON);
+      expect(loggerSpy.getLastMessage()).toMatch(/does not represent a valid FHIR id/s);
+      expect(loggerSpy.getLastMessage()).toMatch(/File: Some\.fsh.*Line: 3 - 6\D/s);
+    });
+
     // Fixing top level elements
     it('should fix top level elements that are fixed on the Structure Definition', () => {
       const fixedValRule = new FixedValueRule('active');
diff --git a/test/export/StructureDefinitionExporter.test.ts b/test/export/StructureDefinitionExporter.test.ts
index e2cec94..4ca83ee 100644
--- a/test/export/StructureDefinitionExporter.test.ts
+++ b/test/export/StructureDefinitionExporter.test.ts
@@ -192,6 +192,27 @@ describe('StructureDefinitionExporter', () => {
     }).toThrow('Parent Bar not found for Foo');
   });
 
+  it('should log a message when the structure definition has an invalid id', () => {
+    const profile = new Profile('Wrong').withFile('Wrong.fsh').withLocation([1, 8, 4, 18]);
+    profile.id = 'will_not_work';
+    doc.profiles.set(profile.name, profile);
+    exporter.exportStructDef(profile);
+    const exported = pkg.profiles[0];
+    expect(exported.id).toBe('will_not_work');
+    expect(loggerSpy.getLastMessage()).toMatch(/does not represent a valid FHIR id/s);
+    expect(loggerSpy.getLastMessage()).toMatch(/File: Wrong\.fsh.*Line: 1 - 4\D/s);
+  });
+
+  it('should log a message when the structure definition has an invalid name', () => {
+    const profile = new Profile('Not-good').withFile('Wrong.fsh').withLocation([2, 8, 5, 18]);
+    doc.profiles.set(profile.name, profile);
+    exporter.exportStructDef(profile);
+    const exported = pkg.profiles[0];
+    expect(exported.name).toBe('Not-good');
+    expect(loggerSpy.getLastMessage()).toMatch(/does not represent a valid FHIR name/s);
+    expect(loggerSpy.getLastMessage()).toMatch(/File: Wrong\.fsh.*Line: 2 - 5\D/s);
+  });
+
   // Rules
   it('should emit an error and continue when the path is not found', () => {
     const profile = new Profile('Foo');
diff --git a/test/export/ValueSetExporter.test.ts b/test/export/ValueSetExporter.test.ts
index fd7d66b..f0f6cad 100644
--- a/test/export/ValueSetExporter.test.ts
+++ b/test/export/ValueSetExporter.test.ts
@@ -85,6 +85,31 @@ describe('ValueSetExporter', () => {
     });
   });
 
+  it('should log a message when the value set has an invalid id', () => {
+    const valueSet = new FshValueSet('BreakfastVS')
+      .withFile('Breakfast.fsh')
+      .withLocation([3, 7, 7, 12]);
+    valueSet.id = 'Delicious!';
+    doc.valueSets.set(valueSet.name, valueSet);
+    const exported = exporter.export().valueSets;
+    expect(exported.length).toBe(1);
+    expect(exported[0].id).toBe('Delicious!');
+    expect(loggerSpy.getLastMessage()).toMatch(/does not represent a valid FHIR id/s);
+    expect(loggerSpy.getLastMessage()).toMatch(/File: Breakfast\.fsh.*Line: 3 - 7\D/s);
+  });
+
+  it('should log a message when the value set has an invalid name', () => {
+    const valueSet = new FshValueSet('All-you-can-eat')
+      .withFile('Breakfast.fsh')
+      .withLocation([2, 4, 8, 23]);
+    doc.valueSets.set(valueSet.name, valueSet);
+    const exported = exporter.export().valueSets;
+    expect(exported.length).toBe(1);
+    expect(exported[0].name).toBe('All-you-can-eat');
+    expect(loggerSpy.getLastMessage()).toMatch(/does not represent a valid FHIR name/s);
+    expect(loggerSpy.getLastMessage()).toMatch(/File: Breakfast\.fsh.*Line: 2 - 8\D/s);
+  });
+
   it('should export each value set once, even if export is called more than once', () => {
     const breakfast = new FshValueSet('BreakfastVS');
     doc.valueSets.set(breakfast.name, breakfast);