Repository: FHIR/sushi
Pull request #646 (2020-10-20 21:19:06)

Change configuration format to support separate group names and IDs
Previously, custom groups defined in `sushi-config.yaml` used the same string for both the `id` and `name` properties for a given group.

For example, given the following YAML:

    groups:
      GroupA:
        name: Group A
        description: The Alpha Group
        resources:
        - StructureDefinition/animal-patient
        - StructureDefinition/arm-procedure

Previously, both the `name` and `id` properties in the generated IG JSON would have been `Group A`. This caused errors like the following when building the IG with the FHIR publisher:

> ImplementationGuide.definition.resource[0].groupingId
> id value 'Group A' is not valid

This commit changes SUSHI to set `id` to `GroupA` and `name` to `Group A`. This avoids the error when building the IG.

Groups where `name` is undefined are still supported:

    groups:
      GroupB:
        description: The Beta Group
        resources:
        ...

In this case, both `id` and `name` will be `GroupB`.

This is related to #590.

cmoesel: Thanks, @masnick.  I'll have to review this more closely, but I understand the approach.  In some ways, it makes sense since `ImplementationGuide.definition.resource.groupingId` refers to the `id` (not the `name`) -- so it makes sense that `id` would be the key for our groups.

That said, `id` is a more restrictive data type than `string` (which is data type of `name`).  If people have existing configs and currently use a group name that has invalid characters for `id` (like spaces or underscores) -- then this would present a problem (although, I guess that is already the case since we were assigning `name` to `id` before).  Alright...  We might want to consider logging an `error` when people use an invalid character for an `id`.

So I'll review this more closely later.  Thanks for the PR!
masnick: Thanks!

I don't feel strongly about this, but another option might be allowing people to manually override `id` like:

	someKeyHere:
		id: some_other_id
 		name: Some name


