diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 1272deb42f1..8656f4b6532 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -88,6 +88,17 @@ inline MaybeLocal Utf8StringMaybeOneByte(Isolate* isolate, } \ } while (0) +#define RESET_OR_THROW(isolate, db, stmt, ret) \ + CHECK_ERROR_OR_THROW((isolate), (db), sqlite3_reset((stmt)), SQLITE_OK, (ret)) + +// Surface deferred SQLite errors that sqlite3_reset() returns from the prior +// sqlite3_step(). Disables the safety-net reset guard via |needs_reset|. +#define RESET_AND_CHECK(isolate, db, stmt, needs_reset, ret) \ + do { \ + (needs_reset) = false; \ + RESET_OR_THROW((isolate), (db), (stmt), (ret)); \ + } while (0) + #define THROW_AND_RETURN_ON_BAD_STATE(env, condition, msg) \ do { \ if ((condition)) { \ @@ -2989,9 +3000,20 @@ MaybeLocal StatementExecutionHelper::Run(Environment* env, bool use_big_ints) { Isolate* isolate = env->isolate(); EscapableHandleScope scope(isolate); - sqlite3_step(stmt); - int r = sqlite3_reset(stmt); - CHECK_ERROR_OR_THROW(isolate, db, r, SQLITE_OK, MaybeLocal()); + bool needs_reset = true; + auto reset = OnScopeLeave([&]() { + if (needs_reset) sqlite3_reset(stmt); + }); + + int step_r = sqlite3_step(stmt); + // SQLITE_ROW is accepted here (and discarded) so that run() can still be + // used on RETURNING/SELECT statements, matching prior behavior of + // ignoring the step result entirely. + if (step_r != SQLITE_DONE && step_r != SQLITE_ROW) { + THROW_ERR_SQLITE_ERROR(isolate, db); + return MaybeLocal(); + } + RESET_AND_CHECK(isolate, db, stmt, needs_reset, MaybeLocal()); sqlite3_int64 last_insert_rowid = sqlite3_last_insert_rowid(db->Connection()); sqlite3_int64 changes = sqlite3_changes64(db->Connection()); @@ -3065,10 +3087,16 @@ MaybeLocal StatementExecutionHelper::Get(Environment* env, bool use_big_ints) { Isolate* isolate = env->isolate(); EscapableHandleScope scope(isolate); - auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt); }); + bool needs_reset = true; + auto reset = OnScopeLeave([&]() { + if (needs_reset) sqlite3_reset(stmt); + }); int r = sqlite3_step(stmt); - if (r == SQLITE_DONE) return scope.Escape(Undefined(isolate)); + if (r == SQLITE_DONE) { + RESET_AND_CHECK(isolate, db, stmt, needs_reset, MaybeLocal()); + return scope.Escape(Undefined(isolate)); + } if (r != SQLITE_ROW) { THROW_ERR_SQLITE_ERROR(isolate, db); return MaybeLocal(); @@ -3076,7 +3104,8 @@ MaybeLocal StatementExecutionHelper::Get(Environment* env, int num_cols = sqlite3_column_count(stmt); if (num_cols == 0) { - return Undefined(isolate); + RESET_AND_CHECK(isolate, db, stmt, needs_reset, MaybeLocal()); + return scope.Escape(Undefined(isolate)); } LocalVector row_values(isolate); @@ -3085,9 +3114,9 @@ MaybeLocal StatementExecutionHelper::Get(Environment* env, return MaybeLocal(); } + Local result; if (return_arrays) { - return scope.Escape( - Array::New(isolate, row_values.data(), row_values.size())); + result = Array::New(isolate, row_values.data(), row_values.size()); } else { LocalVector keys(isolate); keys.reserve(num_cols); @@ -3100,9 +3129,12 @@ MaybeLocal StatementExecutionHelper::Get(Environment* env, } DCHECK_EQ(keys.size(), row_values.size()); - return scope.Escape(Object::New( - isolate, Null(isolate), keys.data(), row_values.data(), num_cols)); + result = Object::New( + isolate, Null(isolate), keys.data(), row_values.data(), num_cols); } + + RESET_AND_CHECK(isolate, db, stmt, needs_reset, MaybeLocal()); + return scope.Escape(result); } void StatementSync::All(const FunctionCallbackInfo& args) { @@ -3119,8 +3151,10 @@ void StatementSync::All(const FunctionCallbackInfo& args) { return; } - auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); - + bool needs_reset = true; + auto reset = OnScopeLeave([&]() { + if (needs_reset) sqlite3_reset(stmt->statement_); + }); Local result; if (StatementExecutionHelper::All(env, stmt->db_.get(), @@ -3128,6 +3162,8 @@ void StatementSync::All(const FunctionCallbackInfo& args) { stmt->return_arrays_, stmt->use_big_ints_) .ToLocal(&result)) { + RESET_AND_CHECK( + isolate, stmt->db_.get(), stmt->statement_, needs_reset, void()); args.GetReturnValue().Set(result); } } @@ -3566,7 +3602,10 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { } } - auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); + bool needs_reset = true; + auto reset = OnScopeLeave([&]() { + if (needs_reset) sqlite3_reset(stmt->statement_); + }); Local result; if (StatementExecutionHelper::All(env, stmt->db_.get(), @@ -3574,6 +3613,8 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { stmt->return_arrays_, stmt->use_big_ints_) .ToLocal(&result)) { + RESET_AND_CHECK( + isolate, stmt->db_.get(), stmt->statement_, needs_reset, void()); args.GetReturnValue().Set(result); } } @@ -3800,7 +3841,10 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo& args) { if (r != SQLITE_ROW) { CHECK_ERROR_OR_THROW( env->isolate(), iter->stmt_->db_.get(), r, SQLITE_DONE, void()); - sqlite3_reset(iter->stmt_->statement_); + RESET_OR_THROW(env->isolate(), + iter->stmt_->db_.get(), + iter->stmt_->statement_, + void()); iter->done_ = true; MaybeLocal values[] = {Boolean::New(isolate, true), Null(isolate)}; Local result; @@ -3853,6 +3897,10 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo& args) { env, iter->stmt_->IsFinalized(), "statement has been finalized"); Isolate* isolate = env->isolate(); + // Unlike Next(), the reset result is intentionally ignored here: Return() + // is invoked by the language during abrupt completion (e.g. a `throw` + // inside a `for...of` body), and throwing on a deferred SQLite error + // would discard the caller's already-pending exception. sqlite3_reset(iter->stmt_->statement_); iter->done_ = true; diff --git a/test/parallel/test-sqlite-statement-sync.js b/test/parallel/test-sqlite-statement-sync.js index cf0e4daa45c..a55e19fd14f 100644 --- a/test/parallel/test-sqlite-statement-sync.js +++ b/test/parallel/test-sqlite-statement-sync.js @@ -79,6 +79,28 @@ suite('StatementSync.prototype.get()', () => { message: /statement has been finalized/, }); }); + + test('surfaces a deferred SQLite error from reset() even though a row was already built', (t) => { + using db = new DatabaseSync(':memory:'); + db.exec(` + PRAGMA foreign_keys = ON; + PRAGMA defer_foreign_keys = ON; + CREATE TABLE parent(id INTEGER PRIMARY KEY); + CREATE TABLE child(id INTEGER PRIMARY KEY, parent_id INTEGER REFERENCES parent(id)); + `); + // The FK check is deferred until the implicit transaction commits, which + // happens inside reset() here because RETURNING leaves the statement's + // VDBE running after the row is produced. + const stmt = db.prepare( + 'INSERT INTO child (parent_id) VALUES (999) RETURNING id' + ); + t.assert.throws(() => { + stmt.get(); + }, { + code: 'ERR_SQLITE_ERROR', + message: /FOREIGN KEY constraint failed/, + }); + }); }); suite('StatementSync.prototype.all()', () => { @@ -144,6 +166,25 @@ suite('StatementSync.prototype.all()', () => { message: /statement has been finalized/, }); }); + + test('surfaces a deferred SQLite error from reset() even though the array was already built', (t) => { + using db = new DatabaseSync(':memory:'); + db.exec(` + PRAGMA foreign_keys = ON; + PRAGMA defer_foreign_keys = ON; + CREATE TABLE parent(id INTEGER PRIMARY KEY); + CREATE TABLE child(id INTEGER PRIMARY KEY, parent_id INTEGER REFERENCES parent(id)); + `); + const stmt = db.prepare( + 'INSERT INTO child (parent_id) VALUES (999) RETURNING id' + ); + t.assert.throws(() => { + stmt.all(); + }, { + code: 'ERR_SQLITE_ERROR', + message: /FOREIGN KEY constraint failed/, + }); + }); }); suite('StatementSync.prototype.iterate()', () => { @@ -322,6 +363,41 @@ suite('StatementSync.prototype.iterate()', () => { message: /statement has been finalized/, }); }); + + test('does not replay results after the iterator is naturally exhausted', (t) => { + using db = new DatabaseSync(':memory:'); + db.exec(` + CREATE TABLE test(key TEXT); + INSERT INTO test (key) VALUES ('key1'); + `); + const it = db.prepare('SELECT * FROM test').iterate(); + t.assert.deepStrictEqual(it.next(), { + __proto__: null, done: false, value: { __proto__: null, key: 'key1' }, + }); + t.assert.deepStrictEqual( + it.next(), { __proto__: null, done: true, value: null }); + // Calling next() again on an exhausted iterator must keep reporting + // done, not silently reset the statement and replay from row 1. + t.assert.deepStrictEqual( + it.next(), { __proto__: null, done: true, value: null }); + }); + + test('propagates a pending exception when the loop body throws mid-iteration', (t) => { + using db = new DatabaseSync(':memory:'); + db.exec(` + CREATE TABLE test(key TEXT); + INSERT INTO test (key) VALUES ('key1'); + INSERT INTO test (key) VALUES ('key2'); + `); + const stmt = db.prepare('SELECT * FROM test'); + const userError = new Error('boom'); + t.assert.throws(() => { + // eslint-disable-next-line no-unused-vars + for (const row of stmt.iterate()) { + throw userError; + } + }, (err) => err === userError); + }); }); suite('StatementSync.prototype.run()', () => {