First step towards implementing an internal transaction in findOrCreate

.jshintrc

@@ -26,7 +26,7 @@
   "globals": {
     "Promise": true
   },
-  
+
   "predef": [
     "alert",
     "describe",

lib/dialects/mariadb/query.js

@@ -96,7 +96,7 @@ module.exports = (function() {
             errorDetected = true;
             self.promise.emit('sql', self.sql);
             err.sql = sql;
-            self.reject(err);
+            self.reject(self.formatError(err));
           })
           .on('end', function(info) {
             if (alreadyEnded || errorDetected) {

lib/dialects/mysql/query.js

@@ -2,7 +2,8 @@
 
 var Utils = require('../../utils')
   , AbstractQuery = require('../abstract/query')
-  , uuid = require('node-uuid');
+  , uuid = require('node-uuid')
+  , sequelizeErrors = require('../../errors.js');
 
 module.exports = (function() {
   var Query = function(connection, sequelize, callee, options) {
@@ -35,7 +36,8 @@ module.exports = (function() {
 
         if (err) {
           err.sql = sql;
-          reject(err);
+
+          reject(self.formatError(err));
         } else {
           resolve(self.formatResults(results));
         }
@@ -46,6 +48,24 @@ module.exports = (function() {
     return promise;
   };
 
+  Query.prototype.formatError = function (err) {
+    var match;
+
+    switch (err.errno || err.code) {
+    case 1062:
+      match = err.message.match(/Duplicate entry '(.*)' for key '(.*?)'$/);
+
+      return new sequelizeErrors.UniqueConstraintError({
+        fields: null,
+        index: match[2],
+        value: match[1],
+        parent: err
+      });
+    }
+
+    return new sequelizeErrors.DatabaseError(err);
+  };
+
   Query.prototype.isShowIndexesQuery = function () {
     return this.sql.toLowerCase().indexOf('show index from') === 0;
   };

lib/dialects/postgres/query.js

@@ -5,7 +5,8 @@ var Utils = require('../../utils')
   , DataTypes = require('../../data-types')
   , hstore = require('./hstore')
   , QueryTypes = require('../../query-types')
-  , Promise = require('../../promise');
+  , Promise = require('../../promise')
+  , sequelizeErrors = require('../../errors.js');
 
 // Parses hstore fields if the model has any hstore fields.
 // This cannot be done in the 'pg' lib because hstore is a UDT.
@@ -61,7 +62,7 @@ module.exports = (function() {
         receivedError = true;
         err.sql = sql;
         promise.emit('sql', sql, self.client.uuid);
-        reject(err);
+        reject(self.formatError(err));
       });
 
       query.on('end', function(result) {
@@ -221,6 +222,24 @@ module.exports = (function() {
     return this;
   };
 
+  Query.prototype.formatError = function (err) {
+    var match;
+
+    switch (err.code) {
+    case '23505':
+      match = err.detail.match(/Key \((.*?)\)=\((.*?)\) already exists/);
+
+      return new sequelizeErrors.UniqueConstraintError({
+        fields: match[1].split(', '),
+        value: match[2].split(', '),
+        index: null,
+        parent: err
+      });
+    }
+
+    return new sequelizeErrors.DatabaseError(err);
+  };
+
   Query.prototype.isShowIndexesQuery = function () {
     return this.sql.indexOf('pg_get_indexdef') !== -1;
   };

lib/dialects/sqlite/query.js

@@ -2,7 +2,8 @@
 
 var Utils = require('../../utils')
   , AbstractQuery = require('../abstract/query')
-  , QueryTypes = require('../../query-types');
+  , QueryTypes = require('../../query-types')
+  , sequelizeErrors = require('../../errors.js');
 
 module.exports = (function() {
   var Query = function(database, sequelize, callee, options) {
@@ -48,10 +49,9 @@ module.exports = (function() {
               self.database[self.getDatabaseMethod()](self.sql, function(err, results) {
                 // allow clients to listen to sql to do their own logging or whatnot
                 promise.emit('sql', self.sql, self.options.uuid);
-
                 if (err) {
                   err.sql = self.sql;
-                  reject(err);
+                  reject(self.formatError(err));
                 } else {
                   var metaData = this;
                   metaData.columnTypes = columnTypes;
@@ -173,6 +173,36 @@ module.exports = (function() {
     });
   };
 
+  Query.prototype.formatError = function (err) {
+    var match;
+
+    switch (err.code) {
+    case 'SQLITE_CONSTRAINT':
+      match = err.message.match(/columns (.*?) are/); // Sqlite pre 2.2 behavior - Error: SQLITE_CONSTRAINT: columns x, y are not unique
+      if (match !== null && match.length >= 2) {
+        return new sequelizeErrors.UniqueConstraintError(match[1].split(', '),null, err);
+      }
+
+      match = err.message.match(/UNIQUE constraint failed: (.*)/); // Sqlite 2.2 behavior - Error: SQLITE_CONSTRAINT: UNIQUE constraint failed: table.x, table.y
+      if (match !== null && match.length >= 2) {
+        var fields = match[1].split(', ').map(function (columnWithTable) {
+          return columnWithTable.split('.')[1];
+        });
+
+        return new sequelizeErrors.UniqueConstraintError({
+          fields: fields,
+          index: null,
+          value: null,
+          parent: err
+        });
+      }
+
+      return err;
+    }
+
+    return new sequelizeErrors.DatabaseError(err);
+  };
+
   Query.prototype.handleShowIndexesQuery = function (data) {
     var self = this;
 

lib/errors.js

@@ -57,6 +57,26 @@ error.ValidationError.prototype.get = function(path) {
   }, []);
 };
 
+error.DatabaseError = function (parent) {
+  error.BaseError.apply(this, arguments);
+  this.name = 'SequelizeDatabaseError';
+
+  this.parent = parent;
+  this.sql = parent.sql;
+};
+util.inherits(error.DatabaseError, error.BaseError);
+
+error.UniqueConstraintError = function (options) {
+  error.DatabaseError.call(this, options.parent);
+  this.name = 'SequelizeUniqueConstraintError';
+
+  this.message = options.message;
+  this.fields = options.fields;
+  this.value = options.value;
+  this.index = options.index;
+};
+util.inherits(error.UniqueConstraintError, error.DatabaseError);
+
 /**
  * An array of ValidationErrorItems
  * @property errors

lib/instance.js

@@ -568,21 +568,25 @@ module.exports = (function() {
           });
         }
       }).then(function() {
-        return self.QueryInterface[query].apply(self.QueryInterface, args).catch(function(err) {
-          if (!!self.__options.uniqueKeys && err.code && self.QueryInterface.QueryGenerator.uniqueConstraintMapping.code === err.code) {
-            var index = self.QueryInterface.QueryGenerator.uniqueConstraintMapping.map(err.toString());
+        return self.QueryInterface[query].apply(self.QueryInterface, args).catch(self.sequelize.UniqueConstraintError, function(err) {
+          if (!!self.__options.uniqueKeys && self.QueryInterface.QueryGenerator.uniqueConstraintMapping.code === err.parent.code) {
+            var index = self.QueryInterface.QueryGenerator.uniqueConstraintMapping.map(err.parent.toString());
 
             if (index !== false) {
               var fields = index.fields.filter(function(f) { return f !== self.Model.tableName; });
 
               Utils._.each(self.__options.uniqueKeys, function(uniqueKey) {
                 if (!!uniqueKey.msg && (Utils._.isEqual(uniqueKey.fields, fields)) || uniqueKey.name === index.indexName) {
-                  err = new Error(uniqueKey.msg);
+                  err = new self.sequelize.UniqueConstraintError({
+                    message: uniqueKey.msg,
+                    fields: fields,
+                    index: index.indexName,
+                    parent: err.parent
+                  });
                 }
               });
             }
           }
-
           throw err;
         }).tap(function(result) {
           // Transfer database generated values (defaults, autoincrement, etc)

lib/model.js

@@ -1077,43 +1077,71 @@ module.exports = (function() {
    * Find a row that matches the query, or build and save the row if none is found
    * The successfull result of the promise will be (instance, created) - Make sure to use .spread()
    *
-   * @param {Object}  where A hash of search attributes. Note that this method differs from finders, in that the syntax is `{ attr1: 42 }` and NOT `{ where: { attr1: 42}}`. This is subject to change in 2.0
-   * @param {Object}  [defaults] Default values to use if creating a new instance
-   * @param {Object}  [options] Options passed to the find and create calls
-   * @deprecated The syntax is due for change, in order to make `where` more consistent with the rest of the API
+   * @param {Object}  [options]
+   * @param {Object}  [options.where] where A hash of search attributes. Note that this method differs from finders, in that the syntax is `{ attr1: 42 }` and NOT `{ where: { attr1: 42}}`. This is subject to change in 2.0
+   * @param {Object}  [options.defaults] Default values to use if creating a new instance
+   * @param {Object}  [queryOptions] Options passed to the find and create calls
    *
-   * @return {Promise<Instance>}
+   * @return {Promise<Instance,created>}
    */
-  Model.prototype.findOrCreate = function(where, defaults, options) {
+  Model.prototype.findOrCreate = function(options, queryOptions) {
     var self = this
+      , internalTransaction = !(queryOptions && queryOptions.transaction)
       , values = {};
 
-    options = Utils._.extend({
-      transaction: null
-    }, options || {});
+    queryOptions = queryOptions ? Utils._.clone(queryOptions) : {};
+
+    if (!options.where) {
+      throw new Error('Missing where attribute in the first parameter passed to findOrCreate. Please note that the API has changed, and is now options (an object with where and defaults keys), queryOptions (transaction etc.)');
+    }
 
-    if (!(where instanceof Utils.or) && !(where instanceof Utils.and) && !Array.isArray(where)) {
-      for (var attrname in where) {
-        values[attrname] = where[attrname];
+    if (!options.where._isSequelizeMethod && !Array.isArray(options.where)) {
+      for (var attrname in options.where) {
+        values[attrname] = options.where[attrname];
       }
     }
 
-    return self.find({
-      where: where
-    }, {
-      transaction: options.transaction
-    }).then(function(instance) {
-      if (instance === null) {
-        for (var attrname in defaults) {
-          values[attrname] = defaults[attrname];
+    // Create a transaction or a savepoint, depending on whether a transaction was passed in
+    return self.sequelize.transaction(queryOptions).bind(this).then(function (transaction) {
+      this.transaction = transaction;
+      queryOptions.transaction = transaction;
+
+      return self.find(options, {
+        transaction: transaction,
+        lock: transaction.LOCK.UPDATE
+      }).then(function(instance) {
+        if (instance !== null) {
+          return [instance, false];
+        }
+
+        for (var attrname in options.defaults) {
+          values[attrname] = options.defaults[attrname];
         }
 
-        return self.create(values, options).then(function(instance) {
-          return Promise.resolve([instance, true]);
+        return self.create(values, queryOptions).then(function(instance) {
+          return [instance, true];
+        }).catch(self.sequelize.UniqueConstraintError, function () {
+          // Someone must have created a matching instance inside the same transaction since we last did a find. Let's find it!
+          // Postgres errors when there is a constraint violation inside a transaction, so we must first rollback to the savepoint we created before
+          return transaction.rollback().then(function () {
+            transaction = transaction.parent;
+
+            return self.find(options, {
+              transaction: transaction,
+              lock: transaction.LOCK.SHARE
+            });
+          }).then(function(instance) {
+            return [instance, false];
+          });
         });
+      });
+    }).spread(function (instance, created) {
+      if (!internalTransaction) {
+        return Promise.resolve([instance, created]);
       }
 
-      return Promise.resolve([instance, false]);
+      // If we created a transaction internally, we should clean it up
+      return this.transaction.commit().return([instance, created]);
     });
   };
 

lib/sequelize.js

@@ -247,6 +247,12 @@ module.exports = (function() {
   Sequelize.prototype.ValidationErrorItem = Sequelize.ValidationErrorItem =
     sequelizeErrors.ValidationErrorItem;
 
+  Sequelize.prototype.DatabaseError = Sequelize.DatabaseError =
+    sequelizeErrors.DatabaseError;
+
+  Sequelize.prototype.UniqueConstraintError = Sequelize.UniqueConstraintError =
+    sequelizeErrors.UniqueConstraintError;
+
   /**
    * Returns the specified dialect.
    *

lib/transaction.js

@@ -22,6 +22,7 @@ var Transaction = module.exports = function(sequelize, options) {
     this.id = this.options.transaction.id;
     this.options.transaction.savepoints.push(this);
     this.name = this.id + '-savepoint-' + this.options.transaction.savepoints.length;
+    this.parent = this.options.transaction;
   } else {
     this.id = this.name = Utils.generateUUID();
   }

test/dao-factory.test.js

@@ -312,16 +312,17 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
     })
 
     it('allows us to customize the error message for unique constraint', function(done) {
-      var User = this.sequelize.define('UserWithUniqueUsername', {
-        username: { type: Sequelize.STRING, unique: { name: 'user_and_email', msg: 'User and email must be unique' }},
-        email: { type: Sequelize.STRING, unique: 'user_and_email' },
-        aCol: { type: Sequelize.STRING, unique: 'a_and_b' },
-        bCol: { type: Sequelize.STRING, unique: 'a_and_b' }
-      })
+      var self = this
+        , User = this.sequelize.define('UserWithUniqueUsername', {
+            username: { type: Sequelize.STRING, unique: { name: 'user_and_email', msg: 'User and email must be unique' }},
+            email: { type: Sequelize.STRING, unique: 'user_and_email' },
+            aCol: { type: Sequelize.STRING, unique: 'a_and_b' },
+            bCol: { type: Sequelize.STRING, unique: 'a_and_b' }
+          })
 
       User.sync({ force: true }).success(function() {
         User.create({username: 'tobi', email: 'tobi@tobi.me'}).success(function() {
-          User.create({username: 'tobi', email: 'tobi@tobi.me'}).error(function(err) {
+          User.create({username: 'tobi', email: 'tobi@tobi.me'}).catch(self.sequelize.UniqueConstraintError, function(err) {
             expect(err.message).to.equal('User and email must be unique')
             done()
           })

test/dao-factory/create.test.js

@@ -2,6 +2,7 @@
 /* jshint expr: true */
 var chai      = require('chai')
   , Sequelize = require('../../index')
+  , Promise   = Sequelize.Promise
   , expect    = chai.expect
   , Support   = require(__dirname + '/../support')
   , DataTypes = require(__dirname + "/../../lib/data-types")
@@ -40,7 +41,7 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
           .sync({ force: true })
           .success(function() {
             sequelize.transaction().then(function(t) {
-              User.findOrCreate({ username: 'Username' }, { data: 'some data' }, { transaction: t }).complete(function(err) {
+              User.findOrCreate({ where: { username: 'Username' }, defaults: { data: 'some data' }}, { transaction: t }).complete(function(err) {
                 expect(err).to.be.null
 
                 User.count().success(function(count) {
@@ -65,9 +66,9 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
         };
 
       this.User.create(data).success(function (user) {
-        self.User.findOrCreate({
+        self.User.findOrCreate({ where: {
           username: user.username
-        }).spread(function (_user, created) {
+        }}).spread(function (_user, created) {
           expect(_user.id).to.equal(user.id)
           expect(_user.username).to.equal('Username')
           expect(created).to.be.false
@@ -84,7 +85,7 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
         };
 
       this.User.create(data).success(function (user) {
-        self.User.findOrCreate(data).done(function (err, _user, created) {
+        self.User.findOrCreate({where: data}).done(function (err, _user, created) {
           expect(_user.id).to.equal(user.id)
           expect(_user.username).to.equal('Username')
           expect(_user.data).to.equal('ThisIsData')
@@ -102,7 +103,7 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
           data: 'ThisIsData'
         };
 
-      this.User.findOrCreate(data, default_values).success(function(user, created) {
+      this.User.findOrCreate({ where: data, defaults: default_values}).success(function(user, created) {
         expect(user.username).to.equal('Username')
         expect(user.data).to.equal('ThisIsData')
         expect(created).to.be.true
@@ -111,10 +112,10 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
     })
 
     it("supports .or() (only using default values)", function (done) {
-      this.User.findOrCreate(
-        Sequelize.or({username: 'Fooobzz'}, {secretValue: 'Yolo'}),
-        {username: 'Fooobzz', secretValue: 'Yolo'}
-      ).done(function (err, user, created) {
+      this.User.findOrCreate({
+        where: Sequelize.or({username: 'Fooobzz'}, {secretValue: 'Yolo'}),
+        defaults: {username: 'Fooobzz', secretValue: 'Yolo'}
+      }).done(function (err, user, created) {
         expect(err).not.to.be.ok
         expect(user.username).to.equal('Fooobzz')
         expect(user.secretValue).to.equal('Yolo')
@@ -123,7 +124,48 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
         done()
       })
     })
-  })
+
+    describe.only('several concurrent calls', function () {
+      beforeEach(function () {
+        var self = this;
+        return Support.prepareTransactionTest(this.sequelize, function(sequelize) {
+          self.sequelize = sequelize;
+
+          // self.User = sequelize.define('User', {
+          //   username: DataTypes.STRING,
+          // });
+        });
+      });
+
+      it('works with a transaction', function () {
+        return this.sequelize.transaction().bind(this).then(function (transaction) {
+          return Promise.join(
+            this.User.findOrCreate({ where: { uniqueName: 'winner' }}, { transaction: transaction }),
+            this.User.findOrCreate({ where: { uniqueName: 'winner' }}, { transaction: transaction }),
+            function (first, second) {
+              console.log(first);
+              console.log(second);
+              // expect(first[1]).to.be.ok;
+              // expect(second[1]).not.to.be.ok;
+
+              return transaction.commit();
+            }
+          );
+        });
+      });
+
+      // it('works without a transaction', function () {
+      //   return Promise.join(
+      //     this.User.findOrCreate({ where: { uniqueName: 'winner' }}),
+      //     this.User.findOrCreate({ where: { uniqueName: 'winner' }}),
+      //     function (first, second) {
+      //       expect(first[1]).to.be.ok;
+      //       expect(second[1]).not.to.be.ok;
+      //     }
+      //   );
+      // });
+    });
+  });
 
   describe('create', function() {
     it('works with non-integer primary keys with a default value', function (done) {
@@ -389,24 +431,14 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
     })
 
     it("doesn't allow duplicated records with unique:true", function(done) {
-      var User = this.sequelize.define('UserWithUniqueUsername', {
-        username: { type: Sequelize.STRING, unique: true }
-      })
+      var self = this
+        , User = this.sequelize.define('UserWithUniqueUsername', {
+            username: { type: Sequelize.STRING, unique: true }
+          })
 
       User.sync({ force: true }).success(function() {
         User.create({ username:'foo' }).success(function() {
-          User.create({ username: 'foo' }).error(function(err) {
-            expect(err).to.exist
-
-            if (dialect === "sqlite") {
-              expect(err.message).to.match(/.*SQLITE_CONSTRAINT.*/)
-            }
-            else if (Support.dialectIsMySQL()) {
-              expect(err.message).to.match(/.*Duplicate\ entry.*/)
-            } else {
-              expect(err.message).to.match(/.*duplicate\ key\ value.*/)
-            }
-
+          User.create({ username: 'foo' }).catch(self.sequelize.UniqueConstraintError, function(err) {
             done()
           })
         })
@@ -441,25 +473,17 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
       })
     })
     it("raises an error if created object breaks definition contraints", function(done) {
-      var UserNull = this.sequelize.define('UserWithNonNullSmth', {
-        username: { type: Sequelize.STRING, unique: true },
-        smth:     { type: Sequelize.STRING, allowNull: false }
-      })
+      var self = this
+        , UserNull = this.sequelize.define('UserWithNonNullSmth', {
+            username: { type: Sequelize.STRING, unique: true },
+            smth:     { type: Sequelize.STRING, allowNull: false }
+          })
 
       this.sequelize.options.omitNull = false
 
       UserNull.sync({ force: true }).success(function() {
         UserNull.create({ username: 'foo', smth: 'foo' }).success(function() {
-          UserNull.create({ username: 'foo', smth: 'bar' }).error(function(err) {
-            expect(err).to.exist
-            if (dialect === "sqlite") {
-              expect(err.message).to.match(/.*SQLITE_CONSTRAINT.*/)
-            }
-            else if (Support.dialectIsMySQL()) {
-              expect(err.message).to.match(/Duplicate entry 'foo' for key 'username'/)
-            } else {
-              expect(err.message).to.match(/.*duplicate key value violates unique constraint.*/)
-            }
+          UserNull.create({ username: 'foo', smth: 'bar' }).catch(self.sequelize.UniqueConstraintError, function(err) {
             done()
           })
         })

test/dao-factory/scopes.test.js

@@ -26,7 +26,7 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
     return this.User.sync({ force: true });
   });
 
-  describe('scopes', function() {
+  describe.only('scopes', function() {
     beforeEach(function() {
       this.ScopeMe = this.sequelize.define('ScopeMe', {
         username: Sequelize.STRING,
@@ -267,7 +267,7 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
     });
 
     it("should have no problem performing findOrCreate", function() {
-      return this.ScopeMe.findOrCreate({username: 'fake'}).spread(function(user) {
+      return this.ScopeMe.findOrCreate({ where: {username: 'fake'}}).spread(function(user) {
         expect(user.username).to.equal('fake');
       });
     });

test/error.test.js

+"use strict";
+
 /* jshint camelcase: false */
 var chai      = require('chai')
+  , sinon     = require('sinon')
   , expect    = chai.expect
   , Support   = require(__dirname + '/support')
-  , Sequelize = Support.Sequelize;
+  , Sequelize = Support.Sequelize
+  , Promise   = Sequelize.Promise;
 
 chai.config.includeStack = true;
 
@@ -49,7 +53,35 @@ describe(Support.getTestDialectTeaser("Sequelize Errors"), function () {
       var matches = validationError.get('first_name');
       expect(matches).to.be.instanceOf(Array);
       expect(matches).to.have.lengthOf(1);
-      expect(matches[0]).to.have.property('message', 'invalid')
+      expect(matches[0]).to.have.property('message', 'invalid');
+    });
+  });
+
+  describe('Constraint error', function () {
+    it('Can be intercepted using .catch', function () {
+      var spy = sinon.spy()
+        , User = this.sequelize.define('user', {
+          first_name: {
+            type: Sequelize.STRING,
+            unique: 'unique_name'
+          },
+          last_name: {
+            type: Sequelize.STRING,
+            unique: 'unique_name'
+          }
+        });
+
+      return this.sequelize.sync({ force: true }).bind(this).then(function () {
+        return User.create({ first_name: 'jan', last_name: 'meier' });
+      }).then(function () {
+        return Promise.join(
+          User.create({ first_name: 'jan', last_name: 'meier' }).catch(this.sequelize.UniqueConstraintError, spy),
+          User.create({ first_name: 'jan', last_name: 'meier' }).catch(this.sequelize.ConstraintError, spy),
+          function () {
+            expect(spy).to.have.been.calledTwice;
+          }
+        );
+      });
     });
-  })
+  });
 });

test/promise.test.js

@@ -321,7 +321,7 @@ describe(Support.getTestDialectTeaser("Promise"), function () {
     describe('with spread', function () {
       it('user not created', function (done) {
         this.User
-          .findOrCreate({ id: 1})
+          .findOrCreate({ where: { id: 1}})
           .spread(function(user, created) {
             expect(user.id).to.equal(1)
             expect(created).to.equal(false)
@@ -331,7 +331,7 @@ describe(Support.getTestDialectTeaser("Promise"), function () {
       })
       it('user created', function (done) {
         this.User
-          .findOrCreate({ id: 2})
+          .findOrCreate({ where: { id: 2}})
           .spread(function(user, created) {
             expect(user.id).to.equal(2)
             expect(created).to.equal(true)

test/support.js

@@ -74,7 +74,7 @@ var Support = {
 
     var sequelizeOptions = _.defaults(options, {
       host:           options.host || config.host,
-      logging:        false,
+      // logging:        false,
       dialect:        options.dialect,
       port:           options.port || process.env.SEQ_PORT || config.port,
       pool:           config.pool,