Repository: FHIR/sushi (ts)

Issue:
SUSHI crashes if you try to put extensions on pages in sushi-config.yaml
Apparently you can put FMM status on pages in an IG.  If you try to do this, however, SUSHI crashes and prints out a usage statement.

To reproduce, add this in your `sushi-config.yaml`:
```yaml
pages:
  index.md:
    title: Example Home
    extension:
      - url: http://hl7.org/fhir/StructureDefinition/structuredefinition-standards-status
        valueCode: trial-use
      - url: http://hl7.org/fhir/StructureDefinition/structuredefinition-fmm
        valueInteger: 3
```

If you run SUSHI, it will crash.  If you delete or comment out the extension parts, it runs fine.

Expected behavior: Generate an IG JSON with those extensions at `ImplementationGuide.definition.page[0].extension`.

See: https://chat.fhir.org/#narrow/stream/215610-shorthand/topic/FMM.20and.20standards.20status/near/284475473

Pull request:
Support extensions on IG pages + minimally better error handling
This PR fixes #1094.

Previously, when including extensions, the `parsePage` function was getting recursively called infinitely many times. This caused SUSHI to throw a "Maximum call stack size exceeded" error.

In order to support `extension` on configured pages in the sushi-config.yaml file, this PR adds the `extension` to the types for the ImplementationGuide pages, supports parsing that property properly when importing the configuration file, and adding the `extension` array when outputting the IG resource in the IGExporter. On this branch, when you add the extension array provided in the issue, SUSHI should not crash and the IG resource should include the extension information.

