Knex imports sqlite in migrate:make when told to use pg #5102

IlyaSemenov · 2022-04-05T07:38:56Z

Environment

Knex version: 1.0.5
Database + version: Postgres 14
OS: macOS 12.3

Bug

knex migrate:make foo wants (and fails) to load @vscode/sqlite3 even though the client is set to pg:

❯ yarn knex migrate:make foo
yarn run v1.22.17
$ /Users/semenov/tmp/p1/node_modules/.bin/knex migrate:make foo
Knex: run
$ npm install @vscode/sqlite3 --save
Cannot find module '@vscode/sqlite3'
Require stack:
- /Users/semenov/tmp/p1/node_modules/knex/lib/dialects/sqlite3/index.js
- /Users/semenov/tmp/p1/node_modules/knex/lib/knex-builder/internal/config-resolver.js
- /Users/semenov/tmp/p1/node_modules/knex/lib/knex-builder/Knex.js
- /Users/semenov/tmp/p1/node_modules/knex/lib/index.js
- /Users/semenov/tmp/p1/node_modules/knex/knex.js
- /Users/semenov/tmp/p1/node_modules/knex/bin/cli.js
Error: Cannot find module '@vscode/sqlite3'
Require stack:
- /Users/semenov/tmp/p1/node_modules/knex/lib/dialects/sqlite3/index.js
- /Users/semenov/tmp/p1/node_modules/knex/lib/knex-builder/internal/config-resolver.js
- /Users/semenov/tmp/p1/node_modules/knex/lib/knex-builder/Knex.js
- /Users/semenov/tmp/p1/node_modules/knex/lib/index.js
- /Users/semenov/tmp/p1/node_modules/knex/knex.js
- /Users/semenov/tmp/p1/node_modules/knex/bin/cli.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Client_SQLite3._driver (/Users/semenov/tmp/p1/node_modules/knex/lib/dialects/sqlite3/index.js:33:12)
    at Client_SQLite3.initializeDriver (/Users/semenov/tmp/p1/node_modules/knex/lib/client.js:190:26)
    at new Client (/Users/semenov/tmp/p1/node_modules/knex/lib/client.js:75:12)
    at new Client_SQLite3 (/Users/semenov/tmp/p1/node_modules/knex/lib/dialects/sqlite3/index.js:22:5)
    at knex (/Users/semenov/tmp/p1/node_modules/knex/lib/knex-builder/Knex.js:12:28)
    at initKnex (/Users/semenov/tmp/p1/node_modules/knex/bin/cli.js:74:10)
/Users/semenov/tmp/p1/node_modules/knex/lib/client.js:197
      throw new Error(`${message}\n${e.message}`);
            ^

Error: Knex: run
$ npm install @vscode/sqlite3 --save
Cannot find module '@vscode/sqlite3'
Require stack:
- /Users/semenov/tmp/p1/node_modules/knex/lib/dialects/sqlite3/index.js
- /Users/semenov/tmp/p1/node_modules/knex/lib/knex-builder/internal/config-resolver.js
- /Users/semenov/tmp/p1/node_modules/knex/lib/knex-builder/Knex.js
- /Users/semenov/tmp/p1/node_modules/knex/lib/index.js
- /Users/semenov/tmp/p1/node_modules/knex/knex.js
- /Users/semenov/tmp/p1/node_modules/knex/bin/cli.js
    at Client_SQLite3.initializeDriver (/Users/semenov/tmp/p1/node_modules/knex/lib/client.js:197:13)
    at new Client (/Users/semenov/tmp/p1/node_modules/knex/lib/client.js:75:12)
    at new Client_SQLite3 (/Users/semenov/tmp/p1/node_modules/knex/lib/dialects/sqlite3/index.js:22:5)
    at knex (/Users/semenov/tmp/p1/node_modules/knex/lib/knex-builder/Knex.js:12:28)
    at initKnex (/Users/semenov/tmp/p1/node_modules/knex/bin/cli.js:74:10)
    at async Command.<anonymous> (/Users/semenov/tmp/p1/node_modules/knex/bin/cli.js:213:24)

Reduced test code

In an empty directory:

yarn add knex

create knexfile.js:

module.exports = {
  client: 'pg',
  connection: 'postgres://localhost/test'
}

run:

pnpm knex migrate:make foo

Cause

This is caused by:

knex/bin/cli.js

Line 212 in f380e48

    
           opts.client = opts.client || 'sqlite3'; // We don't really care about client when creating migrations

This fake client option which user never provided overrides whatever client is set in config.

The upgrade guide (referenced here) says:

