fix(Model): findOrCreate should fail explicitely, not timeout, when a… (#6030)

fix(Model): findOrCreate should fail explicitely, not timeout, when altering hooks before create

Dockerfile

-FROM node:6
+FROM node:4
 
 RUN apt-get install libpq-dev
 

lib/dialects/mssql/query.js

@@ -200,6 +200,7 @@ Query.prototype.handleShowTablesQuery = function(results) {
 
 Query.prototype.formatError = function (err) {
   var match;
+
   match = err.message.match(/Violation of UNIQUE KEY constraint '((.|\s)*)'. Cannot insert duplicate key in object '.*'.(:? The duplicate key value is \((.*)\).)?/);
   match = match || err.message.match(/Cannot insert duplicate key row in object .* with unique index '(.*)'/);
   if (match && match.length > 1) {
@@ -210,12 +211,12 @@ Query.prototype.formatError = function (err) {
     if (uniqueKey && !!uniqueKey.msg) {
       message = uniqueKey.msg;
     }
-    if (!!match[2]) {
-      var values = match[2].split(',').map(Function.prototype.call, String.prototype.trim);
+    if (!!match[4]) {
+      var values = match[4].split(',').map(Function.prototype.call, String.prototype.trim);
       if (!!uniqueKey) {
         fields = Utils._.zipObject(uniqueKey.fields, values);
       } else {
-        fields[match[1]] = match[2];
+        fields[match[1]] = match[4];
       }
     }
 

lib/model.js

@@ -631,12 +631,7 @@ class Model {
       }
     }.bind(this));
 
-    this.attributes = this.rawAttributes = _.mapValues(attributes, function(attribute, name) {
-      if (!Utils._.isPlainObject(attribute)) {
-        attribute = { type: attribute };
-      }
-
-
+    this.attributes = this.rawAttributes = _.mapValues(attributes, (attribute, name) => {
       attribute = this.sequelize.normalizeAttribute(attribute);
 
       if (attribute.references && attribute.references.model && attribute.references.model.prototype instanceof Model) {
@@ -648,7 +643,7 @@ class Model {
       }
 
       return attribute;
-    }.bind(this));
+    });
 
     this.modelManager = modelManager;
     this.primaryKeys = {};
@@ -1913,12 +1908,8 @@ class Model {
     var self = this
       , internalTransaction = !options.transaction
       , values
-      , whereFields = Object.keys(options.where)
-      , defaultFields
       , transaction;
 
-    if (options.defaults) defaultFields = Object.keys(options.defaults);
-
     // Create a transaction or a savepoint, depending on whether a transaction was passed in
     return self.sequelize.transaction(options).bind({}).then(function (t) {
       transaction = t;
@@ -1947,12 +1938,24 @@ class Model {
 
         return [instance, true];
       }).catch(self.sequelize.UniqueConstraintError, function (err) {
+        let whereFields = Object.keys(options.where).map(name => self.rawAttributes[name].field || name)
+          , defaultFields = options.defaults && Object.keys(options.defaults).map(name => self.rawAttributes[name].field || name);
+
         if (defaultFields) {
-          if (!_.intersection(err.fields, whereFields).length && _.intersection(err.fields, defaultFields).length) {
+          if (!_.intersection(Object.keys(err.fields), whereFields).length && _.intersection(Object.keys(err.fields), defaultFields).length) {
             throw err;
           }
         }
 
+        if (_.intersection(Object.keys(err.fields), whereFields).length) {
+          _.each(err.fields, (value, key) => {
+            let name = self.fieldRawAttributesMap[key].fieldName;
+            if (value.toString() !== options.where[name].toString()) {
+              throw new Error(`${self.name}#findOrCreate: value used for ${name} was not equal for both the find and the create calls, '${options.where[name]}' vs '${value}'`);
+            }
+          });
+        }
+
         // Someone must have created a matching instance inside the same transaction since we last did a find. Let's find it!
         return self.findOne(_.defaults({
           transaction: internalTransaction ? null : transaction

test/integration/model/create.test.js

@@ -394,6 +394,45 @@ describe(Support.getTestDialectTeaser('Model'), function() {
         });
       }
 
+      (dialect !== 'sqlite' && dialect !== 'mssql' ? it : it.skip)('should not fail silently with concurrency higher than pool, a unique constraint and a create hook resulting in mismatched values', function() {
+        var User = this.sequelize.define('user', {
+          username: {
+            type: DataTypes.STRING,
+            unique: true,
+            field: 'user_name'
+          }
+        });
+
+        User.beforeCreate(instance => {
+          instance.set('username', instance.get('username').trim());
+        });
+
+        let spy = sinon.spy();
+
+        let names = [
+          'mick ',
+          'mick ',
+          'mick ',
+          'mick ',
+          'mick ',
+          'mick ',
+          'mick '
+        ];
+
+        return User.sync({force: true}).then(() => {
+          return Promise.all(
+            names.map(username => {
+              return User.findOrCreate({where: {username}}).catch(err => {
+                spy();
+                expect(err.message).to.equal(`user#findOrCreate: value used for username was not equal for both the find and the create calls, 'mick ' vs 'mick'`);
+              });
+            })
+          );
+        }).then(() => {
+          expect(spy).to.have.been.called;
+        });
+      });
+
       (dialect !== 'sqlite' ? it : it.skip)('should error correctly when defaults contain a unique key without a transaction', function () {
         var User = this.sequelize.define('user', {
           objectId: {