Fix wrong count for with required includes [#4016](https://github.com/sequelize…

…/sequelize/pull/4016)

changelog.md

+# Next
+- [FEATURE] Geometry support for postgres
+- [BUG] Fix wrong count for `findAndCountAll` with required includes [#4016](https://github.com/sequelize/sequelize/pull/4016)
+
 # 3.3.2
 - [FIXED] upsert no longer updates with default values each time [#3994](https://github.com/sequelize/sequelize/pull/3994)
 

docs/docs/models-usage.md

@@ -84,12 +84,12 @@ User
 
 ### findAndCountAll - Search for multiple elements in the database, returns both data and total count
 
-This is a convienience method that combines`findAll`()and `count`()(see below), this is useful when dealing with queries related to pagination where you want to retrieve data with a `limit` and `offset` but also need to know the total number of records that match the query.
+This is a convienience method that combines`findAll` and `count` (see below) this is useful when dealing with queries related to pagination where you want to retrieve data with a `limit` and `offset` but also need to know the total number of records that match the query:
 
-The success handler will always receive an object with two properties:
+The success handler will always receive an object with two properties:
 
-* `count` - an integer, total number records (matching the where clause)
-* `rows` - an array of objects, the records (matching the where clause) within the limit/offset range
+* `count` - an intege, total number records matching the where clause
+* `rows` - an array of objects, the records matching the where clause, within the limit and offset range
 ```js
 Project
   .findAndCountAll({
@@ -107,7 +107,33 @@ Project
   });
 ```
 
-The options [object] that you pass to`findAndCountAll`()is the same as for`findAll`()(described below).
+`findAndCountAll` also supports includes. Only the includes that are marked as `required` will be added to the count part:
+
+Suppose you want to find all users who have a profile attached:
+```js
+User.findAndCountAll({
+  include: [
+     { model: Profile, required: true}
+  ],
+  limit 3
+});
+```
+
+Because the include for `Profile` has `required` set it will result in an inner join, and only the users who have a profile will be counted. If we remove `required` from the include, both users with and without profiles will be counted. Adding a `where` clause to the include automatically makes it required:
+
+```js
+User.findAndCountAll({
+  include: [
+     { model: Profile, where: { active: true }}
+  ],
+  limit 3
+});
+```
+
+The query above will only count users who have an active profile, because `required` is implicitly set to true when you add a where clause to the include.
+
+
+The options object that you pass to `findAndCountAll` is the same as for `findAll` (described below).
 
 ### findAll - Search for multiple elements in the database
 ```js

lib/model.js

@@ -831,7 +831,7 @@ Model.prototype.refreshAttributes = function() {
   this._hasGeometryAttributes = !!this._geometryAttributes.length;
   this._isGeometryAttribute = Utils._.memoize(function(key) {
     return self._geometryAttributes.indexOf(key) !== -1;
-  });  
+  });
 
   this._hasDefaultValues = !Utils._.isEmpty(this._defaultValues);
 
@@ -1341,6 +1341,7 @@ Model.prototype.count = function(options) {
   Model.$injectScope(this.$scope, options);
 
   var col = '*';
+
   if (options.include) {
     col = this.name + '.' + this.primaryKeyField;
     expandIncludeAll.call(this, options);
@@ -1371,6 +1372,19 @@ Model.prototype.count = function(options) {
  * ```
  * In the above example, `result.rows` will contain rows 13 through 24, while `result.count` will return the total number of rows that matched your query.
  *
+ * When you add includes, only those which are required (either because they have a where clause, or because `required` is explicitly set to true on the include) will be added to the count part.
+ *
+ * Suppose you want to find all users who have a profile attached:
+ * ```js
+ * User.findAndCountAll({
+ *   include: [
+ *      { model: Profile, required: true}
+ *   ],
+ *   limit 3
+ * });
+ * ```
+ * Because the include for `Profile` has `required` set it will result in an inner join, and only the users who have a profile will be counted. If we remove `required` from the include, both users with and without profiles will be counted
+ *
  * @param {Object} [findOptions] See findAll
  *
  * @see {Model#findAll} for a specification of find and query options
@@ -1387,6 +1401,7 @@ Model.prototype.findAndCount = function(findOptions) {
     , countOptions = _.omit(_.clone(findOptions), ['offset', 'limit', 'order', 'attributes']);
 
   conformOptions(countOptions, this);
+
   if (countOptions.include) {
     countOptions.include = _.cloneDeep(countOptions.include, function (element) {
       if (element instanceof Model) return element;
@@ -1406,6 +1421,11 @@ Model.prototype.findAndCount = function(findOptions) {
       });
     };
     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;
+    }
   }
 
   return self.count(countOptions).then(function(count) {

test/integration/include/findAndCountAll.test.js

@@ -181,40 +181,29 @@ describe(Support.getTestDialectTeaser('Include'), function() {
       });
     });
 
-    it.only('should return the correct count and rows when using a required belongsTo with a where condition and a limit', function() {
-      var s = this.sequelize
-        , Foo = s.define('Foo', {})
-        , Bar = s.define('Bar', {m: DataTypes.STRING(40)});
+    it('should return the correct count and rows when using a required belongsTo with a where condition and a limit', function() {
+      var Foo = this.sequelize.define('Foo', {})
+        , Bar = this.sequelize.define('Bar', {m: DataTypes.STRING(40)});
 
       Foo.hasMany(Bar);
       Bar.belongsTo(Foo);
 
-      return s.sync({ force: true }).then(function() {
-        // Make five instances of Foo
+      return this.sequelize.sync({ force: true }).then(function() {
         return Foo.bulkCreate([{id: 1}, {id: 2}, {id: 3}, {id: 4}, {id: 5}]);
       }).then(function() {
-        // Make four instances of Bar, related to the last four instances of Foo
+        // Make four instances of Bar, related to the first two instances of Foo
         return Bar.bulkCreate([{'FooId': 1, m:'yes'}, {'FooId': 1, m:'yes'}, {'FooId': 1, m: 'no'}, {'FooId': 2, m: 'yes'}]);
       }).then(function() {
         // Query for the first instance of Foo which have related Bars with m === 'yes'
         return Foo.findAndCountAll({
-          include: [{ model: Bar, required: true, where: { m: 'yes' } }],
+          include: [{ model: Bar, where: { m: 'yes' } }],
           limit: 1
-        }).tap(function() {
-          // Check for what should be returned as the `count` of the result
-          return Foo.findAll({
-            include: [{ model: Bar, required: true, where: { m: 'yes' } }]
-          }).then(function(items) {
-            expect(items.length).to.equal(2);
-          });
         });
       }).then(function(result) {
-        // There should be 2 instances matching the query (Instances 1 and 2),
-        // see the findAll statement
+        // There should be 2 instances matching the query (Instances 1 and 2), see the findAll statement
         expect(result.count).to.equal(2);
 
-        // The first one of those should be returned due to the limit (Foo
-        // instance 1)
+        // The first one of those should be returned due to the limit (Foo instance 1)
         expect(result.rows.length).to.equal(1);
       });
     });