FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

sqlite: check sqlite3_step() and sqlite3_reset() results · nodejs/node@3ab7e72 · GitHub

/ node Public

Commit 3ab7e72

Browse files
semimikoh
authored and
aduh95
committed
sqlite: check sqlite3_step() and sqlite3_reset() results
Signed-off-by: semimikoh <ejffjeosms@gmail.com> PR-URL: #63319 Fixes: #63311 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
1 parent f2cc681 commit 3ab7e72

2 files changed

Lines changed: 139 additions & 14 deletions

File tree

‎src/node_sqlite.cc‎

Lines changed: 63 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,17 @@ inline MaybeLocal<String> Utf8StringMaybeOneByte(Isolate* isolate,
8888
} \
8989
} while (0)
9090

91+
#define RESET_OR_THROW(isolate, db, stmt, ret) \
92+
CHECK_ERROR_OR_THROW((isolate), (db), sqlite3_reset((stmt)), SQLITE_OK, (ret))
93+
94+
// Surface deferred SQLite errors that sqlite3_reset() returns from the prior
95+
// sqlite3_step(). Disables the safety-net reset guard via |needs_reset|.
96+
#define RESET_AND_CHECK(isolate, db, stmt, needs_reset, ret) \
97+
do { \
98+
(needs_reset) = false; \
99+
RESET_OR_THROW((isolate), (db), (stmt), (ret)); \
100+
} while (0)
101+
91102
#define THROW_AND_RETURN_ON_BAD_STATE(env, condition, msg) \
92103
do { \
93104
if ((condition)) { \
@@ -3020,9 +3031,20 @@ MaybeLocal<Object> StatementExecutionHelper::Run(Environment* env,
30203031
bool use_big_ints) {
30213032
Isolate* isolate = env->isolate();
30223033
EscapableHandleScope scope(isolate);
3023-
sqlite3_step(stmt);
3024-
int r = sqlite3_reset(stmt);
3025-
CHECK_ERROR_OR_THROW(isolate, db, r, SQLITE_OK, MaybeLocal<Object>());
3034+
bool needs_reset = true;
3035+
auto reset = OnScopeLeave([&]() {
3036+
if (needs_reset) sqlite3_reset(stmt);
3037+
});
3038+
3039+
int step_r = sqlite3_step(stmt);
3040+
// SQLITE_ROW is accepted here (and discarded) so that run() can still be
3041+
// used on RETURNING/SELECT statements, matching prior behavior of
3042+
// ignoring the step result entirely.
3043+
if (step_r != SQLITE_DONE && step_r != SQLITE_ROW) {
3044+
THROW_ERR_SQLITE_ERROR(isolate, db);
3045+
return MaybeLocal<Object>();
3046+
}
3047+
RESET_AND_CHECK(isolate, db, stmt, needs_reset, MaybeLocal<Object>());
30263048

30273049
sqlite3_int64 last_insert_rowid = sqlite3_last_insert_rowid(db->Connection());
30283050
sqlite3_int64 changes = sqlite3_changes64(db->Connection());
@@ -3096,18 +3118,25 @@ MaybeLocal<Value> StatementExecutionHelper::Get(Environment* env,
30963118
bool use_big_ints) {
30973119
Isolate* isolate = env->isolate();
30983120
EscapableHandleScope scope(isolate);
3099-
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt); });
3121+
bool needs_reset = true;
3122+
auto reset = OnScopeLeave([&]() {
3123+
if (needs_reset) sqlite3_reset(stmt);
3124+
});
31003125

31013126
int r = sqlite3_step(stmt);
3102-
if (r == SQLITE_DONE) return scope.Escape(Undefined(isolate));
3127+
if (r == SQLITE_DONE) {
3128+
RESET_AND_CHECK(isolate, db, stmt, needs_reset, MaybeLocal<Value>());
3129+
return scope.Escape(Undefined(isolate));
3130+
}
31033131
if (r != SQLITE_ROW) {
31043132
THROW_ERR_SQLITE_ERROR(isolate, db);
31053133
return MaybeLocal<Value>();
31063134
}
31073135

31083136
int num_cols = sqlite3_column_count(stmt);
31093137
if (num_cols == 0) {
3110-
return Undefined(isolate);
3138+
RESET_AND_CHECK(isolate, db, stmt, needs_reset, MaybeLocal<Value>());
3139+
return scope.Escape(Undefined(isolate));
31113140
}
31123141

31133142
LocalVector<Value> row_values(isolate);
@@ -3116,9 +3145,9 @@ MaybeLocal<Value> StatementExecutionHelper::Get(Environment* env,
31163145
return MaybeLocal<Value>();
31173146
}
31183147

3148+
Local<Value> result;
31193149
if (return_arrays) {
3120-
return scope.Escape(
3121-
Array::New(isolate, row_values.data(), row_values.size()));
3150+
result = Array::New(isolate, row_values.data(), row_values.size());
31223151
} else {
31233152
LocalVector<Name> keys(isolate);
31243153
keys.reserve(num_cols);
@@ -3131,9 +3160,12 @@ MaybeLocal<Value> StatementExecutionHelper::Get(Environment* env,
31313160
}
31323161

31333162
DCHECK_EQ(keys.size(), row_values.size());
3134-
return scope.Escape(Object::New(
3135-
isolate, Null(isolate), keys.data(), row_values.data(), num_cols));
3163+
result = Object::New(
3164+
isolate, Null(isolate), keys.data(), row_values.data(), num_cols);
31363165
}
3166+
3167+
RESET_AND_CHECK(isolate, db, stmt, needs_reset, MaybeLocal<Value>());
3168+
return scope.Escape(result);
31373169
}
31383170

