Skip to content

Commit

Permalink
sqlite: enable foreign key constraints by default
Browse files Browse the repository at this point in the history
For historical reasons and to maintain compatibibility with legacy
database schemas, SQLite does not enable foreign key constraints by
default. For new applications, however, this behavior is undesirable.
Currently, any application that wishes to use foreign keys must use

    PRAGMA foreign_keys = ON;

to explicitly enable enforcement of such constraints.

This commit changes the behavior of the SQLite API built into Node.js
to enable foreign key constraints by default. This behavior can be
overridden by users to maintain compatibility with legacy database
schemas.

PR-URL: #54777
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
tniessen authored and aduh95 committed Oct 9, 2024
1 parent f703652 commit 67f5f46
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 3 deletions.
6 changes: 6 additions & 0 deletions doc/api/sqlite.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,11 @@ added: v22.5.0
* `open` {boolean} If `true`, the database is opened by the constructor. When
this value is `false`, the database must be opened via the `open()` method.
**Default:** `true`.
* `enableForeignKeyConstraints` {boolean} If `true`, foreign key constraints
are enabled. This is recommended but can be disabled for compatibility with
legacy database schemas. The enforcement of foreign key constraints can be
enabled and disabled after opening the database using
[`PRAGMA foreign_keys`][]. **Default:** `true`.

Constructs a new `DatabaseSync` instance.

Expand Down Expand Up @@ -317,6 +322,7 @@ exception.

[SQL injection]: https://en.wikipedia.org/wiki/SQL_injection
[`--experimental-sqlite`]: cli.md#--experimental-sqlite
[`PRAGMA foreign_keys`]: https://www.sqlite.org/pragma.html#pragma_foreign_keys
[`sqlite3_changes64()`]: https://www.sqlite.org/c3ref/changes.html
[`sqlite3_close_v2()`]: https://www.sqlite.org/c3ref/close.html
[`sqlite3_exec()`]: https://www.sqlite.org/c3ref/exec.html
Expand Down
35 changes: 33 additions & 2 deletions src/node_sqlite.cc
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,14 @@ inline void THROW_ERR_SQLITE_ERROR(Isolate* isolate, const char* message) {
DatabaseSync::DatabaseSync(Environment* env,
Local<Object> object,
Local<String> location,
bool open)
bool open,
bool enable_foreign_keys_on_open)
: BaseObject(env, object) {
MakeWeak();
node::Utf8Value utf8_location(env->isolate(), location);
location_ = utf8_location.ToString();
connection_ = nullptr;
enable_foreign_keys_on_open_ = enable_foreign_keys_on_open;

if (open) {
Open();
Expand Down Expand Up @@ -125,6 +127,15 @@ bool DatabaseSync::Open() {
int flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE;
int r = sqlite3_open_v2(location_.c_str(), &connection_, flags, nullptr);
CHECK_ERROR_OR_THROW(env()->isolate(), connection_, r, SQLITE_OK, false);

int foreign_keys_enabled;
r = sqlite3_db_config(connection_,
SQLITE_DBCONFIG_ENABLE_FKEY,
static_cast<int>(enable_foreign_keys_on_open_),
&foreign_keys_enabled);
CHECK_ERROR_OR_THROW(env()->isolate(), connection_, r, SQLITE_OK, false);
CHECK_EQ(foreign_keys_enabled, enable_foreign_keys_on_open_);

return true;
}

Expand Down Expand Up @@ -166,6 +177,7 @@ void DatabaseSync::New(const FunctionCallbackInfo<Value>& args) {
}

bool open = true;
bool enable_foreign_keys = true;

if (args.Length() > 1) {
if (!args[1]->IsObject()) {
Expand All @@ -188,9 +200,28 @@ void DatabaseSync::New(const FunctionCallbackInfo<Value>& args) {
}
open = open_v.As<Boolean>()->Value();
}

Local<String> enable_foreign_keys_string =
FIXED_ONE_BYTE_STRING(env->isolate(), "enableForeignKeyConstraints");
Local<Value> enable_foreign_keys_v;
if (!options->Get(env->context(), enable_foreign_keys_string)
.ToLocal(&enable_foreign_keys_v)) {
return;
}
if (!enable_foreign_keys_v->IsUndefined()) {
if (!enable_foreign_keys_v->IsBoolean()) {
node::THROW_ERR_INVALID_ARG_TYPE(
env->isolate(),
"The \"options.enableForeignKeyConstraints\" argument must be a "
"boolean.");
return;
}
enable_foreign_keys = enable_foreign_keys_v.As<Boolean>()->Value();
}
}

new DatabaseSync(env, args.This(), args[0].As<String>(), open);
new DatabaseSync(
env, args.This(), args[0].As<String>(), open, enable_foreign_keys);
}

void DatabaseSync::Open(const FunctionCallbackInfo<Value>& args) {
Expand Down
4 changes: 3 additions & 1 deletion src/node_sqlite.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ class DatabaseSync : public BaseObject {
DatabaseSync(Environment* env,
v8::Local<v8::Object> object,
v8::Local<v8::String> location,
bool open);
bool open,
bool enable_foreign_keys_on_open);
void MemoryInfo(MemoryTracker* tracker) const override;
static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
static void Open(const v8::FunctionCallbackInfo<v8::Value>& args);
Expand All @@ -43,6 +44,7 @@ class DatabaseSync : public BaseObject {
std::string location_;
sqlite3* connection_;
std::unordered_set<StatementSync*> statements_;
bool enable_foreign_keys_on_open_;
};

class StatementSync : public BaseObject {
Expand Down
36 changes: 36 additions & 0 deletions test/parallel/test-sqlite-database-sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,42 @@ suite('DatabaseSync() constructor', () => {
message: /The "options\.open" argument must be a boolean/,
});
});

test('throws if options.enableForeignKeyConstraints is provided but is not a boolean', (t) => {
t.assert.throws(() => {
new DatabaseSync('foo', { enableForeignKeyConstraints: 5 });
}, {
code: 'ERR_INVALID_ARG_TYPE',
message: /The "options\.enableForeignKeyConstraints" argument must be a boolean/,
});
});

test('enables foreign key constraints by default', (t) => {
const dbPath = nextDb();
const db = new DatabaseSync(dbPath);
db.exec(`
CREATE TABLE foo (id INTEGER PRIMARY KEY);
CREATE TABLE bar (foo_id INTEGER REFERENCES foo(id));
`);
t.after(() => { db.close(); });
t.assert.throws(() => {
db.exec('INSERT INTO bar (foo_id) VALUES (1)');
}, {
code: 'ERR_SQLITE_ERROR',
message: 'FOREIGN KEY constraint failed',
});
});

test('allows disabling foreign key constraints', (t) => {
const dbPath = nextDb();
const db = new DatabaseSync(dbPath, { enableForeignKeyConstraints: false });
db.exec(`
CREATE TABLE foo (id INTEGER PRIMARY KEY);
CREATE TABLE bar (foo_id INTEGER REFERENCES foo(id));
`);
t.after(() => { db.close(); });
db.exec('INSERT INTO bar (foo_id) VALUES (1)');
});
});

suite('DatabaseSync.prototype.open()', () => {
Expand Down

0 comments on commit 67f5f46

Please sign in to comment.