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

@@ -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) {
+      cb(err, !ok);
-        return err;
-      }
-      cb(null, !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) {
+    //add new foreign keys
-      return !!m.settings.foreignKeys[name];
+    var correctFks = m.settings.foreignKeys || {};
-    });
-    var dropConstraintSql = [];
     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
+    //since we're passing the actualFks list to this,
-    if (newFkNames.length) {
+    //this code has be called after foreign keys have been dropped
-      //TODO validate that these are in the same DB, etc.
+    //(in case we're replacing FKs)
-      var oldKeyNames = actualFks.map(function(oldKey) { return oldKey.fkName; });
+    var addFksSql = this.getForeignKeySQL(model, actualFks);
-      newFkNames.filter(function(key) { return !~oldKeyNames.indexOf(key); })
-        .forEach(function(key) {
-          var constraint = self.buildForeignKeyDefinition(model, key);
-          if (constraint) {
-            sql.push('ADD ' + constraint);
-          }
-        });
-    }
 
-    if (sql.length || dropConstraintSql.length) {
+    //determine if there are column, index, or foreign keys changes (all require update)
-      var stmtList = [dropConstraintSql, sql]
+    if (sql.length || addFksSql.length) {
-        .filter(function(stmts) { return stmts.length; })
+      //get the required alter statements
-        .map(function(statements) {
+      var alterStmt = self.getAlterStatement(model, sql);
-          return 'ALTER TABLE ' + self.tableEscaped(model) +
+      var newFksStatement = self.getAlterStatement(model, addFksSql);
-            ' ' + statements.join(',\n');
+      var stmtList = [alterStmt, newFksStatement].filter(function(s) { return s.length; });
-        });
 
-      if (checkOnly) {
+      //set up an object to pass back all changes, changes that have been run,
-        done(null, true, {statements: stmtList, query: stmtList.join(';')});
+      //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) {
+        //if there are changes in the alter statement, then execute them and return the object
-          self.execute(stmt, execDone);
+        self.execute(alterStmt, function(err) {
-        }, function(err) {
+          done(err, true, retValues);
-          done(err);
         });
       }
     } 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

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() {
+    //do initial update/creation of table
-      ds.autoupdate(function() { //foreign keys won't be created on table create
+    ds.autoupdate(function() {
-        ds.discoverModelProperties('order_test', function(err, props) {
+      ds.discoverModelProperties('order_test', function(err, props) {
-          assert.equal(props.length, 3);
+        //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);
+            //get and validate the properties on this model
-            ds.autoupdate(function(err, result) {
+            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);
+                //update model (to drop foreign key) and autoupdate
-
+                ds.createModel(schema_v3.name, schema_v3.properties, schema_v3.options);
-                ds.connector.execute(foreignKeySelect, function(err, updatedForeignKeys) {
+                ds.autoupdate(function(err, result) {
                   if (err) return done (err);
-                  assert(updatedForeignKeys);
+                  //validate the properties
-                  assert(updatedForeignKeys.length.should.be.equal(0));
+                  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);
+                    });
+                  });
                 });
               });
             });