diff --git a/src/dialects/sqlite/query-generator.js b/src/dialects/sqlite/query-generator.js index 0e8798ef39dd..d4e97ea111e8 100644 --- a/src/dialects/sqlite/query-generator.js +++ b/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 * diff --git a/src/dialects/sqlite/query-interface.js b/src/dialects/sqlite/query-interface.js index 5e99c5ec770f..9af14f7aab3b 100644 --- a/src/dialects/sqlite/query-interface.js +++ b/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>} 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}`; + } } diff --git a/test/integration/dialects/sqlite/test.sqlite b/test/integration/dialects/sqlite/test.sqlite deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/test/integration/query-interface.test.js b/test/integration/query-interface.test.js index 4f4f5c1ca74c..b8ac141cb5ea 100644 --- a/test/integration/query-interface.test.js +++ b/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); diff --git a/test/integration/query-interface/changeColumn.test.js b/test/integration/query-interface/changeColumn.test.js index c3e2d1774874..10c5b7e5225a 100644 --- a/test/integration/query-interface/changeColumn.test.js +++ b/test/integration/query-interface/changeColumn.test.js @@ -134,115 +134,117 @@ describe(Support.getTestDialectTeaser('QueryInterface'), () => { }); } - // SQlite natively doesn't support ALTER Foreign key - if (dialect !== 'sqlite') { - describe('should support foreign keys', () => { - beforeEach(async function () { - await this.queryInterface.createTable('users', { - id: { - type: DataTypes.INTEGER, - primaryKey: true, - autoIncrement: true, - }, - level_id: { - type: DataTypes.INTEGER, - allowNull: false, - }, - }); + describe('should support foreign keys', () => { + beforeEach(async function () { + await this.queryInterface.createTable('users', { + id: { + type: DataTypes.INTEGER, + primaryKey: true, + autoIncrement: true, + }, + level_id: { + type: DataTypes.INTEGER, + allowNull: false, + }, + }); - await this.queryInterface.createTable('level', { - id: { - type: DataTypes.INTEGER, - primaryKey: true, - autoIncrement: true, - }, - }); + await this.queryInterface.createTable('level', { + id: { + type: DataTypes.INTEGER, + primaryKey: true, + autoIncrement: true, + }, }); + }); - it('able to change column to foreign key', async function () { - const foreignKeys = await this.queryInterface.getForeignKeyReferencesForTable('users'); - expect(foreignKeys).to.be.an('array'); - expect(foreignKeys).to.be.empty; + it('able to change column to foreign key', async function () { + const foreignKeys = await this.queryInterface.getForeignKeyReferencesForTable('users'); + expect(foreignKeys).to.be.an('array'); + expect(foreignKeys).to.be.empty; - await this.queryInterface.changeColumn('users', 'level_id', { - type: DataTypes.INTEGER, - references: { - model: 'level', - key: 'id', - }, - onUpdate: 'cascade', - onDelete: 'cascade', - }); + await this.queryInterface.changeColumn('users', 'level_id', { + type: DataTypes.INTEGER, + references: { + model: 'level', + key: 'id', + }, + onUpdate: 'cascade', + onDelete: 'cascade', + }); - const newForeignKeys = await this.queryInterface.getForeignKeyReferencesForTable('users'); - expect(newForeignKeys).to.be.an('array'); - expect(newForeignKeys).to.have.lengthOf(1); - expect(newForeignKeys[0].columnName).to.be.equal('level_id'); - }); - - it('able to change column property without affecting other properties', async function () { - // 1. look for users table information - // 2. change column level_id on users to have a Foreign Key - // 3. look for users table Foreign Keys information - // 4. change column level_id AGAIN to allow null values - // 5. look for new foreign keys information - // 6. look for new table structure information - // 7. compare foreign keys and tables(before and after the changes) - const firstTable = await this.queryInterface.describeTable({ - tableName: 'users', - }); + const newForeignKeys = await this.queryInterface.getForeignKeyReferencesForTable('users'); + expect(newForeignKeys).to.be.an('array'); + expect(newForeignKeys).to.have.lengthOf(1); + expect(newForeignKeys[0].columnName).to.be.equal('level_id'); + }); - await this.queryInterface.changeColumn('users', 'level_id', { - type: DataTypes.INTEGER, - references: { - model: 'level', - key: 'id', - }, - onUpdate: 'cascade', - onDelete: 'cascade', - }); + it('able to change column property without affecting other properties', async function () { + // 1. look for users table information + // 2. change column level_id on users to have a Foreign Key + // 3. look for users table Foreign Keys information + // 4. change column level_id AGAIN to allow null values + // 5. look for new foreign keys information + // 6. look for new table structure information + // 7. compare foreign keys and tables(before and after the changes) + const firstTable = await this.queryInterface.describeTable({ + tableName: 'users', + }); - const keys = await this.queryInterface.getForeignKeyReferencesForTable('users'); - const firstForeignKeys = keys; + await this.queryInterface.changeColumn('users', 'level_id', { + type: DataTypes.INTEGER, + references: { + model: 'level', + key: 'id', + }, + onUpdate: 'cascade', + onDelete: 'cascade', + }); - await this.queryInterface.changeColumn('users', 'level_id', { - type: DataTypes.INTEGER, - allowNull: true, - }); + const keys = await this.queryInterface.getForeignKeyReferencesForTable('users'); + const firstForeignKeys = keys; - const newForeignKeys = await this.queryInterface.getForeignKeyReferencesForTable('users'); - expect(firstForeignKeys.length).to.be.equal(newForeignKeys.length); - expect(firstForeignKeys[0].columnName).to.be.equal('level_id'); - expect(firstForeignKeys[0].columnName).to.be.equal(newForeignKeys[0].columnName); + await this.queryInterface.changeColumn('users', 'level_id', { + type: DataTypes.INTEGER, + allowNull: true, + }); - const describedTable = await this.queryInterface.describeTable({ - tableName: 'users', - }); + const newForeignKeys = await this.queryInterface.getForeignKeyReferencesForTable('users'); + expect(firstForeignKeys.length).to.be.equal(newForeignKeys.length); + expect(firstForeignKeys[0].columnName).to.be.equal('level_id'); + expect(firstForeignKeys[0].columnName).to.be.equal(newForeignKeys[0].columnName); - expect(describedTable.level_id).to.have.property('allowNull'); - expect(describedTable.level_id.allowNull).to.not.equal(firstTable.level_id.allowNull); - expect(describedTable.level_id.allowNull).to.be.equal(true); + const describedTable = await this.queryInterface.describeTable({ + tableName: 'users', }); - if (!['db2', 'ibmi'].includes(dialect)) { - it('should change the comment of column', async function () { - const describedTable = await this.queryInterface.describeTable({ - tableName: 'users', - }); - expect(describedTable.level_id.comment).to.be.equal(null); + expect(describedTable.level_id).to.have.property('allowNull'); + expect(describedTable.level_id.allowNull).to.not.equal(firstTable.level_id.allowNull); + expect(describedTable.level_id.allowNull).to.be.equal(true); + }); + + if (!['db2', 'ibmi', 'sqlite'].includes(dialect)) { + it('should change the comment of column', async function () { + const describedTable = await this.queryInterface.describeTable({ + tableName: 'users', + }); - await this.queryInterface.changeColumn('users', 'level_id', { - type: DataTypes.INTEGER, - comment: 'FooBar', - }); + expect(describedTable.level_id.comment).to.be.equal(null); - const describedTable2 = await this.queryInterface.describeTable({ tableName: 'users' }); - expect(describedTable2.level_id.comment).to.be.equal('FooBar'); + await this.queryInterface.changeColumn('users', 'level_id', { + type: DataTypes.INTEGER, + comment: 'FooBar', }); - } - }); - } + const describedTable2 = await this.queryInterface.describeTable({ tableName: 'users' }); + expect(describedTable2.level_id.comment).to.be.equal('FooBar'); + }); + } + }); + + // sqlite has limited ALTER TABLE capapibilites which requires a workaround involving recreating tables. + // This leads to issues with losing data or losing foreign key references. + // The tests below address these problems + // TODO: run in all dialects if (dialect === 'sqlite') { it('should not remove unique constraints when adding or modifying columns', async function () { await this.queryInterface.createTable({ @@ -361,6 +363,143 @@ describe(Support.getTestDialectTeaser('QueryInterface'), () => { expect(refs[0].referencedTableName).to.equal('Users'); expect(refs[0].referencedColumnName).to.equal('id'); }); + + it('should retain ON UPDATE and ON DELETE constraints after a column is changed', async function () { + await this.queryInterface.createTable('level', { + id: { + type: DataTypes.INTEGER, + primaryKey: true, + autoIncrement: true, + }, + name: { + type: DataTypes.CHAR, + allowNull: false, + }, + }); + + await this.queryInterface.createTable('users', { + id: { + type: DataTypes.INTEGER, + primaryKey: true, + autoIncrement: true, + }, + name: { + type: DataTypes.STRING, + allowNull: true, + }, + level_id: { + type: DataTypes.INTEGER, + allowNull: false, + references: { + key: 'id', + model: 'level', + }, + onDelete: 'CASCADE', + onUpdate: 'CASCADE', + }, + }); + + await this.queryInterface.changeColumn('users', 'name', { + type: DataTypes.CHAR, + allowNull: false, + }); + + await this.queryInterface.changeColumn('users', 'level_id', { + type: DataTypes.INTEGER, + allowNull: true, + references: { + key: 'id', + model: 'level', + }, + onDelete: 'CASCADE', + onUpdate: 'CASCADE', + }); + + // TODO: replace with queryInterface.showConstraint once it lists foreign keys properly for sqlite + const constraintsQuery = this.queryInterface.queryGenerator.showConstraintsQuery('users'); + const [{ sql: usersSql }] = await this.queryInterface.sequelize.query(constraintsQuery, { + type: 'SELECT', + }); + + expect(usersSql).to.include('ON DELETE CASCADE', 'should include ON DELETE constraint'); + expect(usersSql).to.include('ON UPDATE CASCADE', 'should include ON UPDATE constraint'); + }); + + it('should change columns with foreign key constraints without data loss', async function () { + await this.queryInterface.createTable('users', { + id: { + type: DataTypes.INTEGER, + primaryKey: true, + autoIncrement: true, + }, + name: { + type: DataTypes.STRING, + allowNull: true, + }, + level_id: { + type: DataTypes.INTEGER, + allowNull: false, + references: { + key: 'id', + model: 'level', + }, + onDelete: 'CASCADE', + onUpdate: 'CASCADE', + }, + }); + + await this.queryInterface.createTable('level', { + id: { + type: DataTypes.INTEGER, + primaryKey: true, + autoIncrement: true, + }, + name: { + type: DataTypes.CHAR, + allowNull: false, + }, + }); + + const levels = [{ + id: 1, + name: 'L1', + }, { + id: 2, + name: 'L2', + }, + { + id: 3, + name: 'L3', + }]; + + const users = [ + { + name: 'Morpheus', + level_id: 2, + }, + { + name: 'Neo', + level_id: 1, + }, + ]; + + await Promise.all([ + this.queryInterface.bulkInsert('level', levels), + this.queryInterface.bulkInsert('users', users), + ]); + + await this.queryInterface.changeColumn('level', 'name', { + type: DataTypes.STRING, + allowNull: true, + }); + + const userRows = await this.queryInterface.sequelize.query('SELECT * from users;', { + type: 'SELECT', + }); + + expect(userRows).to.have.length(users.length, 'user records should be unaffected'); + }); } + }); }); diff --git a/test/integration/query-interface/removeColumn.test.js b/test/integration/query-interface/removeColumn.test.js index 24245c4de223..cc40726a8d5a 100644 --- a/test/integration/query-interface/removeColumn.test.js +++ b/test/integration/query-interface/removeColumn.test.js @@ -87,6 +87,130 @@ describe(Support.getTestDialectTeaser('QueryInterface'), () => { expect(table).to.not.have.property('email'); }); } + + // sqlite has limited ALTER TABLE capapibilites which requires a workaround involving recreating tables. + // This leads to issues with losing data or losing foreign key references. + // The tests below address these problems + // TODO: run in all dialects + if (dialect === 'sqlite') { + it('should remove a column with from table with foreign key constraints without losing data', async function () { + await this.queryInterface.createTable('level', { + id: { + type: DataTypes.INTEGER, + primaryKey: true, + autoIncrement: true, + }, + name: { + type: DataTypes.CHAR, + allowNull: false, + }, + }); + + await this.queryInterface.createTable('actors', { + id: { + type: DataTypes.INTEGER, + primaryKey: true, + autoIncrement: true, + }, + name: { + type: DataTypes.STRING, + allowNull: true, + }, + level_id: { + type: DataTypes.INTEGER, + allowNull: false, + references: { + key: 'id', + model: 'level', + }, + onDelete: 'CASCADE', + onUpdate: 'CASCADE', + }, + }); + + const levels = [{ + id: 1, + name: 'L1', + }, { + id: 2, + name: 'L2', + }, + { + id: 3, + name: 'L3', + }]; + + const actors = [ + { + name: 'Keanu Reeves', + level_id: 2, + }, + { + name: 'Laurence Fishburne', + level_id: 1, + }, + ]; + + await Promise.all([ + this.queryInterface.bulkInsert('level', levels), + this.queryInterface.bulkInsert('actors', actors), + ]); + + await this.queryInterface.removeColumn('level', 'name'); + + const actorRows = await this.queryInterface.sequelize.query('SELECT * from actors;', { + type: 'SELECT', + }); + + expect(actorRows).to.have.length(actors.length, 'actors records should be unaffected'); + }); + + it('should retain ON UPDATE and ON DELETE constraints after a column is removed', async function () { + await this.queryInterface.createTable('level', { + id: { + type: DataTypes.INTEGER, + primaryKey: true, + autoIncrement: true, + }, + name: { + type: DataTypes.CHAR, + allowNull: false, + }, + }); + + await this.queryInterface.createTable('actors', { + id: { + type: DataTypes.INTEGER, + primaryKey: true, + autoIncrement: true, + }, + name: { + type: DataTypes.STRING, + allowNull: true, + }, + level_id: { + type: DataTypes.INTEGER, + allowNull: false, + references: { + key: 'id', + model: 'level', + }, + onDelete: 'CASCADE', + onUpdate: 'CASCADE', + }, + }); + + await this.queryInterface.removeColumn('actors', 'name'); + + const constraintsQuery = this.queryInterface.queryGenerator.showConstraintsQuery('actors'); + const [{ sql: actorsSql }] = await this.queryInterface.sequelize.query(constraintsQuery, { + type: 'SELECT', + }); + + expect(actorsSql).to.include('ON DELETE CASCADE', 'should include ON DELETE constraint'); + expect(actorsSql).to.include('ON UPDATE CASCADE', 'should include ON UPDATE constraint'); + }); + } }); describe('(with a schema)', () => { diff --git a/test/unit/dialects/sqlite/query-generator.test.js b/test/unit/dialects/sqlite/query-generator.test.js index 9a9c6d45e186..c18e85c13f4e 100644 --- a/test/unit/dialects/sqlite/query-generator.test.js +++ b/test/unit/dialects/sqlite/query-generator.test.js @@ -633,9 +633,7 @@ if (dialect === 'sqlite') { 'CREATE TABLE IF NOT EXISTS `myTable_backup` (`commit` VARCHAR(255), `bar` VARCHAR(255));' + 'INSERT INTO `myTable_backup` SELECT `commit`, `bar` FROM `myTable`;' + 'DROP TABLE `myTable`;' - + 'CREATE TABLE IF NOT EXISTS `myTable` (`commit` VARCHAR(255), `bar` VARCHAR(255));' - + 'INSERT INTO `myTable` SELECT `commit`, `bar` FROM `myTable_backup`;' - + 'DROP TABLE `myTable_backup`;', + + 'ALTER TABLE `myTable_backup` RENAME TO `myTable`;', }, ], getForeignKeysQuery: [ @@ -645,6 +643,18 @@ if (dialect === 'sqlite') { expectation: 'PRAGMA foreign_key_list(`myTable`)', }, ], + foreignKeyCheckQuery: [ + { + title: 'Properly quotes table names', + arguments: ['myTable'], + expectation: 'PRAGMA foreign_key_check(`myTable`);', + }, + { + title: 'Properly quotes table names as schema', + arguments: [{ schema: 'schema', tableName: 'myTable' }], + expectation: 'PRAGMA foreign_key_check(`schema.myTable`);', + }, + ], }; _.each(suites, (tests, suiteTitle) => { diff --git a/test/unit/sql/remove-column.test.js b/test/unit/sql/remove-column.test.js index 774c881ec745..57c2e41c332d 100644 --- a/test/unit/sql/remove-column.test.js +++ b/test/unit/sql/remove-column.test.js @@ -43,7 +43,7 @@ describe(Support.getTestDialectTeaser('SQL'), () => { mysql: 'ALTER TABLE `user` DROP `email`;', postgres: 'ALTER TABLE "custom"."user" DROP COLUMN "email";', snowflake: 'ALTER TABLE "user" DROP "email";', - sqlite: 'CREATE TABLE IF NOT EXISTS `user_backup` (`0` e, `1` m, `2` a, `3` i, `4` l);INSERT INTO `user_backup` SELECT `0`, `1`, `2`, `3`, `4` FROM `user`;DROP TABLE `user`;CREATE TABLE IF NOT EXISTS `user` (`0` e, `1` m, `2` a, `3` i, `4` l);INSERT INTO `user` SELECT `0`, `1`, `2`, `3`, `4` FROM `user_backup`;DROP TABLE `user_backup`;', + sqlite: 'CREATE TABLE IF NOT EXISTS `user_backup` (`0` e, `1` m, `2` a, `3` i, `4` l);INSERT INTO `user_backup` SELECT `0`, `1`, `2`, `3`, `4` FROM `user`;DROP TABLE `user`;ALTER TABLE `user_backup` RENAME TO `user`;', }); }); });