Fix includes being removed from count queries (#6192)

Add unit test

Don't set distinct to true as a default

changelog.md

@@ -23,6 +23,7 @@
 - [FIXED] Index names not quoted properly in `removeIndex` [#5888](https://github.com/sequelize/sequelize/issues/5888)
 - [FIXED] `Int4` range not properly parsed [#5747](https://github.com/sequelize/sequelize/issues/5747)
 - [FIXED] `upsert` does not fail anymore on not null validations [#5711](https://github.com/sequelize/sequelize/issues/5711)
+- [FIXED] Don't remove includes from count queries and unify findAndCount and count queries. [#6123](https://github.com/sequelize/sequelize/issues/6123)
 
 ## BC breaks:
 - Range type bounds now default to [postgres default](https://www.postgresql.org/docs/9.5/static/rangetypes.html#RANGETYPES-CONSTRUCT) `[)` (inclusive, exclusive), previously was `()` (exclusive, exclusive)

lib/model.js

@@ -1521,7 +1521,7 @@ class Model {
    * @param {Object}        [options.where] A hash of search attributes.
    * @param {Object}        [options.include] Include options. See `find` for details
    * @param {Boolean}       [options.paranoid=true] Set `true` to count only non-deleted records. Can be used on models with `paranoid` enabled
-   * @param {Boolean}       [options.distinct] Apply COUNT(DISTINCT(col)) on primary key, `Model.aggregate` should be used for other columns
+   * @param {Boolean}       [options.distinct] Apply COUNT(DISTINCT(col)) on primary key or on options.col.
    * @param {String}        [options.col] Column on which COUNT() should be applied
    * @param {Object}        [options.attributes] Used in conjunction with `group`
    * @param {Object}        [options.group] For creating complex counts. Will return multiple rows as needed.
@@ -1533,35 +1533,28 @@ class Model {
    * @return {Promise<Integer>}
    */
   static count(options) {
-    options = Utils.cloneDeep(options);
-    _.defaults(options, { hooks: true });
-
-    let col = options.col || '*';
-    let attributes = options.attributes;
-
     return Promise.try(() => {
-      this._conformOptions(options, this);
-      this.$injectScope(options);
-
+      options = _.defaults(Utils.cloneDeep(options), { hooks: true });
       if (options.hooks) {
         return this.runHooks('beforeCount', options);
       }
     }).then(() => {
+      let col = options.col || '*';
       if (options.include) {
         col = this.name + '.' + this.primaryKeyField;
-        this._expandIncludeAll(options);
-        this._validateIncludedElements(options);
       }
 
       Utils.mapOptionFieldNames(options, this);
 
-      options.plain = options.group ? false : true;
+      options.plain = !options.group;
       options.dataType = new DataTypes.INTEGER();
       options.includeIgnoreAttributes = false;
+
+      // No limit, offset or order for the options max be given to count()
+      // Set them to null to prevent scopes setting those values
       options.limit = null;
       options.offset = null;
       options.order = null;
-      options.attributes = attributes || [];
 
       return this.aggregate(col, 'count', options);
     });
@@ -1605,37 +1598,10 @@ class Model {
       throw new Error('The argument passed to findAndCount must be an options object, use findById if you wish to pass a single primary key value');
     }
 
-    // no limit, offset, order, attributes for the options given to count()
-    const countOptions = _.omit(_.clone(options), ['offset', 'limit', 'order', 'attributes']);
-
-    this._conformOptions(countOptions, this);
-
-    if (countOptions.include) {
-      countOptions.include = _.cloneDeepWith(countOptions.include, element => {
-        if (element instanceof Model) return element;
-        if (element instanceof Association) return element;
-        return undefined;
-      });
-
-      this._expandIncludeAll(countOptions);
-
-      this._validateIncludedElements(countOptions);
-
-      const keepNeeded = includes => {
-        return includes.filter(include => {
-          if (include.include) include.include = keepNeeded(include.include);
-
-          return include.required || include.hasIncludeRequired;
-        });
-      };
-      countOptions.include = keepNeeded(countOptions.include);
-
-      if (countOptions.include.length) {
-        // Use distinct to count the number of parent rows, instead of the number of matched includes
-        countOptions.distinct = true;
-      }
+    const countOptions = Utils.cloneDeep(options);
+    if (countOptions.attributes) {
+      countOptions.attributes = undefined;
     }
-
     return this.count(countOptions).then(count => {
       if (count === 0) {
         return {
@@ -1645,7 +1611,7 @@ class Model {
       }
       return this.findAll(options).then(results => ({
         count: count || 0,
-        rows: (results && Array.isArray(results) ? results : [])
+        rows: results
       }));
     });
   }

test/integration/include/findAndCountAll.test.js

@@ -148,7 +148,8 @@ describe(Support.getTestDialectTeaser('Include'), function() {
       }).then(function() {
         return User.findAndCountAll({
           where: {id: userId},
-          include: [Project]
+          include: [Project],
+          distinct: true
         });
       }).then(function(result) {
         expect(result.rows.length).to.equal(1);
@@ -209,7 +210,8 @@ describe(Support.getTestDialectTeaser('Include'), function() {
         // Query for the first instance of Foo which have related Bars with m === 'yes'
         return Foo.findAndCountAll({
           include: [{ model: Bar, where: { m: 'yes' } }],
-          limit: 1
+          limit: 1,
+          distinct: true
         });
       }).then(function(result) {
         // There should be 2 instances matching the query (Instances 1 and 2), see the findAll statement

test/integration/model.test.js

@@ -1772,24 +1772,22 @@ describe(Support.getTestDialectTeaser('Model'), function() {
       });
     });
 
-    it('supports distinct option', function() {
-      var Post = this.sequelize.define('Post', {});
-      var PostComment = this.sequelize.define('PostComment', {});
+    it('supports distinct option', function () {
+      const Post = this.sequelize.define('Post', {});
+      const PostComment = this.sequelize.define('PostComment', {});
       Post.hasMany(PostComment);
-      return Post.sync({ force: true }).then(function() {
-        return PostComment.sync({ force: true }).then(function() {
-          return Post.create({}).then(function(post) {
-            return PostComment.bulkCreate([{ PostId: post.id },{ PostId: post.id }]).then(function() {
-              return Post.count({ include: [{ model: PostComment, required: false }] }).then(function(count1) {
-                return Post.count({ distinct: true, include: [{ model: PostComment, required: false }] }).then(function(count2) {
+      return Post.sync({ force: true })
+        .then(() => PostComment.sync({ force: true }))
+        .then(() => Post.create({}))
+        .then((post) => PostComment.bulkCreate([{ PostId: post.id },{ PostId: post.id }]))
+        .then(() => Promise.join(
+            Post.count({ distinct: false, include: [{ model: PostComment, required: false }] }),
+            Post.count({ distinct: true, include: [{ model: PostComment, required: false }] }),
+          (count1, count2) => {
             expect(count1).to.equal(2);
             expect(count2).to.equal(1);
-                });
-              });
-            });
-          });
-        });
-      });
+          })
+        );
     });
 
   });

test/integration/model/count.test.js

@@ -133,5 +133,22 @@ describe(Support.getTestDialectTeaser('Model'), function() {
       });
     });
 
+    it('should be able to use where clause on included models', function() {
+      const queryObject = {
+        col: 'username',
+        include: [this.Project],
+        where: {
+          '$Projects.name$': 'project1'
+        }
+      };
+      return this.User.count(queryObject).then((count) => {
+        expect(parseInt(count)).to.be.eql(1);
+        queryObject.where['$Projects.name$'] = 'project2';
+        return this.User.count(queryObject);
+      }).then((count) => {
+        expect(parseInt(count)).to.be.eql(0);
+      });
+    });
+
   });
 });

test/unit/model/count.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'), () => {
+  describe('method count', () => {
+    before(() => {
+      this.oldFindAll = current.Model.findAll;
+      this.oldAggregate = current.Model.aggregate;
+
+      current.Model.findAll = sinon.stub().returns(Promise.resolve());
+
+      this.User = current.define('User', {
+        username: DataTypes.STRING,
+        age: DataTypes.INTEGER
+      });
+      this.Project = current.define('Project', {
+        name: DataTypes.STRING
+      });
+
+      this.User.hasMany(this.Project);
+      this.Project.belongsTo(this.User);
+    });
+
+    after(() => {
+      current.Model.findAll = this.oldFindAll;
+      current.Model.aggregate = this.oldAggregate;
+    });
+
+    beforeEach(() => {
+      this.stub = current.Model.aggregate = sinon.stub().returns(Promise.resolve());
+    });
+
+    describe('should pass the same options to model.aggregate as findAndCount', () => {
+      it('with includes', () => {
+        const queryObject = {
+          include: [this.Project]
+        };
+        return this.User.count(queryObject)
+          .then(() => this.User.findAndCount(queryObject))
+          .then(() => {
+            const count = this.stub.getCall(0).args;
+            const findAndCount = this.stub.getCall(1).args;
+            expect(count).to.eql(findAndCount);
+          });
+      });
+
+      it('attributes should be stripped in case of findAndCount', () => {
+        const queryObject = {
+          attributes: ['username']
+        };
+        return this.User.count(queryObject)
+          .then(() => this.User.findAndCount(queryObject))
+          .then(() => {
+            const count = this.stub.getCall(0).args;
+            const findAndCount = this.stub.getCall(1).args;
+            expect(count).not.to.eql(findAndCount);
+            count[2].attributes = undefined;
+            expect(count).to.eql(findAndCount);
+          });
+      });
+    });
+
+  });
+});