31393171
void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
@@ -3150,15 +3182,19 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
31503182
return;
31513183
}
31523184

3153-
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
3154-
3185+
bool needs_reset = true;
3186+
auto reset = OnScopeLeave([&]() {
3187+
if (needs_reset) sqlite3_reset(stmt->statement_);
3188+
});
31553189
Local<Value> result;
31563190
if (StatementExecutionHelper::All(env,
31573191
stmt->db_.get(),
31583192
stmt->statement_,
31593193
stmt->return_arrays_,
31603194
stmt->use_big_ints_)
31613195
.ToLocal(&result)) {
3196+
RESET_AND_CHECK(
3197+
isolate, stmt->db_.get(), stmt->statement_, needs_reset, void());
31623198
args.GetReturnValue().Set(result);
31633199
}
31643200
}
@@ -3592,14 +3628,20 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
35923628
return;
35933629
}
35943630

3595-
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
3631+
Isolate* isolate = env->isolate();
3632+
bool needs_reset = true;
3633+
auto reset = OnScopeLeave([&]() {
3634+
if (needs_reset) sqlite3_reset(stmt->statement_);
3635+
});
35963636
Local<Value> result;
35973637
if (StatementExecutionHelper::All(env,
35983638
stmt->db_.get(),
35993639
stmt->statement_,
36003640
stmt->return_arrays_,
36013641
stmt->use_big_ints_)
36023642
.ToLocal(&result)) {
3643+
RESET_AND_CHECK(
3644+
isolate, stmt->db_.get(), stmt->statement_, needs_reset, void());
36033645
args.GetReturnValue().Set(result);
36043646
}
36053647
}
@@ -3833,8 +3875,11 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
38333875
if (r != SQLITE_ROW) {
38343876
CHECK_ERROR_OR_THROW(
38353877
env->isolate(), iter->stmt_->db_.get(), r, SQLITE_DONE, void());
3836-
sqlite3_reset(iter->stmt_->statement_);
38373878
iter->done_ = true;
3879+
RESET_OR_THROW(env->isolate(),
3880+
iter->stmt_->db_.get(),
3881+
iter->stmt_->statement_,
3882+
void());
38383883
MaybeLocal<Value> values[] = {Boolean::New(isolate, true), Null(isolate)};
38393884
Local<Object> result;
38403885
if (NewDictionaryInstanceNullProto(env->context(), iter_template, values)
@@ -3886,6 +3931,10 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo<Value>& args) {
38863931
env, iter->stmt_->IsFinalized(), "statement has been finalized");
38873932
Isolate* isolate = env->isolate();
38883933

3934+
// Unlike Next(), the reset result is intentionally ignored here: Return()
3935+
// is invoked by the language during abrupt completion (e.g. a `throw`
3936+
// inside a `for...of` body), and throwing on a deferred SQLite error
3937+
// would discard the caller's already-pending exception.
38893938
sqlite3_reset(iter->stmt_->statement_);
38903939
iter->done_ = true;
38913940

‎test/parallel/test-sqlite-statement-sync.js‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,28 @@ suite('StatementSync.prototype.get()', () => {
7979
message: /statement has been finalized/,
8080
});
8181
});
82+
83+
test('surfaces a deferred SQLite error from reset() even though a row was already built', (t) => {
84+
using db = new DatabaseSync(':memory:');
85+
db.exec(`
86+
PRAGMA foreign_keys = ON;
87+
PRAGMA defer_foreign_keys = ON;
88+
CREATE TABLE parent(id INTEGER PRIMARY KEY);
89+
CREATE TABLE child(id INTEGER PRIMARY KEY, parent_id INTEGER REFERENCES parent(id));
90+
`);
91+
// The FK check is deferred until the implicit transaction commits, which
92+
// happens inside reset() here because RETURNING leaves the statement's
93+
// VDBE running after the row is produced.
94+
const stmt = db.prepare(
95+
'INSERT INTO child (parent_id) VALUES (999) RETURNING id'
96+
);
97+
t.assert.throws(() => {
98+
stmt.get();
99+
}, {
100+
code: 'ERR_SQLITE_ERROR',
101+
message: /FOREIGN KEY constraint failed/,
102+
});
103+
});
82104
});
83105

