fix(#966) : Fixed dialects to properly generate ADD CONSTRAINT when using changeColumn

Sushant
Commit 1f1604d6 authored Jan 15, 2016 by Sushant
Showing with 158 additions and 71 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
@@ -181,13 +181,18 @@ var QueryGenerator = {
    var query = 'ALTER TABLE <%= tableName %> ALTER COLUMN <%= attributes %>;';
    var attrString = [];
-    for (var attrName in attributes) {
+    for (var attributeName in attributes) {
-      var definition = attributes[attrName];
+      var definition = attributes[attributeName];
+      if (definition.match(/REFERENCES/)) {
-      attrString.push(Utils._.template('<%= attrName %> <%= definition %>')({
+        query = query.replace('ALTER COLUMN', '');
-        attrName: this.quoteIdentifier(attrName),
+        definition = definition.replace(/.+?(?=REFERENCES)/,'');
-        definition: definition
+        attrString.push('ADD CONSTRAINT ' + this.quoteIdentifier(attributeName + '_foreign_idx') + ' FOREIGN KEY (' + this.quoteIdentifier(attributeName) + ') ' + definition);
-      }));
+      } else {
+        attrString.push(Utils._.template('<%= attrName %> <%= definition %>')({
+          attrName: this.quoteIdentifier(attributeName),
+          definition: definition
+        }));
+      }
    }
    return Utils._.template(query)({

--- a/lib/dialects/mysql/query-generator.js
+++ b/lib/dialects/mysql/query-generator.js
@@ -123,16 +123,19 @@ var QueryGenerator = {
  changeColumnQuery: function(tableName, attributes) {
    var query = 'ALTER TABLE <%= tableName %> CHANGE <%= attributes %>;';
    var attrString = [];
+    for (var attributeName in attributes) {
-    for (var attrName in attributes) {
+      var definition = attributes[attributeName];
-      var definition = attributes[attrName];
+      if (definition.match(/REFERENCES/)) {
+        query = query.replace('CHANGE', '');
-      attrString.push(Utils._.template('`<%= attrName %>` `<%= attrName %>` <%= definition %>')({
+        definition = definition.replace(/.+?(?=REFERENCES)/,'');
-        attrName: attrName,
+        attrString.push('ADD CONSTRAINT ' + this.quoteIdentifier(attributeName + '_foreign_idx') + ' FOREIGN KEY (' + this.quoteIdentifier(attributeName) + ') ' + definition);
-        definition: definition
+      } else {
-      }));
+        attrString.push(Utils._.template('`<%= attrName %>` `<%= attrName %>` <%= definition %>')({
+          attrName: attributeName,
+          definition: definition
+        }));
+      }
    }
    return Utils._.template(query)({ tableName: this.quoteTable(tableName), attributes: attrString.join(', ') });
  },

--- a/lib/dialects/postgres/query-generator.js
+++ b/lib/dialects/postgres/query-generator.js
@@ -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;
        });
@@ -411,41 +411,49 @@ describe(Support.getTestDialectTeaser('QueryInterface'), function() {
      });
    });
-    it('should be able to change to foreign key reference', function() {
+    //SQlite navitely doesnt support ALTER Foreign key
-      return this.queryInterface.createTable('level', {
+    if (dialect !== 'sqlite') {
-        id: {
+      describe('should support foreign keys', function() {
-          type: DataTypes.INTEGER,
+        beforeEach(function() {
-          primaryKey: true,
+          return this.queryInterface.createTable('users', {
-          autoIncrement: true
+            id: {
-        }
+              type: DataTypes.INTEGER,
-      }).bind(this).then(function() {
+              primaryKey: true,
-        this.queryInterface.createTable('users', {
+              autoIncrement: true
-          id: {
+            },
+            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,
-            primaryKey: true,
+            references: {
-            autoIncrement: true
+              model: 'level',
-          },
+              key:   'id'
-          level_id: {
+            },
-            type: DataTypes.INTEGER
+            onUpdate: 'cascade',
-          }
+            onDelete: 'cascade'
+          }, {logging: log}).then(function() {
+            expect(count).to.be.equal(1);
+            count = 0;
+          });
        });
-      })
-      .bind(this).then(function() {
-        return this.queryInterface.changeColumn('users', 'level_id', {
-          type: DataTypes.INTEGER,
-          references: {
-            model: 'level',
-            key:   'id'
-          },
-          onUpdate: 'cascade',
-          onDelete: 'set null'
-        }, {logging: log});
-      }).then(function() {
-        expect(count).to.be.equal(1);
-        count = 0;
-      });
-    });
+      });
+    }
  });
  describe('addColumn', function() {
@@ -565,8 +573,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.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;',
+                 });
+               });
+      });
+    });
+  });
+}