Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,7 @@

# Do not include tsc source maps
**/*.js.map

# Do not include .js files from .ts files
util/version.js
dialects/index.js
Copy link
Collaborator

@kibertoad kibertoad Jan 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

1 change: 1 addition & 0 deletions lib/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ class Client extends EventEmitter {
}

initializeDriver() {
if (this.driver) return;
try {
this.driver = this._driver();
} catch (e) {
Expand Down
13 changes: 13 additions & 0 deletions lib/dialects/better-sqlite3/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,19 @@ class Client_BetterSQLite3 extends Client_SQLite3 {
return require('better-sqlite3');
}

/**
* Get `Version` from client or return `[0, 0, 0]` on failure.
*
* @returns {[number, number, number]}
*/
_clientVersion() {
// hard coding 3.25.1 as better-sqlite3 offers no way to get Sqlite3 version it was built with
// 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];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@code-ape code-ape Jan 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, two thoughts:

  1. 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.
  2. 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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// Get a raw connection from the database, returning a promise with the connection object.
async acquireRawConnection() {
const options = this.connectionSettings.options || {};
Expand Down
24 changes: 24 additions & 0 deletions lib/dialects/sqlite3/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const ViewCompiler = require('./schema/sqlite-viewcompiler');
const SQLite3_DDL = require('./schema/ddl');
const Formatter = require('../../formatter');
const QueryBuilder = require('./query/sqlite-querybuilder');
const { parseVersion, satisfiesVersion } = require('../../util/version');

class Client_SQLite3 extends Client {
constructor(config) {
Expand All @@ -42,6 +43,29 @@ class Client_SQLite3 extends Client {
return require('sqlite3');
}

/**
* Get `Version` from client or return `[0, 0, 0]` on failure.
*
* @returns {[number, number, number]}
*/
_clientVersion() {
this.initializeDriver();
const version = parseVersion(this.driver.VERSION);
if (!version) return [0, 0, 0];
return version;
}

/**
* Takes min, max, or both and returns if given client satisfies version
*
* @param {undefined | [number, number, number]} minVersion
* @param {undefined | [number, number, number]} maxVersion
* @returns {boolean}
*/
_satisfiesVersion(minVersion, maxVersion) {
return satisfiesVersion(this._clientVersion(), minVersion, maxVersion);
}

schemaCompiler() {
return new SchemaCompiler(this, ...arguments);
}
Expand Down
70 changes: 22 additions & 48 deletions lib/dialects/sqlite3/query/sqlite-querycompiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class QueryCompiler_SQLite3 extends QueryCompiler {
const insertValues = this.single.insert || [];
let sql = this.with() + `insert into ${this.tableName} `;

// Handle "empty cases" of: [], [{}], {}
if (Array.isArray(insertValues)) {
if (insertValues.length === 0) {
return '';
Expand Down Expand Up @@ -66,6 +67,7 @@ class QueryCompiler_SQLite3 extends QueryCompiler {
};
}

// 'insert into TABLE_NAME (column1, column2, ...)'
sql += `(${this.formatter.columnize(insertData.columns)})`;

// backwards compatible error
Expand All @@ -82,62 +84,34 @@ class QueryCompiler_SQLite3 extends QueryCompiler {
});
}

if (insertData.values.length === 1) {
const parameters = this.client.parameterize(
insertData.values[0],
this.client.valueForUndefined,
this.builder,
this.bindingsHolder
);
sql += ` values (${parameters})`;

const { onConflict, ignore, merge } = this.single;
if (onConflict && ignore) sql += this._ignore(onConflict);
else if (onConflict && merge) {
sql += this._merge(merge.updates, onConflict, insertValues);
const wheres = this.where();
if (wheres) sql += ` ${wheres}`;
}

const { returning } = this.single;
if (returning) {
sql += this._returning(returning);
}

return {
sql,
returning,
};
if (
!this.client._satisfiesVersion([3, 7, 11]) &&
Copy link
Collaborator

@kibertoad kibertoad Jan 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

insertData.values.length > 1
) {
throw new Error('Requires Sqlite 3.7.11 or newer to do multi-insert');
}

const blocks = [];
// Holds parameters wrapped in `()`
const parametersArray = [];
let i = -1;
while (++i < insertData.values.length) {
let i2 = -1;
const block = (blocks[i] = []);
let current = insertData.values[i];
current = current === undefined ? this.client.valueForUndefined : current;
while (++i2 < insertData.columns.length) {
block.push(
this.client.alias(
this.client.parameter(
current[i2],
this.builder,
this.bindingsHolder
),
this.formatter.wrap(insertData.columns[i2])
)
);
}
blocks[i] = block.join(', ');
const parameter = this.client.parameterize(
insertData.values[i],
this.client.valueForUndefined,
this.builder,
this.bindingsHolder
);
parametersArray.push(`(${parameter})`);
}
sql += ' select ' + blocks.join(' union all select ');
// 'insert into TABLE_NAME (column1, column2, ...) values (v1, v2, ...), (v3, v4, ...), ...'
sql += ` values ${parametersArray.join(', ')}`;

const { onConflict, ignore, merge } = this.single;
if (onConflict && ignore) sql += ' where true' + this._ignore(onConflict);
if (onConflict && ignore) sql += this._ignore(onConflict);
else if (onConflict && merge) {
sql +=
' where true' + this._merge(merge.updates, onConflict, insertValues);
sql += this._merge(merge.updates, onConflict, insertValues);
const wheres = this.where();
if (wheres) sql += ` ${wheres}`;
}

const { returning } = this.single;
Expand Down
22 changes: 22 additions & 0 deletions lib/query/querycompiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -1271,6 +1271,16 @@ class QueryCompiler {
return str;
}

/**
* Takes:
*
* - Single object or array of objects.
*
* Does:
*
* - If a raw value is given return it.
* - Else returns `{ columns, values }` object
*/
_prepInsert(data) {
const isRaw = rawOrFn_(
data,
Expand All @@ -1284,24 +1294,36 @@ class QueryCompiler {
const values = [];
if (!Array.isArray(data)) data = data ? [data] : [];
let i = -1;
// Iterate over each item given ...
while (++i < data.length) {
// ... ignoring null values ...
if (data[i] == null) break;
// ... if this is the first item populate `columns` from its sorted keys ...
if (i === 0) columns = Object.keys(data[i]).sort();
// ... create row value array based on `columns` ...
const row = new Array(columns.length);
// ... populate `keys` with items keys ...
const keys = Object.keys(data[i]);
// ... then for each key ...
let j = -1;
while (++j < keys.length) {
const key = keys[j];
// ... attempt to get the `column` index that key goes to ...
let idx = columns.indexOf(key);
// ... if no index found ...
if (idx === -1) {
// ... then retro-actively update existing columns and existing `values`
// to incorporate newly discovered key ...
columns = columns.concat(key).sort();
idx = columns.indexOf(key);
let k = -1;
while (++k < values.length) {
values[k].splice(idx, 0, undefined);
}
// ... and then update row being actively built.
row.splice(idx, 0, undefined);
}
// ... else if column index found populate it with value.
row[idx] = data[i][key];
}
values.push(row);
Expand Down
102 changes: 102 additions & 0 deletions lib/util/version.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/** Tuple of three non-negative integers representing a semantic version */
export type Version = [number, number, number];

/** Helper function to check `x` a integer where `x >= 0` */
export function isNonNegInt(x: unknown): x is number {
return typeof x === 'number' && Number.isInteger(x) && x >= 0;
}

/** Type-guard for `Version` type */
export function isVersion(x: unknown): x is Version {
return (
Array.isArray(x) &&
x.length === 3 &&
x.findIndex((y) => !isNonNegInt(y)) === -1
);
}

/** Parses given string into `Version` or returns `undefined` */
export function parseVersion(x: string): Version | undefined {
const versionRegex = /^(\d+)\.(\d+)\.(\d+)/m;
const versionNumbers = (versionRegex.exec(x) ?? [])
.slice(1, 4)
.map((x) => parseInt(x));
if (!isVersion(versionNumbers)) return undefined;
return versionNumbers;
}

/** Parses given string into `Version` or throws an error */
export function parseVersionOrError(x: string): Version {
const version = parseVersion(x);
if (version === undefined) {
throw new Error('Could not parse string to Version');
}
return version;
}

/**
* Compares two versions, returning a number to represent the result:
*
* - `1` means `v1 > v2`
* - `0` means `v1 == v2`
* - `-1` means `v1 < v2`
*/
export function compareVersions(v1: Version, v2: Version): 1 | 0 | -1 {
// Check major
if (v1[0] < v2[0]) return -1;
else if (v1[0] > v2[0]) return 1;
else {
// Check minor
if (v1[1] < v2[1]) return -1;
else if (v1[1] > v2[1]) return 1;
else {
// Check patch
if (v1[2] < v2[2]) return -1;
else if (v1[2] > v2[2]) return 1;
else return 0;
}
}
}

/**
* Returns `boolean` for if a given `version` satisfies the given `min` (inclusive) and `max` (exclusive).
*
* This will throw an error if:
*
* - Given `version` is NOT a valid `Version`
* - Neither `min` nor `max` is given
* - `min` is given but is NOT a valid `Version`
* - `max` is given but is NOT a valid `Version`
*/
export function satisfiesVersion(
version: Version,
min?: Version,
max?: Version
): boolean {
if (!min && !max) {
throw new Error('Must pass at least one version constraint');
}
if (!isVersion(version)) {
throw new Error('Invalid value given for: version');
}

// Check Min
let satisfiesMin = true;
if (min) {
if (!isVersion(min)) {
throw new Error('Invalid value given for: min');
}
satisfiesMin = compareVersions(version, min) > -1;
}

// Check max
let satisfiesMax = true;
if (max) {
if (!isVersion(max)) {
throw new Error('Invalid value given for: max');
}
satisfiesMax = compareVersions(version, max) === -1;
}

return satisfiesMin && satisfiesMax;
}
1 change: 1 addition & 0 deletions test/db-less-test-suite.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ describe('Util Tests', function () {
require('./unit/util/save-async-stack');
require('./unit/util/comma-no-paren-regex');
require('./unit/util/security');
require('./unit/util/version');
});

describe('Query Building Tests', function () {
Expand Down
Loading
Loading