From 3635cd96274c01e815ade6df9b4365b70e0ac4c6 Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Tue, 24 Dec 2024 16:20:51 +0100 Subject: [PATCH 01/12] sqlite: pass conflict type to conflict resolution handler --- doc/api/sqlite.md | 57 +++++++-- src/node_sqlite.cc | 25 ++-- test/parallel/test-sqlite-session.js | 165 ++++++++++++++++++++++++--- 3 files changed, 214 insertions(+), 33 deletions(-) diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md index 9e18a12ea90a71..8a3592113a5724 100644 --- a/doc/api/sqlite.md +++ b/doc/api/sqlite.md @@ -234,10 +234,24 @@ added: * `options` {Object} The configuration options for how the changes will be applied. * `filter` {Function} Skip changes that, when targeted table name is supplied to this function, return a truthy value. By default, all changes are attempted. - * `onConflict` {number} Determines how conflicts are handled. **Default**: `SQLITE_CHANGESET_ABORT`. - * `SQLITE_CHANGESET_OMIT`: conflicting changes are omitted. - * `SQLITE_CHANGESET_REPLACE`: conflicting changes replace existing values. - * `SQLITE_CHANGESET_ABORT`: abort on conflict and roll back database. + * `onConflict` {Function} A function that determines how to handle conflicts. The function receives one argument, + which can be one of the following values: + + * `SQLITE_CHANGESET_DATA`: A `DELETE` or `UPDATE` change does not contain the expected "before" values. + * `SQLITE_CHANGESET_NOTFOUND`: A row matching the primary key of the `DELETE` or `UPDATE` change does not exist. + * `SQLITE_CHANGESET_CONFLICT`: An `INSERT` change results in a duplicate primary key. + * `SQLITE_CHANGESET_FOREIGN_KEY`: Applying a change would result in a foreign key violation. + * `SQLITE_CHANGESET_CONSTRAINT`: Applying a change results in a `UNIQUE`, `CHECK`, or `NOT NULL` constraint + violation. + + The function should return one of the following values: + + * `SQLITE_CHANGESET_OMIT`: Omit conflicting changes. + * `SQLITE_CHANGESET_REPLACE`: Replace existing values with conflicting changes (only valid with + `SQLITE_CHANGESET_DATA` or `SQLITE_CHANGESET_CONFLICT` conflicts). + * `SQLITE_CHANGESET_ABORT`: Abort on conflict and roll back the database. + + **Default**: A function that returns `SQLITE_CHANGESET_ABORT`. * Returns: {boolean} Whether the changeset was applied succesfully without being aborted. An exception is thrown if the database is not @@ -496,9 +510,38 @@ An object containing commonly used constants for SQLite operations. The following constants are exported by the `sqlite.constants` object. -#### Conflict-resolution constants +#### Conflict resolution constants + +One of the following constants is available as an argument to the `onConflict` conflict resolution handler passed to [`database.applyChangeset()`](#databaseapplychangesetchangeset-options)). See also [Constants Passed To The Conflict Handler](https://www.sqlite.org/session/c_changeset_conflict.html) in the SQLite documentation. + + + + + + + + + + + + + + + + + + + + + + + + + + +
ConstantDescription
SQLITE_CHANGESET_DATAThe conflict handler is invoked with this constant when processing a DELETE or UPDATE change if a row with the required PRIMARY KEY fields is present in the database, but one or more other (non primary-key) fields modified by the update do not contain the expected "before" values.
SQLITE_CHANGESET_NOTFOUNDThe conflict handler is invoked with this constant when processing a DELETE or UPDATE change if a row with the required PRIMARY KEY fields is not present in the database.
SQLITE_CHANGESET_CONFLICTThis constant is passed to the conflict handler while processing an INSERT change if the operation would result in duplicate primary key values.
SQLITE_CHANGESET_CONSTRAINTIf foreign key handling is enabled, and applying a changeset leaves the database in a state containing foreign key violations, the conflict handler is invoked with this constant exactly once before the changeset is committed. If the conflict handler returns SQLITE_CHANGESET_OMIT, the changes, including those that caused the foreign key constraint violation, are committed. Or, if it returns SQLITE_CHANGESET_ABORT, the changeset is rolled back.
SQLITE_CHANGESET_FOREIGN_KEYIf any other constraint violation occurs while applying a change (i.e. a UNIQUE, CHECK or NOT NULL constraint), the conflict handler is invoked with this constant.
-The following constants are meant for use with [`database.applyChangeset()`](#databaseapplychangesetchangeset-options). +One of the following constants must be returned from the `onConflict` conflict resolution handler passed to [`database.applyChangeset()`](#databaseapplychangesetchangeset-options). See also [Constants Returned From The Conflict Handler](https://www.sqlite.org/session/c_changeset_abort.html) in the SQLite documentation. @@ -511,7 +554,7 @@ The following constants are meant for use with [`database.applyChangeset()`](#da - + diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 7f5e2f89ce9dba..6c0df79238c8fa 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -731,11 +731,11 @@ void DatabaseSync::CreateSession(const FunctionCallbackInfo& args) { // the reason for using static functions here is that SQLite needs a // function pointer -static std::function conflictCallback; +static std::function conflictCallback; static int xConflict(void* pCtx, int eConflict, sqlite3_changeset_iter* pIter) { if (!conflictCallback) return SQLITE_CHANGESET_ABORT; - return conflictCallback(); + return conflictCallback(eConflict); } static std::function filterCallback; @@ -773,15 +773,20 @@ void DatabaseSync::ApplyChangeset(const FunctionCallbackInfo& args) { options->Get(env->context(), env->onconflict_string()).ToLocalChecked(); if (!conflictValue->IsUndefined()) { - if (!conflictValue->IsNumber()) { + if (!conflictValue->IsFunction()) { THROW_ERR_INVALID_ARG_TYPE( env->isolate(), - "The \"options.onConflict\" argument must be a number."); + "The \"options.onConflict\" argument must be a function."); return; } - - int conflictInt = conflictValue->Int32Value(env->context()).FromJust(); - conflictCallback = [conflictInt]() -> int { return conflictInt; }; + Local conflictFunc = conflictValue.As(); + conflictCallback = [env, conflictFunc](int conflictType) -> int { + Local argv[] = {Integer::New(env->isolate(), conflictType)}; + Local result = + conflictFunc->Call(env->context(), Null(env->isolate()), 1, argv) + .ToLocalChecked(); + return result->Int32Value(env->context()).FromJust(); + }; } if (options->HasOwnProperty(env->context(), env->filter_string()) @@ -1662,6 +1667,12 @@ void DefineConstants(Local target) { NODE_DEFINE_CONSTANT(target, SQLITE_CHANGESET_OMIT); NODE_DEFINE_CONSTANT(target, SQLITE_CHANGESET_REPLACE); NODE_DEFINE_CONSTANT(target, SQLITE_CHANGESET_ABORT); + + NODE_DEFINE_CONSTANT(target, SQLITE_CHANGESET_DATA); + NODE_DEFINE_CONSTANT(target, SQLITE_CHANGESET_NOTFOUND); + NODE_DEFINE_CONSTANT(target, SQLITE_CHANGESET_CONFLICT); + NODE_DEFINE_CONSTANT(target, SQLITE_CHANGESET_CONSTRAINT); + NODE_DEFINE_CONSTANT(target, SQLITE_CHANGESET_FOREIGN_KEY); } static void Initialize(Local target, diff --git a/test/parallel/test-sqlite-session.js b/test/parallel/test-sqlite-session.js index 617c0c2aa71181..2d93a3bf69220b 100644 --- a/test/parallel/test-sqlite-session.js +++ b/test/parallel/test-sqlite-session.js @@ -128,15 +128,15 @@ test('database.createSession() - use table option to track specific table', (t) }); suite('conflict resolution', () => { + const createDataTableSql = `CREATE TABLE data ( + key INTEGER PRIMARY KEY, + value TEXT UNIQUE + ) STRICT`; + const prepareConflict = () => { const database1 = new DatabaseSync(':memory:'); const database2 = new DatabaseSync(':memory:'); - const createDataTableSql = `CREATE TABLE data ( - key INTEGER PRIMARY KEY, - value TEXT - ) STRICT - `; database1.exec(createDataTableSql); database2.exec(createDataTableSql); @@ -151,7 +151,91 @@ suite('conflict resolution', () => { }; }; - test('database.applyChangeset() - conflict with default behavior (abort)', (t) => { + const prepareDataConflict = () => { + const database1 = new DatabaseSync(':memory:'); + const database2 = new DatabaseSync(':memory:'); + + database1.exec(createDataTableSql); + database2.exec(createDataTableSql); + + const insertSql = 'INSERT INTO data (key, value) VALUES (?, ?)'; + database1.prepare(insertSql).run(1, 'hello'); + database2.prepare(insertSql).run(1, 'othervalue'); + const session = database1.createSession(); + database1.prepare('UPDATE data SET value = ? WHERE key = ?').run('foo', 1); + return { + database2, + changeset: session.changeset() + }; + }; + + const prepareNotFoundConflict = () => { + const database1 = new DatabaseSync(':memory:'); + const database2 = new DatabaseSync(':memory:'); + + database1.exec(createDataTableSql); + database2.exec(createDataTableSql); + + const insertSql = 'INSERT INTO data (key, value) VALUES (?, ?)'; + database1.prepare(insertSql).run(1, 'hello'); + const session = database1.createSession(); + database1.prepare('DELETE FROM data WHERE key = 1').run(); + return { + database2, + changeset: session.changeset() + }; + }; + + const prepareFkConflict = () => { + const database1 = new DatabaseSync(':memory:'); + const database2 = new DatabaseSync(':memory:'); + + database1.exec(createDataTableSql); + database2.exec(createDataTableSql); + const fkTableSql = `CREATE TABLE other ( + key INTEGER PRIMARY KEY, + ref REFERENCES data(key) + )`; + database1.exec(fkTableSql); + database2.exec(fkTableSql); + + const insertDataSql = 'INSERT INTO data (key, value) VALUES (?, ?)'; + const insertOtherSql = 'INSERT INTO other (key, ref) VALUES (?, ?)'; + database1.prepare(insertDataSql).run(1, 'hello'); + database2.prepare(insertDataSql).run(1, 'hello'); + database1.prepare(insertOtherSql).run(1, 1); + database2.prepare(insertOtherSql).run(1, 1); + + database1.exec('DELETE FROM other WHERE key = 1'); // So we don't get a fk violation in database1 + const session = database1.createSession(); + database1.prepare('DELETE FROM data WHERE key = 1').run(); // Changeset with fk violation + database2.exec('PRAGMA foreign_keys = ON'); // Needs to be supported, otherwise will fail here + + return { + database2, + changeset: session.changeset() + }; + }; + + const prepareConstraintConflict = () => { + const database1 = new DatabaseSync(':memory:'); + const database2 = new DatabaseSync(':memory:'); + + database1.exec(createDataTableSql); + database2.exec(createDataTableSql); + + const insertSql = 'INSERT INTO data (key, value) VALUES (?, ?)'; + const session = database1.createSession(); + database1.prepare(insertSql).run(1, 'hello'); + database2.prepare(insertSql).run(2, 'hello'); // database2 already constains hello + + return { + database2, + changeset: session.changeset() + }; + }; + + test('database.applyChangeset() - SQLITE_CHANGESET_CONFLICT conflict with default behavior (abort)', (t) => { const { database2, changeset } = prepareConflict(); // When changeset is aborted due to a conflict, applyChangeset should return false t.assert.strictEqual(database2.applyChangeset(changeset), false); @@ -160,40 +244,83 @@ suite('conflict resolution', () => { [{ value: 'world' }]); // unchanged }); - test('database.applyChangeset() - conflict with SQLITE_CHANGESET_ABORT', (t) => { + test('database.applyChangeset() - SQLITE_CHANGESET_CONFLICT conflict handled with SQLITE_CHANGESET_ABORT', (t) => { const { database2, changeset } = prepareConflict(); + let conflictType = null; const result = database2.applyChangeset(changeset, { - onConflict: constants.SQLITE_CHANGESET_ABORT + onConflict: (conflictType_) => { + conflictType = conflictType_; + return constants.SQLITE_CHANGESET_ABORT; + } }); // When changeset is aborted due to a conflict, applyChangeset should return false t.assert.strictEqual(result, false); + t.assert.strictEqual(conflictType, constants.SQLITE_CHANGESET_CONFLICT); deepStrictEqual(t)( database2.prepare('SELECT value from data').all(), [{ value: 'world' }]); // unchanged }); - test('database.applyChangeset() - conflict with SQLITE_CHANGESET_REPLACE', (t) => { - const { database2, changeset } = prepareConflict(); + test('database.applyChangeset() - SQLITE_CHANGESET_DATA conflict handled with SQLITE_CHANGESET_REPLACE', (t) => { + const { database2, changeset } = prepareDataConflict(); + let conflictType = null; const result = database2.applyChangeset(changeset, { - onConflict: constants.SQLITE_CHANGESET_REPLACE + onConflict: (conflictType_) => { + conflictType = conflictType_; + return constants.SQLITE_CHANGESET_REPLACE; + } }); // Not aborted due to conflict, so should return true t.assert.strictEqual(result, true); + t.assert.strictEqual(conflictType, constants.SQLITE_CHANGESET_DATA); deepStrictEqual(t)( database2.prepare('SELECT value from data ORDER BY key').all(), - [{ value: 'hello' }, { value: 'foo' }]); // replaced + [{ value: 'foo' }]); // replaced }); - test('database.applyChangeset() - conflict with SQLITE_CHANGESET_OMIT', (t) => { - const { database2, changeset } = prepareConflict(); + test('database.applyChangeset() - SQLITE_CHANGESET_NOTFOUND conflict with SQLITE_CHANGESET_OMIT', (t) => { + const { database2, changeset } = prepareNotFoundConflict(); + let conflictType = null; const result = database2.applyChangeset(changeset, { - onConflict: constants.SQLITE_CHANGESET_OMIT + onConflict: (conflictType_) => { + conflictType = conflictType_; + return constants.SQLITE_CHANGESET_OMIT; + } }); // Not aborted due to conflict, so should return true t.assert.strictEqual(result, true); - deepStrictEqual(t)( - database2.prepare('SELECT value from data ORDER BY key ASC').all(), - [{ value: 'world' }, { value: 'foo' }]); // Conflicting change omitted + t.assert.strictEqual(conflictType, constants.SQLITE_CHANGESET_NOTFOUND); + deepStrictEqual(t)(database2.prepare('SELECT value from data').all(), []); + }); + + test('database.applyChangeset() - SQLITE_CHANGESET_FOREIGN_KEY conflict', (t) => { + const { database2, changeset } = prepareFkConflict(); + let conflictType = null; + const result = database2.applyChangeset(changeset, { + onConflict: (conflictType_) => { + conflictType = conflictType_; + return constants.SQLITE_CHANGESET_OMIT; + } + }); + // Not aborted due to conflict, so should return true + t.assert.strictEqual(result, true); + t.assert.strictEqual(conflictType, constants.SQLITE_CHANGESET_FOREIGN_KEY); + deepStrictEqual(t)(database2.prepare('SELECT value from data').all(), []); + }); + + test('database.applyChangeset() - SQLITE_CHANGESET_CONSTRAINT conflict', (t) => { + const { database2, changeset } = prepareConstraintConflict(); + let conflictType = null; + const result = database2.applyChangeset(changeset, { + onConflict: (conflictType_) => { + conflictType = conflictType_; + return constants.SQLITE_CHANGESET_OMIT; + } + }); + // Not aborted due to conflict, so should return true + t.assert.strictEqual(result, true); + t.assert.strictEqual(conflictType, constants.SQLITE_CHANGESET_CONSTRAINT); + deepStrictEqual(t)(database2.prepare('SELECT key, value from data').all(), [{ key: 2, value: 'hello' }]); }); }); @@ -299,7 +426,7 @@ test('database.applyChangeset() - wrong arguments', (t) => { }, null); }, { name: 'TypeError', - message: 'The "options.onConflict" argument must be a number.' + message: 'The "options.onConflict" argument must be a function.' }); }); From e6001b39ffa2aab6e0a6c4d3884bb3ce20da3e40 Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Tue, 24 Dec 2024 19:45:24 +0100 Subject: [PATCH 02/12] sqlite: improve error message when applying changeset fails --- src/node_sqlite.cc | 21 +++++++++++++++++++-- test/parallel/test-sqlite-session.js | 14 ++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 6c0df79238c8fa..5f3a8aae3d6b24 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -114,6 +114,19 @@ inline void THROW_ERR_SQLITE_ERROR(Isolate* isolate, const char* message) { } } +inline void THROW_ERR_SQLITE_ERROR(Isolate* isolate, int errcode) { + const char* errstr = sqlite3_errstr(errcode); + Local e; + + if (CreateSQLiteError(isolate, errstr).ToLocal(&e)) { + e->Set(isolate->GetCurrentContext(), + OneByteString(isolate, "errcode"), + Integer::New(isolate, errcode)) + .IsNothing(); + isolate->ThrowException(e); + } +} + class UserDefinedFunction { public: explicit UserDefinedFunction(Environment* env, @@ -824,12 +837,16 @@ void DatabaseSync::ApplyChangeset(const FunctionCallbackInfo& args) { xFilter, xConflict, nullptr); + if (r == SQLITE_OK) { + args.GetReturnValue().Set(true); + return; + } if (r == SQLITE_ABORT) { + // this is not an error, return false args.GetReturnValue().Set(false); return; } - CHECK_ERROR_OR_THROW(env->isolate(), db->connection_, r, SQLITE_OK, void()); - args.GetReturnValue().Set(true); + THROW_ERR_SQLITE_ERROR(env->isolate(), r); } void DatabaseSync::EnableLoadExtension( diff --git a/test/parallel/test-sqlite-session.js b/test/parallel/test-sqlite-session.js index 2d93a3bf69220b..4289724abe1247 100644 --- a/test/parallel/test-sqlite-session.js +++ b/test/parallel/test-sqlite-session.js @@ -322,6 +322,20 @@ suite('conflict resolution', () => { t.assert.strictEqual(conflictType, constants.SQLITE_CHANGESET_CONSTRAINT); deepStrictEqual(t)(database2.prepare('SELECT key, value from data').all(), [{ key: 2, value: 'hello' }]); }); + + test("conflict resolution handler returns invalid value", (t) => { + const { database2, changeset } = prepareConflict(); + t.assert.throws(() => { + database2.applyChangeset(changeset, { + onConflict: () => { + return -1; + } + }); + }, { + name: 'Error', + message: 'bad parameter or other API misuse' + }); + }); }); test('database.createSession() - filter changes', (t) => { From a06a068518fb6dc746ccca4a9d96b682ee93eae9 Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Tue, 24 Dec 2024 21:52:31 +0100 Subject: [PATCH 03/12] sqlite: handle exception thrown in conflict handler --- doc/api/sqlite.md | 2 ++ src/node_sqlite.cc | 9 ++++++++- test/parallel/test-sqlite-session.js | 16 +++++++++++++++- 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md index 8a3592113a5724..7693738182cfaf 100644 --- a/doc/api/sqlite.md +++ b/doc/api/sqlite.md @@ -251,6 +251,8 @@ added: `SQLITE_CHANGESET_DATA` or `SQLITE_CHANGESET_CONFLICT` conflicts). * `SQLITE_CHANGESET_ABORT`: Abort on conflict and roll back the database. + When an error is thrown in the conflict handler, applying the changeset is aborted and the database is rolled back. + **Default**: A function that returns `SQLITE_CHANGESET_ABORT`. * Returns: {boolean} Whether the changeset was applied succesfully without being aborted. diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 5f3a8aae3d6b24..bcd90f129786df 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -9,6 +9,7 @@ #include "node_mem-inl.h" #include "sqlite3.h" #include "util-inl.h" +#include "v8-exception.h" #include @@ -42,6 +43,7 @@ using v8::Number; using v8::Object; using v8::SideEffectType; using v8::String; +using v8::TryCatch; using v8::Uint8Array; using v8::Value; @@ -795,9 +797,14 @@ void DatabaseSync::ApplyChangeset(const FunctionCallbackInfo& args) { Local conflictFunc = conflictValue.As(); conflictCallback = [env, conflictFunc](int conflictType) -> int { Local argv[] = {Integer::New(env->isolate(), conflictType)}; + TryCatch try_catch(env->isolate()); Local result = conflictFunc->Call(env->context(), Null(env->isolate()), 1, argv) - .ToLocalChecked(); + .FromMaybe(Local()); + if (try_catch.HasCaught()) { + try_catch.ReThrow(); + return SQLITE_CHANGESET_ABORT; + } return result->Int32Value(env->context()).FromJust(); }; } diff --git a/test/parallel/test-sqlite-session.js b/test/parallel/test-sqlite-session.js index 4289724abe1247..d3adee5a5c79c7 100644 --- a/test/parallel/test-sqlite-session.js +++ b/test/parallel/test-sqlite-session.js @@ -323,7 +323,7 @@ suite('conflict resolution', () => { deepStrictEqual(t)(database2.prepare('SELECT key, value from data').all(), [{ key: 2, value: 'hello' }]); }); - test("conflict resolution handler returns invalid value", (t) => { + test('conflict resolution handler returns invalid value', (t) => { const { database2, changeset } = prepareConflict(); t.assert.throws(() => { database2.applyChangeset(changeset, { @@ -336,6 +336,20 @@ suite('conflict resolution', () => { message: 'bad parameter or other API misuse' }); }); + + test('conflict resolution handler throws', (t) => { + const { database2, changeset } = prepareConflict(); + t.assert.throws(() => { + database2.applyChangeset(changeset, { + onConflict: () => { + throw new Error('some error'); + } + }); + }, { + name: 'Error', + message: 'some error' + }); + }); }); test('database.createSession() - filter changes', (t) => { From 5d2fc4e38fe37732c535e687cc4534aa88a83ad9 Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Sat, 28 Dec 2024 13:02:42 +0100 Subject: [PATCH 04/12] sqlite: avoid coercion when returning invalid value from conflict handler --- src/node_sqlite.cc | 2 ++ test/parallel/test-sqlite-session.js | 27 ++++++++++++++++----------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index bcd90f129786df..9f674e10f3009a 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -805,6 +805,8 @@ void DatabaseSync::ApplyChangeset(const FunctionCallbackInfo& args) { try_catch.ReThrow(); return SQLITE_CHANGESET_ABORT; } + constexpr auto invalid_value = -1; + if (!result->IsInt32()) return invalid_value; return result->Int32Value(env->context()).FromJust(); }; } diff --git a/test/parallel/test-sqlite-session.js b/test/parallel/test-sqlite-session.js index d3adee5a5c79c7..eff33c029a7498 100644 --- a/test/parallel/test-sqlite-session.js +++ b/test/parallel/test-sqlite-session.js @@ -324,17 +324,22 @@ suite('conflict resolution', () => { }); test('conflict resolution handler returns invalid value', (t) => { - const { database2, changeset } = prepareConflict(); - t.assert.throws(() => { - database2.applyChangeset(changeset, { - onConflict: () => { - return -1; - } - }); - }, { - name: 'Error', - message: 'bad parameter or other API misuse' - }); + const invalidValues = [-1, {}, null]; + + for (const invalidValue of invalidValues) { + const { database2, changeset } = prepareConflict(); + t.assert.throws(() => { + database2.applyChangeset(changeset, { + onConflict: () => { + return invalidValue; + } + }); + }, { + name: 'Error', + message: 'bad parameter or other API misuse', + errcode: 21 + }, `Did not throw expected exception when returning '${invalidValue}' from conflict handler`); + } }); test('conflict resolution handler throws', (t) => { From a45e4c07ef9a0e1f1f1a13ef785364aaf6128030 Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Sat, 28 Dec 2024 13:06:07 +0100 Subject: [PATCH 05/12] sqlite: remove redundant header --- src/node_sqlite.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 9f674e10f3009a..20c5bfda6d97c0 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -9,7 +9,6 @@ #include "node_mem-inl.h" #include "sqlite3.h" #include "util-inl.h" -#include "v8-exception.h" #include From c4a03b5e37fa8f419e3dfc0aa103c49084de3416 Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Sat, 28 Dec 2024 19:03:37 +0100 Subject: [PATCH 06/12] doc: sqlite documentation improvements --- doc/api/sqlite.md | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md index 7693738182cfaf..f1cbac711cf0cb 100644 --- a/doc/api/sqlite.md +++ b/doc/api/sqlite.md @@ -514,7 +514,9 @@ The following constants are exported by the `sqlite.constants` object. #### Conflict resolution constants -One of the following constants is available as an argument to the `onConflict` conflict resolution handler passed to [`database.applyChangeset()`](#databaseapplychangesetchangeset-options)). See also [Constants Passed To The Conflict Handler](https://www.sqlite.org/session/c_changeset_conflict.html) in the SQLite documentation. +One of the following constants is available as an argument to the `onConflict` +conflict resolution handler passed to [`database.applyChangeset()`][]. See also +[Constants Passed To The Conflict Handler]() in the SQLite documentation.
SQLITE_CHANGESET_REPLACEConflicting changes replace existing values.Conflicting changes replace existing values. Note that this value can only be returned when the type of conflict is either `SQLITE_CHANGESET_DATA` or `SQLITE_CHANGESET_CONFLICT`.
SQLITE_CHANGESET_ABORT
@@ -535,7 +537,7 @@ One of the following constants is available as an argument to the `onConflict` c - + @@ -543,7 +545,9 @@ One of the following constants is available as an argument to the `onConflict` c
SQLITE_CHANGESET_CONSTRAINTIf foreign key handling is enabled, and applying a changeset leaves the database in a state containing foreign key violations, the conflict handler is invoked with this constant exactly once before the changeset is committed. If the conflict handler returns SQLITE_CHANGESET_OMIT, the changes, including those that caused the foreign key constraint violation, are committed. Or, if it returns SQLITE_CHANGESET_ABORT, the changeset is rolled back.If foreign key handling is enabled, and applying a changeset leaves the database in a state containing foreign key violations, the conflict handler is invoked with this constant exactly once before the changeset is committed. If the conflict handler returns SQLITE_CHANGESET_OMIT, the changes, including those that caused the foreign key constraint violation, are committed. Or, if it returns SQLITE_CHANGESET_ABORT, the changeset is rolled back.
SQLITE_CHANGESET_FOREIGN_KEY
-One of the following constants must be returned from the `onConflict` conflict resolution handler passed to [`database.applyChangeset()`](#databaseapplychangesetchangeset-options). See also [Constants Returned From The Conflict Handler](https://www.sqlite.org/session/c_changeset_abort.html) in the SQLite documentation. +One of the following constants must be returned from the `onConflict` conflict +resolution handler passed to [`database.applyChangeset()`][]. See also +[Constants Returned From The Conflict Handler][] in the SQLite documentation. @@ -556,7 +560,7 @@ One of the following constants must be returned from the `onConflict` conflict r - + @@ -565,6 +569,9 @@ One of the following constants must be returned from the `onConflict` conflict r
SQLITE_CHANGESET_REPLACEConflicting changes replace existing values. Note that this value can only be returned when the type of conflict is either `SQLITE_CHANGESET_DATA` or `SQLITE_CHANGESET_CONFLICT`.Conflicting changes replace existing values. Note that this value can only be returned when the type of conflict is either SQLITE_CHANGESET_DATA or SQLITE_CHANGESET_CONFLICT.
SQLITE_CHANGESET_ABORT
[Changesets and Patchsets]: https://www.sqlite.org/sessionintro.html#changesets_and_patchsets +[`database.applyChangeset()`]: #databaseapplychangesetchangeset-options +[Constants Passed To The Conflict Handler]: https://www.sqlite.org/session/c_changeset_conflict.html +[Constants Returned From The Conflict Handler]: https://www.sqlite.org/session/c_changeset_abort.html [SQL injection]: https://en.wikipedia.org/wiki/SQL_injection [`ATTACH DATABASE`]: https://www.sqlite.org/lang_attach.html [`PRAGMA foreign_keys`]: https://www.sqlite.org/pragma.html#pragma_foreign_keys From c674327ade01df4c548f0b2c33b12c992739f991 Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Sat, 28 Dec 2024 19:29:22 +0100 Subject: [PATCH 07/12] sqlite: improve error handling --- src/env_properties.h | 5 ++++- src/node_sqlite.cc | 25 +++++++++++++------------ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/env_properties.h b/src/env_properties.h index 592e95b0584a87..d9e18cbc4515ac 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -144,10 +144,13 @@ V(entry_type_string, "entryType") \ V(env_pairs_string, "envPairs") \ V(env_var_settings_string, "envVarSettings") \ + V(err_sqlite_error_string, "ERR_SQLITE_ERROR") \ + V(errcode_string, "errcode") \ V(errno_string, "errno") \ V(error_string, "error") \ - V(events, "events") \ + V(errstr_string, "errstr") \ V(events_waiting, "eventsWaiting") \ + V(events, "events") \ V(exchange_string, "exchange") \ V(expire_string, "expire") \ V(exponent_string, "exponent") \ diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 20c5bfda6d97c0..c6606d57f6fe26 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -67,13 +67,14 @@ inline MaybeLocal CreateSQLiteError(Isolate* isolate, const char* message) { Local js_msg; Local e; + Environment* env = Environment::GetCurrent(isolate); if (!String::NewFromUtf8(isolate, message).ToLocal(&js_msg) || !Exception::Error(js_msg) ->ToObject(isolate->GetCurrentContext()) .ToLocal(&e) || e->Set(isolate->GetCurrentContext(), - OneByteString(isolate, "code"), - OneByteString(isolate, "ERR_SQLITE_ERROR")) + env->error_string(), + env->err_sqlite_error_string()) .IsNothing()) { return MaybeLocal(); } @@ -86,14 +87,15 @@ inline MaybeLocal CreateSQLiteError(Isolate* isolate, sqlite3* db) { const char* errmsg = sqlite3_errmsg(db); Local js_errmsg; Local e; + Environment* env = Environment::GetCurrent(isolate); if (!String::NewFromUtf8(isolate, errstr).ToLocal(&js_errmsg) || !CreateSQLiteError(isolate, errmsg).ToLocal(&e) || e->Set(isolate->GetCurrentContext(), - OneByteString(isolate, "errcode"), + env->errcode_string(), Integer::New(isolate, errcode)) .IsNothing() || e->Set(isolate->GetCurrentContext(), - OneByteString(isolate, "errstr"), + env->errstr_string(), js_errmsg) .IsNothing()) { return MaybeLocal(); @@ -117,15 +119,14 @@ inline void THROW_ERR_SQLITE_ERROR(Isolate* isolate, const char* message) { inline void THROW_ERR_SQLITE_ERROR(Isolate* isolate, int errcode) { const char* errstr = sqlite3_errstr(errcode); - Local e; - if (CreateSQLiteError(isolate, errstr).ToLocal(&e)) { - e->Set(isolate->GetCurrentContext(), - OneByteString(isolate, "errcode"), - Integer::New(isolate, errcode)) - .IsNothing(); - isolate->ThrowException(e); - } + Environment* env = Environment::GetCurrent(isolate); + auto error = CreateSQLiteError(isolate, errstr).ToLocalChecked(); + error->Set(isolate->GetCurrentContext(), + env->errcode_string(), + Integer::New(isolate, errcode)) + .ToChecked(); + isolate->ThrowException(error); } class UserDefinedFunction { From 7ccdfd19840fbccd7c0ddf110bdc9ff3a8aa7025 Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Sat, 28 Dec 2024 19:36:07 +0100 Subject: [PATCH 08/12] sqlite: add test returning Promise from conflict handler --- doc/api/sqlite.md | 3 ++- test/parallel/test-sqlite-session.js | 17 ++++++++++------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md index f1cbac711cf0cb..b473d8498e1be3 100644 --- a/doc/api/sqlite.md +++ b/doc/api/sqlite.md @@ -251,7 +251,8 @@ added: `SQLITE_CHANGESET_DATA` or `SQLITE_CHANGESET_CONFLICT` conflicts). * `SQLITE_CHANGESET_ABORT`: Abort on conflict and roll back the database. - When an error is thrown in the conflict handler, applying the changeset is aborted and the database is rolled back. + When an error is thrown in the conflict handler or when any other value is returned from the handler, + applying the changeset is aborted and the database is rolled back. **Default**: A function that returns `SQLITE_CHANGESET_ABORT`. * Returns: {boolean} Whether the changeset was applied succesfully without being aborted. diff --git a/test/parallel/test-sqlite-session.js b/test/parallel/test-sqlite-session.js index eff33c029a7498..9de6712160aa11 100644 --- a/test/parallel/test-sqlite-session.js +++ b/test/parallel/test-sqlite-session.js @@ -324,21 +324,24 @@ suite('conflict resolution', () => { }); test('conflict resolution handler returns invalid value', (t) => { - const invalidValues = [-1, {}, null]; - - for (const invalidValue of invalidValues) { + const invalidHandlers = [ + () => -1, + () => ({}), + () => null, + async () => constants.SQLITE_CHANGESET_ABORT + ]; + + for (const invalidHandler of invalidHandlers) { const { database2, changeset } = prepareConflict(); t.assert.throws(() => { database2.applyChangeset(changeset, { - onConflict: () => { - return invalidValue; - } + onConflict: invalidHandler }); }, { name: 'Error', message: 'bad parameter or other API misuse', errcode: 21 - }, `Did not throw expected exception when returning '${invalidValue}' from conflict handler`); + }, `Did not throw expected exception when returning '${invalidHandler}' from conflict handler`); } }); From df86bb171438777b6be171adaa9ea4e5a83fe82c Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Sat, 28 Dec 2024 19:41:26 +0100 Subject: [PATCH 09/12] sqlite: fix error code key and add test --- src/node_sqlite.cc | 2 +- test/parallel/test-sqlite-session.js | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index c6606d57f6fe26..0e1796488c49de 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -73,7 +73,7 @@ inline MaybeLocal CreateSQLiteError(Isolate* isolate, ->ToObject(isolate->GetCurrentContext()) .ToLocal(&e) || e->Set(isolate->GetCurrentContext(), - env->error_string(), + env->code_string(), env->err_sqlite_error_string()) .IsNothing()) { return MaybeLocal(); diff --git a/test/parallel/test-sqlite-session.js b/test/parallel/test-sqlite-session.js index 9de6712160aa11..17b9271f90eaba 100644 --- a/test/parallel/test-sqlite-session.js +++ b/test/parallel/test-sqlite-session.js @@ -328,7 +328,7 @@ suite('conflict resolution', () => { () => -1, () => ({}), () => null, - async () => constants.SQLITE_CHANGESET_ABORT + async () => constants.SQLITE_CHANGESET_ABORT, ]; for (const invalidHandler of invalidHandlers) { @@ -340,7 +340,8 @@ suite('conflict resolution', () => { }, { name: 'Error', message: 'bad parameter or other API misuse', - errcode: 21 + errcode: 21, + code: 'ERR_SQLITE_ERROR' }, `Did not throw expected exception when returning '${invalidHandler}' from conflict handler`); } }); From eb1936b3227651cd85ba60be2abf107dfa08b5cb Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Sat, 28 Dec 2024 19:44:14 +0100 Subject: [PATCH 10/12] sqlite: formatting --- src/node_sqlite.cc | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 0e1796488c49de..abd85a98c5aebb 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -94,9 +94,7 @@ inline MaybeLocal CreateSQLiteError(Isolate* isolate, sqlite3* db) { env->errcode_string(), Integer::New(isolate, errcode)) .IsNothing() || - e->Set(isolate->GetCurrentContext(), - env->errstr_string(), - js_errmsg) + e->Set(isolate->GetCurrentContext(), env->errstr_string(), js_errmsg) .IsNothing()) { return MaybeLocal(); } @@ -122,9 +120,10 @@ inline void THROW_ERR_SQLITE_ERROR(Isolate* isolate, int errcode) { Environment* env = Environment::GetCurrent(isolate); auto error = CreateSQLiteError(isolate, errstr).ToLocalChecked(); - error->Set(isolate->GetCurrentContext(), - env->errcode_string(), - Integer::New(isolate, errcode)) + error + ->Set(isolate->GetCurrentContext(), + env->errcode_string(), + Integer::New(isolate, errcode)) .ToChecked(); isolate->ThrowException(error); } From 9b4255c58e954837e71a6d2443d1a5f323382d84 Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Sat, 28 Dec 2024 19:47:02 +0100 Subject: [PATCH 11/12] sqlite: fix error message test --- test/parallel/test-sqlite-session.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-sqlite-session.js b/test/parallel/test-sqlite-session.js index 17b9271f90eaba..5cba37e337e835 100644 --- a/test/parallel/test-sqlite-session.js +++ b/test/parallel/test-sqlite-session.js @@ -342,7 +342,7 @@ suite('conflict resolution', () => { message: 'bad parameter or other API misuse', errcode: 21, code: 'ERR_SQLITE_ERROR' - }, `Did not throw expected exception when returning '${invalidHandler}' from conflict handler`); + }, `Did not throw expected exception when using invalid onConflict handler: ${invalidHandler}`); } }); From 5e39d2201e60ae3d1584793c53e88a80c3af1b14 Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Sat, 28 Dec 2024 19:56:57 +0100 Subject: [PATCH 12/12] doc: sort references --- doc/api/sqlite.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md index b473d8498e1be3..5b1831f40f2076 100644 --- a/doc/api/sqlite.md +++ b/doc/api/sqlite.md @@ -517,7 +517,7 @@ The following constants are exported by the `sqlite.constants` object. One of the following constants is available as an argument to the `onConflict` conflict resolution handler passed to [`database.applyChangeset()`][]. See also -[Constants Passed To The Conflict Handler]() in the SQLite documentation. +[Constants Passed To The Conflict Handler][] in the SQLite documentation. @@ -570,7 +570,6 @@ resolution handler passed to [`database.applyChangeset()`][]. See also
[Changesets and Patchsets]: https://www.sqlite.org/sessionintro.html#changesets_and_patchsets -[`database.applyChangeset()`]: #databaseapplychangesetchangeset-options [Constants Passed To The Conflict Handler]: https://www.sqlite.org/session/c_changeset_conflict.html [Constants Returned From The Conflict Handler]: https://www.sqlite.org/session/c_changeset_abort.html [SQL injection]: https://en.wikipedia.org/wiki/SQL_injection @@ -578,6 +577,7 @@ resolution handler passed to [`database.applyChangeset()`][]. See also [`PRAGMA foreign_keys`]: https://www.sqlite.org/pragma.html#pragma_foreign_keys [`SQLITE_DETERMINISTIC`]: https://www.sqlite.org/c3ref/c_deterministic.html [`SQLITE_DIRECTONLY`]: https://www.sqlite.org/c3ref/c_deterministic.html +[`database.applyChangeset()`]: #databaseapplychangesetchangeset-options [`sqlite3_changes64()`]: https://www.sqlite.org/c3ref/changes.html [`sqlite3_close_v2()`]: https://www.sqlite.org/c3ref/close.html [`sqlite3_create_function_v2()`]: https://www.sqlite.org/c3ref/create_function.html