Repository: knex/knex
Pull request #5106 (2022-04-06 21:15:03)

Fix cli migrate:make sqlite dependency
This is a proposed fix for #5102 .

Recent changes to how command line options are prioritized relative to configured options in the CLI exposed a bug in `migrate:make`. If a command line `client` option was not supplied, the CLI would use `sqlite3` instead of whatever is configured in the configuration file. This causes an error in environments where the optional sqlite3 peer dependency is not installed.

This PR alters the CLI initialization for commands that don't care about the client (`migrate:make` and `seed:make`) to skip an early check for the presence of a `client` option, and supply a default value of `sqlite3` later in the initialization process where it won't override a configured value.

kibertoad: Is it possible to add CLI test for this?
dstarke: > Is it possible to add CLI test for this?

I don't think this can be done easily with the current CLI testing framework. The error this is attempting to fix only occurs when sqlite3 is not installed, and I don't see how to make that condition happen in a test. 

The configuration parameter that causes the problem has (I think) no observable effect on the output of the affected commands, so the testing methodology used for other tests of the CLI -- running a command then checking its output -- doesn't work here. 

In more fine-grained unit tests of the internals of the cli code, it might be possible to intercept and check the derived initialization parameters to knex based on various input and configuration setups. I don't see any existing tests of this type for the CLI currently, so figuring out how to do that that would be something of a project (for me, at least, with no prior experience in this repository).

That test would look something like this:
- create a configuration file with a `client` option of `pg`
- run the command line `migrate:make` command with no `client` option specified (or simulate running that command)
- intercept the knex initialization and check that the client option it was passed was in fact `pg`



dstarke: This issue should be fixed by #5109 . Thanks!
dstarke: Unfortunately, #5109 did not fix the issue, and its implementation has conflicts with this one. Also, this solution addressed a lot of issues that the proposed fix in #5111 does not.
OlivierCavadenti: @dstarke can you rebase ? I am quite sorry about conflicts with my PR by the way.


dstarke: @OlivierCavadenti I'll look at it now.
coveralls: 
[![Coverage Status](https://coveralls.io/builds/48226113/badge)](https://coveralls.io/builds/48226113)

Coverage remained the same at 92.273% when pulling **176d2936180616a5885efb87ba72cf3a40a6b34f on dstarke:fix-cli-migrate-make-sqlite-dependency** into **f2a75277a779fd74688378fd904ed5631cf0c5d0 on knex:master**.

dstarke: @OlivierCavadenti Ok, I think I rebased this correctly. This should roll back only your changes to the CLI and CLI utilities, and replace them with the strategy originally proposed in my earlier implementation. The test you added is still present, but as discussed above, I don't think it actually captures this problem, and I'm not sure how to add one that does.
kibertoad: @dstarke For test: shouldn't it be fairly simple to pass some kind of a fake client and expect it to fail because fake client is preserved?
dstarke: @kibertoad I added some tests that work as you described. In order to get the error message propagated to output correctly, I had to wrap the commands for `migrate:make` and 'seed:make` in a try/catch block to catch and exit with the error (as some other commands do).

Patch:
diff --git a/bin/cli.js b/bin/cli.js
index fa2e161b..3232ceb2 100755
--- a/bin/cli.js
+++ b/bin/cli.js
@@ -16,6 +16,7 @@ const {
   exit,
   success,
   checkLocalModule,
+  checkConfigurationOptions,
   getMigrationExtension,
   getSeedExtension,
   getStubPath,
@@ -42,7 +43,7 @@ async function openKnexfile(configPath) {
   return config;
 }
 
-async function initKnex(env, opts, noClientOverride) {
+async function initKnex(env, opts, useDefaultClientIfNotSpecified) {
   checkLocalModule(env);
   if (process.cwd() !== env.cwd) {
     process.chdir(env.cwd);
@@ -52,29 +53,35 @@ async function initKnex(env, opts, noClientOverride) {
     );
   }
 
+  if (!useDefaultClientIfNotSpecified) {
+    checkConfigurationOptions(env, opts);
+  }
+
   env.configuration = env.configPath
     ? await openKnexfile(env.configPath)
-    : mkConfigObj(opts, noClientOverride);
+    : mkConfigObj(opts);
 
-  let resolvedConfig = resolveEnvironmentConfig(
+  const resolvedConfig = resolveEnvironmentConfig(
     opts,
     env.configuration,
     env.configPath
   );
 
-  // In the other case, config is already override in mkConfigObj.
-  if (env.configPath) {
-    const optionsConfig = parseConfigObj(opts, noClientOverride);
-    resolvedConfig = merge(resolvedConfig, optionsConfig);
-  }
+  const optionsConfig = parseConfigObj(opts);
+  const config = merge(resolvedConfig, optionsConfig);
 
   // Migrations directory gets defaulted if it is undefined.
-  if (!env.configPath && !resolvedConfig.migrations.directory) {
-    resolvedConfig.migrations.directory = null;
+  if (!env.configPath && !config.migrations.directory) {
+    config.migrations.directory = null;
+  }
+
+  // Client gets defaulted if undefined and it's allowed
+  if (useDefaultClientIfNotSpecified && config.client === undefined) {
+    config.client = 'sqlite3';
   }
 
   const knex = require(env.modulePath);
-  return knex(resolvedConfig);
+  return knex(config);
 }
 
 function invoke() {
@@ -209,23 +216,26 @@ function invoke() {
       'Specify the migration stub to use. If using <name> the file must be located in config.migrations.directory'
     )
     .action(async (name) => {
-      const opts = commander.opts();
-      opts.client = opts.client || 'sqlite3'; // We don't really care about client when creating migrations
-      const instance = await initKnex(env, opts, true);
-      const ext = getMigrationExtension(env, opts);
-      const configOverrides = { extension: ext };
-
-      const stub = getStubPath('migrations', env, opts);
-      if (stub) {
-        configOverrides.stub = stub;
-      }
+      try {
+        const opts = commander.opts();
+        const instance = await initKnex(env, opts, true);  // Skip config check, we don't really care about client when creating migrations
+        const ext = getMigrationExtension(env, opts);
+        const configOverrides = { extension: ext };
+
+        const stub = getStubPath('migrations', env, opts);
+        if (stub) {
+          configOverrides.stub = stub;
+        }
 
-      instance.migrate
-        .make(name, configOverrides)
-        .then((name) => {
-          success(color.green(`Created Migration: ${name}`));
-        })
-        .catch(exit);
+        instance.migrate
+          .make(name, configOverrides)
+          .then((name) => {
+            success(color.green(`Created Migration: ${name}`));
+          })
+          .catch(exit);
+      } catch(err) {
+        exit(err);
+      }
     });
 
   commander
@@ -376,26 +386,29 @@ function invoke() {
       false
     )
     .action(async (name) => {
-      const opts = commander.opts();
-      opts.client = opts.client || 'sqlite3'; // We don't really care about client when creating seeds
-      const instance = await initKnex(env, opts);
-      const ext = getSeedExtension(env, opts);
-      const configOverrides = { extension: ext };
-      const stub = getStubPath('seeds', env, opts);
-      if (stub) {
-        configOverrides.stub = stub;
-      }
+      try {
+        const opts = commander.opts();
+        const instance = await initKnex(env, opts, true); // Skip config check, we don't really care about client when creating seeds
+        const ext = getSeedExtension(env, opts);
+        const configOverrides = { extension: ext };
+        const stub = getStubPath('seeds', env, opts);
+        if (stub) {
+          configOverrides.stub = stub;
+        }
 
-      if (opts.timestampFilenamePrefix) {
-        configOverrides.timestampFilenamePrefix = opts.timestampFilenamePrefix;
-      }
+        if (opts.timestampFilenamePrefix) {
+          configOverrides.timestampFilenamePrefix = opts.timestampFilenamePrefix;
+        }
 
-      instance.seed
-        .make(name, configOverrides)
-        .then((name) => {
-          success(color.green(`Created seed file: ${name}`));
-        })
-        .catch(exit);
+        instance.seed
+          .make(name, configOverrides)
+          .then((name) => {
+            success(color.green(`Created seed file: ${name}`));
+          })
+          .catch(exit);
+      } catch(err) {
+        exit(err);
+      }   
     });
 
   commander
diff --git a/bin/utils/cli-config-utils.js b/bin/utils/cli-config-utils.js
index 9e31d398..32f891ec 100644
--- a/bin/utils/cli-config-utils.js
+++ b/bin/utils/cli-config-utils.js
@@ -6,10 +6,10 @@ const tildify = require('tildify');
 const color = require('colorette');
 const argv = require('getopts')(process.argv.slice(2));
 
-function parseConfigObj(opts, noClientOverride) {
+function parseConfigObj(opts) {
   const config = { migrations: {} };
 
-  if (opts.client && (!noClientOverride || !config.client)) {
+  if (opts.client) {
     config.client = opts.client;
   }
 
@@ -28,17 +28,11 @@ function parseConfigObj(opts, noClientOverride) {
   return config;
 }
 
-function mkConfigObj(opts, noClientOverride) {
-  if (!opts.client) {
-    throw new Error(
-      `No configuration file found and no commandline connection parameters passed`
-    );
-  }
-
+function mkConfigObj(opts) {
   const envName = opts.env || process.env.NODE_ENV || 'development';
   const resolvedClientName = resolveClientNameWithAliases(opts.client);
   const useNullAsDefault = resolvedClientName === 'sqlite3';
-  const parsedConfig = parseConfigObj(opts, noClientOverride);
+  const parsedConfig = parseConfigObj(opts);
 
   return {
     ext: DEFAULT_EXT,
@@ -105,6 +99,14 @@ function checkLocalModule(env) {
   }
 }
 
+function checkConfigurationOptions(env, opts) {
+  if (!env.configPath && !opts.client) {
+    throw new Error(
+      `No configuration file found and no commandline connection parameters passed`
+    );
+  }
+}
+
 function getMigrationExtension(env, opts) {
   const config = resolveEnvironmentConfig(
     opts,
@@ -199,6 +201,7 @@ module.exports = {
   exit,
   success,
   checkLocalModule,
+  checkConfigurationOptions,
   getSeedExtension,
   getMigrationExtension,
   getStubPath,


Test patch:
diff --git a/test/cli/migrate-make.spec.js b/test/cli/migrate-make.spec.js
index 32cd38c3..0a5362e9 100644
--- a/test/cli/migrate-make.spec.js
+++ b/test/cli/migrate-make.spec.js
@@ -163,6 +163,30 @@ module.exports = {
       );
     });
 
+    it('Does not create new migration with default knexfile with invalid client', () => {
+      fileHelper.registerGlobForCleanup(
+        'test/jake-util/knexfile_migrations/*_somename.js'
+      );
+      fileHelper.createFile(
+        process.cwd() + '/knexfile.js',
+        `
+module.exports = {
+  client: 'invalidclient',
+  migrations: {
+    directory: __dirname + '/test/jake-util/knexfile_migrations',
+  },
+};
+    `,
+        { isPathAbsolute: true }
+      );
+      return execCommand(
+        `node ${KNEX} migrate:make somename --knexpath=../knex.js`,
+        {
+          expectedErrorMessage: `Unknown configuration option 'client' value invalidclient.`,
+        }
+      );
+    });
+
     it('Create new migration with default ts knexfile', async () => {
       fileHelper.registerGlobForCleanup(
         'test/jake-util/knexfile_migrations/*_somename1.ts'
diff --git a/test/cli/seed-make.spec.js b/test/cli/seed-make.spec.js
index 8cc0b519..08dc03d5 100644
--- a/test/cli/seed-make.spec.js
+++ b/test/cli/seed-make.spec.js
@@ -133,6 +133,30 @@ module.exports = {
       expect(fileCount).to.equal(1);
     });
 
+    it('Does not create new seed with default knexfile with invalid client', () => {
+      fileHelper.registerGlobForCleanup(
+        'test/jake-util/knexfile_seeds/somename.js'
+      );
+      fileHelper.createFile(
+        process.cwd() + '/knexfile.js',
+        `
+module.exports = {
+  client: 'invalidclient',
+  migrations: {
+    directory: __dirname + '/test/jake-util/knexfile_seeds',
+  },
+};
+    `,
+        { isPathAbsolute: true }
+      );
+      return execCommand(
+        `node ${KNEX} seed:make somename --knexpath=../knex.js`,
+        {
+          expectedErrorMessage: `Unknown configuration option 'client' value invalidclient.`,
+        }
+      );
+    });
+
     it('Creates new seed with default ts knexfile', async () => {
       fileHelper.registerGlobForCleanup(
         'test/jake-util/knexfile_seeds/somename.ts'