Skip to content

Commit

Permalink
fix(sqlite): describeTable now returns unique constraint and referenc…
Browse files Browse the repository at this point in the history
…es (#12420)

This will allow changeColumn/renameColumn/removeColumn to preserve constraints and references
  • Loading branch information
dyc3 committed Jun 28, 2020
1 parent 9c446f9 commit 2de3377
Show file tree
Hide file tree
Showing 6 changed files with 206 additions and 7 deletions.
3 changes: 2 additions & 1 deletion lib/dialects/sqlite/query-generator.js
Expand Up @@ -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)
Expand Down
67 changes: 66 additions & 1 deletion lib/dialects/sqlite/query-interface.js
Expand Up @@ -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
Expand Down Expand Up @@ -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 !== '');
Expand Down Expand Up @@ -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;
12 changes: 10 additions & 2 deletions test/integration/model.test.js
Expand Up @@ -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');
}
Expand All @@ -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');
}
Expand Down
9 changes: 7 additions & 2 deletions test/integration/model/sync.test.js
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
120 changes: 120 additions & 0 deletions test/integration/query-interface/changeColumn.test.js
Expand Up @@ -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');
});
}
});
});
2 changes: 1 addition & 1 deletion test/unit/dialects/sqlite/query-generator.test.js
Expand Up @@ -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));' +
Expand Down

0 comments on commit 2de3377

Please sign in to comment.