Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit d949222

Browse filesBrowse files
geeksilva97targos
authored andcommitted
sqlite: replace ToLocalChecked and improve filter error handling
PR-URL: #60028 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent c412b18 commit d949222
Copy full SHA for d949222

File tree

Expand file treeCollapse file tree

2 files changed

+51
-23
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+51
-23
lines changed
Open diff view settings
Collapse file

‎src/node_sqlite.cc‎

Copy file name to clipboardExpand all lines: src/node_sqlite.cc
+27-23Lines changed: 27 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1763,21 +1763,27 @@ void DatabaseSync::ApplyChangeset(const FunctionCallbackInfo<Value>& args) {
17631763

17641764
Local<Function> filterFunc = filterValue.As<Function>();
17651765

1766-
context.filterCallback = [env,
1767-
filterFunc](std::string_view item) -> bool {
1768-
// TODO(@jasnell): The use of ToLocalChecked here means that if
1769-
// the filter function throws an error the process will crash.
1770-
// The filterCallback should be updated to avoid the check and
1771-
// propagate the error correctly.
1772-
Local<Value> argv[] = {
1773-
String::NewFromUtf8(env->isolate(),
1774-
item.data(),
1775-
NewStringType::kNormal,
1776-
static_cast<int>(item.size()))
1777-
.ToLocalChecked()};
1778-
Local<Value> result =
1779-
filterFunc->Call(env->context(), Null(env->isolate()), 1, argv)
1780-
.ToLocalChecked();
1766+
context.filterCallback = [&](std::string_view item) -> bool {
1767+
// If there was an error in the previous call to the filter's
1768+
// callback, we skip calling it again.
1769+
if (db->ignore_next_sqlite_error_) {
1770+
return false;
1771+
}
1772+
1773+
Local<Value> argv[1];
1774+
if (!ToV8Value(env->context(), item, env->isolate())
1775+
.ToLocal(&argv[0])) {
1776+
db->SetIgnoreNextSQLiteError(true);
1777+
return false;
1778+
}
1779+
1780+
Local<Value> result;
1781+
if (!filterFunc->Call(env->context(), Null(env->isolate()), 1, argv)
1782+
.ToLocal(&result)) {
1783+
db->SetIgnoreNextSQLiteError(true);
1784+
return false;
1785+
}
1786+
17811787
return result->BooleanValue(env->isolate());
17821788
};
17831789
}
@@ -2239,9 +2245,11 @@ Local<Value> StatementExecutionHelper::Get(Environment* env,
22392245
LocalVector<Name> keys(isolate);
22402246
keys.reserve(num_cols);
22412247
for (int i = 0; i < num_cols; ++i) {
2242-
MaybeLocal<Name> key = ColumnNameToName(env, stmt, i);
2243-
if (key.IsEmpty()) return Undefined(isolate);
2244-
keys.emplace_back(key.ToLocalChecked());
2248+
Local<Name> key;
2249+
if (!ColumnNameToName(env, stmt, i).ToLocal(&key)) {
2250+
return Undefined(isolate);
2251+
}
2252+
keys.emplace_back(key);
22452253
}
22462254

22472255
DCHECK_EQ(keys.size(), row_values.size());
@@ -2755,12 +2763,8 @@ BaseObjectPtr<StatementSync> SQLTagStore::PrepareStatement(
27552763

27562764
if (stmt == nullptr) {
27572765
sqlite3_stmt* s = nullptr;
2758-
Local<String> sql_str =
2759-
String::NewFromUtf8(isolate, sql.c_str()).ToLocalChecked();
2760-
Utf8Value sql_utf8(isolate, sql_str);
2761-
27622766
int r = sqlite3_prepare_v2(
2763-
session->database_->connection_, *sql_utf8, -1, &s, 0);
2767+
session->database_->connection_, sql.c_str(), -1, &s, 0);
27642768

27652769
if (r != SQLITE_OK) {
27662770
THROW_ERR_SQLITE_ERROR(isolate, "Failed to prepare statement");
Collapse file

‎test/parallel/test-sqlite-session.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-sqlite-session.js
+24Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,30 @@ suite('conflict resolution', () => {
366366
});
367367
});
368368

369+
test('filter handler throws', (t) => {
370+
const database1 = new DatabaseSync(':memory:');
371+
const database2 = new DatabaseSync(':memory:');
372+
const createTableSql = 'CREATE TABLE data1(key INTEGER PRIMARY KEY); CREATE TABLE data2(key INTEGER PRIMARY KEY);';
373+
database1.exec(createTableSql);
374+
database2.exec(createTableSql);
375+
376+
const session = database1.createSession();
377+
378+
database1.exec('INSERT INTO data1 (key) VALUES (1), (2), (3)');
379+
database1.exec('INSERT INTO data2 (key) VALUES (1), (2), (3), (4), (5)');
380+
381+
t.assert.throws(() => {
382+
database2.applyChangeset(session.changeset(), {
383+
filter: (tableName) => {
384+
throw new Error(`Error filtering table ${tableName}`);
385+
}
386+
});
387+
}, {
388+
name: 'Error',
389+
message: 'Error filtering table data1'
390+
});
391+
});
392+
369393
test('database.createSession() - filter changes', (t) => {
370394
const database1 = new DatabaseSync(':memory:');
371395
const database2 = new DatabaseSync(':memory:');

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.