Issue #5769: Sqlite support multi-value JSON insert #5782

code-ape · 2023-12-29T18:39:11Z

Issues:

Sqlite multi-insert fails to convert Object to JSON string but does for single insert #5769

Changes:

Added version utility functions for easily parsing and validating version comparisons, via lib/util/version.ts.
Added thorough testing for above version utility functions, via test/unit/util/version.js.
Updated sqlite3 client to support getting parsed client version and validating it against min and max version, via lib/dialects/sqlite3/index.js.
Updated better-sqlite3 client with known safe default client version and opened issue for getting it in the future, via lib/dialects/better-sqlite3/index.js.
Updated base client logic to not re-initialize driver if it is already initialized, via lib/client.js.
Updated sqlite3 insert query compiler to use "standard" SQL syntax instead of "select union all" pattern as it has not been needed for over a decade now since sqlite3 3.7.11 with error thrown on multi-inserts if sqlite3 client is found to be older than that version, via lib/dialects/sqlite3/query/sqlite-querycompiler.js.
Added tests for issue Sqlite multi-insert fails to convert Object to JSON string but does for single insert #5769 to validate multi-json insert works and client version check works, via test/integration2/query/insert/inserts.spec.js.
Fixed various broken tests related to sqlite3 from above changes, via test/.

coveralls · 2023-12-29T19:00:27Z

coverage: 92.8% (+0.01%) from 92.787%
when pulling 5db4d79 on code-ape:issue-5769/sqlite-multi-json
into 4ca3dd5 on knex:master.

kibertoad · 2024-01-01T17:23:23Z

lib/.gitignore

+
+# Do not include .js files from .ts files
+util/version.js
+dialects/index.js


this causes files not to be published either, I'd rather have them committed

Ohhhh! Good to know. There's a script generating these .gitignore items . I can update it stop doing that and check in the generated code.

it's ok to generate them prepublish too, to protect against someone forgetting to run it

kibertoad · 2024-01-01T17:27:50Z

lib/dialects/sqlite3/query/sqlite-querycompiler.js

-        returning,
-      };
+    if (
+      !this.client._satisfiesVersion([3, 7, 11]) &&


I'd rather do this check once on driver instantiation and set a feature boolean flag for subsequent checks

@kibertoad what do you want to do if we initialize with the driver with a version below the 3.7.11 threshold? Throw an error and halt, forcing at least that version? Something else?

Wouldn't it break anyway if someone attempts a multi-value JSON insert anyway? Do we need to force-break anything?

kibertoad · 2024-01-01T17:30:58Z

lib/dialects/better-sqlite3/index.js

+    // and better-sqlite3 1.0.0 shipped with 3.25.1 in 2016.
+    //
+    // Issue opened for this: https://github.com/WiseLibs/better-sqlite3/issues/1122
+    return [3, 25, 1];


can we require minimum better-sqlite with support for multi-insert via peerDependencies? I'd rather force people to upgrade rather than lock out them out of the beneficial feature

So, two thoughts:

If better-sqlite3 users are using the bundled sqlite3 build then they're fine. The 1.0.0 version of better-sqlite3 satisfies the 10+ year old requirement of being at least 3.11.7.

Technically the users of better-sqlite3 (and sqlite3 for that matter) can use a different C compilation of the sqlite3 library. So the NPM package version doesn't guarantee the version of sqlite3.c they've compiled.

What would you like to do?

PS - I've opened a PR to better-sqlite3 to fix this: WiseLibs/better-sqlite3#1124

After WiseLibs/better-sqlite3#1124 is merged we can do a more explicit check, but for older versions I would let user proceed by default and let them crash if their version is too old.

kibertoad · 2024-01-01T17:33:19Z

test/integration2/query/insert/inserts.spec.js

+
+        describe(`#5769 sqlite multi-insert uses standard syntax and supports JSON (does NOT include better-sqlite3)`, function () {
+          it(`should JSON.stringify Sqlite multi-insert of Objects`, async function () {
+            if (!isSQLite3(knex)) {


does newer better-sqlite support this feature?

I'll update the test and check. I think so, when I was originally authoring this I was just trying to focus on making sure that sqlite3 worked.

code-ape self-assigned this Dec 29, 2023

Initial work, needs polish

4e5c9a0

code-ape force-pushed the issue-5769/sqlite-multi-json branch from b35e0f1 to 4e5c9a0 Compare December 29, 2023 18:56

code-ape requested a review from kibertoad December 29, 2023 18:58

Adding tests and fixing existing ones

128dde5

code-ape force-pushed the issue-5769/sqlite-multi-json branch from 5616f55 to 128dde5 Compare December 30, 2023 18:46

code-ape added 2 commits December 30, 2023 14:01

Fixing better-sqlite3 _clientVersion function

1c20a18

Reverting change that broke CI test but passes on my mac

3cba457

code-ape marked this pull request as ready for review December 30, 2023 19:23

Fixing test to ONLY run on sqlite3

5db4d79

code-ape force-pushed the issue-5769/sqlite-multi-json branch from 5a5251e to 5db4d79 Compare December 30, 2023 19:24

code-ape mentioned this pull request Dec 31, 2023

Allow easy way to get Sqlite3 version WiseLibs/better-sqlite3#1122

Closed

kibertoad reviewed Jan 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #5769: Sqlite support multi-value JSON insert #5782

Issue #5769: Sqlite support multi-value JSON insert #5782

code-ape commented Dec 29, 2023 •

edited

coveralls commented Dec 29, 2023 •

edited

kibertoad Jan 1, 2024 •

edited

code-ape Jan 1, 2024

kibertoad Jan 1, 2024

kibertoad Jan 1, 2024 •

edited

code-ape Jan 1, 2024

kibertoad Jan 2, 2024

kibertoad Jan 1, 2024

code-ape Jan 1, 2024 •

edited

kibertoad Jan 2, 2024

kibertoad Jan 1, 2024 •

edited

code-ape Jan 1, 2024

Issue #5769: Sqlite support multi-value JSON insert #5782

Are you sure you want to change the base?

Issue #5769: Sqlite support multi-value JSON insert #5782

Conversation

code-ape commented Dec 29, 2023 • edited

coveralls commented Dec 29, 2023 • edited

kibertoad Jan 1, 2024 • edited

Choose a reason for hiding this comment

code-ape Jan 1, 2024

Choose a reason for hiding this comment

kibertoad Jan 1, 2024

Choose a reason for hiding this comment

kibertoad Jan 1, 2024 • edited

Choose a reason for hiding this comment

code-ape Jan 1, 2024

Choose a reason for hiding this comment

kibertoad Jan 2, 2024

Choose a reason for hiding this comment

kibertoad Jan 1, 2024

Choose a reason for hiding this comment

code-ape Jan 1, 2024 • edited

Choose a reason for hiding this comment

kibertoad Jan 2, 2024

Choose a reason for hiding this comment

kibertoad Jan 1, 2024 • edited

Choose a reason for hiding this comment

code-ape Jan 1, 2024

Choose a reason for hiding this comment

code-ape commented Dec 29, 2023 •

edited

coveralls commented Dec 29, 2023 •

edited

kibertoad Jan 1, 2024 •

edited

kibertoad Jan 1, 2024 •

edited

code-ape Jan 1, 2024 •

edited

kibertoad Jan 1, 2024 •

edited