If you are using sqlite3 driver dependency, please replace it with @vscode/sqlite3 in your package.json;

Note that it doesn't say that @vscode/sqlite3 is now obligatory peer dependency when using migrations. This is a bug, there is no point in having @vscode/sqlite3, it's not being imported for any reasonable purpose.

The text was updated successfully, but these errors were encountered:

dhensby · 2022-04-05T11:55:36Z

Yep, I'm experiencing this too.

The problem is that lodash/merge (used here) is replacing the original client value from the provided config with the "default" value that is set here.

I don't think lodash/merge works as assumed here, it will try to deep merge all configs and for non-mergable items (like string values) the last seen value is the one that is used. Locally, swapping the arguments works as expected, but that may also not be desired behaviour, it may be better just to remove the default client value completely (which also works locally).

dstarke · 2022-04-05T20:01:38Z

Until this is fixed, you can work around the issue by specifying your client on the command line, for example:
knex migrate:make --client pg foo

dhensby · 2022-04-05T21:29:26Z

An easier workaround is using 1.0.4 ;)

matusekma · 2022-04-06T09:57:22Z

Until this is fixed, you can work around the issue by specifying your client on the command line, for example: knex migrate:make --client pg foo

@dstarke This does not help unfortunately, because the cli does not pick up the client option.
So you either have to install the @vscode/sqlite3 package or remove this line locally.

dstarke · 2022-04-06T17:44:48Z

This does not help unfortunately, because the cli does not pick up the client option.

It did for me, and I don't have sqlite installed in the project where I tried it. However, I agree that the real solution is to remove the referenced line of code. It looks like it is supposed to be enforcing a default value for the configuration, but what it is actually doing is creating a default command line argument value, and the command line argument values override the values specified in the configuration file.

I'm happy to create a pull request that does that, but there are some additional complexities here that I'm not sure how to handle. It appears that the reason that line of code exists is that when you initialize knex, it will throw an error if no client is supplied. This makes sense for most other cases, but per the comment, for this particular command, we don't want to throw the error. If you want to preserve the behavior that this command can be run with no client configured, there needs to be an additional code path through initialization that skips throwing that error in this case.

So there's a choice to be made between preserving the current expected behavior of the command, and making the setup code somewhat messier for this special case.

fuzzysaj · 2022-04-08T22:31:51Z

Also had to rollback to Knex 1.0.4 just now. @matusekma, installing @vscode/sqlite3 package got past this first problem, but then another problem shows up where it complains about a sqlite3 option when creating a migration even though the client is specified as 'pg':
sqlite does not support inserting default values. Set the useNullAsDefault flag to hide this warning.

JakeTompkins · 2022-04-10T06:09:14Z

Until this is fixed, you can work around the issue by specifying your client on the command line, for example: knex migrate:make --client pg foo

This worked for me, thank you. I was banging my head against the problem for a couple of hours thinking my knexfile was wrong.

nmsobri · 2022-04-11T06:37:57Z

thx god i found this issues.. it wasted me 2 hours already.. how come patch version introduce breaking changes?

kibertoad · 2022-04-11T07:36:35Z

@OlivierCavadenti we need to hotfix this

OlivierCavadenti · 2022-04-11T07:48:57Z

I will check this evening and dig into it. Is PR is enough to solve the problem ? #5106

dhensby · 2022-04-11T11:52:01Z

@kibertoad / @OlivierCavadenti the regression is from the use of lodash/merge introduced in this PR: #4047

merge is not working in the way assumed by the usage and so the value from the second object is taking precedent over the first whereas the implementation implies the expectation that it is the other way round.

OlivierCavadenti · 2022-04-11T12:19:30Z

@kibertoad / @OlivierCavadenti the regression is from the use of lodash/merge introduced in this PR: #4047

merge is not working in the way assumed by the usage and so the value from the second object is taking precedent over the first whereas the implementation implies the expectation that it is the other way round.

Ok thanks for the analysis ! I will fix that this evening as soon as possible.

dstarke · 2022-04-11T16:20:57Z

merge is not working in the way assumed by the usage and so the value from the second object is taking precedent over the first

@OlivierCavadenti I don't think this is correct. That merge causes the command line arguments to override the settings from the configuration file, which I think is what you want. The problem is that a default client is assigned early as a command line argument if none is supplied there, so it acts like a command line argument and overrides the configuration file value.

kibertoad · 2022-04-12T07:20:14Z

Released in 1.0.6
Could you please confirm the fix helped?

IlyaSemenov · 2022-04-12T12:24:25Z

