fix(sqlite): Fix removeColumn and changeColumn foreign key issues for…

… the sqlite dialect (#14070)

* fix(sqlite): include fk constraints data

* test(sqlite): test for constraints key presence

* refactor(sqlite): reduce sql queries in removeColumnQuery

* test(sqlite): remove sqlite exclude checks from test suite

* test(sqlite): fix failing test for removeColumnQuery

* test(sqlite): add tests for foreignKeyCheckQuery

* fix(sqlite): prevent data loss when changeColumn is used

* test(sqlite): add tests for sqlite changeColumn

* fix(sqlite): fix removeColumn issues with fk constraints

* test(sqlite): add tests for removeColumn sqlite issues

* test(sqlite): move sqlite tests under a separate branch

* test(sqlite): remove unused import

* refactor: reduce code duplication in changeColumn and removeColumn

* test(sqlite): enable new removeColumn tests for all dialects

* test(sqlite): fix new removeColumn tests to work for postgres

* fix(sqlite): fix constraint tests. add todos

* fix: update expected removeColumnQuery sql for sqlite dialect

* fix(sqlite): run a subset of removeColumn tests only for sqlite until support is added of other dialects

Co-authored-by: Zoé <zoe@ephys.dev>
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>

src/dialects/sqlite/query-generator.js

@@ -399,13 +399,10 @@ export class SqliteQueryGenerator extends MySqlQueryGenerator {
     const quotedBackupTableName = this.quoteTable(backupTableName);
     const attributeNames = Object.keys(attributes).map(attr => this.quoteIdentifier(attr)).join(', ');
 
-    // Temporary table cannot work for foreign keys.
-    return `${this.createTableQuery(backupTableName, attributes)
-    }INSERT INTO ${quotedBackupTableName} SELECT ${attributeNames} FROM ${quotedTableName};`
-      + `DROP TABLE ${quotedTableName};${
-        this.createTableQuery(tableName, attributes)
-      }INSERT INTO ${quotedTableName} SELECT ${attributeNames} FROM ${quotedBackupTableName};`
-      + `DROP TABLE ${quotedBackupTableName};`;
+    return `${this.createTableQuery(backupTableName, attributes)}`
+      + `INSERT INTO ${quotedBackupTableName} SELECT ${attributeNames} FROM ${quotedTableName};`
+      + `DROP TABLE ${quotedTableName};`
+      + `ALTER TABLE ${quotedBackupTableName} RENAME TO ${quotedTableName};`;
   }
 
   _alterConstraintQuery(tableName, attributes, createTableSql) {
@@ -505,6 +502,15 @@ export class SqliteQueryGenerator extends MySqlQueryGenerator {
     return `SELECT name FROM sqlite_master WHERE type='table' AND name=${this.escape(this.addSchema(tableName))};`;
   }
 
+  /**
+   * Generates an SQL query to check if there are any foreign key violations in the db schema
+   *
+   * @param {string} tableName  The name of the table
+   */
+  foreignKeyCheckQuery(tableName) {
+    return `PRAGMA foreign_key_check(${this.quoteTable(tableName)});`;
+  }
+
   /**
    * Quote identifier in sql clause
    *

src/dialects/sqlite/query-interface.js

@@ -2,9 +2,10 @@
 
 const sequelizeErrors = require('../../errors');
 const { QueryTypes } = require('../../query-types');
-const { QueryInterface } = require('../abstract/query-interface');
+const { QueryInterface, QueryOptions, ColumnsDescription } = require('../abstract/query-interface');
 const { cloneDeep } = require('../../utils');
 const _ = require('lodash');
+const uuid = require('uuid').v4;
 
 /**
  * The interface that Sequelize uses to talk with SQLite database
@@ -23,12 +24,7 @@ export class SqliteQueryInterface extends QueryInterface {
     const fields = await this.describeTable(tableName, options);
     delete fields[attributeName];
 
-    const sql = this.queryGenerator.removeColumnQuery(tableName, fields);
-    const subQueries = sql.split(';').filter(q => q !== '');
-
-    for (const subQuery of subQueries) {
-      await this.sequelize.queryRaw(`${subQuery};`, { raw: true, ...options });
-    }
+    return this.alterTableInternal(tableName, fields, options);
   }
 
   /**
@@ -44,12 +40,7 @@ export class SqliteQueryInterface extends QueryInterface {
     const fields = await this.describeTable(tableName, options);
     Object.assign(fields[attributeName], this.normalizeAttribute(dataTypeOrOptions));
 
-    const sql = this.queryGenerator.removeColumnQuery(tableName, fields);
-    const subQueries = sql.split(';').filter(q => q !== '');
-
-    for (const subQuery of subQueries) {
-      await this.sequelize.queryRaw(`${subQuery};`, { raw: true, ...options });
-    }
+    return this.alterTableInternal(tableName, fields, options);
   }
 
   /**
@@ -166,6 +157,10 @@ export class SqliteQueryInterface extends QueryInterface {
       referencedColumnName: row.to,
       tableCatalog: database,
       referencedTableCatalog: database,
+      constraints: {
+        onUpdate: row.on_update,
+        onDelete: row.on_delete,
+      },
     }));
   }
 
@@ -235,6 +230,12 @@ export class SqliteQueryInterface extends QueryInterface {
           model: foreignKey.referencedTableName,
           key: foreignKey.referencedColumnName,
         };
+
+        // Add constraints to column definition
+        Object.assign(data[foreignKey.columnName], {
+          onUpdate: foreignKey.constraints.onUpdate,
+          onDelete: foreignKey.constraints.onDelete,
+        });
       }
 
       return data;
@@ -246,4 +247,78 @@ export class SqliteQueryInterface extends QueryInterface {
       throw error;
     }
   }
+
+  /**
+   * Alters a table in sqlite.
+   * Workaround for sqlite's limited alter table support.
+   *
+   * @param {string} tableName - The table's name
+   * @param {ColumnsDescription} fields - The table's description
+   * @param {QueryOptions} options - Query options
+   * @private
+   */
+  async alterTableInternal(tableName, fields, options) {
+    return this.withForeignKeysOff(options, async () => {
+      const savepointName = this.getSavepointName();
+      await this.sequelize.query(`SAVEPOINT ${savepointName};`, options);
+
+      try {
+        const sql = this.queryGenerator.removeColumnQuery(tableName, fields);
+        const subQueries = sql.split(';').filter(q => q !== '');
+
+        for (const subQuery of subQueries) {
+          await this.sequelize.query(`${subQuery};`, { raw: true, ...options });
+        }
+
+        // Run a foreign keys integrity check
+        const foreignKeyCheckResult = await this.sequelize.query(this.queryGenerator.foreignKeyCheckQuery(tableName), {
+          ...options,
+          type: QueryTypes.SELECT,
+        });
+
+        if (foreignKeyCheckResult.length > 0) {
+          // There are foreign key violations, exit
+          throw new sequelizeErrors.ForeignKeyConstraintError({
+            message: `Foreign key violations detected: ${JSON.stringify(foreignKeyCheckResult, null, 2)}`,
+            table: tableName,
+          });
+        }
+
+        await this.sequelize.query(`RELEASE ${savepointName};`, options);
+      } catch (error) {
+        await this.sequelize.query(`ROLLBACK TO ${savepointName};`, options);
+        throw error;
+      }
+    });
+  }
+
+  /**
+   * Runs the provided callback with foreign keys disabled.
+   *
+   * @param {QueryOptions} [options]
+   * @param {Function<Promise<any>>} cb
+   * @private
+   */
+  async withForeignKeysOff(options, cb) {
+    await this.sequelize.query('PRAGMA foreign_keys = OFF;', options);
+
+    try {
+      return await cb();
+    } finally {
+      await this.sequelize.query('PRAGMA foreign_keys = ON;', options);
+    }
+  }
+
+  /**
+   * Returns a randomly generated savepoint name
+   *
+   * @param {string} prefix
+   * @returns {string}
+   */
+  getSavepointName(prefix = 'sequelize') {
+    // sqlite does not support "-" (dashes) in transaction's name
+    const suffix = uuid().replace(/-/g, '_');
+
+    return `${prefix}_${suffix}`;
+  }
 }

test/integration/query-interface.test.js

@@ -505,6 +505,9 @@ describe(Support.getTestDialectTeaser('QueryInterface'), () => {
         expect(ref.tableName).to.equal('hosts');
         expect(ref.referencedColumnName).to.equal('id');
         expect(ref.referencedTableName).to.equal('users');
+        if (dialect === 'sqlite') {
+          expect(ref).to.have.property('constraints');
+        }
       }
 
       const columnNames = references.map(reference => reference.columnName);