84106
suite('StatementSync.prototype.all()', () => {
@@ -144,6 +166,25 @@ suite('StatementSync.prototype.all()', () => {
144166
message: /statement has been finalized/,
145167
});
146168
});
169+
170+
test('surfaces a deferred SQLite error from reset() even though the array was already built', (t) => {
171+
using db = new DatabaseSync(':memory:');
172+
db.exec(`
173+
PRAGMA foreign_keys = ON;
174+
PRAGMA defer_foreign_keys = ON;
175+
CREATE TABLE parent(id INTEGER PRIMARY KEY);
176+
CREATE TABLE child(id INTEGER PRIMARY KEY, parent_id INTEGER REFERENCES parent(id));
177+
`);
178+
const stmt = db.prepare(
179+
'INSERT INTO child (parent_id) VALUES (999) RETURNING id'
180+
);
181+
t.assert.throws(() => {
182+
stmt.all();
183+
}, {
184+
code: 'ERR_SQLITE_ERROR',
185+
message: /FOREIGN KEY constraint failed/,
186+
});
187+
});
147188
});
148189

149190
suite('StatementSync.prototype.iterate()', () => {
@@ -322,6 +363,41 @@ suite('StatementSync.prototype.iterate()', () => {
322363
message: /statement has been finalized/,
323364
});
324365
});
366+
367+
test('does not replay results after the iterator is naturally exhausted', (t) => {
368+
using db = new DatabaseSync(':memory:');
369+
db.exec(`
370+
CREATE TABLE test(key TEXT);
371+
INSERT INTO test (key) VALUES ('key1');
372+
`);
373+
const it = db.prepare('SELECT * FROM test').iterate();
374+
t.assert.deepStrictEqual(it.next(), {
375+
__proto__: null, done: false, value: { __proto__: null, key: 'key1' },
376+
});
377+
t.assert.deepStrictEqual(
378+
it.next(), { __proto__: null, done: true, value: null });
379+
// Calling next() again on an exhausted iterator must keep reporting
380+
// done, not silently reset the statement and replay from row 1.
381+
t.assert.deepStrictEqual(
382+
it.next(), { __proto__: null, done: true, value: null });
383+
});
384+
385+
test('propagates a pending exception when the loop body throws mid-iteration', (t) => {
386+
using db = new DatabaseSync(':memory:');
387+
db.exec(`
388+
CREATE TABLE test(key TEXT);
389+
INSERT INTO test (key) VALUES ('key1');
390+
INSERT INTO test (key) VALUES ('key2');
391+
`);
392+
const stmt = db.prepare('SELECT * FROM test');
393+
const userError = new Error('boom');
394+
t.assert.throws(() => {
395+
// eslint-disable-next-line no-unused-vars
396+
for (const row of stmt.iterate()) {
397+
throw userError;
398+
}
399+
}, (err) => err === userError);
400+
});
325401
});
326402

327403
suite('StatementSync.prototype.run()', () => {

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL