Skip to content

Commit e3b117d

Browse files
targoslouwers
authored andcommitted
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: nodejs#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]>
1 parent d22ea2d commit e3b117d

8 files changed

+76
-61
lines changed

src/node_sqlite.cc

Lines changed: 45 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,10 @@ using v8::Integer;
2727
using v8::Isolate;
2828
using v8::Local;
2929
using v8::NewStringType;
30+
using v8::LocalVector;
31+
using v8::MaybeLocal;
32+
using v8::Name;
33+
using v8::Null;
3034
using v8::Number;
3135
using v8::Object;
3236
using v8::String;
@@ -587,7 +591,7 @@ bool StatementSync::BindValue(const Local<Value>& value, const int index) {
587591
return true;
588592
}
589593

590-
Local<Value> StatementSync::ColumnToValue(const int column) {
594+
MaybeLocal<Value> StatementSync::ColumnToValue(const int column) {
591595
switch (sqlite3_column_type(statement_, column)) {
592596
case SQLITE_INTEGER: {
593597
sqlite3_int64 value = sqlite3_column_int64(statement_, column);
@@ -601,7 +605,7 @@ Local<Value> StatementSync::ColumnToValue(const int column) {
601605
"represented as a JavaScript number: %" PRId64,
602606
column,
603607
value);
604-
return Local<Value>();
608+
return MaybeLocal<Value>();
605609
}
606610
}
607611
case SQLITE_FLOAT:
@@ -610,14 +614,10 @@ Local<Value> StatementSync::ColumnToValue(const int column) {
610614
case SQLITE_TEXT: {
611615
const char* value = reinterpret_cast<const char*>(
612616
sqlite3_column_text(statement_, column));
613-
Local<Value> val;
614-
if (!String::NewFromUtf8(env()->isolate(), value).ToLocal(&val)) {
615-
return Local<Value>();
616-
}
617-
return val;
617+
return String::NewFromUtf8(env()->isolate(), value).As<Value>();
618618
}
619619
case SQLITE_NULL:
620-
return v8::Null(env()->isolate());
620+
return Null(env()->isolate());
621621
case SQLITE_BLOB: {
622622
size_t size =
623623
static_cast<size_t>(sqlite3_column_bytes(statement_, column));
@@ -633,19 +633,15 @@ Local<Value> StatementSync::ColumnToValue(const int column) {
633633
}
634634
}
635635

636-
Local<Value> StatementSync::ColumnNameToValue(const int column) {
636+
MaybeLocal<Name> StatementSync::ColumnNameToName(const int column) {
637637
const char* col_name = sqlite3_column_name(statement_, column);
638638
if (col_name == nullptr) {
639639
node::THROW_ERR_INVALID_STATE(
640640
env(), "Cannot get name of column %d", column);
641-
return Local<Value>();
641+
return MaybeLocal<Name>();
642642
}
643643

644-
Local<String> key;
645-
if (!String::NewFromUtf8(env()->isolate(), col_name).ToLocal(&key)) {
646-
return Local<Value>();
647-
}
648-
return key;
644+
return String::NewFromUtf8(env()->isolate(), col_name).As<Name>();
649645
}
650646

651647
void StatementSync::MemoryInfo(MemoryTracker* tracker) const {}
@@ -656,38 +652,40 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
656652
Environment* env = Environment::GetCurrent(args);
657653
THROW_AND_RETURN_ON_BAD_STATE(
658654
env, stmt->IsFinalized(), "statement has been finalized");
655+
Isolate* isolate = env->isolate();
659656
int r = sqlite3_reset(stmt->statement_);
660-
CHECK_ERROR_OR_THROW(
661-
env->isolate(), stmt->db_->Connection(), r, SQLITE_OK, void());
657+
CHECK_ERROR_OR_THROW(isolate, stmt->db_->Connection(), r, SQLITE_OK, void());
662658

663659
if (!stmt->BindParams(args)) {
664660
return;
665661
}
666662

667663
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
668664
int num_cols = sqlite3_column_count(stmt->statement_);
669-
std::vector<Local<Value>> rows;
665+
LocalVector<Value> rows(isolate);
670666
while ((r = sqlite3_step(stmt->statement_)) == SQLITE_ROW) {
671-
Local<Object> row = Object::New(env->isolate());
667+
LocalVector<Name> row_keys(isolate);
668+
row_keys.reserve(num_cols);
669+
LocalVector<Value> row_values(isolate);
670+
row_values.reserve(num_cols);
672671

673672
for (int i = 0; i < num_cols; ++i) {
674-
Local<Value> key = stmt->ColumnNameToValue(i);
675-
if (key.IsEmpty()) return;
676-
Local<Value> val = stmt->ColumnToValue(i);
677-
if (val.IsEmpty()) return;
678-
679-
if (row->Set(env->context(), key, val).IsNothing()) {
680-
return;
681-
}
673+
Local<Name> key;
674+
if (!stmt->ColumnNameToName(i).ToLocal(&key)) return;
675+
Local<Value> val;
676+
if (!stmt->ColumnToValue(i).ToLocal(&val)) return;
677+
row_keys.emplace_back(key);
678+
row_values.emplace_back(val);
682679
}
683680

681+
Local<Object> row = Object::New(
682+
isolate, Null(isolate), row_keys.data(), row_values.data(), num_cols);
684683
rows.emplace_back(row);
685684
}
686685

687686
CHECK_ERROR_OR_THROW(
688-
env->isolate(), stmt->db_->Connection(), r, SQLITE_DONE, void());
689-
args.GetReturnValue().Set(
690-
Array::New(env->isolate(), rows.data(), rows.size()));
687+
isolate, stmt->db_->Connection(), r, SQLITE_DONE, void());
688+
args.GetReturnValue().Set(Array::New(isolate, rows.data(), rows.size()));
691689
}
692690

693691
void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
@@ -696,9 +694,9 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
696694
Environment* env = Environment::GetCurrent(args);
697695
THROW_AND_RETURN_ON_BAD_STATE(
698696
env, stmt->IsFinalized(), "statement has been finalized");
697+
Isolate* isolate = env->isolate();
699698
int r = sqlite3_reset(stmt->statement_);
700-
CHECK_ERROR_OR_THROW(
701-
env->isolate(), stmt->db_->Connection(), r, SQLITE_OK, void());
699+
CHECK_ERROR_OR_THROW(isolate, stmt->db_->Connection(), r, SQLITE_OK, void());
702700

703701
if (!stmt->BindParams(args)) {
704702
return;
@@ -708,7 +706,7 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
708706
r = sqlite3_step(stmt->statement_);
709707
if (r == SQLITE_DONE) return;
710708
if (r != SQLITE_ROW) {
711-
THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_->Connection());
709+
THROW_ERR_SQLITE_ERROR(isolate, stmt->db_->Connection());
712710
return;
713711
}
714712

@@ -717,19 +715,23 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
717715
return;
718716
}
719717

720-
Local<Object> result = Object::New(env->isolate());
718+
LocalVector<Name> keys(isolate);
719+
keys.reserve(num_cols);
720+
LocalVector<Value> values(isolate);
721+
values.reserve(num_cols);
721722

722723
for (int i = 0; i < num_cols; ++i) {
723-
Local<Value> key = stmt->ColumnNameToValue(i);
724-
if (key.IsEmpty()) return;
725-
Local<Value> val = stmt->ColumnToValue(i);
726-
if (val.IsEmpty()) return;
727-
728-
if (result->Set(env->context(), key, val).IsNothing()) {
729-
return;
730-
}
724+
Local<Name> key;
725+
if (!stmt->ColumnNameToName(i).ToLocal(&key)) return;
726+
Local<Value> val;
727+
if (!stmt->ColumnToValue(i).ToLocal(&val)) return;
728+
keys.emplace_back(key);
729+
values.emplace_back(val);
731730
}
732731

732+
Local<Object> result =
733+
Object::New(isolate, Null(isolate), keys.data(), values.data(), num_cols);
734+
733735
args.GetReturnValue().Set(result);
734736
}
735737

@@ -858,7 +860,7 @@ Local<FunctionTemplate> StatementSync::GetConstructorTemplate(
858860
if (tmpl.IsEmpty()) {
859861
Isolate* isolate = env->isolate();
860862
tmpl = NewFunctionTemplate(isolate, IllegalConstructor);
861-
tmpl->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "StatementSync"));
863+
tmpl->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "StatementSync"));
862864
tmpl->InstanceTemplate()->SetInternalFieldCount(
863865
StatementSync::kInternalFieldCount);
864866
SetProtoMethod(isolate, tmpl, "all", StatementSync::All);

src/node_sqlite.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,8 @@ class StatementSync : public BaseObject {
8989
std::optional<std::map<std::string, std::string>> bare_named_params_;
9090
bool BindParams(const v8::FunctionCallbackInfo<v8::Value>& args);
9191
bool BindValue(const v8::Local<v8::Value>& value, const int index);
92-
v8::Local<v8::Value> ColumnToValue(const int column);
93-
v8::Local<v8::Value> ColumnNameToValue(const int column);
92+
v8::MaybeLocal<v8::Value> ColumnToValue(const int column);
93+
v8::MaybeLocal<v8::Name> ColumnNameToName(const int column);
9494
};
9595

9696
using Sqlite3ChangesetGenFunc = int (*)(sqlite3_session*, int*, void**);

test/parallel/test-sqlite-data-types.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,27 +49,31 @@ suite('data binding and mapping', () => {
4949

5050
const query = db.prepare('SELECT * FROM types WHERE key = ?');
5151
t.assert.deepStrictEqual(query.get(1), {
52+
__proto__: null,
5253
key: 1,
5354
int: 42,
5455
double: 3.14159,
5556
text: 'foo',
5657
buf: u8a,
5758
});
5859
t.assert.deepStrictEqual(query.get(2), {
60+
__proto__: null,
5961
key: 2,
6062
int: null,
6163
double: null,
6264
text: null,
6365
buf: null,
6466
});
6567
t.assert.deepStrictEqual(query.get(3), {
68+
__proto__: null,
6669
key: 3,
6770
int: 8,
6871
double: 2.718,
6972
text: 'bar',
7073
buf: new TextEncoder().encode('x☃y☃'),
7174
});
7275
t.assert.deepStrictEqual(query.get(4), {
76+
__proto__: null,
7377
key: 4,
7478
int: 99,
7579
double: 0xf,
@@ -151,7 +155,7 @@ suite('data binding and mapping', () => {
151155
);
152156
t.assert.deepStrictEqual(
153157
db.prepare('SELECT * FROM data ORDER BY key').all(),
154-
[{ key: 1, val: 5 }, { key: 2, val: null }],
158+
[{ __proto__: null, key: 1, val: 5 }, { __proto__: null, key: 2, val: null }],
155159
);
156160
});
157161
});

test/parallel/test-sqlite-database-sync.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,8 @@ suite('DatabaseSync.prototype.exec()', () => {
143143
t.assert.strictEqual(result, undefined);
144144
const stmt = db.prepare('SELECT * FROM data ORDER BY key');
145145
t.assert.deepStrictEqual(stmt.all(), [
146-
{ key: 1, val: 2 },
147-
{ key: 8, val: 9 },
146+
{ __proto__: null, key: 1, val: 2 },
147+
{ __proto__: null, key: 8, val: 9 },
148148
]);
149149
});
150150

test/parallel/test-sqlite-named-parameters.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ suite('named parameters', () => {
4242
stmt.run({ k: 1, v: 9 });
4343
t.assert.deepStrictEqual(
4444
db.prepare('SELECT * FROM data').get(),
45-
{ key: 1, val: 9 },
45+
{ __proto__: null, key: 1, val: 9 },
4646
);
4747
});
4848

@@ -57,7 +57,7 @@ suite('named parameters', () => {
5757
stmt.run({ k: 1 });
5858
t.assert.deepStrictEqual(
5959
db.prepare('SELECT * FROM data').get(),
60-
{ key: 1, val: 1 },
60+
{ __proto__: null, key: 1, val: 1 },
6161
);
6262
});
6363

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

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,15 @@ suite('StatementSync.prototype.get()', () => {
4343
t.assert.strictEqual(stmt.get('key1', 'val1'), undefined);
4444
t.assert.strictEqual(stmt.get('key2', 'val2'), undefined);
4545
stmt = db.prepare('SELECT * FROM storage ORDER BY key');
46-
t.assert.deepStrictEqual(stmt.get(), { key: 'key1', val: 'val1' });
46+
t.assert.deepStrictEqual(stmt.get(), { __proto__: null, key: 'key1', val: 'val1' });
47+
});
48+
49+
test('executes a query that returns special columns', (t) => {
50+
const db = new DatabaseSync(nextDb());
51+
t.after(() => { db.close(); });
52+
const stmt = db.prepare('SELECT 1 as __proto__, 2 as constructor, 3 as toString');
53+
// eslint-disable-next-line no-dupe-keys
54+
t.assert.deepStrictEqual(stmt.get(), { __proto__: null, ['__proto__']: 1, constructor: 2, toString: 3 });
4755
});
4856
});
4957

