Repository: FHIR/sushi
Pull request #1082 (2022-05-11 01:13:00)

Honor ENV variable for NPM_CONFIG_REGISTRY when checking sushi version
For large organizations it is common to have a local mirror of npmjs.org, and even go so far as disallowing installing NPM packages directly or blocking requests to registry.npmjs.org. This patch will honor the ENV setting to allow sushi to work in these situations -- preventing the sushi command from always failing.

This problem emerged after https://github.com/FHIR/sushi/pull/1036 was merged.

bkaney: FYI - I plan update this. I just realized a better approach would be to use `npm get registry` to obtain the URL - this way it would honor the ENV or `~/.npmrc` settings, etc.
cmoesel: Thanks, @bkaney!  We'll gladly take this once it is ready.

Be sure to see #1085 for some related changes.  One thing we did in that PR is we demoted the failure to reach NPM from _error_ to _warning_.
bkaney: @cmoesel  -- this last commit is what I think is better - using the output of `npm get registry`. What do you think of this approach?
cmoesel: Thanks, @bkaney.  I always like to think twice before introducing code that does an `exec` call.  I was hoping there would be a way to do it through an existing library (that doesn't call `exec` itself).  I just spent way too much time learning that:
* NPM gets the registry by looking at env vars, command flags, and files in four different possible locations (some dependent on where the `npm` binary lives).
* The `npm` module used to expose an API prior to version 8.0.0 (but we probably don't want to forever depend on an old version).
* While [npm-registry-fetch](https://github.com/npm/npm-registry-fetch) and [pacote](https://github.com/npm/pacote/) are good for fetching from the registry, they don't have the built-in logic to detect the registry (I tried).
* [@npmcli/config](https://github.com/npm/config/) doesn't load your current NPM config for you (I tried).

So it seems that if we want a robust solution (and we do), doing a simple `exec` on npm is the best way to go.  The only remaining question is... if we're going to do an exec on `npm` anyway, should we just cut to the chase and do `npm view fsh-sushi version`?  And maybe fallback to axios (w/ default registry) if calling npm fails? Or is that just silly?

Also, did you happen to test this when invoked via the IG Publisher to see if calling `npm` in that context still works?  IIRC, it may do some funky stuff w/ PATH.  I just want to be sure.
bkaney: Well it depends on what you think the importance for this information is. 

That fix that doesn't make the build fail if the orig implementation is what I would say is the minimum required (e.g. : https://github.com/FHIR/sushi/pull/1085/files#diff-3fef64f89103af27309d9b93d97ae52e1f0858ea40e887534c83a9829a69bc47).

But I would be happy to:
1. Replace with `npm view ...` (which if we are going to do any of this I agree makes the most sense)
2. Test with IG Publisher
3. Test on Windows

Let me know if this would be helpful.
cmoesel: Hi @bkaney -- Thanks for the response. I think I'm fine w/ using `npm view...` if we fail silently and fallback to the axios call to get the version from `https://registry.npmjs.org`.  If that sounds reasonable to you, then let's do it!  If you can test w/ IG Publisher and Windows, I can test with IG Publisher and Mac.

We're likely going to put out a SUSHI release today w/ the other fix.  If you continue w/ this PR, it will make it into the next release after that.
bkaney: Hey @cmoesel  - here is a reimplementation. I tested on mac/linux including IG Publisher. It will take a bit of time for me to test on windows with IG Publisher, because I don't really ever use windows and will have to learn how to setup java, ruby, etc on that platform.

Does someone on your team have windows?
cmoesel: Hey @bkaney -- thanks for continuing on this.  I can ask @mint-thompson to test this on Windows.  But first, I was really hoping to keep the GET request to the npmjs registry as a fallback for if the `npm` call fails.  There may be systems where `npm` is not on the path or the user cannot execute `npm` for some other reason.  In these cases, I'd like to try the NPMJS API call, as that stands a chance of succeeding when `npm view` does not.  Does that make sense?
cmoesel: Hi @bkaney.  It was nice to see you in Cleveland a few weeks ago!  I just wanted to check in to see if you were planning to reintegrate the GET call to npmjs just in case the NPM exec call fails...  If so, great!  If not, do you mind if I make that update before merging in this PR?
bkaney: > Hi @bkaney. It was nice to see you in Cleveland a few weeks ago! I just wanted to check in to see if you were planning to reintegrate the GET call to npmjs just in case the NPM exec call fails... If so, great! If not, do you mind if I make that update before merging in this PR?

I have a local WIP for this, let me try to get it pushed this evening for you to review.
bkaney: @cmoesel  - Here is an implementation that will fallback to use axios, and I rebased so there should be no conflicts.
cmoesel: Thanks, @bkaney!  Things have been pretty busy here, so I haven't had a chance to review yet, but we will! I see all of those nice unit tests!  Thanks!
cmoesel: @bkaney -- thanks for putting up with all my requests!  Functionally, this is looking good and I appreciate the tests!  One thing I'm not sure about though... If `npm` fails for whatever reason, SUSHI will log an error saying it can't determine the latest SUSHI version; but since we have a fallback, this isn't necessarily true (because the fallback method might work fine).  It ends up being confusing since SUSHI says it failed to get the latest version, but then later might tell you that your local version is not the latest version.

I'm a little torn, because as long as SUSHI finds the latest version _some way_, then the user should not care (and maybe shouldn't be bothered by error/warning messages).  On the other hand, if it does fail all the way through, it's helpful to have those messages for debugging.

I think we should either:
* Change the npm-based error and warning's log-level to debug and only log visibly to the user if the fallback also fails.
* Change the npm-based error's log-level to warning and update both NPM log messages to indicate that SUSHI failed to get the latest version using the local npm executable, but will attempt to get the latest version by querying the npm registry directly.
* Store the error causes in variables and if the primary and fallback both fail, log all relevant errors/warnings.

What do you think?
bkaney: @cmoesel - I like the first bullet, only warn-level, visible log if everything fails. Otherwise put in debug log.
cmoesel: @bkaney -- did you want to make that last change or shall I?

Patch:
diff --git a/src/utils/Processing.ts b/src/utils/Processing.ts
index 7fe8caa..738d543 100644
--- a/src/utils/Processing.ts
+++ b/src/utils/Processing.ts
@@ -2,6 +2,7 @@ import path from 'path';
 import fs from 'fs-extra';
 import readlineSync from 'readline-sync';
 import YAML from 'yaml';
+import { execSync } from 'child_process';
 import { isPlainObject, padEnd, sortBy, upperFirst } from 'lodash';
 import { EOL } from 'os';
 import { logger } from './FSHLogger';
@@ -558,21 +559,44 @@ export function getLocalSushiVersion(): string {
   return null;
 }
 
-export async function getLatestSushiVersion(): Promise<string> {
+async function getLatestSushiVersionFallback(): Promise<string> {
+  logger.info('Attempting fallback to determine version of sushi.');
   try {
     const res = await axiosGet('https://registry.npmjs.org/fsh-sushi');
     const latestVer = res.data['dist-tags'].latest;
-    if (latestVer == null) {
-      logger.warn('Unable to determine the latest version of sushi.');
-    } else {
+    if (latestVer.match(/^[0-9\.]*$/)) {
       return latestVer;
+    } else {
+      logger.warn('Unable to determine the latest version of sushi.');
     }
   } catch (e) {
     logger.warn(`Unable to determine the latest version of sushi: ${e.message}`);
   }
 }
 
-export async function checkSushiVersion(): Promise<any> {
+export async function getLatestSushiVersion(): Promise<string | undefined> {
+  let latestVer = undefined;
+
+  const getRegistryCmd = 'npm view fsh-sushi version';
+  try {
+    const execResult = execSync(getRegistryCmd)?.toString()?.replace(/\s*$/, '');
+    if (execResult.match(/^[0-9\.]*$/)) {
+      latestVer = execResult;
+    }
+  } catch (e) {
+    logger.info(`Unable to determine the latest version of sushi: ${e.message}`);
+  }
+
+  if (latestVer === undefined) {
+    latestVer = await getLatestSushiVersionFallback();
+  }
+  return latestVer;
+}
+
+export async function checkSushiVersion(): Promise<{
+  latest: string | undefined;
+  current: string;
+}> {
   const latest = await getLatestSushiVersion();
   const current = getLocalSushiVersion();
 


Test patch:
diff --git a/test/utils/Processing.test.ts b/test/utils/Processing.test.ts
index a8faeed..2584b50 100644
--- a/test/utils/Processing.test.ts
+++ b/test/utils/Processing.test.ts
@@ -1,4 +1,5 @@
 import axios from 'axios';
+import child_process from 'child_process';
 import fs from 'fs-extra';
 import path from 'path';
 import temp from 'temp';
@@ -1280,37 +1281,40 @@ describe('Processing', () => {
   });
 
   describe('#getLatestSushiVersion()', () => {
+    let mockedChildProcess: jest.Mocked<typeof child_process>;
     let mockedAxios: jest.Mocked<typeof axios>;
 
     beforeAll(() => {
+      jest.mock('child_process');
+      mockedChildProcess = child_process as jest.Mocked<typeof child_process>;
+      mockedChildProcess.execSync = jest.fn();
+
       jest.mock('axios');
       mockedAxios = axios as jest.Mocked<typeof axios>;
+      mockedAxios.get = jest.fn();
     });
 
     beforeEach(() => {
       loggerSpy.reset();
     });
+
     it('successfully fetches data', async () => {
-      const data = {
-        data: {
-          name: 'fsh-sushi',
-          'dist-tags': {
-            latest: '2.2.6',
-            beta: '2.0.0-beta.3',
-            'pre-1.0': '0.16.1',
-            internal: '2.0.0-beta.1-fshonline-hotfix'
-          }
-        }
-      };
-      mockedAxios.get.mockImplementationOnce(() => Promise.resolve(data));
+      // prettier-ignore
+      mockedChildProcess.execSync.mockImplementationOnce(() => Buffer.from("2.2.6\n")); // eslint-disable-line quotes
+      await expect(getLatestSushiVersion()).resolves.toEqual('2.2.6');
+
+      mockedChildProcess.execSync.mockImplementationOnce(() => Buffer.from('2.2.6'));
       await expect(getLatestSushiVersion()).resolves.toEqual('2.2.6');
     });
 
-    it('unsuccessfully fetches data due to latest tag being missing', async () => {
+    it('falls back to URL if primary process fails', async () => {
+      mockedChildProcess.execSync.mockImplementationOnce(() => Buffer.from('npm ERR! code E404'));
+
       const data = {
         data: {
           name: 'fsh-sushi',
           'dist-tags': {
+            latest: '2.1.6',
             beta: '2.0.0-beta.3',
             'pre-1.0': '0.16.1',
             internal: '2.0.0-beta.1-fshonline-hotfix'
@@ -1318,65 +1322,49 @@ describe('Processing', () => {
         }
       };
       mockedAxios.get.mockImplementationOnce(() => Promise.resolve(data));
-      await getLatestSushiVersion();
-      expect(loggerSpy.getLastMessage('warn')).toMatch(
-        'Unable to determine the latest version of sushi.'
-      );
+
+      await expect(getLatestSushiVersion()).resolves.toEqual('2.1.6');
     });
 
-    it("unsuccessfully fetches data due to 'dist-tags' being missing", async () => {
+    it("unsuccessfully fetches data if it doesn't look like a semver is not found", async () => {
+      mockedChildProcess.execSync.mockImplementationOnce(() => Buffer.from('npm ERR! code E404'));
       const data = {};
       mockedAxios.get.mockImplementationOnce(() => Promise.resolve(data));
-      await getLatestSushiVersion();
+
+      const version = await getLatestSushiVersion();
+      expect(version).toBeUndefined;
       expect(loggerSpy.getLastMessage('warn')).toMatch(
-        "Unable to determine the latest version of sushi: Cannot read property 'dist-tags' of undefined"
+        'Unable to determine the latest version of sushi'
       );
     });
   });
 
   describe('#checkSushiVersion()', () => {
-    let mockedAxios: jest.Mocked<typeof axios>;
+    let mockedChildProcess: jest.Mocked<typeof child_process>;
 
     beforeAll(() => {
-      jest.mock('axios');
-      mockedAxios = axios as jest.Mocked<typeof axios>;
+      jest.mock('child_process');
+      mockedChildProcess = child_process as jest.Mocked<typeof child_process>;
+      mockedChildProcess.execSync = jest.fn();
     });
 
-    beforeEach(() => {
-      loggerSpy.reset();
-    });
     it('returns an object with the latest and current sushi verisons', async () => {
       const localVersion = getLocalSushiVersion();
-      const data = {
-        data: {
-          name: 'fsh-sushi',
-          'dist-tags': {
-            latest: localVersion,
-            beta: '2.0.0-beta.3',
-            'pre-1.0': '0.16.1',
-            internal: '2.0.0-beta.1-fshonline-hotfix'
-          }
-        }
-      };
-      mockedAxios.get.mockImplementationOnce(() => Promise.resolve(data));
+
+      mockedChildProcess.execSync.mockImplementationOnce(() => Buffer.from(`${localVersion}\n`));
       const versionObj = await checkSushiVersion();
       expect(versionObj).toHaveProperty('latest');
       expect(versionObj).toHaveProperty('current');
       expect(versionObj).toStrictEqual({ latest: localVersion, current: localVersion });
     });
+
     it('should return an object with an undefined latest value when latest is not present', async () => {
       const localVersion = getLocalSushiVersion();
-      const data = {
-        data: {
-          name: 'fsh-sushi',
-          'dist-tags': {
-            beta: '2.0.0-beta.3',
-            'pre-1.0': '0.16.1',
-            internal: '2.0.0-beta.1-fshonline-hotfix'
-          }
-        }
-      };
-      mockedAxios.get.mockImplementationOnce(() => Promise.resolve(data));
+
+      // prettier-ignore
+      mockedChildProcess.execSync.mockImplementationOnce(() =>
+        Buffer.from("zsh: command not found: npm\n") // eslint-disable-line quotes
+      );
       const versionObj = await checkSushiVersion();
       expect(versionObj).toHaveProperty('latest');
       expect(versionObj).toHaveProperty('current');