From 7fea0108d57224c9aafbdf7c72d31f4b9da95c43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Sun, 25 Aug 2024 12:43:17 +0200 Subject: [PATCH] sqlite: return results with null prototype These objects are dictionaries, and a query can return columns with special names like `__proto__` (which would be ignored without this change). Also construct the object by passing vectors of properties for better performance and improve error handling by using `MaybeLocal`. PR-URL: https://github.com/nodejs/node/pull/54350 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Yagiz Nizipli Reviewed-By: Franziska Hinkelmann Reviewed-By: James M Snell --- src/node_sqlite.cc | 88 ++++++++++--------- src/node_sqlite.h | 4 +- test/parallel/test-sqlite-data-types.js | 6 +- test/parallel/test-sqlite-database-sync.js | 4 +- test/parallel/test-sqlite-named-parameters.js | 4 +- test/parallel/test-sqlite-statement-sync.js | 21 +++-- test/parallel/test-sqlite-transactions.js | 2 +- test/parallel/test-sqlite.js | 8 +- 8 files changed, 76 insertions(+), 61 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 4821db5303501a..14b8263310279a 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -25,6 +25,10 @@ using v8::FunctionTemplate; using v8::Integer; using v8::Isolate; using v8::Local; +using v8::LocalVector; +using v8::MaybeLocal; +using v8::Name; +using v8::Null; using v8::Number; using v8::Object; using v8::String; @@ -405,7 +409,7 @@ bool StatementSync::BindValue(const Local& value, const int index) { return true; } -Local StatementSync::ColumnToValue(const int column) { +MaybeLocal StatementSync::ColumnToValue(const int column) { switch (sqlite3_column_type(statement_, column)) { case SQLITE_INTEGER: { sqlite3_int64 value = sqlite3_column_int64(statement_, column); @@ -419,7 +423,7 @@ Local StatementSync::ColumnToValue(const int column) { "represented as a JavaScript number: %" PRId64, column, value); - return Local(); + return MaybeLocal(); } } case SQLITE_FLOAT: @@ -428,14 +432,10 @@ Local StatementSync::ColumnToValue(const int column) { case SQLITE_TEXT: { const char* value = reinterpret_cast( sqlite3_column_text(statement_, column)); - Local val; - if (!String::NewFromUtf8(env()->isolate(), value).ToLocal(&val)) { - return Local(); - } - return val; + return String::NewFromUtf8(env()->isolate(), value).As(); } case SQLITE_NULL: - return v8::Null(env()->isolate()); + return Null(env()->isolate()); case SQLITE_BLOB: { size_t size = static_cast(sqlite3_column_bytes(statement_, column)); @@ -451,19 +451,15 @@ Local StatementSync::ColumnToValue(const int column) { } } -Local StatementSync::ColumnNameToValue(const int column) { +MaybeLocal StatementSync::ColumnNameToName(const int column) { const char* col_name = sqlite3_column_name(statement_, column); if (col_name == nullptr) { node::THROW_ERR_INVALID_STATE( env(), "Cannot get name of column %d", column); - return Local(); + return MaybeLocal(); } - Local key; - if (!String::NewFromUtf8(env()->isolate(), col_name).ToLocal(&key)) { - return Local(); - } - return key; + return String::NewFromUtf8(env()->isolate(), col_name).As(); } void StatementSync::MemoryInfo(MemoryTracker* tracker) const {} @@ -474,9 +470,9 @@ void StatementSync::All(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); + Isolate* isolate = env->isolate(); int r = sqlite3_reset(stmt->statement_); - CHECK_ERROR_OR_THROW( - env->isolate(), stmt->db_->Connection(), r, SQLITE_OK, void()); + CHECK_ERROR_OR_THROW(isolate, stmt->db_->Connection(), r, SQLITE_OK, void()); if (!stmt->BindParams(args)) { return; @@ -484,28 +480,30 @@ void StatementSync::All(const FunctionCallbackInfo& args) { auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); int num_cols = sqlite3_column_count(stmt->statement_); - std::vector> rows; + LocalVector rows(isolate); while ((r = sqlite3_step(stmt->statement_)) == SQLITE_ROW) { - Local row = Object::New(env->isolate()); + LocalVector row_keys(isolate); + row_keys.reserve(num_cols); + LocalVector row_values(isolate); + row_values.reserve(num_cols); for (int i = 0; i < num_cols; ++i) { - Local key = stmt->ColumnNameToValue(i); - if (key.IsEmpty()) return; - Local val = stmt->ColumnToValue(i); - if (val.IsEmpty()) return; - - if (row->Set(env->context(), key, val).IsNothing()) { - return; - } + Local key; + if (!stmt->ColumnNameToName(i).ToLocal(&key)) return; + Local val; + if (!stmt->ColumnToValue(i).ToLocal(&val)) return; + row_keys.emplace_back(key); + row_values.emplace_back(val); } + Local row = Object::New( + isolate, Null(isolate), row_keys.data(), row_values.data(), num_cols); rows.emplace_back(row); } CHECK_ERROR_OR_THROW( - env->isolate(), stmt->db_->Connection(), r, SQLITE_DONE, void()); - args.GetReturnValue().Set( - Array::New(env->isolate(), rows.data(), rows.size())); + isolate, stmt->db_->Connection(), r, SQLITE_DONE, void()); + args.GetReturnValue().Set(Array::New(isolate, rows.data(), rows.size())); } void StatementSync::Get(const FunctionCallbackInfo& args) { @@ -514,9 +512,9 @@ void StatementSync::Get(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); + Isolate* isolate = env->isolate(); int r = sqlite3_reset(stmt->statement_); - CHECK_ERROR_OR_THROW( - env->isolate(), stmt->db_->Connection(), r, SQLITE_OK, void()); + CHECK_ERROR_OR_THROW(isolate, stmt->db_->Connection(), r, SQLITE_OK, void()); if (!stmt->BindParams(args)) { return; @@ -526,7 +524,7 @@ void StatementSync::Get(const FunctionCallbackInfo& args) { r = sqlite3_step(stmt->statement_); if (r == SQLITE_DONE) return; if (r != SQLITE_ROW) { - THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_->Connection()); + THROW_ERR_SQLITE_ERROR(isolate, stmt->db_->Connection()); return; } @@ -535,19 +533,23 @@ void StatementSync::Get(const FunctionCallbackInfo& args) { return; } - Local result = Object::New(env->isolate()); + LocalVector keys(isolate); + keys.reserve(num_cols); + LocalVector values(isolate); + values.reserve(num_cols); for (int i = 0; i < num_cols; ++i) { - Local key = stmt->ColumnNameToValue(i); - if (key.IsEmpty()) return; - Local val = stmt->ColumnToValue(i); - if (val.IsEmpty()) return; - - if (result->Set(env->context(), key, val).IsNothing()) { - return; - } + Local key; + if (!stmt->ColumnNameToName(i).ToLocal(&key)) return; + Local val; + if (!stmt->ColumnToValue(i).ToLocal(&val)) return; + keys.emplace_back(key); + values.emplace_back(val); } + Local result = + Object::New(isolate, Null(isolate), keys.data(), values.data(), num_cols); + args.GetReturnValue().Set(result); } @@ -676,7 +678,7 @@ Local StatementSync::GetConstructorTemplate( if (tmpl.IsEmpty()) { Isolate* isolate = env->isolate(); tmpl = NewFunctionTemplate(isolate, IllegalConstructor); - tmpl->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "StatementSync")); + tmpl->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "StatementSync")); tmpl->InstanceTemplate()->SetInternalFieldCount( StatementSync::kInternalFieldCount); SetProtoMethod(isolate, tmpl, "all", StatementSync::All); diff --git a/src/node_sqlite.h b/src/node_sqlite.h index ca6e8c7f23cf40..f62b2e991cb95a 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -80,8 +80,8 @@ class StatementSync : public BaseObject { std::optional> bare_named_params_; bool BindParams(const v8::FunctionCallbackInfo& args); bool BindValue(const v8::Local& value, const int index); - v8::Local ColumnToValue(const int column); - v8::Local ColumnNameToValue(const int column); + v8::MaybeLocal ColumnToValue(const int column); + v8::MaybeLocal ColumnNameToName(const int column); }; } // namespace sqlite diff --git a/test/parallel/test-sqlite-data-types.js b/test/parallel/test-sqlite-data-types.js index 582d5bd611edf4..df77a90908eed5 100644 --- a/test/parallel/test-sqlite-data-types.js +++ b/test/parallel/test-sqlite-data-types.js @@ -49,6 +49,7 @@ suite('data binding and mapping', () => { const query = db.prepare('SELECT * FROM types WHERE key = ?'); t.assert.deepStrictEqual(query.get(1), { + __proto__: null, key: 1, int: 42, double: 3.14159, @@ -56,6 +57,7 @@ suite('data binding and mapping', () => { buf: u8a, }); t.assert.deepStrictEqual(query.get(2), { + __proto__: null, key: 2, int: null, double: null, @@ -63,6 +65,7 @@ suite('data binding and mapping', () => { buf: null, }); t.assert.deepStrictEqual(query.get(3), { + __proto__: null, key: 3, int: 8, double: 2.718, @@ -70,6 +73,7 @@ suite('data binding and mapping', () => { buf: new TextEncoder().encode('x☃y☃'), }); t.assert.deepStrictEqual(query.get(4), { + __proto__: null, key: 4, int: 99, double: 0xf, @@ -151,7 +155,7 @@ suite('data binding and mapping', () => { ); t.assert.deepStrictEqual( db.prepare('SELECT * FROM data ORDER BY key').all(), - [{ key: 1, val: 5 }, { key: 2, val: null }], + [{ __proto__: null, key: 1, val: 5 }, { __proto__: null, key: 2, val: null }], ); }); }); diff --git a/test/parallel/test-sqlite-database-sync.js b/test/parallel/test-sqlite-database-sync.js index 1bc409926ae446..e528daf227c507 100644 --- a/test/parallel/test-sqlite-database-sync.js +++ b/test/parallel/test-sqlite-database-sync.js @@ -143,8 +143,8 @@ suite('DatabaseSync.prototype.exec()', () => { t.assert.strictEqual(result, undefined); const stmt = db.prepare('SELECT * FROM data ORDER BY key'); t.assert.deepStrictEqual(stmt.all(), [ - { key: 1, val: 2 }, - { key: 8, val: 9 }, + { __proto__: null, key: 1, val: 2 }, + { __proto__: null, key: 8, val: 9 }, ]); }); diff --git a/test/parallel/test-sqlite-named-parameters.js b/test/parallel/test-sqlite-named-parameters.js index 27857111953d27..3060e252235401 100644 --- a/test/parallel/test-sqlite-named-parameters.js +++ b/test/parallel/test-sqlite-named-parameters.js @@ -42,7 +42,7 @@ suite('named parameters', () => { stmt.run({ k: 1, v: 9 }); t.assert.deepStrictEqual( db.prepare('SELECT * FROM data').get(), - { key: 1, val: 9 }, + { __proto__: null, key: 1, val: 9 }, ); }); @@ -57,7 +57,7 @@ suite('named parameters', () => { stmt.run({ k: 1 }); t.assert.deepStrictEqual( db.prepare('SELECT * FROM data').get(), - { key: 1, val: 1 }, + { __proto__: null, key: 1, val: 1 }, ); }); diff --git a/test/parallel/test-sqlite-statement-sync.js b/test/parallel/test-sqlite-statement-sync.js index 7a4069678af966..1052e9fb76390b 100644 --- a/test/parallel/test-sqlite-statement-sync.js +++ b/test/parallel/test-sqlite-statement-sync.js @@ -43,7 +43,15 @@ suite('StatementSync.prototype.get()', () => { t.assert.strictEqual(stmt.get('key1', 'val1'), undefined); t.assert.strictEqual(stmt.get('key2', 'val2'), undefined); stmt = db.prepare('SELECT * FROM storage ORDER BY key'); - t.assert.deepStrictEqual(stmt.get(), { key: 'key1', val: 'val1' }); + t.assert.deepStrictEqual(stmt.get(), { __proto__: null, key: 'key1', val: 'val1' }); + }); + + test('executes a query that returns special columns', (t) => { + const db = new DatabaseSync(nextDb()); + t.after(() => { db.close(); }); + const stmt = db.prepare('SELECT 1 as __proto__, 2 as constructor, 3 as toString'); + // eslint-disable-next-line no-dupe-keys + t.assert.deepStrictEqual(stmt.get(), { __proto__: null, ['__proto__']: 1, constructor: 2, toString: 3 }); }); }); @@ -71,8 +79,8 @@ suite('StatementSync.prototype.all()', () => { ); stmt = db.prepare('SELECT * FROM storage ORDER BY key'); t.assert.deepStrictEqual(stmt.all(), [ - { key: 'key1', val: 'val1' }, - { key: 'key2', val: 'val2' }, + { __proto__: null, key: 'key1', val: 'val1' }, + { __proto__: null, key: 'key2', val: 'val2' }, ]); }); }); @@ -171,11 +179,11 @@ suite('StatementSync.prototype.setReadBigInts()', () => { t.assert.strictEqual(setup, undefined); const query = db.prepare('SELECT val FROM data'); - t.assert.deepStrictEqual(query.get(), { val: 42 }); + t.assert.deepStrictEqual(query.get(), { __proto__: null, val: 42 }); t.assert.strictEqual(query.setReadBigInts(true), undefined); - t.assert.deepStrictEqual(query.get(), { val: 42n }); + t.assert.deepStrictEqual(query.get(), { __proto__: null, val: 42n }); t.assert.strictEqual(query.setReadBigInts(false), undefined); - t.assert.deepStrictEqual(query.get(), { val: 42 }); + t.assert.deepStrictEqual(query.get(), { __proto__: null, val: 42 }); const insert = db.prepare('INSERT INTO data (key) VALUES (?)'); t.assert.deepStrictEqual( @@ -223,6 +231,7 @@ suite('StatementSync.prototype.setReadBigInts()', () => { const good = db.prepare(`SELECT ${Number.MAX_SAFE_INTEGER} + 1`); good.setReadBigInts(true); t.assert.deepStrictEqual(good.get(), { + __proto__: null, [`${Number.MAX_SAFE_INTEGER} + 1`]: 2n ** 53n, }); }); diff --git a/test/parallel/test-sqlite-transactions.js b/test/parallel/test-sqlite-transactions.js index a37e635541bbf2..b5ed187e067e6d 100644 --- a/test/parallel/test-sqlite-transactions.js +++ b/test/parallel/test-sqlite-transactions.js @@ -37,7 +37,7 @@ suite('manual transactions', () => { ); t.assert.deepStrictEqual( db.prepare('SELECT * FROM data').all(), - [{ key: 100 }], + [{ __proto__: null, key: 100 }], ); }); diff --git a/test/parallel/test-sqlite.js b/test/parallel/test-sqlite.js index 8acabb96fceab4..f8b122131fe7a2 100644 --- a/test/parallel/test-sqlite.js +++ b/test/parallel/test-sqlite.js @@ -77,11 +77,11 @@ test('in-memory databases are supported', (t) => { t.assert.strictEqual(setup2, undefined); t.assert.deepStrictEqual( db1.prepare('SELECT * FROM data').all(), - [{ key: 1 }] + [{ __proto__: null, key: 1 }] ); t.assert.deepStrictEqual( db2.prepare('SELECT * FROM data').all(), - [{ key: 1 }] + [{ __proto__: null, key: 1 }] ); }); @@ -90,10 +90,10 @@ test('PRAGMAs are supported', (t) => { t.after(() => { db.close(); }); t.assert.deepStrictEqual( db.prepare('PRAGMA journal_mode = WAL').get(), - { journal_mode: 'wal' }, + { __proto__: null, journal_mode: 'wal' }, ); t.assert.deepStrictEqual( db.prepare('PRAGMA journal_mode').get(), - { journal_mode: 'wal' }, + { __proto__: null, journal_mode: 'wal' }, ); });