bug(findOne) Add limit when querying for a range on the primary key. Closes #4416

changelog.md

@@ -6,6 +6,7 @@
 - [INTERNALS] Updated devDependencies [#4594](https://github.com/sequelize/sequelize/pull/4594)
 - [INTERNALS] Updated devDependencies [#4594](https://github.com/sequelize/sequelize/pull/4594)
     + mysql@2.9.0
     + mysql@2.9.0
     - coffee-script
     - coffee-script
+- [FIXED] Add limit to `findOne` when using queries like `{ id: { $gt ...` [#4416](https://github.com/sequelize/sequelize/issues/4416)
 
 
 # 3.10.0
 # 3.10.0
 - [ADDED] support `search_path` for postgres with lots of schemas [#4534](https://github.com/sequelize/sequelize/pull/4534)
 - [ADDED] support `search_path` for postgres with lots of schemas [#4534](https://github.com/sequelize/sequelize/pull/4534)

lib/instance.js

@@ -8,7 +8,6 @@ var Utils = require('./utils')
   , Dottie = require('dottie')
   , Dottie = require('dottie')
   , Promise = require('./promise')
   , Promise = require('./promise')
   , _ = require('lodash')
   , _ = require('lodash')
-  , primitives = ['string', 'number', 'boolean']
   , defaultsOptions = { raw: true };
   , defaultsOptions = { raw: true };
 
 
 // private
 // private
@@ -159,7 +158,7 @@ Instance.prototype.getDataValue = function(key) {
  */
  */
 Instance.prototype.setDataValue = function(key, value) {
 Instance.prototype.setDataValue = function(key, value) {
   var originalValue = this._previousDataValues[key];
   var originalValue = this._previousDataValues[key];
-  if (primitives.indexOf(typeof value) === -1 || value !== originalValue) {
+  if (!Utils.isPrimitive(value) || value !== originalValue) {
     this.changed(key, true);
     this.changed(key, true);
   }
   }
 
 
@@ -373,7 +372,7 @@ Instance.prototype.set = function(key, value, options) { // testhint options:non
           value = this.Model.attributes[key].type.parse(value);
           value = this.Model.attributes[key].type.parse(value);
         }
         }
 
 
-        if (!options.raw && ((primitives.indexOf(typeof value) === -1 && value !== null) || value !== originalValue)) {
+        if (!options.raw && ((!Utils.isPrimitive(value) && value !== null) || value !== originalValue)) {
           this._previousDataValues[key] = originalValue;
           this._previousDataValues[key] = originalValue;
           this.changed(key, true);
           this.changed(key, true);
         }
         }
@@ -1050,4 +1049,4 @@ Instance.prototype.toJSON = function() {
   });
   });
 };
 };
 
 
-module.exports = Instance;
+module.exports = Instance;
\ No newline at end of file

lib/model.js

@@ -1468,8 +1468,13 @@ Model.prototype.findOne = function(options) {
   }
   }
   options = optClone(options);
   options = optClone(options);
 
 
-  if (options.limit === undefined && !(options.where && options.where[this.primaryKeyAttribute])) {
-    options.limit = 1;
+  if (options.limit === undefined) {
+    var pkVal = options.where && options.where[this.primaryKeyAttribute];
+
+    // Don't add limit if querying directly on the pk
+    if (!options.where || !(Utils.isPrimitive(pkVal) || Buffer.isBuffer(pkVal))) {
+      options.limit = 1;
+    }
   }
   }
 
 
   // Bypass a possible overloaded findAll.
   // Bypass a possible overloaded findAll.

lib/utils.js

@@ -6,7 +6,8 @@ var DataTypes = require('./data-types')
   , parameterValidator = require('./utils/parameter-validator')
   , parameterValidator = require('./utils/parameter-validator')
   , inflection = require('inflection')
   , inflection = require('inflection')
   , uuid = require('node-uuid')
   , uuid = require('node-uuid')
-  , deprecate = require('depd')('Utils');
+  , deprecate = require('depd')('Utils')
+  , primitives = ['string', 'number', 'boolean'];
 
 
 var Utils = module.exports = {
 var Utils = module.exports = {
   inflection: inflection,
   inflection: inflection,
@@ -29,6 +30,9 @@ var Utils = module.exports = {
 
 
     return result;
     return result;
   },
   },
+  isPrimitive: function (val) {
+    return primitives.indexOf(typeof val) !== -1;
+  },
   // Same concept as _.merge, but don't overwrite properties that have already been assigned
   // Same concept as _.merge, but don't overwrite properties that have already been assigned
   mergeDefaults: function (a, b) {
   mergeDefaults: function (a, b) {
     return this._.merge(a, b, function (objectValue, sourceValue) {
     return this._.merge(a, b, function (objectValue, sourceValue) {
@@ -76,7 +80,7 @@ var Utils = module.exports = {
     });
     });
   },
   },
 
 
-  /* Expand and normalize finder options */ 
+  /* Expand and normalize finder options */
   mapFinderOptions: function(options, Model) {
   mapFinderOptions: function(options, Model) {
     if (Model._hasVirtualAttributes && options.attributes) {
     if (Model._hasVirtualAttributes && options.attributes) {
       options.attributes.forEach(function (attribute) {
       options.attributes.forEach(function (attribute) {

test/unit/model/findone.test.js

+'use strict';
+
+/* jshint -W030 */
+var chai = require('chai')
+  , expect = chai.expect
+  , Support = require(__dirname + '/../support')
+  , current = Support.sequelize
+  , sinon = require('sinon')
+  , DataTypes = require(__dirname + '/../../../lib/data-types')
+  , Promise = require('bluebird');
+
+describe(Support.getTestDialectTeaser('Model'), function() {
+  describe('method findOne', function () {
+    before(function () {
+      this.oldfindAll = current.Model.prototype.findAll;
+    });
+    after(function () {
+      current.Model.prototype.findAll = this.oldfindall;
+    });
+
+    beforeEach(function () {
+      this.stub = current.Model.prototype.findAll = sinon.stub().returns(Promise.resolve());
+    });
+
+    describe('should not add limit when querying on a primary key', function () {
+      it('with id primary key', function () {
+        var Model = current.define('model');
+
+        return Model.findOne({ where: { id: 42 }}).bind(this).then(function () {
+          expect(this.stub.getCall(0).args[0]).to.be.an('object').not.to.have.property('limit');
+        });
+      });
+
+      it('with custom primary key', function () {
+        var Model = current.define('model', {
+            uid: {
+              type: DataTypes.INTEGER,
+              primaryKey: true,
+              autoIncrement: true
+            }
+          });
+
+        return Model.findOne({ where: { uid: 42 }}).bind(this).then(function () {
+          expect(this.stub.getCall(0).args[0]).to.be.an('object').not.to.have.property('limit');
+        });
+      });
+
+      it('with blob primary key', function () {
+        var Model = current.define('model', {
+          id: {
+            type: DataTypes.BLOB,
+            primaryKey: true,
+            autoIncrement: true
+          }
+        });
+
+        return Model.findOne({ where: { id: new Buffer('foo') }}).bind(this).then(function () {
+          expect(this.stub.getCall(0).args[0]).to.be.an('object').not.to.have.property('limit');
+        });
+      });
+    });
+
+    it('should add limit when using { $ gt on the primary key', function () {
+      var Model = current.define('model');
+
+      return Model.findOne({ where: { id: { $gt: 42 }}}).bind(this).then(function () {
+        expect(this.stub.getCall(0).args[0]).to.be.an('object').to.have.property('limit');
+      });
+    });
+  });
+});