Only create indexes which don't exist already in sync. Closes #2162

Jan Aagaard Meier
Commit 71a1faaf authored Sep 18, 2014 by Jan Aagaard Meier
Showing with 61 additions and 25 deletions
changelog.md
lib/dialects/abstract/query-generator.js
lib/dialects/mysql/query-generator.js
lib/dialects/sqlite/query-generator.js
lib/model.js
lib/query-interface.js
test/dao-factory.test.js
test/query-interface.test.js
--- a/changelog.md
+++ b/changelog.md
@@ -3,6 +3,7 @@
 - [FEATURE] Added `field` and `name` to the object form of foreign key definitions
 - [FEATURE] Added support for calling `Promise.done`, thus explicitly ending the promise chain by calling done with no arguments. Done with a function argument still continues the promise chain, to maintain BC.
 - [FEATURE] Added `scope` to hasMany association definitions, provides default values to association setters/finders [#2268](https://github.com/sequelize/sequelize/pull/2268)
+- [BUG] Only try to create indexes which don't already exist. Closes [#2162](https://github.com/sequelize/sequelize/issues/2162)
 #### Backwards compatability changes
 - The `fieldName` property, used in associations with a foreign key object `(A.hasMany(B, { foreignKey: { ... }})`, has been renamed to `name` to avoid confusion with `field`. 

--- a/lib/dialects/abstract/query-generator.js
+++ b/lib/dialects/abstract/query-generator.js
@@ -333,6 +333,20 @@ module.exports = (function() {
      return Utils._.template(query)(replacements);
    },
+    nameIndexes: function (indexes, rawTablename) {
+      return Utils._.map(indexes, function (index) {
+        if (!index.hasOwnProperty('name')) {
+          var onlyAttributeNames = index.fields.map(function(attribute) {
+            return (typeof attribute === 'string') ? attribute : attribute.attribute;
+          }.bind(this));
+          index.name = Utils.inflection.underscore(rawTablename + '_' + onlyAttributeNames.join('_'));
+        }
+        return index;
+      });
+    },
    /*
      Returns an add index query.
      Parameters:
@@ -380,15 +394,18 @@ module.exports = (function() {
        }
      }.bind(this));
-      var onlyAttributeNames = attributes.map(function(attribute) {
+      if (!options.name) {
-        return (typeof attribute === 'string') ? attribute : attribute.attribute;
+        // Mostly for cases where addIndex is called directly by the user without an options object (for example in migrations)
-      }.bind(this));
+        // All calls that go through sequelize should already have a name
+        options.fields = options.fields || attributes;
+        options = this.nameIndexes([options], rawTablename)[0];
+      }
      options = Utils._.defaults(options, {
        type: '',
        indicesType: options.type || '',
        indexType: options.method || undefined,
-        indexName: options.name || Utils.inflection.underscore(rawTablename + '_' + onlyAttributeNames.join('_')),
+        indexName: options.name,
        parser: null
      });

--- a/lib/dialects/mysql/query-generator.js
+++ b/lib/dialects/mysql/query-generator.js
@@ -223,7 +223,7 @@ module.exports = (function() {
    showIndexQuery: function(tableName, options) {
      var sql = 'SHOW INDEX FROM <%= tableName %><%= options %>';
      return Utils._.template(sql)({
-        tableName: this.quoteIdentifiers(tableName),
+        tableName: this.quoteTable(tableName),
        options: (options || {}).database ? ' FROM `' + options.database + '`' : ''
      });
    },

--- a/lib/dialects/sqlite/query-generator.js
+++ b/lib/dialects/sqlite/query-generator.js
@@ -318,7 +318,7 @@ module.exports = (function() {
    showIndexQuery: function(tableName) {
      var sql = "PRAGMA INDEX_LIST(<%= tableName %>)";
-      return Utils._.template(sql, { tableName: this.quoteIdentifiers(tableName) });
+      return Utils._.template(sql, { tableName: this.quoteTable(tableName) });
    },
    removeIndexQuery: function(tableName, indexNameOrAttributes) {

--- a/lib/model.js
+++ b/lib/model.js
@@ -405,7 +405,18 @@ module.exports = (function() {
    }).then(function () {
      return self.QueryInterface.createTable(self.getTableName(), attributes, options);
    }).then(function () {
-      return Promise.map(self.options.indexes, function (index) {
+      return self.QueryInterface.showIndex(self.getTableName(), options);
+    }).then(function (indexes) {
+      // Assign an auto-generated name to indexes which are not named by the user
+      self.options.indexes = self.QueryInterface.nameIndexes(self.options.indexes, self.tableName);
+      indexes = Utils._.filter(self.options.indexes, function (item1) {
+        return !Utils._.any(indexes, function (item2) {
+          return item1.name === item2.name;
+        });
+      });
+      return Promise.map(indexes, function (index) {
        return self.QueryInterface.addIndex(self.getTableName(), index.fields, index, self.tableName);
      });
    }).return(this);

--- a/lib/query-interface.js
+++ b/lib/query-interface.js
@@ -388,13 +388,19 @@ module.exports = (function() {
      rawTablename = tableName;
    }
+    options = options || {};
    var sql = this.QueryGenerator.addIndexQuery(tableName, attributes, options, rawTablename);
-    return this.sequelize.query(sql, null, {logging: this.sequelize.options.logging});
+    return this.sequelize.query(sql, null, {logging: options.hasOwnProperty('logging') ? options.logging : this.sequelize.options.logging});
  };
  QueryInterface.prototype.showIndex = function(tableName, options) {
    var sql = this.QueryGenerator.showIndexQuery(tableName, options);
-    return this.sequelize.query(sql, null, {logging: this.sequelize.options.logging});
+    options = options || {};
+    return this.sequelize.query(sql, null, {logging: options.hasOwnProperty('logging') ? options.logging : this.sequelize.options.logging});
+  };
+  QueryInterface.prototype.nameIndexes = function(indexes, rawTablename) {
+    return this.QueryGenerator.nameIndexes(indexes, rawTablename);
  };
  QueryInterface.prototype.getForeignKeysForTables = function(tableNames) {

--- a/test/dao-factory.test.js
+++ b/test/dao-factory.test.js
@@ -304,11 +304,11 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        bCol: { type: Sequelize.STRING, unique: 'a_and_b' }
      })
-      User.sync({ force: true }).on('sql', _.after(2, function(sql) {
+      User.sync({ force: true }).on('sql', _.after(2, _.once(function(sql) {
        expect(sql).to.match(/UNIQUE\s*(user_and_email)?\s*\([`"]?username[`"]?, [`"]?email[`"]?\)/)
        expect(sql).to.match(/UNIQUE\s*(a_and_b)?\s*\([`"]?aCol[`"]?, [`"]?bCol[`"]?\)/)
        done()
-      }))
+      })))
    })
    it('allows us to customize the error message for unique constraint', function(done) {
@@ -353,6 +353,8 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
      })
      return this.sequelize.sync().bind(this).then(function () {
+        return this.sequelize.sync(); // The second call should not try to create the indices again
+      }).then(function () {
        return this.sequelize.queryInterface.showIndex(Model.tableName);
      }).spread(function () {
        var primary, idx1, idx2;
@@ -1759,14 +1761,14 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
      var run = function() {
        UserPub.sync({ force: true }).success(function() {
-          ItemPub.sync({ force: true }).on('sql', _.after(2, function(sql) {
+          ItemPub.sync({ force: true }).on('sql', _.after(2, _.once(function(sql) {
            if (dialect === "postgres") {
              expect(sql).to.match(/REFERENCES\s+"prefix"\."UserPubs" \("id"\)/)
            } else {
              expect(sql).to.match(/REFERENCES\s+`prefix\.UserPubs` \(`id`\)/)
            }
            done()
-          }))
+          })))
        })
      }
@@ -1853,7 +1855,7 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
      Post.belongsTo(this.Author)
      // The posts table gets dropped in the before filter.
-      Post.sync().on('sql', function(sql) {
+      Post.sync().on('sql', _.once(function(sql) {
        if (dialect === 'postgres') {
          expect(sql).to.match(/"authorId" INTEGER REFERENCES "authors" \("id"\)/)
        } else if (Support.dialectIsMySQL()) {
@@ -1865,7 +1867,7 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        }
        done()
-      })
+      }))
    })
    it('uses a table name as a string and references the author table', function(done) {
@@ -1883,7 +1885,7 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
      Post.belongsTo(this.Author)
      // The posts table gets dropped in the before filter.
-      Post.sync().on('sql', function(sql) {
+      Post.sync().on('sql', _.once(function(sql) {
        if (dialect === 'postgres') {
          expect(sql).to.match(/"authorId" INTEGER REFERENCES "authors" \("id"\)/)
        } else if (Support.dialectIsMySQL()) {
@@ -1895,7 +1897,7 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        }
        done()
-      })
+      }))
    })
    it("emits an error event as the referenced table name is invalid", function(done) {
@@ -2348,10 +2350,10 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        str: { type: Sequelize.STRING, unique: true }
      })
-      uniqueTrue.sync({force: true}).on('sql', _.after(2, function(s) {
+      uniqueTrue.sync({force: true}).on('sql', _.after(2, _.once(function(s) {
        expect(s).to.match(/UNIQUE/)
        done()
-      }))
+      })))
    })
    it("should not set unique when unique is false", function(done) {
@@ -2360,10 +2362,10 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        str: { type: Sequelize.STRING, unique: false }
      })
-      uniqueFalse.sync({force: true}).on('sql', _.after(2, function(s) {
+      uniqueFalse.sync({force: true}).on('sql', _.after(2, _.once(function(s) {
        expect(s).not.to.match(/UNIQUE/)
        done()
-      }))
+      })))
    })
    it("should not set unique when unique is unset", function(done) {
@@ -2372,10 +2374,10 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        str: { type: Sequelize.STRING }
      })
-      uniqueUnset.sync({force: true}).on('sql', _.after(2, function(s) {
+      uniqueUnset.sync({force: true}).on('sql', _.after(2, _.once(function(s) {
        expect(s).not.to.match(/UNIQUE/)
        done()
-      }))
+      })))
    })
  })
@@ -2403,5 +2405,4 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
    return this.sequelize.sync({force: true});
  });
 })
--- a/test/query-interface.test.js
+++ b/test/query-interface.test.js
@@ -53,7 +53,7 @@ describe(Support.getTestDialectTeaser("QueryInterface"), function () {
      }).success(function() {
        self.queryInterface.dropAllTables({skip: ['skipme']}).complete(function(err){
          expect(err).to.be.null
          self.queryInterface.showAllTables().complete(function(err, tableNames) {
            expect(err).to.be.null