From 5ba6687c7c46090f86cf0d512bbbf6e51f15c605 Mon Sep 17 00:00:00 2001 From: Matthew Dickinson Date: Tue, 18 Oct 2016 15:53:05 -0400 Subject: [PATCH] Fix autoupdate to create foreign keys after tables Updated the autoupdate function so that it collects foreign key statements and runs them once all tables have been updated. This prevents foreign keys being created before referenced tables. --- lib/migration.js | 142 +++++++++++++++++++++++---------- test/mysql.autoupdate.test.js | 146 ++++++++++++++++++++++++++++------ 2 files changed, 224 insertions(+), 64 deletions(-) diff --git a/lib/migration.js b/lib/migration.js index 7449ceb..ae5f616 100644 --- a/lib/migration.js +++ b/lib/migration.js @@ -21,6 +21,7 @@ function mixinMigration(MySQL, mysql) { */ MySQL.prototype.autoupdate = function(models, cb) { var self = this; + var foreignKeyStatements = []; if ((!cb) && ('function' === typeof models)) { cb = models; @@ -50,14 +51,46 @@ function mixinMigration(MySQL, mysql) { if (err) console.log('Failed to discover "' + table + '" foreign keys', err); if (!err && fields && fields.length) { - self.alterTable(model, fields, indexes, foreignKeys, done); + //if we already have a definition, update this table + self.alterTable(model, fields, indexes, foreignKeys, function(err, changed, res) { + //check to see if there were any new foreign keys for this table. + //If so, we'll create them once all tables have been updated + if (!err && res && res.newFks && res.newFks.length) { + foreignKeyStatements.push(res.newFks); + } + + done(err); + }); } else { - self.createTable(model, done); + //if there is not yet a definition, create this table + var res = self.createTable(model, function(err) { + if (!err) { + //get a list of the alter statements needed to add the defined foreign keys + var newFks = self.getForeignKeySQL(model, foreignKeys); + + //check to see if there were any new foreign keys for this table. + //If so, we'll create them once all tables have been updated + if (newFks && newFks.length) { + foreignKeyStatements.push(self.getAlterStatement(model, newFks)); + } + } + + done(err); + }); } }); }); }); - }, cb); + }, function(err) { + if (err) return cb(err); + + //add any new foreign keys + async.each(foreignKeyStatements, function(addFkStmt, execDone) { + self.execute(addFkStmt, execDone); + }, function(err) { + cb(err); + }); + }); }; /*! @@ -120,13 +153,35 @@ function mixinMigration(MySQL, mysql) { }); }); }, function(err) { - if (err) { - return err; - } - cb(null, !ok); + cb(err, !ok); }); }; + MySQL.prototype.getForeignKeySQL = function(model, actualFks) { + var self = this; + var m = this.getModelDefinition(model); + var addFksSql = []; + var newFks = m.settings.foreignKeys || {}; + var newFkNames = Object.keys(newFks).filter(function(name) { + return !!m.settings.foreignKeys[name]; + }); + + //add new foreign keys + if (newFkNames.length) { + //narrow down our key names to only those that don't already exist + var oldKeyNames = actualFks.map(function(oldKey) { return oldKey.fkName; }); + newFkNames.filter(function(key) { return !~oldKeyNames.indexOf(key); }) + .forEach(function(key) { + var constraint = self.buildForeignKeyDefinition(model, key); + if (constraint) { + addFksSql.push('ADD ' + constraint); + } + }); + } + + return addFksSql; + }; + MySQL.prototype.alterTable = function(model, actualFields, actualIndexes, actualFks, done, checkOnly) { //if this is using an old signature, then grab the correct callback and check boolean if ('function' == typeof actualFks && typeof done !== 'function') { @@ -143,11 +198,9 @@ function mixinMigration(MySQL, mysql) { var indexNames = Object.keys(indexes).filter(function(name) { return !!m.settings.indexes[name]; }); - var newFks = m.settings.foreignKeys || {}; - var newFkNames = Object.keys(newFks).filter(function(name) { - return !!m.settings.foreignKeys[name]; - }); - var dropConstraintSql = []; + + //add new foreign keys + var correctFks = m.settings.foreignKeys || {}; var sql = []; var ai = {}; @@ -191,10 +244,11 @@ function mixinMigration(MySQL, mysql) { var removedFks = []; actualFks.forEach(function(fk) { var needsToDrop = false; - var newFk = newFks[fk.fkName]; + var newFk = correctFks[fk.fkName]; if (newFk) { var fkCol = expectedColName(newFk.foreignKey); - var fkRefKey = expectedColNameForModel(newFk.entityKey, newFk.entity); + var fkEntity = self.getModelDefinition(newFk.entity); + var fkRefKey = expectedColNameForModel(newFk.entityKey, fkEntity); var fkRefTable = newFk.entity.name; //TODO check for mysql name needsToDrop = fkCol != fk.fkColumnName || fkRefKey != fk.pkColumnName || @@ -204,7 +258,7 @@ function mixinMigration(MySQL, mysql) { } if (needsToDrop) { - dropConstraintSql.push('DROP FOREIGN KEY ' + fk.fkName); + sql.push('DROP FOREIGN KEY ' + fk.fkName); removedFks.push(fk); //keep track that we removed these } }); @@ -234,7 +288,7 @@ function mixinMigration(MySQL, mysql) { if (indexName === 'PRIMARY' || (m.properties[indexName] && self.id(model, indexName))) return; - if (newFkNames.indexOf(indexName) > -1) return; //this index is from an FK + if (Object.keys(actualFks).indexOf(indexName) > -1) return; //this index is from an FK if (indexNames.indexOf(indexName) === -1 && !m.properties[indexName] || m.properties[indexName] && !m.properties[indexName].index) { sql.push('DROP INDEX ' + self.client.escapeId(indexName)); @@ -351,34 +405,34 @@ function mixinMigration(MySQL, mysql) { } }); - //add new foreign keys - if (newFkNames.length) { - //TODO validate that these are in the same DB, etc. - var oldKeyNames = actualFks.map(function(oldKey) { return oldKey.fkName; }); - newFkNames.filter(function(key) { return !~oldKeyNames.indexOf(key); }) - .forEach(function(key) { - var constraint = self.buildForeignKeyDefinition(model, key); - if (constraint) { - sql.push('ADD ' + constraint); - } - }); - } + //since we're passing the actualFks list to this, + //this code has be called after foreign keys have been dropped + //(in case we're replacing FKs) + var addFksSql = this.getForeignKeySQL(model, actualFks); - if (sql.length || dropConstraintSql.length) { - var stmtList = [dropConstraintSql, sql] - .filter(function(stmts) { return stmts.length; }) - .map(function(statements) { - return 'ALTER TABLE ' + self.tableEscaped(model) + - ' ' + statements.join(',\n'); - }); + //determine if there are column, index, or foreign keys changes (all require update) + if (sql.length || addFksSql.length) { + //get the required alter statements + var alterStmt = self.getAlterStatement(model, sql); + var newFksStatement = self.getAlterStatement(model, addFksSql); + var stmtList = [alterStmt, newFksStatement].filter(function(s) { return s.length; }); - if (checkOnly) { - done(null, true, {statements: stmtList, query: stmtList.join(';')}); + //set up an object to pass back all changes, changes that have been run, + //and foreign key statements that haven't been run + var retValues = { + statements: stmtList, + query: stmtList.join(';'), + newFks: newFksStatement, + }; + + //if we're running in read only mode OR if the only changes are foreign keys additions, + //then just return the object directly + if (checkOnly || !alterStmt.length) { + done(null, true, retValues); } else { - async.eachSeries(stmtList, function(stmt, execDone) { - self.execute(stmt, execDone); - }, function(err) { - done(err); + //if there are changes in the alter statement, then execute them and return the object + self.execute(alterStmt, function(err) { + done(err, true, retValues); }); } } else { @@ -432,6 +486,12 @@ function mixinMigration(MySQL, mysql) { } }; + MySQL.prototype.getAlterStatement = function(model, statements) { + return statements.length ? + 'ALTER TABLE ' + this.tableEscaped(model) + ' ' + statements.join(',\n') : + ''; + }; + MySQL.prototype.buildForeignKeyDefinition = function(model, keyName) { //TODO validate that these are in the same DB, etc. //TODO this is probably a better place to check this than in autoupdate or buildColumnDefinitions diff --git a/test/mysql.autoupdate.test.js b/test/mysql.autoupdate.test.js index 07be0fe..98077d2 100644 --- a/test/mysql.autoupdate.test.js +++ b/test/mysql.autoupdate.test.js @@ -261,8 +261,77 @@ describe('MySQL connector', function() { }, }, }; + var customer3_schema = + { + 'name': 'CustomerTest3', + 'options': { + 'idInjection': false, + 'mysql': { + 'schema': 'myapp_test', + 'table': 'customer_test3', + }, + }, + 'properties': { + 'id': { + 'type': 'String', + 'length': 20, + 'id': 1, + }, + 'name': { + 'type': 'String', + 'required': false, + 'length': 40, + }, + 'email': { + 'type': 'String', + 'required': true, + 'length': 40, + }, + 'age': { + 'type': 'Number', + 'required': false, + }, + }, + }; var schema_v1 = + { + 'name': 'OrderTest', + 'options': { + 'idInjection': false, + 'mysql': { + 'schema': 'myapp_test', + 'table': 'order_test', + }, + 'foreignKeys': { + 'fk_ordertest_customerId': { + 'name': 'fk_ordertest_customerId', + 'entity': 'CustomerTest3', + 'entityKey': 'id', + 'foreignKey': 'customerId', + }, + }, + }, + 'properties': { + 'id': { + 'type': 'String', + 'length': 20, + 'id': 1, + }, + 'customerId': { + 'type': 'String', + 'length': 20, + 'id': 1, + }, + 'description': { + 'type': 'String', + 'required': false, + 'length': 40, + }, + }, + }; + + var schema_v2 = { 'name': 'OrderTest', 'options': { @@ -299,7 +368,7 @@ describe('MySQL connector', function() { }, }; - var schema_v2 = + var schema_v3 = { 'name': 'OrderTest', 'options': { @@ -329,42 +398,73 @@ describe('MySQL connector', function() { }; var foreignKeySelect = - 'SELECT TABLE_NAME,COLUMN_NAME,CONSTRAINT_NAME,REFERENCED_COLUMN_NAME ' + + 'SELECT COLUMN_NAME,CONSTRAINT_NAME,REFERENCED_TABLE_NAME, REFERENCED_COLUMN_NAME ' + 'FROM INFORMATION_SCHEMA.KEY_COLUMN_USAGE ' + 'WHERE REFERENCED_TABLE_SCHEMA = "myapp_test" ' + - 'AND REFERENCED_TABLE_NAME = "customer_test2"'; + 'AND TABLE_NAME = "order_test"'; ds.createModel(customer2_schema.name, customer2_schema.properties, customer2_schema.options); + ds.createModel(customer3_schema.name, customer3_schema.properties, customer3_schema.options); ds.createModel(schema_v1.name, schema_v1.properties, schema_v1.options); - ds.automigrate(function() { - ds.autoupdate(function() { //foreign keys won't be created on table create - ds.discoverModelProperties('order_test', function(err, props) { - assert.equal(props.length, 3); + //do initial update/creation of table + ds.autoupdate(function() { + ds.discoverModelProperties('order_test', function(err, props) { + //validate that we have the correct number of properties + assert.equal(props.length, 3); - ds.connector.execute(foreignKeySelect, function(err, foreignKeys) { + //get the foreign keys for this table + ds.connector.execute(foreignKeySelect, function(err, foreignKeys) { + if (err) return done (err); + //validate that the foreign key exists and points to the right column + assert(foreignKeys); + assert(foreignKeys.length.should.be.equal(1)); + assert.equal(foreignKeys[0].REFERENCED_TABLE_NAME, 'customer_test3'); + assert.equal(foreignKeys[0].COLUMN_NAME, 'customerId'); + assert.equal(foreignKeys[0].CONSTRAINT_NAME, 'fk_ordertest_customerId'); + assert.equal(foreignKeys[0].REFERENCED_COLUMN_NAME, 'id'); + + //update our model (move foreign key) and run autoupdate to migrate + ds.createModel(schema_v2.name, schema_v2.properties, schema_v2.options); + ds.autoupdate(function(err, result) { if (err) return done (err); - assert(foreignKeys); - assert(foreignKeys.length.should.be.equal(1)); - assert.equal(foreignKeys[0].TABLE_NAME, 'order_test'); - assert.equal(foreignKeys[0].COLUMN_NAME, 'customerId'); - assert.equal(foreignKeys[0].CONSTRAINT_NAME, 'fk_ordertest_customerId'); - assert.equal(foreignKeys[0].REFERENCED_COLUMN_NAME, 'id'); - ds.createModel(schema_v2.name, schema_v2.properties, schema_v2.options); - ds.autoupdate(function(err, result) { + //get and validate the properties on this model + ds.discoverModelProperties('order_test', function(err, props) { if (err) return done (err); - ds.discoverModelProperties('order_test', function(err, props) { + + assert.equal(props.length, 3); + + //get the foreign keys that exist after the migration + ds.connector.execute(foreignKeySelect, function(err, updatedForeignKeys) { if (err) return done (err); + //validate that the foreign keys was moved to the new column + assert(updatedForeignKeys); + assert(updatedForeignKeys.length.should.be.equal(1)); + assert.equal(updatedForeignKeys[0].REFERENCED_TABLE_NAME, 'customer_test2'); + assert.equal(updatedForeignKeys[0].COLUMN_NAME, 'customerId'); + assert.equal(updatedForeignKeys[0].CONSTRAINT_NAME, 'fk_ordertest_customerId'); + assert.equal(updatedForeignKeys[0].REFERENCED_COLUMN_NAME, 'id'); - assert.equal(props.length, 3); - - ds.connector.execute(foreignKeySelect, function(err, updatedForeignKeys) { + //update model (to drop foreign key) and autoupdate + ds.createModel(schema_v3.name, schema_v3.properties, schema_v3.options); + ds.autoupdate(function(err, result) { if (err) return done (err); - assert(updatedForeignKeys); - assert(updatedForeignKeys.length.should.be.equal(0)); + //validate the properties + ds.discoverModelProperties('order_test', function(err, props) { + if (err) return done (err); - done(err, result); + assert.equal(props.length, 3); + + //get the foreign keys and validate the foreign key has been dropped + ds.connector.execute(foreignKeySelect, function(err, thirdForeignKeys) { + if (err) return done (err); + assert(thirdForeignKeys); + assert(thirdForeignKeys.length.should.be.equal(0)); + + done(err, result); + }); + }); }); }); });