Merge pull request #251 from strongloop/refactor-alter-table

Refactor alter table

lib/migration.js

@@ -71,41 +71,31 @@ function mixinMigration(MySQL, mysql) {
 
           if (!err && fields && fields.length) {
             //if we already have a definition, update this table
-            self.alterTable(model, fields, indexes, foreignKeys, function(err, changed, res) {
+            self.alterTable(model, fields, indexes, foreignKeys, function(err, result) {
-              //check to see if there were any new foreign keys for this table.
+              if (!err) {
-              //If so, we'll create them once all tables have been updated
+                self.addForeignKeys(model, function(err, result) {
-              if (!err && res && res.newFks && res.newFks.length) {
-                foreignKeyStatements.push(res.newFks);
-              }
                   done(err);
                 });
+              } else {
+                done(err);
+              }
+            });
           } else {
             //if there is not yet a definition, create this table
-            var res = self.createTable(model, function(err) {
+            self.createTable(model, function(err) {
               if (!err) {
-                //get a list of the alter statements needed to add the defined foreign keys
+                self.addForeignKeys(model, function(err, result) {
-                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);
                 });
+              } else {
+                done(err);
+              }
+            });
           }
         });
       });
     }, function(err) {
-      if (err) return cb(err);
+      return cb(err);
-
-      //add any new foreign keys
-      async.each(foreignKeyStatements, function(addFkStmt, execDone) {
-        self.execute(addFkStmt, execDone);
-      }, function(err) {
-        cb(err);
-      });
     });
   };
 
@@ -166,50 +156,114 @@ function mixinMigration(MySQL, mysql) {
     });
   };
 
