sqlite with protocol in connection string fails

[juanpaco]

I'm using knex@0.12.9.

I'm using sqlite:///:memory: as a connection string, and that's the only config I'm passing in.

I expect this to work, but I get the following error:

TypeError: Argument 0 must be a string
    at TypeError (native)
    at /home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/knex/lib/dialects/sqlite3/index.js:99:16
    at Promise._execute (/home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/bluebird/js/release/debuggability.js:300:9)
    at Promise._resolveFromExecutor (/home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/bluebird/js/release/promise.js:483:18)
    at new Promise (/home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/bluebird/js/release/promise.js:79:10)
    at Client_SQLite3.acquireRawConnection (/home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/knex/lib/dialects/sqlite3/index.js:98:12)
    at Object.create (/home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/knex/lib/client.js:239:16)
    at Pool._createResource (/home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/generic-pool/lib/generic-pool.js:354:17)
    at Pool.dispense [as _dispense] (/home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/generic-pool/lib/generic-pool.js:314:10)
    at Pool.acquire (/home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/generic-pool/lib/generic-pool.js:436:8)
    at /home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/knex/lib/client.js:289:19
    at Promise._execute (/home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/bluebird/js/release/debuggability.js:300:9)
    at Promise._resolveFromExecutor (/home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/bluebird/js/release/promise.js:483:18)
    at new Promise (/home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/bluebird/js/release/promise.js:79:10)
    at Client_SQLite3.acquireConnection (/home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/knex/lib/client.js:280:12)
    at /home/dev/source/suchsoftware/event-sourcing-experiments/button-clicks/node_modules/knex/lib/runner.js:208:30

At line 99 of /knex/lib/dialects/sqlite3/index.js (in the transpiled version) it's trying to use _this.connectionSettings.filename, but having submitted just a connection string for the config, knex/lib/util/parse-connection.js doesn't set the filename property, so that will be undefined at the time it gets to the dialect. I did a quick, cruddy fix where I just tacked on the filename property, and that resolved the issue. I don't think that's the right fix though.

I'm happy to submit a PR for this one, assuming that this is a valid issue.

Edit: Updated the connection string to start the path correctly after discovering that was the problem with #2039. This issue is still there though, even with that fix.

[elhigu]

I just tried:

knex = require('knex')({client: 'sqlite3', connection: 'sqlite://:memory:'})

and it did work fine.

Could you provide complete example how to reproduce your problem?

[juanpaco]

The issue doesn't happen until you actually go to do something with the connection. I discovered it when trying to run a migration. My knexfile was:

module.exports = 'sqlite:///button-clicks.dev.sqlite'

I then just tried to run the migrations, and I saw the error and traced it to where I mentioned above.

[elhigu]

thats an invalid knexfile you have to export an object.

module.exports = { client: 'sqlite3', connection: 'sqlite:///button-clicks.dev.sqlite' };

[juanpaco]

I get the same result using the object. The code does handle the string-only version (https://github.com/tgriesser/knex/blob/master/src/index.js#L21).

On further inspection if no protocol is specified, then it looks like it will handle it properly (and indeed running it works) at https://github.com/tgriesser/knex/blob/master/src/util/parse-connection.js#L11, but if a protocol is supplied then it ends up not setting the filename property on the connection: https://github.com/tgriesser/knex/blob/master/src/util/parse-connection.js#L11.

It isn't obvious from the documentation or (https://github.com/tgriesser/knex/issues/780) that one shouldn't use the protocol for sqlite, given that not supplying it departs from the other adapters.

So, whether using an object or not, if a connection string is supplied for sqlite that contains a protocol, I'm seeing the failure.

[elhigu]

Yeah, I was able to reproduce it now: https://runkit.com/mikaelle/5909c9fe674d13001200008d

Seems like a bug. Thanks for the report!

[juanpaco]

yw. Thanks for the work maintaining the library. :)

I'm happy to make a PR to fix this. I don't relish long if-else lists, but it seems like an addition to capture sqlite around https://github.com/tgriesser/knex/blob/master/src/util/parse-connection.js#L36 would do the trick?

Are you open to getting a PR on it, and does that seem like an okay approach to the fix?

[elhigu]

If-else sounds good, I would gladly check a PR fixing this.

I'm not very picky about the internal implementation of this kind of really small and encapsulated functionality. Mostly I am interested to have complete set of test cases to make sure all the supported use cases will work in the future if the code is rewritten later on (I'm pretty sure that currently there are not enough tests for that part).

[francisbrito]

In case someone's wondering how to get knex to work with sqlite in memory, this snippet should do the trick:

export const createDatabaseClient = async () =>
  Knex({
    connection: {
      filename: ":memory:",
    },
    client: "sqlite3",
    useNullAsDefault: true,
  });

[kibertoad]

@juanpaco @elhigu Is this even valid syntax? https://github.com/mapbox/node-sqlite3/wiki/API#new-sqlite3databasefilename-mode-callback doesn't mention it, nor can I find it anywhere else. When I pass such value to driver, it chokes. We could remove the protocol part ourselves and just pass the rest of the string, but should we?

[elhigu]

I think we should have separate native connection string which is just passed to driver directly and separate knex connection string that is parsed as well as knex can understand it and then parsed data would be passed to driver... I think there is actually some PR / FR about this somewhere... (edit: found it https://github.com/knex/knex/issues/2354)

If current connection: string is already parsed, I think we could just ignore the sqlite:// part.