From 2de3377dab13959c4073515ce29287de14d4221e Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Sun, 28 Jun 2020 00:22:19 -0400 Subject: [PATCH] fix(sqlite): describeTable now returns unique constraint and references (#12420) This will allow changeColumn/renameColumn/removeColumn to preserve constraints and references --- lib/dialects/sqlite/query-generator.js | 3 +- lib/dialects/sqlite/query-interface.js | 67 +++++++++- test/integration/model.test.js | 12 +- test/integration/model/sync.test.js | 9 +- .../query-interface/changeColumn.test.js | 120 ++++++++++++++++++ .../dialects/sqlite/query-generator.test.js | 2 +- 6 files changed, 206 insertions(+), 7 deletions(-) diff --git a/lib/dialects/sqlite/query-generator.js b/lib/dialects/sqlite/query-generator.js index 9d9691fe700e..8997bb6a0e9a 100644 --- a/lib/dialects/sqlite/query-generator.js +++ b/lib/dialects/sqlite/query-generator.js @@ -418,7 +418,8 @@ class SQLiteQueryGenerator extends MySqlQueryGenerator { ).join(', '); const attributeNamesExport = Object.keys(attributes).map(attr => this.quoteIdentifier(attr)).join(', '); - return `${this.createTableQuery(backupTableName, attributes).replace('CREATE TABLE', 'CREATE TEMPORARY TABLE') + // Temporary tables don't support foreign keys, so creating a temporary table will not allow foreign keys to be preserved + return `${this.createTableQuery(backupTableName, attributes) }INSERT INTO ${quotedBackupTableName} SELECT ${attributeNamesImport} FROM ${quotedTableName};` + `DROP TABLE ${quotedTableName};${ this.createTableQuery(tableName, attributes) diff --git a/lib/dialects/sqlite/query-interface.js b/lib/dialects/sqlite/query-interface.js index 8f31bd793ad2..ca2b68653d0f 100644 --- a/lib/dialects/sqlite/query-interface.js +++ b/lib/dialects/sqlite/query-interface.js @@ -4,6 +4,7 @@ const sequelizeErrors = require('../../errors'); const QueryTypes = require('../../query-types'); const { QueryInterface } = require('../abstract/query-interface'); const { cloneDeep } = require('../../utils'); +const _ = require('lodash'); /** * The interface that Sequelize uses to talk with SQLite database @@ -39,7 +40,7 @@ class SQLiteQueryInterface extends QueryInterface { options = options || {}; const fields = await this.describeTable(tableName, options); - fields[attributeName] = this.normalizeAttribute(dataTypeOrOptions); + Object.assign(fields[attributeName], this.normalizeAttribute(dataTypeOrOptions)); const sql = this.queryGenerator.removeColumnQuery(tableName, fields); const subQueries = sql.split(';').filter(q => q !== ''); @@ -168,6 +169,70 @@ class SQLiteQueryInterface extends QueryInterface { await this._dropAllTables(tableNames, skip, options); await this.sequelize.query('PRAGMA foreign_keys = ON', options); } + + /** + * @override + */ + async describeTable(tableName, options) { + let schema = null; + let schemaDelimiter = null; + + if (typeof options === 'string') { + schema = options; + } else if (typeof options === 'object' && options !== null) { + schema = options.schema || null; + schemaDelimiter = options.schemaDelimiter || null; + } + + if (typeof tableName === 'object' && tableName !== null) { + schema = tableName.schema; + tableName = tableName.tableName; + } + + const sql = this.queryGenerator.describeTableQuery(tableName, schema, schemaDelimiter); + options = { ...options, type: QueryTypes.DESCRIBE }; + const sqlIndexes = this.queryGenerator.showIndexesQuery(tableName); + + try { + const data = await this.sequelize.query(sql, options); + /* + * If no data is returned from the query, then the table name may be wrong. + * Query generators that use information_schema for retrieving table info will just return an empty result set, + * it will not throw an error like built-ins do (e.g. DESCRIBE on MySql). + */ + if (_.isEmpty(data)) { + throw new Error(`No description found for "${tableName}" table. Check the table name and schema; remember, they _are_ case sensitive.`); + } + + const indexes = await this.sequelize.query(sqlIndexes, options); + for (const prop in data) { + data[prop].unique = false; + } + for (const index of indexes) { + for (const field of index.fields) { + if (index.unique !== undefined) { + data[field.attribute].unique = index.unique; + } + } + } + + const foreignKeys = await this.getForeignKeyReferencesForTable(tableName, options); + for (const foreignKey of foreignKeys) { + data[foreignKey.columnName].references = { + model: foreignKey.referencedTableName, + key: foreignKey.referencedColumnName + }; + } + + return data; + } catch (e) { + if (e.original && e.original.code === 'ER_NO_SUCH_TABLE') { + throw new Error(`No description found for "${tableName}" table. Check the table name and schema; remember, they _are_ case sensitive.`); + } + + throw e; + } + } } exports.SQLiteQueryInterface = SQLiteQueryInterface; diff --git a/test/integration/model.test.js b/test/integration/model.test.js index c52b228ac107..444d277fb92a 100644 --- a/test/integration/model.test.js +++ b/test/integration/model.test.js @@ -2014,7 +2014,11 @@ describe(Support.getTestDialectTeaser('Model'), () => { let table = await this.sequelize.queryInterface.describeTable('Publics', { logging(sql) { - if (['sqlite', 'mysql', 'mssql', 'mariadb'].includes(dialect)) { + if (dialect === 'sqlite' && sql.includes('TABLE_INFO')) { + test++; + expect(sql).to.not.contain('special'); + } + else if (['mysql', 'mssql', 'mariadb'].includes(dialect)) { test++; expect(sql).to.not.contain('special'); } @@ -2029,7 +2033,11 @@ describe(Support.getTestDialectTeaser('Model'), () => { table = await this.sequelize.queryInterface.describeTable('Publics', { schema: 'special', logging(sql) { - if (['sqlite', 'mysql', 'mssql', 'mariadb'].includes(dialect)) { + if (dialect === 'sqlite' && sql.includes('TABLE_INFO')) { + test++; + expect(sql).to.contain('special'); + } + else if (['mysql', 'mssql', 'mariadb'].includes(dialect)) { test++; expect(sql).to.contain('special'); } diff --git a/test/integration/model/sync.test.js b/test/integration/model/sync.test.js index 630fd9baaa73..c615efc78595 100644 --- a/test/integration/model/sync.test.js +++ b/test/integration/model/sync.test.js @@ -215,12 +215,16 @@ describe(Support.getTestDialectTeaser('Model'), () => { const results = await this.sequelize.getQueryInterface().showIndex(User.getTableName()); if (dialect === 'sqlite') { // SQLite doesn't treat primary key as index - expect(results).to.have.length(4); + // However it does create an extra "autoindex", except primary == false + expect(results).to.have.length(4 + 1); } else { expect(results).to.have.length(4 + 1); expect(results.filter(r => r.primary)).to.have.length(1); } + if (dialect === 'sqlite') { + expect(results.filter(r => r.name === 'sqlite_autoindex_testSyncs_1')).to.have.length(1); + } expect(results.filter(r => r.name === 'another_index_email_mobile')).to.have.length(1); expect(results.filter(r => r.name === 'another_index_phone_mobile')).to.have.length(1); expect(results.filter(r => r.name === 'another_index_email')).to.have.length(1); @@ -254,7 +258,8 @@ describe(Support.getTestDialectTeaser('Model'), () => { const results = await this.sequelize.getQueryInterface().showIndex(User.getTableName()); if (dialect === 'sqlite') { // SQLite doesn't treat primary key as index - expect(results).to.have.length(4); + // However it does create an extra "autoindex", except primary == false + expect(results).to.have.length(4 + 1); } else { expect(results).to.have.length(4 + 1); expect(results.filter(r => r.primary)).to.have.length(1); diff --git a/test/integration/query-interface/changeColumn.test.js b/test/integration/query-interface/changeColumn.test.js index 82817a08b1b1..7bc12b77a2d8 100644 --- a/test/integration/query-interface/changeColumn.test.js +++ b/test/integration/query-interface/changeColumn.test.js @@ -233,5 +233,125 @@ describe(Support.getTestDialectTeaser('QueryInterface'), () => { }); }); } + + if (dialect === 'sqlite') { + it('should not remove unique constraints when adding or modifying columns', async function() { + await this.queryInterface.createTable({ + tableName: 'Foos' + }, { + id: { + allowNull: false, + autoIncrement: true, + primaryKey: true, + type: DataTypes.INTEGER + }, + name: { + allowNull: false, + unique: true, + type: DataTypes.STRING + }, + email: { + allowNull: false, + unique: true, + type: DataTypes.STRING + } + }); + + await this.queryInterface.addColumn('Foos', 'phone', { + type: DataTypes.STRING, + defaultValue: null, + allowNull: true + }); + + let table = await this.queryInterface.describeTable({ + tableName: 'Foos' + }); + expect(table.phone.allowNull).to.equal(true, '(1) phone column should allow null values'); + expect(table.phone.defaultValue).to.equal(null, '(1) phone column should have a default value of null'); + expect(table.email.unique).to.equal(true, '(1) email column should remain unique'); + expect(table.name.unique).to.equal(true, '(1) name column should remain unique'); + + await this.queryInterface.changeColumn('Foos', 'email', { + type: DataTypes.STRING, + allowNull: true + }); + + table = await this.queryInterface.describeTable({ + tableName: 'Foos' + }); + expect(table.email.allowNull).to.equal(true, '(2) email column should allow null values'); + expect(table.email.unique).to.equal(true, '(2) email column should remain unique'); + expect(table.name.unique).to.equal(true, '(2) name column should remain unique'); + }); + + it('should add unique constraints to 2 columns and keep allowNull', async function() { + await this.queryInterface.createTable({ + tableName: 'Foos' + }, { + id: { + allowNull: false, + autoIncrement: true, + primaryKey: true, + type: DataTypes.INTEGER + }, + name: { + allowNull: false, + type: DataTypes.STRING + }, + email: { + allowNull: true, + type: DataTypes.STRING + } + }); + + await this.queryInterface.changeColumn('Foos', 'name', { + type: DataTypes.STRING, + unique: true + }); + await this.queryInterface.changeColumn('Foos', 'email', { + type: DataTypes.STRING, + unique: true + }); + + const table = await this.queryInterface.describeTable({ + tableName: 'Foos' + }); + expect(table.name.allowNull).to.equal(false); + expect(table.name.unique).to.equal(true); + expect(table.email.allowNull).to.equal(true); + expect(table.email.unique).to.equal(true); + }); + + it('should not remove foreign keys when adding or modifying columns', async function() { + const Task = this.sequelize.define('Task', { title: DataTypes.STRING }), + User = this.sequelize.define('User', { username: DataTypes.STRING }); + + User.hasOne(Task); + + await User.sync({ force: true }); + await Task.sync({ force: true }); + + await this.queryInterface.addColumn('Tasks', 'bar', DataTypes.INTEGER); + let refs = await this.queryInterface.getForeignKeyReferencesForTable('Tasks'); + expect(refs.length).to.equal(1, 'should keep foreign key after adding column'); + expect(refs[0].columnName).to.equal('UserId'); + expect(refs[0].referencedTableName).to.equal('Users'); + expect(refs[0].referencedColumnName).to.equal('id'); + + await this.queryInterface.changeColumn('Tasks', 'bar', DataTypes.STRING); + refs = await this.queryInterface.getForeignKeyReferencesForTable('Tasks'); + expect(refs.length).to.equal(1, 'should keep foreign key after changing column'); + expect(refs[0].columnName).to.equal('UserId'); + expect(refs[0].referencedTableName).to.equal('Users'); + expect(refs[0].referencedColumnName).to.equal('id'); + + await this.queryInterface.renameColumn('Tasks', 'bar', 'foo'); + refs = await this.queryInterface.getForeignKeyReferencesForTable('Tasks'); + expect(refs.length).to.equal(1, 'should keep foreign key after renaming column'); + expect(refs[0].columnName).to.equal('UserId'); + expect(refs[0].referencedTableName).to.equal('Users'); + expect(refs[0].referencedColumnName).to.equal('id'); + }); + } }); }); diff --git a/test/unit/dialects/sqlite/query-generator.test.js b/test/unit/dialects/sqlite/query-generator.test.js index 062352d78b08..a3dd0dd51440 100644 --- a/test/unit/dialects/sqlite/query-generator.test.js +++ b/test/unit/dialects/sqlite/query-generator.test.js @@ -607,7 +607,7 @@ if (dialect === 'sqlite') { title: 'Properly quotes column names', arguments: ['myTable', 'foo', 'commit', { commit: 'VARCHAR(255)', bar: 'VARCHAR(255)' }], expectation: - 'CREATE TEMPORARY TABLE IF NOT EXISTS `myTable_backup` (`commit` VARCHAR(255), `bar` VARCHAR(255));' + + 'CREATE TABLE IF NOT EXISTS `myTable_backup` (`commit` VARCHAR(255), `bar` VARCHAR(255));' + 'INSERT INTO `myTable_backup` SELECT `foo` AS `commit`, `bar` FROM `myTable`;' + 'DROP TABLE `myTable`;' + 'CREATE TABLE IF NOT EXISTS `myTable` (`commit` VARCHAR(255), `bar` VARCHAR(255));' +