-  MySQL.prototype.getForeignKeySQL = function(model, actualFks) {
+  MySQL.prototype.getAddModifyColumns = function(model, actualFields) {
+    var sql = [];
     var self = this;
-    var m = this.getModelDefinition(model);
+    sql = sql.concat(self.getColumnsToAdd(model, actualFields));
-    var addFksSql = [];
+    return sql;
-    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) {
+  MySQL.prototype.getColumnsToAdd = function(model, actualFields) {
-    //if this is using an old signature, then grab the correct callback and check boolean
-    if ('function' == typeof actualFks && typeof done !== 'function') {
-      checkOnly = done || false;
-      done = actualFks;
-    }
-
     var self = this;
     var m = this.getModelDefinition(model);
     var propNames = Object.keys(m.properties).filter(function(name) {
       return !!m.properties[name];
     });
-    var indexes = m.settings.indexes || {};
+    var sql = [];
-    var indexNames = Object.keys(indexes).filter(function(name) {
+
-      return !!m.settings.indexes[name];
+    propNames.forEach(function(propName) {
+      if (m.properties[propName] && self.id(model, propName)) return;
+      var found;
+      var colName = expectedColNameForModel(propName, m);
+      if (actualFields) {
+        actualFields.forEach(function(f) {
+          if (f.Field === colName) {
+            found = f;
+          }
+        });
+      }
+      if (found) {
+        actualize(colName, found);
+      } else {
+        sql.push('ADD COLUMN ' + self.client.escapeId(colName) + ' ' +
+          self.buildColumnDefinition(model, propName));
+      }
     });
 
-    //add new foreign keys
+    function actualize(propName, oldSettings) {
-    var correctFks = m.settings.foreignKeys || {};
+      var newSettings = m.properties[propName];
+      if (newSettings && changed(newSettings, oldSettings)) {
+        var pName = self.client.escapeId(propName);
+        sql.push('CHANGE COLUMN ' + pName + ' ' + pName + ' ' +
+          self.buildColumnDefinition(model, propName));
+      }
+    }
+
+    function changed(newSettings, oldSettings) {
+      if (oldSettings.Null === 'YES') {
+        // Used to allow null and does not now.
+        if (!self.isNullable(newSettings)) {
+          return true;
+        }
+      }
+      if (oldSettings.Null === 'NO') {
+        // Did not allow null and now does.
+        if (self.isNullable(newSettings)) {
+          return true;
+        }
+      }
+
+      if (oldSettings.Type.toUpperCase() !==
+        self.buildColumnType(newSettings).toUpperCase()) {
+        return true;
+      }
+      return false;
+    }
+    return sql;
+  };
+
+  MySQL.prototype.getDropColumns = function(model, actualFields) {
+    var sql = [];
+    var self = this;
+    sql = sql.concat(self.getColumnsToDrop(model, actualFields));
+    return sql;
+  };
+
+  MySQL.prototype.getColumnsToDrop = function(model, actualFields) {
+    var self = this;
+    var fields = actualFields;
+    var sql = [];
+    var m = this.getModelDefinition(model);
+    var propNames = Object.keys(m.properties).filter(function(name) {
+      return !!m.properties[name];
+    });
+    // drop columns
+    if (fields) {
+      fields.forEach(function(f) {
+        var colNames = propNames.map(function expectedColName(propName) {
+          return expectedColNameForModel(propName, m);
+        });
+        var index = colNames.indexOf(f.Field);
+        var propName = index >= 0 ? propNames[index] : f.Field;
+        var notFound = !~index;
+        if (m.properties[propName] && self.id(model, propName)) return;
+        if (notFound || !m.properties[propName]) {
+          sql.push('DROP COLUMN ' + self.client.escapeId(f.Field));
+        }
+      });
+    }
+    return sql;
+  };
+
+  MySQL.prototype.addIndexes = function(model, actualIndexes) {
+    var self = this;
+    var m = this.getModelDefinition(model);
+    var propNames = Object.keys(m.properties).filter(function(name) {
+      return !!m.properties[name];
+    });
+    var indexNames = m.settings.indexes && Object.keys(m.settings.indexes).filter(function(name) {
+      return !!m.settings.indexes[name];
+    }) || [];
     var sql = [];
     var ai = {};
 
@@ -227,71 +281,6 @@ function mixinMigration(MySQL, mysql) {
     }
     var aiNames = Object.keys(ai);
 
-    // change/add new fields
-    propNames.forEach(function(propName) {
-      if (m.properties[propName] && self.id(model, propName)) return;
-      var found;
-      var colName = expectedColName(propName);
-      if (actualFields) {
-        actualFields.forEach(function(f) {
-          if (f.Field === colName) {
-            found = f;
-          }
-        });
-      }
-
-      if (found) {
-        actualize(colName, found);
-      } else {
-        sql.push('ADD COLUMN ' + self.client.escapeId(colName) + ' ' +
-          self.buildColumnDefinition(model, propName));
-      }
-    });
-
-    //drop foreign keys for removed fields
-    if (actualFks) {
-      var removedFks = [];
-      actualFks.forEach(function(fk) {
-        var needsToDrop = false;
-        var newFk = correctFks[fk.fkName];
-        if (newFk) {
-          var fkCol = expectedColName(newFk.foreignKey);
-          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 ||
-                        fkRefTable != fk.pkTableName;
-        } else {
-          needsToDrop = true;
-        }
-
-        if (needsToDrop) {
-          sql.push('DROP FOREIGN KEY ' + fk.fkName);
-          removedFks.push(fk); //keep track that we removed these
-        }
-      });
-
-      //update out list of existing keys by removing dropped keys
-      actualFks = actualFks.filter(function(k) {
-        return removedFks.indexOf(k) == -1;
-      });
-    }
-
-    // drop columns
-    if (actualFields) {
-      actualFields.forEach(function(f) {
-        var colNames = propNames.map(expectedColName);
-        var index = colNames.indexOf(f.Field);
-        var propName = index >= 0 ? propNames[index] : f.Field;
-        var notFound = !~index;
-        if (m.properties[propName] && self.id(model, propName)) return;
-        if (notFound || !m.properties[propName]) {
-          sql.push('DROP COLUMN ' + self.client.escapeId(f.Field));
-        }
-      });
-    }
-
     // remove indexes
     aiNames.forEach(function(indexName) {
       if (indexName === 'PRIMARY' ||
@@ -347,7 +336,7 @@ function mixinMigration(MySQL, mysql) {
       }
       var found = ai[propName] && ai[propName].info;
       if (!found) {
-        var colName = expectedColName(propName);
+        var colName = expectedColNameForModel(propName, m);
         var pName = self.client.escapeId(colName);
         var type = '';
         var kind = '';
@@ -412,30 +401,142 @@ function mixinMigration(MySQL, mysql) {
         }
       }
     });
+    return sql;
+  };
 
-    //since we're passing the actualFks list to this,
+  MySQL.prototype.getForeignKeySQL = function(model, actualFks) {
-    //this code has be called after foreign keys have been dropped
+    var self = this;
-    //(in case we're replacing FKs)
+    var m = this.getModelDefinition(model);
-    var addFksSql = this.getForeignKeySQL(model, actualFks);
+    var addFksSql = [];
+
+    if (actualFks) {
+      var keys = Object.keys(actualFks);
+      for (var i = 0; i < keys.length; i++) {
+        var constraint = self.buildForeignKeyDefinition(model, keys[i]);
+
+        if (constraint) {
+          addFksSql.push('ADD ' + constraint);
+        }
+      }
+    }
+    return addFksSql;
+  };
+
+  MySQL.prototype.addForeignKeys = function(model, fkSQL, cb) {
+    var self = this;
+    var m = this.getModelDefinition(model);
+
+    if ((!cb) && ('function' === typeof fkSQL)) {
+      cb = fkSQL;
+      fkSQL = undefined;
+    }
+
+    if (!fkSQL) {
+      var newFks = m.settings.foreignKeys;
+      if (newFks)
+        fkSQL = self.getForeignKeySQL(model, newFks);
+    }
+    if (fkSQL && fkSQL.length) {
+      self.applySqlChanges(model, fkSQL, function(err, result) {
+        if (err) cb(err);
+        else
+          cb(null, result);
+      });
+    } else cb(null, {});
+  };
+
+  MySQL.prototype.dropForeignKeys = function(model, actualFks) {
+    var self = this;
+    var m = this.getModelDefinition(model);
+
+    var fks = actualFks;
+    var sql = [];
+    var correctFks = m.settings.foreignKeys || {};
+
+    //drop foreign keys for removed fields
+    if (fks && fks.length) {
+      var removedFks = [];
+      fks.forEach(function(fk) {
+        var needsToDrop = false;
+        var newFk = correctFks[fk.fkName];
+        if (newFk) {
+          var fkCol = expectedColNameForModel(newFk.foreignKey, m);
+          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 ||
+                        fkRefTable != fk.pkTableName;
+        } else {
+          needsToDrop = true;
+        }
+
+        if (needsToDrop) {
+          sql.push('DROP FOREIGN KEY ' + fk.fkName);
+          removedFks.push(fk); //keep track that we removed these
+        }
+      });
+
+      //update out list of existing keys by removing dropped keys
+      fks = actualFks.filter(function(k) {
+        return removedFks.indexOf(k) == -1;
+      });
+    }
+    return sql;
+  };
+
+  MySQL.prototype.getAlterStatement = function(model, statements) {
+    return statements.length ?
+      'ALTER TABLE ' + this.tableEscaped(model) + ' ' + statements.join(',\n') :
+      '';
+  };
+
+  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') {
+      checkOnly = done || false;
+      done = actualFks;
+    }
+    var self = this;
+
+    var statements = [];
+
+    async.series([
+      function(cb) {
+        statements = self.getAddModifyColumns(model, actualFields);
+        cb();
+      },
+      function(cb) {
+        statements = statements.concat(self.getDropColumns(model, actualFields));
+        cb();
+      },
+      function(cb) {
+        statements = statements.concat(self.addIndexes(model, actualIndexes));
+        cb();
+      },
+      function(cb) {
+        statements = statements.concat(self.dropForeignKeys(model, actualFks));
+        cb();
+      },
+    ], function(err, result) {
+      if (err) done(err);
 
       //determine if there are column, index, or foreign keys changes (all require update)
-    if (sql.length || addFksSql.length) {
+      if (statements.length) {
         //get the required alter statements
-      var alterStmt = self.getAlterStatement(model, sql);
+        var alterStmt = self.getAlterStatement(model, statements);
-      var newFksStatement = self.getAlterStatement(model, addFksSql);
+        var stmtList = [alterStmt];
-      var stmtList = [alterStmt, newFksStatement].filter(function(s) { return s.length; });
 
         //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) {
+        if (checkOnly) {
           done(null, true, retValues);
         } else {
           //if there are changes in the alter statement, then execute them and return the object
@@ -446,58 +547,7 @@ function mixinMigration(MySQL, mysql) {
       } else {
         done();
       }
-
+    });
-    function actualize(propName, oldSettings) {
-      var newSettings = m.properties[propName];
-      if (newSettings && changed(newSettings, oldSettings)) {
-        var pName = self.client.escapeId(propName);
-        sql.push('CHANGE COLUMN ' + pName + ' ' + pName + ' ' +
-          self.buildColumnDefinition(model, propName));
-      }
-    }
-
-    function changed(newSettings, oldSettings) {
-      if (oldSettings.Null === 'YES') {
-        // Used to allow null and does not now.
-        if (!self.isNullable(newSettings)) {
-          return true;
-        }
-      }
-      if (oldSettings.Null === 'NO') {
-        // Did not allow null and now does.
-        if (self.isNullable(newSettings)) {
-          return true;
-        }
-      }
-
-      if (oldSettings.Type.toUpperCase() !==
-        self.buildColumnType(newSettings).toUpperCase()) {
-        return true;
-      }
-      return false;
-    }
-
-    function expectedColName(propName) {
-      return expectedColNameForModel(propName, m);
-    }
-
-    function expectedColNameForModel(propName, modelToCheck) {
-      var mysql = modelToCheck.properties[propName].mysql;
-      if (typeof mysql === 'undefined') {
-        return propName;
-      }
-      var colName = mysql.columnName;
-      if (typeof colName === 'undefined') {
-        return propName;
-      }
-      return colName;
-    }
-  };
-
-  MySQL.prototype.getAlterStatement = function(model, statements) {
-    return statements.length ?
-      'ALTER TABLE ' + this.tableEscaped(model) + ' ' + statements.join(',\n') :
-      '';
   };
 
   MySQL.prototype.buildForeignKeyDefinition = function(model, keyName) {
@@ -850,4 +900,15 @@ function mixinMigration(MySQL, mysql) {
     }
     return columnType;
   }
+  function expectedColNameForModel(propName, modelToCheck) {
+    var mysql = modelToCheck.properties[propName].mysql;
+    if (typeof mysql === 'undefined') {
+      return propName;
+    }
+    var colName = mysql.columnName;
+    if (typeof colName === 'undefined') {
+      return propName;
+    }
+    return colName;
+  }
 }