diff --git a/doc/api/assert.md b/doc/api/assert.md index 1a99709767ef..c33f4ba82336 100644 --- a/doc/api/assert.md +++ b/doc/api/assert.md @@ -317,7 +317,7 @@ const assert2 = new Assert({ skipPrototype: true }); assert2.deepStrictEqual(foo, bar); // OK ``` -When destructured, methods lose access to the instance's `this` context and revert to default assertion behavior +When destructured, methods lose access to the instance's `this` context and revert to the default assertion behavior (diff: 'simple', non-strict mode). To maintain custom options when using destructured methods, avoid destructuring and call methods directly on the instance. @@ -423,8 +423,8 @@ are also recursively evaluated by the following rules. ### Comparison details * Primitive values are compared with the [`==` operator][], - with the exception of {NaN}. It is treated as being identical in case - both sides are {NaN}. + except for {NaN}, which is treated as identical when both + sides are {NaN}. * [Type tags][Object.prototype.toString()] of objects should be the same. * Only [enumerable "own" properties][] are considered. * Object constructors are compared when available. @@ -938,7 +938,7 @@ error messages as expressive as possible. If specified, `error` can be a [`Class`][], {RegExp} or a validation function. See [`assert.throws()`][] for more details. -Besides the async nature to await the completion behaves identically to +Aside from asynchronously awaiting completion, it behaves identically to [`assert.doesNotThrow()`][]. ```mjs diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md index ae194ff2acaf..49c1260708d3 100644 --- a/doc/api/sqlite.md +++ b/doc/api/sqlite.md @@ -670,6 +670,10 @@ console.log(query.get()); * `sql` {string} A SQL string to compile to a prepared statement. diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 0898e450a503..97384bdf449d 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -1581,6 +1581,16 @@ void DatabaseSync::Prepare(const FunctionCallbackInfo& args) { int r = sqlite3_prepare_v2(db->connection_, *sql, -1, &s, nullptr); CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void()); + + // sqlite3_prepare_v2() reports success without producing a statement when + // the input holds no SQL, such as a comment. Such a statement can never be + // stepped, and tracking it would leave a dangling pointer in statements_ + // because its destructor treats a null statement as already finalized. + if (s == nullptr) { + THROW_ERR_INVALID_ARG_VALUE(env, "The SQL query contains no statements."); + return; + } + BaseObjectPtr stmt = StatementSync::Create(env, BaseObjectPtr(db), s); db->statements_.insert(stmt.get()); @@ -3659,6 +3669,13 @@ BaseObjectPtr SQLTagStore::PrepareStatement( return BaseObjectPtr(); } + // As in DatabaseSync::Prepare(), reject input that holds no SQL rather + // than caching a statement that can never be bound or stepped. + if (s == nullptr) { + THROW_ERR_INVALID_ARG_VALUE(env, "The SQL query contains no statements."); + return BaseObjectPtr(); + } + BaseObjectPtr stmt_obj = StatementSync::Create( env, BaseObjectPtr(session->database_), s); diff --git a/test/parallel/test-sqlite-database-sync.js b/test/parallel/test-sqlite-database-sync.js index af7677a3cfc0..08a636c9cbdc 100644 --- a/test/parallel/test-sqlite-database-sync.js +++ b/test/parallel/test-sqlite-database-sync.js @@ -397,6 +397,32 @@ suite('DatabaseSync.prototype.prepare()', () => { message: /The "sql" argument must be a string/, }); }); + + test('throws if sql contains no statements', (t) => { + using db = new DatabaseSync(nextDb()); + + for (const sql of ['', ' ', ';', '-- comment', '/* comment */']) { + t.assert.throws(() => { + db.prepare(sql); + }, { + code: 'ERR_INVALID_ARG_VALUE', + message: /contains no statements/, + }); + } + }); + + test('prepares statements that contain comments', (t) => { + using db = new DatabaseSync(nextDb()); + const queries = [ + '-- lead\nSELECT 1 AS v', + 'SELECT 1 AS v -- trail', + 'SELECT /* mid */ 1 AS v', + ]; + + for (const sql of queries) { + t.assert.strictEqual(db.prepare(sql).get().v, 1); + } + }); }); suite('DatabaseSync.prototype.exec()', () => { diff --git a/test/parallel/test-sqlite-template-tag.js b/test/parallel/test-sqlite-template-tag.js index 445231bef0bd..20376e199d1b 100644 --- a/test/parallel/test-sqlite-template-tag.js +++ b/test/parallel/test-sqlite-template-tag.js @@ -317,6 +317,29 @@ test('sql error messages are descriptive', () => { }); }); +test('rejects SQL that contains no statements', () => { + const expectedError = { + code: 'ERR_INVALID_ARG_VALUE', + message: /contains no statements/, + }; + + for (const method of ['run', 'get', 'all', 'iterate']) { + assert.throws(() => { + // eslint-disable-next-line no-unused-expressions + sql[method]`-- comment`; + }, expectedError); + + assert.throws(() => { + // eslint-disable-next-line no-unused-expressions + sql[method]``; + }, expectedError); + } + + // A rejected statement must not be cached, so a later valid query with the + // same tag store still works. + assert.strictEqual(sql.run`INSERT INTO foo (text) VALUES (${'bob'})`.changes, 1); +}); + test('a tag store keeps the database alive by itself', () => { const sql = new DatabaseSync(':memory:').createTagStore();