Yep, I confirm that it worked.

mohito12 · 2022-04-12T18:26:48Z

I'm still facing the same issue even though I've updated to 1.0.6 🤔

dstarke · 2022-04-12T19:11:43Z

The problem is not fixed for me either.

see: knex/knex#5102

dhensby · 2022-04-13T10:30:05Z

not fixed for me

OlivierCavadenti · 2022-04-13T10:38:34Z

Fixed here : #5106

kibertoad · 2022-04-13T10:39:04Z

@dhensby Can you try 1.0.7?

dhensby · 2022-04-13T10:42:25Z

FYI - just removing this line fixed it locally for me

Also, you have not pushed the tags into github for 1.0.6 or 1.0.7

I'll test 1.0.7 locally too

dhensby · 2022-04-13T10:44:43Z

1.0.7 appears to work locally for me now

kibertoad · 2022-04-13T11:11:20Z

@dhensby Thank you, published tags and documentation now

see: knex/knex#5102

dstarke mentioned this issue Apr 6, 2022

Fix cli migrate:make sqlite dependency #5106

Merged

OlivierCavadenti self-assigned this Apr 11, 2022

OlivierCavadenti mentioned this issue Apr 11, 2022

No client override during migrate:make #5109

Merged

OlivierCavadenti closed this as completed in #5109 Apr 11, 2022

dimkl pushed a commit to dimkl/ordering_system that referenced this issue Apr 12, 2022

fix: knex commands [api]

13de571

see: knex/knex#5102

dimkl added a commit to dimkl/ordering_system that referenced this issue Apr 26, 2022

fix: knex commands [api]

7352865

see: knex/knex#5102

dimkl added a commit to dimkl/ordering_system that referenced this issue Jul 27, 2022

fix: knex commands [api]

ec37b0b

see: knex/knex#5102

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Knex imports sqlite in migrate:make when told to use pg #5102

Knex imports sqlite in migrate:make when told to use pg #5102

IlyaSemenov commented Apr 5, 2022

dhensby commented Apr 5, 2022 •

edited

dstarke commented Apr 5, 2022

dhensby commented Apr 5, 2022

matusekma commented Apr 6, 2022 •

edited

dstarke commented Apr 6, 2022 •

edited

fuzzysaj commented Apr 8, 2022 •

edited

JakeTompkins commented Apr 10, 2022

nmsobri commented Apr 11, 2022

kibertoad commented Apr 11, 2022

OlivierCavadenti commented Apr 11, 2022

dhensby commented Apr 11, 2022

OlivierCavadenti commented Apr 11, 2022

dstarke commented Apr 11, 2022

kibertoad commented Apr 12, 2022

IlyaSemenov commented Apr 12, 2022

mohito12 commented Apr 12, 2022

dstarke commented Apr 12, 2022

dhensby commented Apr 13, 2022

OlivierCavadenti commented Apr 13, 2022

kibertoad commented Apr 13, 2022

dhensby commented Apr 13, 2022

dhensby commented Apr 13, 2022

kibertoad commented Apr 13, 2022

Knex imports sqlite in migrate:make when told to use pg #5102

Knex imports sqlite in migrate:make when told to use pg #5102

Comments

IlyaSemenov commented Apr 5, 2022

Environment

Bug

Reduced test code

Cause

dhensby commented Apr 5, 2022 • edited

dstarke commented Apr 5, 2022

dhensby commented Apr 5, 2022

matusekma commented Apr 6, 2022 • edited

dstarke commented Apr 6, 2022 • edited

fuzzysaj commented Apr 8, 2022 • edited

JakeTompkins commented Apr 10, 2022

nmsobri commented Apr 11, 2022

kibertoad commented Apr 11, 2022

OlivierCavadenti commented Apr 11, 2022

dhensby commented Apr 11, 2022

OlivierCavadenti commented Apr 11, 2022

dstarke commented Apr 11, 2022

kibertoad commented Apr 12, 2022

IlyaSemenov commented Apr 12, 2022

mohito12 commented Apr 12, 2022

dstarke commented Apr 12, 2022

dhensby commented Apr 13, 2022

OlivierCavadenti commented Apr 13, 2022

kibertoad commented Apr 13, 2022

dhensby commented Apr 13, 2022

dhensby commented Apr 13, 2022

kibertoad commented Apr 13, 2022

dhensby commented Apr 5, 2022 •

edited

matusekma commented Apr 6, 2022 •

edited

dstarke commented Apr 6, 2022 •

edited

fuzzysaj commented Apr 8, 2022 •

edited