@@ -71,8 +79,8 @@ suite('StatementSync.prototype.all()', () => {
7179
);
7280
stmt = db.prepare('SELECT * FROM storage ORDER BY key');
7381
t.assert.deepStrictEqual(stmt.all(), [
74-
{ key: 'key1', val: 'val1' },
75-
{ key: 'key2', val: 'val2' },
82+
{ __proto__: null, key: 'key1', val: 'val1' },
83+
{ __proto__: null, key: 'key2', val: 'val2' },
7684
]);
7785
});
7886
});
@@ -171,11 +179,11 @@ suite('StatementSync.prototype.setReadBigInts()', () => {
171179
t.assert.strictEqual(setup, undefined);
172180

173181
const query = db.prepare('SELECT val FROM data');
174-
t.assert.deepStrictEqual(query.get(), { val: 42 });
182+
t.assert.deepStrictEqual(query.get(), { __proto__: null, val: 42 });
175183
t.assert.strictEqual(query.setReadBigInts(true), undefined);
176-
t.assert.deepStrictEqual(query.get(), { val: 42n });
184+
t.assert.deepStrictEqual(query.get(), { __proto__: null, val: 42n });
177185
t.assert.strictEqual(query.setReadBigInts(false), undefined);
178-
t.assert.deepStrictEqual(query.get(), { val: 42 });
186+
t.assert.deepStrictEqual(query.get(), { __proto__: null, val: 42 });
179187

180188
const insert = db.prepare('INSERT INTO data (key) VALUES (?)');
181189
t.assert.deepStrictEqual(
@@ -223,6 +231,7 @@ suite('StatementSync.prototype.setReadBigInts()', () => {
223231
const good = db.prepare(`SELECT ${Number.MAX_SAFE_INTEGER} + 1`);
224232
good.setReadBigInts(true);
225233
t.assert.deepStrictEqual(good.get(), {
234+
__proto__: null,
226235
[`${Number.MAX_SAFE_INTEGER} + 1`]: 2n ** 53n,
227236
});
228237
});

test/parallel/test-sqlite-transactions.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ suite('manual transactions', () => {
3737
);
3838
t.assert.deepStrictEqual(
3939
db.prepare('SELECT * FROM data').all(),
40-
[{ key: 100 }],
40+
[{ __proto__: null, key: 100 }],
4141
);
4242
});
4343

test/parallel/test-sqlite.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,11 @@ test('in-memory databases are supported', (t) => {
8282
t.assert.strictEqual(setup2, undefined);
8383
t.assert.deepStrictEqual(
8484
db1.prepare('SELECT * FROM data').all(),
85-
[{ key: 1 }]
85+
[{ __proto__: null, key: 1 }]
8686
);
8787
t.assert.deepStrictEqual(
8888
db2.prepare('SELECT * FROM data').all(),
89-
[{ key: 1 }]
89+
[{ __proto__: null, key: 1 }]
9090
);
9191
});
9292

@@ -95,11 +95,11 @@ test('PRAGMAs are supported', (t) => {
9595
t.after(() => { db.close(); });
9696
t.assert.deepStrictEqual(
9797
db.prepare('PRAGMA journal_mode = WAL').get(),
98-
{ journal_mode: 'wal' },
98+
{ __proto__: null, journal_mode: 'wal' },
9999
);
100100
t.assert.deepStrictEqual(
101101
db.prepare('PRAGMA journal_mode').get(),
102-
{ journal_mode: 'wal' },
102+
{ __proto__: null, journal_mode: 'wal' },
103103
);
104104
});
105105

0 commit comments

Comments
 (0)