Repository: FHIR/sushi (ts)

Issue:
Should SUSHI default id to name even when it is an invalid id
If authors don't provide an explicit `Id:` for profiles, extensions, value sets, or code systems, SUSHI defaults the `id` to be the same as the `name`.  It's possible, however, that the name contains characters that are invalid for an id.  In this case, we should probably do one of the following:
* sanitize the id to remove invalid characters
* log an error indicating that the id cannot safely default to the name value

If we sanitize the id, we could optionally log a warning or info indicating that we did that.

This is probably something that should be governed by the spec.  @markkramerus -- do you have an opinion on this?  I'd lean toward sanitizing the id and logging an info message -- but I don't have a strong opinion.

(For reference, see: https://chat.fhir.org/#narrow/stream/215610-shorthand/topic/Naming.20issues)

Pull request:
Sanitize ids that are valid names
Fixes #441.

Now when a FHIR id is specified that is an invalid id, but a valid name, that name will be turned into an id by replacing `_` with `-` and by truncating to 64 characters. A warning will be emitted, since the users really should just specify the id directly if they are running into this case. If the id is not a valid name, then we can't be sure we can turn it into a valid id, so we do nothing. This will happen even if the id is not being set via a name. Aka even if the user does:
```
CodeSystem: CodeSystem_MaritalStatus
Id: My_Invalid_Id
```

I thought this would be fine, since the old approach would be to log an error and export using `My_Invalid_Id`. In this approach, we would fix `My_Invalid_Id` to be `My-Invalid-Id`, which is not a valid id, and only log a warning. Either way the user will probably want to fix it, but this way the user at least gets a valid id in their output.

Patch:
diff --git a/src/fhirtypes/common.ts b/src/fhirtypes/common.ts
index 186d9f0..71145c9 100644
--- a/src/fhirtypes/common.ts
+++ b/src/fhirtypes/common.ts
@@ -481,11 +481,26 @@ export class HasId {
    * 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.
+   * If the id is a valid name, sanitize it to a valid id and log a warning
    *
    * @param sourceInfo - The FSH file and location that specified the id
    */
   validateId(sourceInfo: SourceInfo) {
-    if (!idRegex.test(this.id)) {
+    let validId = idRegex.test(this.id);
+    if (!validId && nameRegex.test(this.id)) {
+      // A valid name can be turned into a valid id by replacing _ with - and slicing to 64 character limit
+      const sanitizedId = this.id.replace(/_/g, '-').slice(0, 64);
+      if (idRegex.test(sanitizedId)) {
+        // Use the sanitized id, but warn the user to fix this
+        logger.warn(
+          `The string "${this.id}" represents a valid FHIR name but not a valid FHIR id. FHIR ids cannot contain "_" and can be at most 64 characters. The id will be exported as "${sanitizedId}". Avoid this warning by specifying a valid id directly using the "Id" keyword.`,
+          sourceInfo
+        );
+        this.id = sanitizedId;
+        validId = true;
+      }
+    }
+    if (!validId) {
       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


Test patch:
diff --git a/test/export/CodeSystemExporter.test.ts b/test/export/CodeSystemExporter.test.ts
index f96347f..92f60fe 100644
--- a/test/export/CodeSystemExporter.test.ts
+++ b/test/export/CodeSystemExporter.test.ts
@@ -165,6 +165,37 @@ describe('CodeSystemExporter', () => {
     expect(loggerSpy.getLastMessage('error')).toMatch(/File: Strange\.fsh.*Line: 3 - 8\D*/s);
   });
 
+  it('should sanitize the id and log a message when a valid name is used to make an invalid id', () => {
+    const codeSystem = new FshCodeSystem('Not_good_id')
+      .withFile('Wrong.fsh')
+      .withLocation([2, 8, 5, 18]);
+    doc.codeSystems.set(codeSystem.name, codeSystem);
+    const exported = exporter.export().codeSystems;
+    expect(exported[0].name).toBe('Not_good_id');
+    expect(exported[0].id).toBe('Not-good-id');
+    expect(loggerSpy.getLastMessage('warn')).toMatch(
+      /The string "Not_good_id" represents a valid FHIR name but not a valid FHIR id.*The id will be exported as "Not-good-id"/s
+    );
+    expect(loggerSpy.getLastMessage('warn')).toMatch(/File: Wrong\.fsh.*Line: 2 - 5\D*/s);
+  });
+
+  it('should sanitize the id and log a message when a long valid name is used to make an invalid id', () => {
+    let longId = 'Toolong';
+    while (longId.length < 65) longId += 'longer';
+    const codeSystem = new FshCodeSystem(longId).withFile('Wrong.fsh').withLocation([2, 8, 5, 18]);
+    doc.codeSystems.set(codeSystem.name, codeSystem);
+    const exported = exporter.export().codeSystems;
+    const expectedId = longId.slice(0, 64);
+    expect(exported[0].name).toBe(longId);
+    expect(exported[0].id).toBe(expectedId);
+    const warning = new RegExp(
+      `The string "${longId}" represents a valid FHIR name but not a valid FHIR id.*The id will be exported as "${expectedId}"`,
+      's'
+    );
+    expect(loggerSpy.getLastMessage('warn')).toMatch(warning);
+    expect(loggerSpy.getLastMessage('warn')).toMatch(/File: Wrong\.fsh.*Line: 2 - 5\D*/s);
+  });
+
   // CaretValueRules
   it('should apply a CaretValueRule', () => {
     const codeSystem = new FshCodeSystem('CaretCodeSystem');
diff --git a/test/export/InstanceExporter.test.ts b/test/export/InstanceExporter.test.ts
index 3357386..ccb29ff 100644
--- a/test/export/InstanceExporter.test.ts
+++ b/test/export/InstanceExporter.test.ts
@@ -233,6 +233,47 @@ describe('InstanceExporter', () => {
       expect(loggerSpy.getLastMessage()).toMatch(/File: Some\.fsh.*Line: 3 - 6\D*/s);
     });
 
+    it('should sanitize the id and log a message when a valid name is used to make an invalid id', () => {
+      const instance = new Instance('Foo').withFile('Wrong.fsh').withLocation([2, 8, 5, 18]);
+      instance.instanceOf = 'Patient';
+      const fixedValRule = new FixedValueRule('id');
+      fixedValRule.fixedValue = 'Some_Patient';
+      instance.rules.push(fixedValRule);
+      const exported = exportInstance(instance);
+      const expectedInstanceJSON = {
+        resourceType: 'Patient',
+        id: 'Some-Patient'
+      };
+      expect(exported.toJSON()).toEqual(expectedInstanceJSON);
+      expect(loggerSpy.getLastMessage('warn')).toMatch(
+        /The string "Some_Patient" represents a valid FHIR name but not a valid FHIR id.*The id will be exported as "Some-Patient"/s
+      );
+      expect(loggerSpy.getLastMessage('warn')).toMatch(/File: Wrong\.fsh.*Line: 2 - 5\D*/s);
+    });
+
+    it('should sanitize the id and log a message when a long valid name is used to make an invalid id', () => {
+      const instance = new Instance('Foo').withFile('Wrong.fsh').withLocation([2, 8, 5, 18]);
+      instance.instanceOf = 'Patient';
+      const fixedValRule = new FixedValueRule('id');
+      let longId = 'Toolong';
+      while (longId.length < 65) longId += 'longer';
+      fixedValRule.fixedValue = longId;
+      instance.rules.push(fixedValRule);
+      const exported = exportInstance(instance);
+      const expectedId = longId.slice(0, 64);
+      const expectedInstanceJSON = {
+        resourceType: 'Patient',
+        id: expectedId
+      };
+      expect(exported.toJSON()).toEqual(expectedInstanceJSON);
+      const warning = new RegExp(
+        `The string "${longId}" represents a valid FHIR name but not a valid FHIR id.*The id will be exported as "${expectedId}"`,
+        's'
+      );
+      expect(loggerSpy.getLastMessage('warn')).toMatch(warning);
+      expect(loggerSpy.getLastMessage('warn')).toMatch(/File: Wrong\.fsh.*Line: 2 - 5\D*/s);
+    });
+
     // Fixing top level elements
     it('should fix top level elements that are fixed by pattern[x] on the Structure Definition', () => {
       const cardRule = new CardRule('active');
diff --git a/test/export/StructureDefinitionExporter.test.ts b/test/export/StructureDefinitionExporter.test.ts
index 9fda2ce..adf2654 100644
--- a/test/export/StructureDefinitionExporter.test.ts
+++ b/test/export/StructureDefinitionExporter.test.ts
@@ -375,11 +375,11 @@ describe('StructureDefinitionExporter', () => {
 
   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';
+    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(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);
   });
@@ -394,6 +394,37 @@ describe('StructureDefinitionExporter', () => {
     expect(loggerSpy.getLastMessage()).toMatch(/File: Wrong\.fsh.*Line: 2 - 5\D*/s);
   });
 
+  it('should sanitize the id and log a message when a valid name is used to make an invalid id', () => {
+    const profile = new Profile('Not_good_id').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_id');
+    expect(exported.id).toBe('Not-good-id');
+    expect(loggerSpy.getLastMessage('warn')).toMatch(
+      /The string "Not_good_id" represents a valid FHIR name but not a valid FHIR id.*The id will be exported as "Not-good-id"/s
+    );
+    expect(loggerSpy.getLastMessage('warn')).toMatch(/File: Wrong\.fsh.*Line: 2 - 5\D*/s);
+  });
+
+  it('should sanitize the id and log a message when a long valid name is used to make an invalid id', () => {
+    let longId = 'Toolong';
+    while (longId.length < 65) longId += 'longer';
+    const profile = new Profile(longId).withFile('Wrong.fsh').withLocation([2, 8, 5, 18]);
+    doc.profiles.set(profile.name, profile);
+    exporter.exportStructDef(profile);
+    const exported = pkg.profiles[0];
+    const expectedId = longId.slice(0, 64);
+    expect(exported.name).toBe(longId);
+    expect(exported.id).toBe(expectedId);
+    const warning = new RegExp(
+      `The string "${longId}" represents a valid FHIR name but not a valid FHIR id.*The id will be exported as "${expectedId}"`,
+      's'
+    );
+    expect(loggerSpy.getLastMessage('warn')).toMatch(warning);
+    expect(loggerSpy.getLastMessage('warn')).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 1e2d444..0aef934 100644
--- a/test/export/ValueSetExporter.test.ts
+++ b/test/export/ValueSetExporter.test.ts
@@ -114,6 +114,37 @@ describe('ValueSetExporter', () => {
     expect(loggerSpy.getLastMessage('error')).toMatch(/File: Breakfast\.fsh.*Line: 2 - 8\D*/s);
   });
 
+  it('should sanitize the id and log a message when a valid name is used to make an invalid id', () => {
+    const valueSet = new FshValueSet('Not_good_id')
+      .withFile('Wrong.fsh')
+      .withLocation([2, 8, 5, 18]);
+    doc.valueSets.set(valueSet.name, valueSet);
+    const exported = exporter.export().valueSets;
+    expect(exported[0].name).toBe('Not_good_id');
+    expect(exported[0].id).toBe('Not-good-id');
+    expect(loggerSpy.getLastMessage('warn')).toMatch(
+      /The string "Not_good_id" represents a valid FHIR name but not a valid FHIR id.*The id will be exported as "Not-good-id"/s
+    );
+    expect(loggerSpy.getLastMessage('warn')).toMatch(/File: Wrong\.fsh.*Line: 2 - 5\D*/s);
+  });
+
+  it('should sanitize the id and log a message when a long valid name is used to make an invalid id', () => {
+    let longId = 'Toolong';
+    while (longId.length < 65) longId += 'longer';
+    const valueSet = new FshValueSet(longId).withFile('Wrong.fsh').withLocation([2, 8, 5, 18]);
+    doc.valueSets.set(valueSet.name, valueSet);
+    const exported = exporter.export().valueSets;
+    const expectedId = longId.slice(0, 64);
+    expect(exported[0].name).toBe(longId);
+    expect(exported[0].id).toBe(expectedId);
+    const warning = new RegExp(
+      `The string "${longId}" represents a valid FHIR name but not a valid FHIR id.*The id will be exported as "${expectedId}"`,
+      's'
+    );
+    expect(loggerSpy.getLastMessage('warn')).toMatch(warning);
+    expect(loggerSpy.getLastMessage('warn')).toMatch(/File: Wrong\.fsh.*Line: 2 - 5\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);