Repository: nodejs/node
Pull request: sqlite: enable foreign key constraints by default

For historical reasons and to maintain compatibility with legacy database schemas, [SQLite does not enable foreign key constraints by default.](https://www.sqlite.org/quirks.html#foreign_key_enforcement_is_off_by_default) For new applications, however, this behavior is undesirable. Currently, any application that wishes to use foreign keys must use

```sql
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.

I decided against using the compile-time option to set the default behavior to avoid limiting compatibility with third-party code that links against node's copy of SQLite.

<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: doc/api/sqlite.md
patch: @@ -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.
 
@@ -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

filename: src/node_sqlite.cc
patch: @@ -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();
@@ -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;
 }
 
@@ -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()) {
@@ -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) {

filename: src/node_sqlite.h
patch: @@ -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);
@@ -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 {

filename: test/parallel/test-sqlite-database-sync.js
patch: @@ -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()', () => {