Merge pull request #5227 from sushantdhiman/fix-966

Fix 966

Merge pull request #5227 from sushantdhiman/fix-966
Fix 966
Mick Hansen
Commit 87a22eb4 authored Jan 31, 2016 by Mick Hansen
Showing with 230 additions and 45 deletions
changelog.md
lib/dialects/mssql/query-generator.js
lib/dialects/mysql/query-generator.js
lib/dialects/postgres/query-generator.js
lib/dialects/sqlite/query-interface.js
test/integration/query-interface.test.js
test/unit/sql/change-column.test.js
--- a/changelog.md
+++ b/changelog.md
 # Future
+- [FIXED] Migrations failed to add foreign key [#966](https://github.com/sequelize/sequelize/issues/966)
 - [FIXED] Prevent race condition after transaction finished [#5222](https://github.com/sequelize/sequelize/issues/5222)
 - [FIXED] Fixed Instance.reload issues ([#4844](https://github.com/sequelize/sequelize/issues/4844) and [#4452](https://github.com/sequelize/sequelize/issues/4452))
 - [FIXED] Fix upsert when primary key contains `.field` (internal API change for `queryInterface.upsert`) [#4755](https://github.com/sequelize/sequelize/issues/4755)
 - [ADDED] Geography support for postgres
- [FIXED] Default value for `defaultScope` is now an empty object. This fixes calling `.scope('defaultScope')` when no scope is explicitly defined, see [#5277](https://github.com/sequelize/sequelize/issues/5277) 
+- [FIXED] Default value for `defaultScope` is now an empty object. This fixes calling `.scope('defaultScope')` when no scope is explicitly defined, see [#5277](https://github.com/sequelize/sequelize/issues/5277)
 # 3.18.0
 - [ADDED] Support silent: true in bulk update [#5200](https://github.com/sequelize/sequelize/issues/5200)

--- a/lib/dialects/mssql/query-generator.js
+++ b/lib/dialects/mssql/query-generator.js
@@ -178,21 +178,37 @@ var QueryGenerator = {
  },
  changeColumnQuery: function(tableName, attributes) {
-    var query = 'ALTER TABLE <%= tableName %> ALTER COLUMN <%= attributes %>;';
+    var query = 'ALTER TABLE <%= tableName %> <%= query %>;';
-    var attrString = [];
+    var attrString = [], constraintString = [];
-    for (var attrName in attributes) {
+    for (var attributeName in attributes) {
-      var definition = attributes[attrName];
+      var definition = attributes[attributeName];
+      if (definition.match(/REFERENCES/)) {
+        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),
+          definition: definition
+        }));
+      }
+    }
-      attrString.push(Utils._.template('<%= attrName %> <%= definition %>')({
+    var finalQuery = '';
-        attrName: this.quoteIdentifier(attrName),
+    if (attrString.length) {
-        definition: definition
+      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
    });
  },

--- a/lib/dialects/mysql/query-generator.js
+++ b/lib/dialects/mysql/query-generator.js
@@ -121,19 +121,38 @@ var QueryGenerator = {
  },
  changeColumnQuery: function(tableName, attributes) {
-    var query = 'ALTER TABLE <%= tableName %> CHANGE <%= attributes %>;';
+    var query = 'ALTER TABLE <%= tableName %> <%= query %>;';
-    var attrString = [];
+    var attrString = [], constraintString = [];
-    for (var attrName in attributes) {
+    for (var attributeName in attributes) {
-      var definition = attributes[attrName];
+      var definition = attributes[attributeName];
+      if (definition.match(/REFERENCES/)) {
+        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,
+          definition: definition
+        }));
+      }
+    }
-      attrString.push(Utils._.template('`<%= attrName %>` `<%= attrName %>` <%= definition %>')({
+    var finalQuery = '';
-        attrName: attrName,
+    if (attrString.length) {
-        definition: definition
+      finalQuery += 'CHANGE ' + attrString.join(', ');
-      }));
+      finalQuery += constraintString.length ? ' ' : '';
+    }
+    if (constraintString.length) {
+      finalQuery += 'ADD CONSTRAINT ' + constraintString.join(', ');
    }
-    return Utils._.template(query)({ tableName: this.quoteTable(tableName), attributes: attrString.join(', ') });
+    return Utils._.template(query)({
+      tableName: this.quoteTable(tableName),
+      query: finalQuery
+    });
  },
  renameColumnQuery: function(tableName, attrBefore, attributes) {

--- a/lib/dialects/postgres/query-generator.js
+++ b/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'
@@ -257,10 +257,18 @@ var QueryGenerator = {
        });
      }
-      attrSql += Utils._.template(query)({
+      if (definition.match(/REFERENCES/)) {
-        tableName: this.quoteTable(tableName),
+        definition = definition.replace(/.+?(?=REFERENCES)/,'');
-        query: this.quoteIdentifier(attributeName) + ' TYPE ' + definition
+        attrSql += Utils._.template(query.replace('ALTER COLUMN', ''))({
-      });
+          tableName: this.quoteTable(tableName),
+          query: 'ADD CONSTRAINT ' + this.quoteIdentifier(attributeName + '_foreign_idx') + ' FOREIGN KEY (' + this.quoteIdentifier(attributeName) + ') ' + definition
+        });
+      } else {
+        attrSql += Utils._.template(query)({
+          tableName: this.quoteTable(tableName),
+          query: this.quoteIdentifier(attributeName) + ' TYPE ' + definition
+        });
+      }
      sql.push(attrSql);
    }

--- a/lib/dialects/sqlite/query-interface.js
+++ b/lib/dialects/sqlite/query-interface.js
@@ -116,4 +116,4 @@ module.exports = {
  removeColumn: removeColumn,
  changeColumn: changeColumn,
  renameColumn: renameColumn
 };
\ No newline at end of file
--- a/test/integration/query-interface.test.js
+++ b/test/integration/query-interface.test.js
@@ -123,20 +123,20 @@ describe(Support.getTestDialectTeaser('QueryInterface'), function() {
          schema: 'schema'
        });
      }).then(function() {
-          return self.queryInterface.addIndex({
+        return self.queryInterface.addIndex({
+          schema: 'schema',
+          tableName: 'table'
+        }, ['name', 'isAdmin'], {
+          logging: log
+        }, 'schema_table').then(function() {
+          return self.queryInterface.showIndex({
            schema: 'schema',
            tableName: 'table'
-          }, ['name', 'isAdmin'], {
+          }, {logging: log}).then(function(indexes) {
-            logging: log
+            expect(indexes.length).to.eq(1);
-          }, 'schema_table').then(function() {
+            count = 0;
-            return self.queryInterface.showIndex({
-              schema: 'schema',
-              tableName: 'table'
-            }, {logging: log}).then(function(indexes) {
-              expect(indexes.length).to.eq(1);
-              count = 0;
-            });
          });
+        });
      });
    });
@@ -151,8 +151,8 @@ describe(Support.getTestDialectTeaser('QueryInterface'), function() {
      var Users = self.sequelize.define('_Users', {
        username: DataTypes.STRING,
        city: {
-            type: DataTypes.STRING,
+          type: DataTypes.STRING,
-            defaultValue: null
+          defaultValue: null
        },
        isAdmin: DataTypes.BOOLEAN,
        enumVals: DataTypes.ENUM('hello', 'world')
@@ -306,7 +306,7 @@ describe(Support.getTestDialectTeaser('QueryInterface'), function() {
      return Users.sync({ force: true }).then(function() {
        return self.queryInterface.renameColumn('_Users', 'username', 'pseudo', {logging: log}).then(function() {
          if (dialect === 'sqlite')
-            count++;
+          count++;
          expect(count).to.be.equal(2);
          count = 0;
        });
@@ -410,6 +410,73 @@ 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() {
+        beforeEach(function() {
+          return this.queryInterface.createTable('users', {
+            id: {
+              type: DataTypes.INTEGER,
+              primaryKey: true,
+              autoIncrement: true
+            },
+            level_id: {
+              type: DataTypes.INTEGER,
+              allowNull: false
+            }
+          })
+          .bind(this).then(function() {
+            return this.queryInterface.createTable('level', {
+              id: {
+                type: DataTypes.INTEGER,
+                primaryKey: true,
+                autoIncrement: true
+              }
+            });
+          });
+        });
+        it('able to change column to foreign key', function() {
+          return this.queryInterface.changeColumn('users', 'level_id', {
+            type: DataTypes.INTEGER,
+            references: {
+              model: 'level',
+              key:   'id'
+            },
+            onUpdate: 'cascade',
+            onDelete: 'cascade'
+          }, {logging: log}).then(function() {
+            expect(count).to.be.equal(1);
+            count = 0;
+          });
+        });
+      });
+    }
  });
  describe('addColumn', function() {
@@ -529,8 +596,8 @@ describe(Support.getTestDialectTeaser('QueryInterface'), function() {
        expect(fks).to.have.length(3);
        count = 0;
        var keys = Object.keys(fks[0]),
-          keys2 = Object.keys(fks[1]),
+            keys2 = Object.keys(fks[1]),
-          keys3 = Object.keys(fks[2]);
+            keys3 = Object.keys(fks[2]);
        if (dialect === 'postgres' || dialect === 'postgres-native') {
          expect(keys).to.have.length(6);

--- a/test/unit/sql/change-column.test.js
+++ b/test/unit/sql/change-column.test.js
+'use strict';
+/* jshint -W030, -W110 */
+var Support   = require(__dirname + '/../support')
+  , DataTypes = require('../../../lib/data-types')
+  , expectsql = Support.expectsql
+  , sinon = require('sinon')
+  , current   = Support.sequelize
+  , Promise = current.Promise;
+if (current.dialect.name !== 'sqlite') {
+  describe(Support.getTestDialectTeaser('SQL'), function() {
+    describe('changeColumn', function () {
+      var Model = current.define('users', {
+        id: {
+          type: DataTypes.INTEGER,
+          primaryKey: true,
+          autoIncrement: true
+        },
+        level_id: {
+          type: DataTypes.INTEGER
+        }
+      }, { timestamps: false });
+      before(function () {
+        this.stub = sinon.stub(current, 'query', function (sql) {
+          return Promise.resolve(sql);
+        });
+      });
+      beforeEach(function () {
+        this.stub.reset();
+      });
+      after(function () {
+        this.stub.restore();
+      });
+      it('properly generate alter queries', function(){
+        return current.getQueryInterface().changeColumn(Model.getTableName(), 'level_id', {
+          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;',
+          });
+        });
+      });
+    });
+  });
+}