Skip to content

Commit

Permalink
lib: Prevent error when changing columns or keys with foreign keys
Browse files Browse the repository at this point in the history
  • Loading branch information
@nwoltman
nwoltman committed Feb 9, 2017
1 parent 5f0a2ec commit d43235a
Show file tree
Hide file tree
Showing 5 changed files with 153 additions and 32 deletions.
4 changes: 2 additions & 2 deletions lib/Operation.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ const Operation = {
ADD_FOREIGN_KEY: 11,
},

create(type, sql) {
return {sql, type};
create(type, sql, columns) {
return {columns, sql, type};
},
};

Expand Down
79 changes: 67 additions & 12 deletions lib/TableDefinition.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,13 @@ class TableDefinition {
}

_getMigrateTableOperations(oldSchema) {
return this._getMigrateColumnsOperations(oldSchema).concat(
const operations = this._getMigrateColumnsOperations(oldSchema).concat(
this._getMigratePrimaryKeyOperations(oldSchema),
this._getMigrateUniqueKeysOperations(oldSchema),
this._getMigrateIndexesOperations(oldSchema),
this._getMigrateForeignKeysOperations(oldSchema),
this._getMigrateIndexesOperations(oldSchema)
);
return operations.concat(
this._getMigrateForeignKeysOperations(oldSchema, operations),
this._getMigrateTableOptionsOperations(oldSchema)
);
}
Expand Down Expand Up @@ -133,7 +135,8 @@ class TableDefinition {
operations.push(Operation.create(
Operation.Types.MODIFY_COLUMN,
'ALTER TABLE ' + escapedName + ' MODIFY COLUMN ' + pool.escapeId(columnName) +
' ' + newColumnDefinition.$toSQL()
' ' + newColumnDefinition.$toSQL(),
[columnName]
));
}
}
Expand All @@ -160,7 +163,8 @@ class TableDefinition {
if (oldSchema.primaryKey) {
operations.push(Operation.create(
Operation.Types.DROP_KEY,
'ALTER TABLE ' + this._escapedName + ' DROP PRIMARY KEY'
'ALTER TABLE ' + this._escapedName + ' DROP PRIMARY KEY',
oldSchema.primaryKey
));
}
if (newSchema.primaryKey) {
Expand All @@ -186,7 +190,8 @@ class TableDefinition {
const keyName = this._createKeyName(KEY_TYPES.UNIQUE, removedKeys[i]);
operations.push(Operation.create(
Operation.Types.DROP_KEY,
'ALTER TABLE ' + this._escapedName + ' DROP KEY ' + this._pool.escapeId(keyName)
'ALTER TABLE ' + this._escapedName + ' DROP KEY ' + this._pool.escapeId(keyName),
removedKeys[i]
));
}

Expand All @@ -212,7 +217,8 @@ class TableDefinition {
const keyName = this._createKeyName(KEY_TYPES.INDEX, removedKeys[i]);
operations.push(Operation.create(
Operation.Types.DROP_KEY,
'ALTER TABLE ' + this._escapedName + ' DROP KEY ' + this._pool.escapeId(keyName)
'ALTER TABLE ' + this._escapedName + ' DROP KEY ' + this._pool.escapeId(keyName),
removedKeys[i]
));
}

Expand All @@ -227,18 +233,24 @@ class TableDefinition {
return operations;
}

_getMigrateForeignKeysOperations(oldSchema) {
_getMigrateForeignKeysOperations(oldSchema, otherOperations) {
const operations = [];
const oldForeignKeys = oldSchema.foreignKeys;
const newForeignKeys = this._schema.foreignKeys;
var mustAddBackKey = false;
var columnNames;

for (columnNames in oldForeignKeys) {
const columnNamesArray = columnNames.split(','); // Don't need /\s*,\s*/ regex since string was created internally

if (isEqual(oldForeignKeys[columnNames], newForeignKeys[columnNames])) {
continue;
if (!mustAvoidforeignKeyConflict(columnNamesArray, otherOperations)) {
continue;
}
mustAddBackKey = true;
}
columnNames = columnNames.split(','); // Don't need /\s*,\s*/ regex since this value was created internally
const keyName = this._createKeyName(KEY_TYPES.FOREIGN, columnNames);

const keyName = this._createKeyName(KEY_TYPES.FOREIGN, columnNamesArray);
operations.push(Operation.create(
Operation.Types.DROP_FOREIGN_KEY,
'ALTER TABLE ' + this._escapedName + ' DROP FOREIGN KEY ' + this._pool.escapeId(keyName)
Expand All @@ -247,7 +259,7 @@ class TableDefinition {

for (columnNames in newForeignKeys) {
const newForeignKeyData = newForeignKeys[columnNames];
if (isEqual(newForeignKeyData, oldForeignKeys[columnNames])) {
if (!mustAddBackKey && isEqual(newForeignKeyData, oldForeignKeys[columnNames])) {
continue;
}
const keySQL = this._generateForeignKeySQL(columnNames, newForeignKeyData);
Expand Down Expand Up @@ -744,4 +756,47 @@ function generateForegnKeysSchema(createDefinitions) {
return isEmpty(foreignKeys) ? null : foreignKeys;
}

function arraysEqual(a, b) {
if (a.length !== b.length) {
return false;
}

for (var i = 0; i < a.length; i++) {
if (a[i] !== b[i]) {
return false;
}
}

return true;
}

// Check if a foreign key needs to be removed before performing other operations
function mustAvoidforeignKeyConflict(fkColumnNames, operations) {
for (var i = 0; i < operations.length; i++) {
const operation = operations[i];

if (operation.type === Operation.Types.MODIFY_COLUMN) {
const columnName = operation.columns[0];

if (fkColumnNames.indexOf(columnName) >= 0) {
return true;
}

continue;
}

if (operation.type === Operation.Types.DROP_KEY) { // Check if the key columns are identical
const opColumnNames = typeof operation.columns === 'string'
? operation.columns.split(/\s*,\s*/)
: operation.columns;

if (arraysEqual(fkColumnNames, opColumnNames)) {
return true;
}
}
}

return false;
}

module.exports = TableDefinition;
80 changes: 67 additions & 13 deletions test/integration/MySQLPlus.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ describe('MySQLPlus', function() {
neverchange: ColTypes.tinyint().oldName('fake_column'),
norename: ColTypes.tinyint().oldName('fake_column'),
},
indexes: ['email'],
indexes: ['email', ['id', 'email']],
};
const columnsTableExpectedSQL =
'CREATE TABLE `columns_table` (\n' +
Expand All @@ -150,7 +150,8 @@ describe('MySQLPlus', function() {
' `norename` tinyint(4) DEFAULT NULL,\n' +
' PRIMARY KEY (`id`),\n' +
' UNIQUE KEY `unique_columns_table_uuid` (`uuid`),\n' +
' KEY `index_columns_table_email` (`email`)\n' +
' KEY `index_columns_table_email` (`email`),\n' +
' KEY `index_columns_table_id_email` (`id`,`email`)\n' +
') ENGINE=InnoDB DEFAULT CHARSET=utf8';

const columnsTableMigratedSchema = {
Expand All @@ -165,6 +166,7 @@ describe('MySQLPlus', function() {
norename: ColTypes.smallint().oldName('fake_column'),
added: ColTypes.text(),
},
indexes: [['id', 'email']],
uniqueKeys: ['email'],
};
const columnsTableMigratedExpectedSQL =
Expand All @@ -180,7 +182,8 @@ describe('MySQLPlus', function() {
' `added` text,\n' +
' PRIMARY KEY (`id`),\n' +
' UNIQUE KEY `unique_columns_table_email` (`email`),\n' +
' UNIQUE KEY `unique_columns_table_uuid` (`uuid`)\n' +
' UNIQUE KEY `unique_columns_table_uuid` (`uuid`),\n' +
' KEY `index_columns_table_id_email` (`id`,`email`)\n' +
') ENGINE=InnoDB DEFAULT CHARSET=utf8';

const primaryKeyTableName = 'pk_table';
Expand Down Expand Up @@ -300,21 +303,35 @@ describe('MySQLPlus', function() {
a: ColTypes.bigint().unsigned(),
b: ColTypes.bigint().unsigned(),
c: ColTypes.bigint().unsigned(),
d: ColTypes.int().unsigned().notNull(),
ai: ColTypes.int(),
bi: ColTypes.bigint(),
eb: ColTypes.varchar(63),
fb: ColTypes.char(1).default('a'),
gc: ColTypes.int().unsigned().notNull(),
hc: ColTypes.char(255),
},
indexes: ['b', 'c', ['ai', 'bi']],
indexes: ['b', 'c', 'd', ['ai', 'bi'], ['eb', 'fb'], ['gc', 'hc']],
foreignKeys: {
'ai, bi': {
table: 'indexes_table',
column: ['a', 'b'],
},
b: 'big_table.id',
c: {
table: 'big_table',
column: 'id',
onDelete: 'CASCADE',
onUpdate: 'NO ACTION',
},
'ai, bi': {
table: 'indexes_table',
column: ['a', 'b'],
d: 'columns_table.id',
'eb, fb': {
table: 'big_table',
column: ['name', 'letter'],
},
'gc, hc': {
table: 'columns_table',
column: ['id', 'email'],
},
},
};
Expand All @@ -323,36 +340,62 @@ describe('MySQLPlus', function() {
' `a` bigint(20) unsigned DEFAULT NULL,\n' +
' `b` bigint(20) unsigned DEFAULT NULL,\n' +
' `c` bigint(20) unsigned DEFAULT NULL,\n' +
' `d` int(10) unsigned NOT NULL,\n' +
' `ai` int(11) DEFAULT NULL,\n' +
' `bi` bigint(20) DEFAULT NULL,\n' +
' `eb` varchar(63) DEFAULT NULL,\n' +
' `fb` char(1) DEFAULT \'a\',\n' +
' `gc` int(10) unsigned NOT NULL,\n' +
' `hc` char(255) DEFAULT NULL,\n' +
' KEY `index_fk_table_b` (`b`),\n' +
' KEY `index_fk_table_c` (`c`),\n' +
' KEY `index_fk_table_d` (`d`),\n' +
' KEY `index_fk_table_ai_bi` (`ai`,`bi`),\n' +
' KEY `index_fk_table_eb_fb` (`eb`,`fb`),\n' +
' KEY `index_fk_table_gc_hc` (`gc`,`hc`),\n' +
' CONSTRAINT `fk_fk_table_ai_bi` FOREIGN KEY (`ai`, `bi`) REFERENCES `indexes_table` (`a`, `b`),\n' +
' CONSTRAINT `fk_fk_table_b` FOREIGN KEY (`b`) REFERENCES `big_table` (`id`),\n' +
' CONSTRAINT `fk_fk_table_c` FOREIGN KEY (`c`) REFERENCES `big_table` (`id`) ON DELETE CASCADE ON UPDATE NO ACTION\n' +
' CONSTRAINT `fk_fk_table_c` FOREIGN KEY (`c`) REFERENCES `big_table` (`id`) ON DELETE CASCADE ON UPDATE NO ACTION,\n' +
' CONSTRAINT `fk_fk_table_d` FOREIGN KEY (`d`) REFERENCES `columns_table` (`id`),\n' +
' CONSTRAINT `fk_fk_table_eb_fb` FOREIGN KEY (`eb`, `fb`) REFERENCES `big_table` (`name`, `letter`),\n' +
' CONSTRAINT `fk_fk_table_gc_hc` FOREIGN KEY (`gc`, `hc`) REFERENCES `columns_table` (`id`, `email`)\n' +
') ENGINE=InnoDB DEFAULT CHARSET=utf8';

const foreignKeysTableMigratedSchema = {
columns: {
a: ColTypes.bigint().unsigned(),
b: ColTypes.bigint().unsigned(),
c: ColTypes.bigint().unsigned(),
d: ColTypes.bigint(5).unsigned().notNull(),
ai: ColTypes.int(),
ci: ColTypes.char(1),
eb: ColTypes.varchar(63),
fb: ColTypes.char(1).default('a'),
gc: ColTypes.bigint(5).unsigned().notNull(),
hc: ColTypes.varchar(255).notNull(),
},
indexes: ['a', 'c', ['ai', 'ci']],
indexes: ['a', 'c', 'd', ['ai', 'ci'], ['gc', 'hc']],
uniqueKeys: [['eb', 'fb']],
foreignKeys: {
a: 'big_table.id',
'ai, ci': {
table: 'indexes_table',
column: ['a', 'c'],
},
c: {
table: 'big_table',
column: 'id',
onDelete: 'SET NULL',
onUpdate: 'CASCADE',
},
'ai, ci': {
table: 'indexes_table',
column: ['a', 'c'],
d: 'columns_table.id',
'eb, fb': {
table: 'big_table',
column: ['name', 'letter'],
},
'gc, hc': {
table: 'columns_table',
column: ['id', 'email'],
},
},
};
Expand All @@ -361,14 +404,25 @@ describe('MySQLPlus', function() {
' `a` bigint(20) unsigned DEFAULT NULL,\n' +
' `b` bigint(20) unsigned DEFAULT NULL,\n' +
' `c` bigint(20) unsigned DEFAULT NULL,\n' +
' `d` bigint(5) unsigned NOT NULL,\n' +
' `ai` int(11) DEFAULT NULL,\n' +
' `eb` varchar(63) DEFAULT NULL,\n' +
' `fb` char(1) DEFAULT \'a\',\n' +
' `gc` bigint(5) unsigned NOT NULL,\n' +
' `hc` varchar(255) NOT NULL,\n' +
' `ci` char(1) DEFAULT NULL,\n' +
' UNIQUE KEY `unique_fk_table_eb_fb` (`eb`,`fb`),\n' +
' KEY `index_fk_table_c` (`c`),\n' +
' KEY `index_fk_table_d` (`d`),\n' +
' KEY `index_fk_table_gc_hc` (`gc`,`hc`),\n' +
' KEY `index_fk_table_a` (`a`),\n' +
' KEY `index_fk_table_ai_ci` (`ai`,`ci`),\n' +
' CONSTRAINT `fk_fk_table_a` FOREIGN KEY (`a`) REFERENCES `big_table` (`id`),\n' +
' CONSTRAINT `fk_fk_table_ai_ci` FOREIGN KEY (`ai`, `ci`) REFERENCES `indexes_table` (`a`, `c`),\n' +
' CONSTRAINT `fk_fk_table_c` FOREIGN KEY (`c`) REFERENCES `big_table` (`id`) ON DELETE SET NULL ON UPDATE CASCADE\n' +
' CONSTRAINT `fk_fk_table_c` FOREIGN KEY (`c`) REFERENCES `big_table` (`id`) ON DELETE SET NULL ON UPDATE CASCADE,\n' +
' CONSTRAINT `fk_fk_table_d` FOREIGN KEY (`d`) REFERENCES `columns_table` (`id`),\n' +
' CONSTRAINT `fk_fk_table_eb_fb` FOREIGN KEY (`eb`, `fb`) REFERENCES `big_table` (`name`, `letter`),\n' +
' CONSTRAINT `fk_fk_table_gc_hc` FOREIGN KEY (`gc`, `hc`) REFERENCES `columns_table` (`id`, `email`)\n' +
') ENGINE=InnoDB DEFAULT CHARSET=utf8';

const optionsTableName = 'options_table';
Expand Down
5 changes: 4 additions & 1 deletion test/unit/Operation.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,17 @@ describe('Operation', () => {
).should.deepEqual({
type: Operation.Types.ADD_COLUMN,
sql: 'some SQL',
columns: undefined,
});

Operation.create(
Operation.Types.MODIFY_TABLE_OPTIONS,
'SQL'
'SQL',
['column', 'names']
).should.deepEqual({
type: Operation.Types.MODIFY_TABLE_OPTIONS,
sql: 'SQL',
columns: ['column', 'names'],
});
});

Expand Down
17 changes: 13 additions & 4 deletions test/unit/TableDefinition.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,10 @@ describe('TableDefinition', () => {
describe('if the table already exists', () => {

const newSchema = {
columns: {id: MySQLPlus.ColTypes.int().unsigned().notNull()},
columns: {
id: MySQLPlus.ColTypes.int().unsigned().notNull(),
newCol: MySQLPlus.ColTypes.tinyint(),
},
};

describe('and the migration strategy is "safe"', () => {
Expand All @@ -133,11 +136,17 @@ describe('TableDefinition', () => {
const expectedOperations = [
Operation.create(
Operation.Types.MODIFY_COLUMN,
'ALTER TABLE `' + existingTableName + '` MODIFY COLUMN `id` int unsigned NOT NULL'
'ALTER TABLE `' + existingTableName + '` MODIFY COLUMN `id` int unsigned NOT NULL',
['id']
),
Operation.create(
Operation.Types.ADD_COLUMN,
'ALTER TABLE `' + existingTableName + '` ADD COLUMN `newCol` tinyint'
),
Operation.create(
Operation.Types.DROP_KEY,
'ALTER TABLE `' + existingTableName + '` DROP PRIMARY KEY'
'ALTER TABLE `' + existingTableName + '` DROP PRIMARY KEY',
'id'
),
];
new TableDefinition(existingTableName, newSchema, mockPool, 'alter')
Expand All @@ -159,7 +168,7 @@ describe('TableDefinition', () => {
),
Operation.create(
Operation.Types.CREATE_TABLE,
'CREATE TABLE `' + existingTableName + '` (`id` int unsigned NOT NULL)'
'CREATE TABLE `' + existingTableName + '` (`id` int unsigned NOT NULL,`newCol` tinyint)'
),
];
new TableDefinition(existingTableName, newSchema, mockPool, 'drop')
Expand Down

0 comments on commit d43235a

Please sign in to comment.