Fix(#966) : Added more test, fixed mysql,mssql,postgres changeColumn logic

Changed MySQL, MSSQL changeColumn dont overwrite query template for changeColumn

lib/dialects/mssql/query-generator.js

@@ -178,15 +178,17 @@ var QueryGenerator = {
   },
 
   changeColumnQuery: function(tableName, attributes) {
-    var query = 'ALTER TABLE <%= tableName %> ALTER COLUMN <%= attributes %>;';
-    var attrString = [];
+    var query = 'ALTER TABLE <%= tableName %> <%= query %>;';
+    var attrString = [], constraintString = [];
 
     for (var attributeName in attributes) {
       var definition = attributes[attributeName];
       if (definition.match(/REFERENCES/)) {
-        query = query.replace('ALTER COLUMN', '');
-        definition = definition.replace(/.+?(?=REFERENCES)/,'');
-        attrString.push('ADD CONSTRAINT ' + this.quoteIdentifier(attributeName + '_foreign_idx') + ' FOREIGN KEY (' + this.quoteIdentifier(attributeName) + ') ' + definition);
+        constraintString.push(Utils._.template('<%= fkName %> FOREIGN KEY (<%= attrName %>) <%= definition %>')({
+          fkName: this.quoteIdentifier(attributeName + '_foreign_idx'),
+          attrName: this.quoteIdentifier(attributeName),
+          definition: definition.replace(/.+?(?=REFERENCES)/,'')
+        }));
       } else {
         attrString.push(Utils._.template('<%= attrName %> <%= definition %>')({
           attrName: this.quoteIdentifier(attributeName),
@@ -195,9 +197,18 @@ var QueryGenerator = {
       }
     }
 
+    var finalQuery = '';
+    if (attrString.length) {
+      finalQuery += 'ALTER COLUMN ' + attrString.join(', ');
+      finalQuery += constraintString.length ? ' ' : '';
+    }
+    if (constraintString.length) {
+      finalQuery += 'ADD CONSTRAINT ' + constraintString.join(', ');
+    }
+
     return Utils._.template(query)({
       tableName: this.quoteTable(tableName),
-      attributes: attrString.join(', ')
+      query: finalQuery
     });
   },
 

lib/dialects/mysql/query-generator.js

@@ -121,14 +121,17 @@ var QueryGenerator = {
   },
 
   changeColumnQuery: function(tableName, attributes) {
-    var query = 'ALTER TABLE <%= tableName %> CHANGE <%= attributes %>;';
-    var attrString = [];
+    var query = 'ALTER TABLE <%= tableName %> <%= query %>;';
+    var attrString = [], constraintString = [];
+
     for (var attributeName in attributes) {
       var definition = attributes[attributeName];
       if (definition.match(/REFERENCES/)) {
-        query = query.replace('CHANGE', '');
-        definition = definition.replace(/.+?(?=REFERENCES)/,'');
-        attrString.push('ADD CONSTRAINT ' + this.quoteIdentifier(attributeName + '_foreign_idx') + ' FOREIGN KEY (' + this.quoteIdentifier(attributeName) + ') ' + definition);
+        constraintString.push(Utils._.template('<%= fkName %> FOREIGN KEY (<%= attrName %>) <%= definition %>')({
+          fkName: this.quoteIdentifier(attributeName + '_foreign_idx'),
+          attrName: this.quoteIdentifier(attributeName),
+          definition: definition.replace(/.+?(?=REFERENCES)/,'')
+        }));
       } else {
         attrString.push(Utils._.template('`<%= attrName %>` `<%= attrName %>` <%= definition %>')({
           attrName: attributeName,
@@ -136,7 +139,20 @@ var QueryGenerator = {
         }));
       }
     }
-    return Utils._.template(query)({ tableName: this.quoteTable(tableName), attributes: attrString.join(', ') });
+
+    var finalQuery = '';
+    if (attrString.length) {
+      finalQuery += 'CHANGE ' + attrString.join(', ');
+      finalQuery += constraintString.length ? ' ' : '';
+    }
+    if (constraintString.length) {
+      finalQuery += 'ADD CONSTRAINT ' + constraintString.join(', ');
+    }
+
+    return Utils._.template(query)({
+      tableName: this.quoteTable(tableName),
+      query: finalQuery
+    });
   },
 
   renameColumnQuery: function(tableName, attrBefore, attributes) {

lib/dialects/postgres/query-generator.js

@@ -221,7 +221,7 @@ var QueryGenerator = {
         });
 
         definition = definition.replace('NOT NULL', '').trim();
-      } else {
+      } else if (!definition.match(/REFERENCES/)) {
         attrSql += Utils._.template(query)({
           tableName: this.quoteTable(tableName),
           query: this.quoteIdentifier(attributeName) + ' DROP NOT NULL'
@@ -235,7 +235,7 @@ var QueryGenerator = {
         });
 
         definition = definition.replace(/(DEFAULT[^;]+)/, '').trim();
-      } else {
+      } else if (!definition.match(/REFERENCES/)) {
         attrSql += Utils._.template(query)({
           tableName: this.quoteTable(tableName),
           query: this.quoteIdentifier(attributeName) + ' DROP DEFAULT'

test/integration/query-interface.test.js

@@ -411,6 +411,29 @@ describe(Support.getTestDialectTeaser('QueryInterface'), function() {
       });
     });
 
+    it('should change columns', function() {
+        return this.queryInterface.createTable({
+          tableName: 'users',
+        }, {
+          id: {
+            type: DataTypes.INTEGER,
+            primaryKey: true,
+            autoIncrement: true
+          },
+          currency: DataTypes.INTEGER
+        }).bind(this).then(function() {
+          return this.queryInterface.changeColumn('users', 'currency', {
+            type: DataTypes.FLOAT,
+            allowNull: true
+          }, {
+            logging: log
+          }).then(function() {
+            expect(count).to.be.equal(1);
+            count = 0;
+          });
+        });
+    });
+
     //SQlite navitely doesnt support ALTER Foreign key
     if (dialect !== 'sqlite') {
       describe('should support foreign keys', function() {

test/unit/sql/change-column.test.js

@@ -41,21 +41,33 @@ if (current.dialect.name !== 'sqlite') {
 
       it('properly generate alter queries', function(){
         return current.getQueryInterface().changeColumn(Model.getTableName(), 'level_id', {
-                 type: DataTypes.INTEGER,
-                 references: {
-                   model: 'level',
-                   key:   'id'
-                 },
-                 onUpdate: 'cascade',
-                 onDelete: 'cascade'
-               }).then(function(sql){
-                 expectsql(sql, {
-                   mssql: 'ALTER TABLE [users]  ADD CONSTRAINT [level_id_foreign_idx] FOREIGN KEY ([level_id]) REFERENCES [level] ([id]) ON DELETE CASCADE;',
-                   mysql: 'ALTER TABLE `users`  ADD CONSTRAINT `level_id_foreign_idx` FOREIGN KEY (`level_id`) REFERENCES `level` (`id`) ON DELETE CASCADE ON UPDATE CASCADE;',
-                   //Postgres issue multiple queries when changing schema to drop default indexes
-                   postgres: 'ALTER TABLE "users" ALTER COLUMN "level_id" DROP NOT NULL;ALTER TABLE "users" ALTER COLUMN "level_id" DROP DEFAULT;ALTER TABLE "users"  ADD CONSTRAINT "level_id_foreign_idx" FOREIGN KEY ("level_id") REFERENCES "level" ("id") ON DELETE CASCADE ON UPDATE CASCADE;',
-                 });
-               });
+          type: DataTypes.FLOAT,
+          allowNull: false,
+        }).then(function(sql){
+          expectsql(sql, {
+            mssql: 'ALTER TABLE [users] ALTER COLUMN [level_id] FLOAT NOT NULL;',
+            mysql: 'ALTER TABLE `users` CHANGE `level_id` `level_id` FLOAT NOT NULL;',
+            postgres: 'ALTER TABLE "users" ALTER COLUMN "level_id" SET NOT NULL;ALTER TABLE "users" ALTER COLUMN "level_id" DROP DEFAULT;ALTER TABLE "users" ALTER COLUMN "level_id" TYPE FLOAT;',
+          });
+        });
+      });
+
+      it('properly generate alter queries for foreign keys', function(){
+        return current.getQueryInterface().changeColumn(Model.getTableName(), 'level_id', {
+          type: DataTypes.INTEGER,
+          references: {
+            model: 'level',
+            key:   'id'
+          },
+          onUpdate: 'cascade',
+          onDelete: 'cascade'
+        }).then(function(sql){
+          expectsql(sql, {
+            mssql: 'ALTER TABLE [users] ADD CONSTRAINT [level_id_foreign_idx] FOREIGN KEY ([level_id]) REFERENCES [level] ([id]) ON DELETE CASCADE;',
+            mysql: 'ALTER TABLE `users` ADD CONSTRAINT `level_id_foreign_idx` FOREIGN KEY (`level_id`) REFERENCES `level` (`id`) ON DELETE CASCADE ON UPDATE CASCADE;',
+            postgres: 'ALTER TABLE "users"  ADD CONSTRAINT "level_id_foreign_idx" FOREIGN KEY ("level_id") REFERENCES "level" ("id") ON DELETE CASCADE ON UPDATE CASCADE;',
+          });
+        });
       });
 
     });