The part of this that caused the infinite recursion was not having the proper handling for the `extension` property [here](https://github.com/FHIR/sushi/pull/1102/files#diff-2b3d27d0825edf7a5d03c048e22bac40f11a3bfdafc16ade10e21efa0e9c6f5aR665). Including the `extension` property stops the recursion appropriately. The SUSHI usage message is logged when it crashes because it reaches the `catch` statement [here](https://github.com/FHIR/sushi/pull/1102/files#diff-841254fe75488c1bd4cd7f68f00b4be0e48dcfbc4a16b45847b68295e0e3b27bR170-R171), which includes outputting the usage. Part of this task was to also understand why the usage was logged and ideally not log it. However, it seems like the usage statement would be helpful in some of the cases where that try block could throw an error. Instead of removing the usage statement, I added in an additional log statement to hopefully give a better idea of the error that caused SUSHI to crash. I also added in a try/catch while parsing the configured pages in `parsePage` in case this issue happens on a different property. This will let the `parsePages` function catch the error, log it, and then continue importing the rest of the configuration as usual. ~However, I'm not sure if this is exactly what we wanted to resolve this portion of the task. Potential alternate options I'd like input on:~ Edit: I already changed my mind and picked a couple options, but if reviewers have strong feelings about ones I didn't pick, let me know.

- ✅  Keep it as is
- ❌ Have a safer way to ensure the recursion in `parsePage` stops
- ✅  Throw an error closer to `parsePage` and continue with the rest of configuration input after that
- ❌  Something else I haven't thought of


Patch:
diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json
index 979d086..14afb7b 100644
--- a/npm-shrinkwrap.json
+++ b/npm-shrinkwrap.json
@@ -2808,9 +2808,9 @@
       }
     },
     "commander": {
-      "version": "4.1.1",
-      "resolved": "https://registry.npmjs.org/commander/-/commander-4.1.1.tgz",
-      "integrity": "sha512-NOKm8xhkzAjzFx8B2v5OAHT+u5pRQc2UCa2Vq9jYL/31o2wi9mxBA7LIFs3sV5VSC49z6pEhfbMULvShKj26WA=="
+      "version": "8.3.0",
+      "resolved": "https://registry.npmjs.org/commander/-/commander-8.3.0.tgz",
+      "integrity": "sha512-OkTL9umf+He2DZkUq8f8J9of7yL6RJKI24dVITBmNfZBmri9zYZQrKkuXiKhyfPSu8tUhnVBB1iKXevvnlR4Ww=="
     },
     "concat-map": {
       "version": "0.0.1",
diff --git a/package.json b/package.json
index 873b785..fe1b098 100644
--- a/package.json
+++ b/package.json
@@ -84,7 +84,7 @@
     "antlr4": "~4.8.0",
     "axios": "^0.21.4",
     "chalk": "^3.0.0",
-    "commander": "^4.1.1",
+    "commander": "^8.2.0",
     "fhir": "^4.9.0",
     "fs-extra": "^8.1.0",
     "html-minifier": "^4.0.0",
diff --git a/src/app.ts b/src/app.ts
index 8d151a0..68e1e9e 100644
--- a/src/app.ts
+++ b/src/app.ts
@@ -2,7 +2,7 @@
 
 import path from 'path';
 import fs from 'fs-extra';
-import program from 'commander';
+import { Command } from 'commander';
 import chalk from 'chalk';
 import process from 'process';
 import { pad, padStart, padEnd } from 'lodash';
@@ -43,7 +43,7 @@ app().catch(e => {
 async function app() {
   let input: string;
 
-  program
+  const program = new Command()
     .name('sushi')
     .usage('[path-to-fsh-project] [options]')
     .option('-o, --out <out>', 'the path to the output folder')
@@ -69,7 +69,9 @@ async function app() {
     .action(function (pathToFshDefs) {
       input = pathToFshDefs;
     })
-    .parse(process.argv);
+    .showHelpAfterError()
+    .parse(process.argv)
+    .opts();
 
   if (program.init) {
     await init();
@@ -167,8 +169,11 @@ async function app() {
     }
     config = readConfig(originalInput);
     tank = fillTank(rawFSH, config);
-  } catch {
-    program.outputHelp();
+  } catch (e) {
+    // If no errors have been logged yet, log this exception so the user knows why we're exiting
+    if (stats.numError === 0) {
+      logger.error(`An unexpected error occurred: ${e.message ?? e}`);
+    }
     process.exit(1);
   }
 
diff --git a/src/fhirtypes/ImplementationGuide.ts b/src/fhirtypes/ImplementationGuide.ts
index c59d453..78b6630 100644
--- a/src/fhirtypes/ImplementationGuide.ts
+++ b/src/fhirtypes/ImplementationGuide.ts
@@ -74,6 +74,8 @@ export type ImplementationGuideDefinitionPage = {
   nameReference?: Reference;
   title?: string; // optional to support Configuration use case where title has a default
   generation?: ImplementationGuideDefinitionPageGeneration; // optional to support Configuration...
+  extension?: Extension[];
+  modifierExtension?: Extension[];
   page?: ImplementationGuideDefinitionPage[];
 };
 
diff --git a/src/ig/IGExporter.ts b/src/ig/IGExporter.ts
index 2098fa4..93c76a1 100644
--- a/src/ig/IGExporter.ts
+++ b/src/ig/IGExporter.ts
@@ -505,7 +505,9 @@ export class IGExporter {
       const igPage: ImplementationGuideDefinitionPage = {
         nameUrl: `${name}.html`,
         title: page.title ?? titleCase(words(name).join(' ')),
-        generation: page.generation ?? (fileType === 'md' ? 'markdown' : 'html')
+        generation: page.generation ?? (fileType === 'md' ? 'markdown' : 'html'),
+        ...(page.extension && { extension: page.extension }),
+        ...(page.modifierExtension && { modifierExtension: page.modifierExtension })
       };
       if (page.page?.length) {
         const igSubpages: ImplementationGuideDefinitionPage[] = [];
diff --git a/src/import/YAMLConfiguration.ts b/src/import/YAMLConfiguration.ts
index 853b718..8ce0513 100644
--- a/src/import/YAMLConfiguration.ts
+++ b/src/import/YAMLConfiguration.ts
@@ -15,6 +15,7 @@ import {
   ImplementationGuideGlobal,
   ImplementationGuideDefinitionGrouping,
   Reference,
+  Extension,
   Identifier
 } from '../fhirtypes';
 
@@ -359,7 +360,9 @@ export type YAMLConfigurationPage = null | {
     | '#markdown'
     | '#xml'
     | '#generated';
-  [key: string]: YAMLConfigurationPage | string; // string allowed so title/generation work
+  extension?: Extension[];
+  modifierExtension?: Extension[];
+  [key: string]: YAMLConfigurationPage | string | Extension[]; // other types allowed so title/generation/extension/modifierExtension work
 };
 
 export type YAMLConfigurationParameterMap = {
diff --git a/src/import/importConfiguration.ts b/src/import/importConfiguration.ts
index 530676c..7780cce 100644
--- a/src/import/importConfiguration.ts
+++ b/src/import/importConfiguration.ts
@@ -657,15 +657,31 @@ function parsePage(
       file
     );
   }
+  if (details?.extension) {
+    page.extension = details.extension;
+  }
+  if (details?.modifierExtension) {
+    page.modifierExtension = details.modifierExtension;
+  }
   if (details != null) {
     Object.entries(details).forEach(([key, value]) => {
-      if (key == 'title' || key == 'generation') {
+      if (
+        key == 'title' ||
+        key == 'generation' ||
+        key == 'extension' ||
+        key == 'modifierExtension'
+      ) {
         return;
       }
       if (page.page == null) {
         page.page = [];
       }
-      page.page.push(parsePage(key, value as YAMLConfigurationPage, `${property}[${key}]`, file));
+      // We only want to recursively parse the page if it defines another page
+      // Unfortunately, we can't just check typeof page === YAMLConfigurationPage so do our best
+      // This will ensure the recursion ends eventually because calling Object.entries('string') will loop forever
+      if (typeof value === 'object') {
+        page.page.push(parsePage(key, value as YAMLConfigurationPage, `${property}[${key}]`, file));
+      }
     });
   }
   return page;


Test patch:
diff --git a/test/ig/IGExporter.test.ts b/test/ig/IGExporter.test.ts
index 03f4297..12a83c8 100644
--- a/test/ig/IGExporter.test.ts
+++ b/test/ig/IGExporter.test.ts
@@ -96,6 +96,18 @@ describe('IGExporter', () => {
       expect(igContent.definition.resource[0].name).toBe('My Example Patient');
       expect(igContent.definition.page.page).toHaveLength(3);
       expect(igContent.definition.page.page[0].title).toBe('Example Home');
+      expect(igContent.definition.page.page[2].extension).toEqual([
+        {
+          url: 'http://example.org/my-extension',
+          valueCode: 'example-code'
+        }
+      ]);
+      expect(igContent.definition.page.page[2].modifierExtension).toEqual([
+        {
+          url: 'http://example.org/my-modifier-extension',
+          valueBoolean: true
+        }
+      ]);
       // one each for releaselabel and copyrightyear, three configured parameters, and one for the history
       expect(igContent.definition.parameter).toHaveLength(6);
       expect(igContent.definition.parameter[2]).toEqual({
diff --git a/test/import/YAMLConfiguration.test.ts b/test/import/YAMLConfiguration.test.ts
index 1da3fb1..738a921 100644
--- a/test/import/YAMLConfiguration.test.ts
+++ b/test/import/YAMLConfiguration.test.ts
@@ -88,7 +88,19 @@ describe('YAMLConfiguration', () => {
         'examples.xml': {
           title: 'Examples Overview',
           'simpleExamples.xml': null,
-          'complexExamples.xml': null
+          'complexExamples.xml': null,
+          extension: [
+            {
+              url: 'http://example.org/my-extension',
+              valueCode: 'example-code'
+            }
+          ],
+          modifierExtension: [
+            {
+              url: 'http://example.org/my-modifier-extension',
+              valueBoolean: true
+            }
+          ]
         }
       });
       expect(config.menu).toEqual({
diff --git a/test/import/fixtures/example-config.yaml b/test/import/fixtures/example-config.yaml
index f1abd74..609a399 100644
--- a/test/import/fixtures/example-config.yaml
+++ b/test/import/fixtures/example-config.yaml
@@ -147,6 +147,12 @@ pages:
     title: Examples Overview
     simpleExamples.xml:
     complexExamples.xml:
+    extension:
+      - url: http://example.org/my-extension
+        valueCode: example-code
+    modifierExtension:
+      - url: http://example.org/my-modifier-extension
+        valueBoolean: true
 
 # The ImplementationGuide resource defines several other properties
 # not represented above. These properties can be used as-is and
diff --git a/test/import/importConfiguration.test.ts b/test/import/importConfiguration.test.ts
index 420cbb3..6a8e7dd 100644
--- a/test/import/importConfiguration.test.ts
+++ b/test/import/importConfiguration.test.ts
@@ -135,6 +135,18 @@ describe('importConfiguration', () => {
         {
           nameUrl: 'examples.xml',
           title: 'Examples Overview',
+          extension: [
+            {
+              url: 'http://example.org/my-extension',
+              valueCode: 'example-code'
+            }
+          ],
+          modifierExtension: [
+            {
+              url: 'http://example.org/my-modifier-extension',
+              valueBoolean: true
+            }
+          ],
           page: [{ nameUrl: 'simpleExamples.xml' }, { nameUrl: 'complexExamples.xml' }]
         }
       ],
@@ -1620,6 +1632,55 @@ describe('importConfiguration', () => {
         }
       ]);
     });
+
+    it('should support pages with extensions', () => {
+      minYAML.pages = {
+        'index.md': {
+          title: 'Example Home'
+        },
+        'implementation.xml': null,
+        'examples.xml': {
+          title: 'Examples Overview',
+          'simpleExamples.xml': null,
+          'complexExamples.xml': null,
+          extension: [
+            {
+              url: 'http://extension.org/my-extension',
+              valueBoolean: true
+            }
+          ],
+          modifierExtension: [
+            {
+              url: 'http://extension.org/my-modifier-extension',
+              valueBoolean: true
+            }
+          ]
+        }
+      };
+      const config = importConfiguration(minYAML, 'test-config.yaml');
+      expect(config.pages).toEqual([
+        { nameUrl: 'index.md', title: 'Example Home' },
+        { nameUrl: 'implementation.xml' },
+        {
+          nameUrl: 'examples.xml',
+          title: 'Examples Overview',
+          extension: [
+            {
+              url: 'http://extension.org/my-extension',
+              valueBoolean: true
+            }
+          ],
+          modifierExtension: [
+            {
+              url: 'http://extension.org/my-modifier-extension',
+              valueBoolean: true
+            }
+          ],
+          page: [{ nameUrl: 'simpleExamples.xml' }, { nameUrl: 'complexExamples.xml' }]
+        }
+      ]);
+    });
+
     it('should support FSH syntax for pages.[name].generation', () => {
       minYAML.pages = {
         'index.md': {
@@ -1649,8 +1710,8 @@ describe('importConfiguration', () => {
         },
         'examples.xml': {
           title: 'Examples Overview',
+          // @ts-ignore Type '{ generation: "gas" }' is not assignable to type string | Extension[] | YAMLConfigurationPage. Types of property generation are incompatible
           'simpleExamples.xml': {
-            // @ts-ignore Type '"gas"' is not assignable to type '"generated" | "#generated" ...'.
             generation: 'gas'
           }
         }