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: #54350 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Aug 25, 2024 · 7fea010 · 7fea010
1 parent c3fe2d6
commit 7fea010
Show file tree

Hide file tree

Showing 8 changed files with 76 additions and 61 deletions.
diff --git 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>& value, const int index) {
   return true;
 }
 
-Local<Value> StatementSync::ColumnToValue(const int column) {
+MaybeLocal<Value> 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<Value> StatementSync::ColumnToValue(const int column) {
                                "represented as a JavaScript number: %" PRId64,
                                column,
                                value);
-        return Local<Value>();
+        return MaybeLocal<Value>();
       }
     }
     case SQLITE_FLOAT:
@@ -428,14 +432,10 @@ Local<Value> StatementSync::ColumnToValue(const int column) {
     case SQLITE_TEXT: {
       const char* value = reinterpret_cast<const char*>(
           sqlite3_column_text(statement_, column));
-      Local<Value> val;
-      if (!String::NewFromUtf8(env()->isolate(), value).ToLocal(&val)) {
-        return Local<Value>();
-      }
-      return val;
+      return String::NewFromUtf8(env()->isolate(), value).As<Value>();
     }
     case SQLITE_NULL:
-      return v8::Null(env()->isolate());
+      return Null(env()->isolate());
     case SQLITE_BLOB: {
       size_t size =
           static_cast<size_t>(sqlite3_column_bytes(statement_, column));
@@ -451,19 +451,15 @@ Local<Value> StatementSync::ColumnToValue(const int column) {
   }
 }
 
-Local<Value> StatementSync::ColumnNameToValue(const int column) {
+MaybeLocal<Name> 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<Value>();
+    return MaybeLocal<Name>();
   }
 
-  Local<String> key;
-  if (!String::NewFromUtf8(env()->isolate(), col_name).ToLocal(&key)) {
-    return Local<Value>();
-  }
-  return key;
+  return String::NewFromUtf8(env()->isolate(), col_name).As<Name>();
 }
 
 void StatementSync::MemoryInfo(MemoryTracker* tracker) const {}
@@ -474,38 +470,40 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& 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;
   }
 
   auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
   int num_cols = sqlite3_column_count(stmt->statement_);
-  std::vector<Local<Value>> rows;
+  LocalVector<Value> rows(isolate);
   while ((r = sqlite3_step(stmt->statement_)) == SQLITE_ROW) {
-    Local<Object> row = Object::New(env->isolate());
+    LocalVector<Name> row_keys(isolate);
+    row_keys.reserve(num_cols);
+    LocalVector<Value> row_values(isolate);
+    row_values.reserve(num_cols);
 
     for (int i = 0; i < num_cols; ++i) {
-      Local<Value> key = stmt->ColumnNameToValue(i);
-      if (key.IsEmpty()) return;
-      Local<Value> val = stmt->ColumnToValue(i);
-      if (val.IsEmpty()) return;
-
-      if (row->Set(env->context(), key, val).IsNothing()) {
-        return;
-      }
+      Local<Name> key;
+      if (!stmt->ColumnNameToName(i).ToLocal(&key)) return;
+      Local<Value> val;
+      if (!stmt->ColumnToValue(i).ToLocal(&val)) return;
+      row_keys.emplace_back(key);
+      row_values.emplace_back(val);
     }
 
+    Local<Object> 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<Value>& args) {
@@ -514,9 +512,9 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& 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<Value>& 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<Value>& args) {
     return;
   }
 
-  Local<Object> result = Object::New(env->isolate());
+  LocalVector<Name> keys(isolate);
+  keys.reserve(num_cols);
+  LocalVector<Value> values(isolate);
+  values.reserve(num_cols);
 
   for (int i = 0; i < num_cols; ++i) {
-    Local<Value> key = stmt->ColumnNameToValue(i);
-    if (key.IsEmpty()) return;
-    Local<Value> val = stmt->ColumnToValue(i);
-    if (val.IsEmpty()) return;
-
-    if (result->Set(env->context(), key, val).IsNothing()) {
-      return;
-    }
+    Local<Name> key;
+    if (!stmt->ColumnNameToName(i).ToLocal(&key)) return;
+    Local<Value> val;
+    if (!stmt->ColumnToValue(i).ToLocal(&val)) return;
+    keys.emplace_back(key);
+    values.emplace_back(val);
   }
 
+  Local<Object> result =
+      Object::New(isolate, Null(isolate), keys.data(), values.data(), num_cols);
+
   args.GetReturnValue().Set(result);
 }
 
@@ -676,7 +678,7 @@ Local<FunctionTemplate> 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
@@ -80,8 +80,8 @@ class StatementSync : public BaseObject {
   std::optional<std::map<std::string, std::string>> bare_named_params_;
   bool BindParams(const v8::FunctionCallbackInfo<v8::Value>& args);
   bool BindValue(const v8::Local<v8::Value>& value, const int index);
-  v8::Local<v8::Value> ColumnToValue(const int column);
-  v8::Local<v8::Value> ColumnNameToValue(const int column);
+  v8::MaybeLocal<v8::Value> ColumnToValue(const int column);
+  v8::MaybeLocal<v8::Name> ColumnNameToName(const int column);
 };
 
 }  // namespace sqlite

diff --git a/test/parallel/test-sqlite-data-types.js b/test/parallel/test-sqlite-data-types.js
@@ -49,27 +49,31 @@ 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,
       text: 'foo',
       buf: u8a,
     });
     t.assert.deepStrictEqual(query.get(2), {
+      __proto__: null,
       key: 2,
       int: null,
       double: null,
       text: null,
       buf: null,
     });
     t.assert.deepStrictEqual(query.get(3), {
+      __proto__: null,
       key: 3,
       int: 8,
       double: 2.718,
       text: 'bar',
       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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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' },
   );
 });