Patch:
diff --git a/src/fshtypes/Configuration.ts b/src/fshtypes/Configuration.ts
index 98454d7..cc014e7 100644
--- a/src/fshtypes/Configuration.ts
+++ b/src/fshtypes/Configuration.ts
@@ -138,6 +138,7 @@ export type Configuration = {
 };
 
 export type ConfigurationGroup = {
+  id: string;
   name: string;
   description?: string;
   resources: string[];
diff --git a/src/ig/IGExporter.ts b/src/ig/IGExporter.ts
index fb9e72d..57030a4 100644
--- a/src/ig/IGExporter.ts
+++ b/src/ig/IGExporter.ts
@@ -1080,21 +1080,28 @@ export class IGExporter {
   /**
    * Adds or updates a group in the implementation guide.
    *
-   * @param {string} name - name of the group, used as unique identifier
+   * @param {string} id - unique identifier for group
+   * @param {string} name - name of the group
    * @param {string} description - optional description of the group
    */
-  private addGroup(name: string, description?: string): void {
+  private addGroup(id: string, name?: string, description?: string): void {
     if (!this.ig.definition.grouping) {
       this.ig.definition.grouping = [];
     }
-    const existingGroup = this.ig.definition.grouping.find(group => group.name === name);
+    // Initially `name` and `id` were derived from the same parameter. This allows
+    // the function to still work with just a single parameter.
+    if (name === undefined) {
+      name = id;
+    }
+    const existingGroup = this.ig.definition.grouping.find(group => group.id === id);
     if (existingGroup) {
+      existingGroup.name = name;
       if (description) {
         existingGroup.description = description;
       }
     } else {
       this.ig.definition.grouping.push({
-        id: name,
+        id: id,
         name: name,
         ...(description && { description })
       });
@@ -1112,26 +1119,26 @@ export class IGExporter {
 
   private addConfiguredGroups(): void {
     for (const group of this.config.groups ?? []) {
-      this.addGroup(group.name, group.description);
+      this.addGroup(group.id, group.name, group.description);
       for (const resourceKey of group.resources) {
         const existingResource = this.ig.definition.resource.find(
           resource => resource.reference?.reference === resourceKey
         );
         if (!existingResource) {
-          logger.error(`Group ${group.name} configured with nonexistent resource ${resourceKey}`);
+          logger.error(`Group ${group.id} configured with nonexistent resource ${resourceKey}`);
         } else {
           if (existingResource.groupingId) {
-            if (existingResource.groupingId === group.name) {
+            if (existingResource.groupingId === group.id) {
               logger.warn(
-                `Resource ${resourceKey} is listed as a member of group ${group.name}, and does not need a groupingId.`
+                `Resource ${resourceKey} is listed as a member of group ${group.id}, and does not need a groupingId.`
               );
             } else {
               logger.error(
-                `Resource ${resourceKey} configured with groupingId ${existingResource.groupingId}, but listed as member of group ${group.name}.`
+                `Resource ${resourceKey} configured with groupingId ${existingResource.groupingId}, but listed as member of group ${group.id}.`
               );
             }
           }
-          existingResource.groupingId = group.name;
+          existingResource.groupingId = group.id;
         }
       }
     }
diff --git a/src/import/YAMLConfiguration.ts b/src/import/YAMLConfiguration.ts
index 2a6b216..794fc7c 100644
--- a/src/import/YAMLConfiguration.ts
+++ b/src/import/YAMLConfiguration.ts
@@ -318,6 +318,7 @@ export type YAMLConfigurationGlobalMap = {
 
 export type YAMLConfigurationGroupMap = {
   [key: string]: {
+    name?: ImplementationGuide['name'];
     description?: ImplementationGuideDefinitionGrouping['description']; // string
     resources: string[];
   };
diff --git a/src/import/importConfiguration.ts b/src/import/importConfiguration.ts
index ec920a8..9713dbb 100644
--- a/src/import/importConfiguration.ts
+++ b/src/import/importConfiguration.ts
@@ -589,8 +589,16 @@ function parseGroups(yamlGroups: YAMLConfigurationGroupMap): ConfigurationGroup[
   if (yamlGroups == null) {
     return;
   }
-  return Object.entries(yamlGroups).map(([name, details]) => {
-    return { name, ...details };
+  return Object.entries(yamlGroups).map(([id, groupObj]) => {
+    if (groupObj.name === undefined) {
+      groupObj.name = id;
+    }
+    return {
+      id: id,
+      name: groupObj.name,
+      description: groupObj.description,
+      resources: groupObj.resources
+    };
   });
 }
 


Test patch:
diff --git a/test/ig/IGExporter.IG.test.ts b/test/ig/IGExporter.IG.test.ts
index 2f80af6..3623ded 100644
--- a/test/ig/IGExporter.IG.test.ts
+++ b/test/ig/IGExporter.IG.test.ts
@@ -451,12 +451,14 @@ describe('IGExporter', () => {
     it('should create groups for each configured group', () => {
       config.groups = [
         {
-          name: 'MyPatientGroup',
+          id: 'MyPatientGroup',
+          name: 'My Patient Group',
           description: 'Group for some patient-related things.',
           resources: ['StructureDefinition/sample-patient', 'Patient/patient-example']
         },
         {
-          name: 'MyObservationGroup',
+          id: 'MyObservationGroup',
+          name: 'My Observation Group',
           description: 'Group for some observation-related things.',
           resources: ['StructureDefinition/sample-observation']
         }
@@ -467,12 +469,12 @@ describe('IGExporter', () => {
       const content = fs.readJSONSync(igPath);
       expect(content.definition.grouping).toContainEqual({
         id: 'MyPatientGroup',
-        name: 'MyPatientGroup',
+        name: 'My Patient Group',
         description: 'Group for some patient-related things.'
       });
       expect(content.definition.grouping).toContainEqual({
         id: 'MyObservationGroup',
-        name: 'MyObservationGroup',
+        name: 'My Observation Group',
         description: 'Group for some observation-related things.'
       });
       const samplePatient: ImplementationGuideDefinitionResource = content.definition.resource.find(
@@ -495,7 +497,8 @@ describe('IGExporter', () => {
     it('should log an error when a group is configured with a nonexistent resource', () => {
       config.groups = [
         {
-          name: 'BananaGroup',
+          id: 'BananaGroup',
+          name: 'Banana Group',
           description: 'Holds all the bananas.',
           resources: ['StructureDefinition/sample-banana']
         }
@@ -515,6 +518,7 @@ describe('IGExporter', () => {
       ];
       config.groups = [
         {
+          id: 'Capulets',
           name: 'Capulets',
           resources: ['StructureDefinition/sample-patient']
         }
@@ -534,7 +538,8 @@ describe('IGExporter', () => {
       ];
       config.groups = [
         {
-          name: 'JustOneGroup',
+          id: 'JustOneGroup',
+          name: 'Just One Group',
           resources: ['StructureDefinition/sample-patient']
         }
       ];
diff --git a/test/ig/IGExporter.test.ts b/test/ig/IGExporter.test.ts
index 915a40b..4e97252 100644
--- a/test/ig/IGExporter.test.ts
+++ b/test/ig/IGExporter.test.ts
@@ -89,7 +89,8 @@ describe('IGExporter', () => {
       const igPath = path.join(tempOut, 'input', 'ImplementationGuide-fhir.us.example.json');
       const igContent = fs.readJSONSync(igPath);
       expect(igContent.definition.grouping).toHaveLength(2);
-      expect(igContent.definition.grouping[0].name).toBe('GroupA');
+      expect(igContent.definition.grouping[0].id).toBe('GroupA');
+      expect(igContent.definition.grouping[0].name).toBe('Group A');
       expect(igContent.definition.resource).toHaveLength(1);
       expect(igContent.definition.resource[0].name).toBe('My Example Patient');
       expect(igContent.definition.page.page).toHaveLength(3);
diff --git a/test/import/YAMLConfiguration.test.ts b/test/import/YAMLConfiguration.test.ts
index 435cc1a..869083d 100644
--- a/test/import/YAMLConfiguration.test.ts
+++ b/test/import/YAMLConfiguration.test.ts
@@ -70,10 +70,12 @@ describe('YAMLConfiguration', () => {
       });
       expect(config.groups).toEqual({
         GroupA: {
+          name: 'Group A',
           description: 'The Alpha Group',
           resources: ['StructureDefinition/animal-patient', 'StructureDefinition/arm-procedure']
         },
         GroupB: {
+          name: 'Group B',
           description: 'The Beta Group',
           resources: ['StructureDefinition/bark-control', 'StructureDefinition/bee-sting']
         }
diff --git a/test/import/fixtures/example-config.yaml b/test/import/fixtures/example-config.yaml
index 6223f5b..c5e00a7 100644
--- a/test/import/fixtures/example-config.yaml
+++ b/test/import/fixtures/example-config.yaml
@@ -109,11 +109,13 @@ resources:
 # resource entries with the corresponding groupIds.
 groups:
   GroupA:
+    name: Group A
     description: The Alpha Group
     resources:
     - StructureDefinition/animal-patient
     - StructureDefinition/arm-procedure
   GroupB:
+    name: Group B
     description: The Beta Group
     resources:
     - StructureDefinition/bark-control
diff --git a/test/import/importConfiguration.test.ts b/test/import/importConfiguration.test.ts
index 30c019a..52da2f7 100644
--- a/test/import/importConfiguration.test.ts
+++ b/test/import/importConfiguration.test.ts
@@ -116,12 +116,14 @@ describe('importConfiguration', () => {
       ],
       groups: [
         {
-          name: 'GroupA',
+          id: 'GroupA',
+          name: 'Group A',
           description: 'The Alpha Group',
           resources: ['StructureDefinition/animal-patient', 'StructureDefinition/arm-procedure']
         },
         {
-          name: 'GroupB',
+          id: 'GroupB',
+          name: 'Group B',
           description: 'The Beta Group',
           resources: ['StructureDefinition/bark-control', 'StructureDefinition/bee-sting']
         }
@@ -1450,6 +1452,7 @@ describe('importConfiguration', () => {
     it('should convert the groups map to a list', () => {
       minYAML.groups = {
         GroupA: {
+          name: 'Group A',
           description: 'The Alpha Group',
           resources: ['StructureDefinition/animal-patient', 'StructureDefinition/arm-procedure']
         },
@@ -1461,11 +1464,13 @@ describe('importConfiguration', () => {
       const config = importConfiguration(minYAML, 'test-config.yaml');
       expect(config.groups).toEqual([
         {
-          name: 'GroupA',
+          id: 'GroupA',
+          name: 'Group A',
           description: 'The Alpha Group',
           resources: ['StructureDefinition/animal-patient', 'StructureDefinition/arm-procedure']
         },
         {
+          id: 'GroupB',
           name: 'GroupB',
           description: 'The Beta Group',
           resources: ['StructureDefinition/bark-control', 'StructureDefinition/bee-sting']