Fix incorrect rowCount value when using groupBy with fetchPage

lib/plugins/pagination.js

@@ -108,7 +108,11 @@ module.exports = function paginationPlugin (bookshelf) {
     const offset = options.offset;
     const fetchOptions = _.omit(options, ['page', 'pageSize', 'limit', 'offset']);
     const transacting = fetchOptions.transacting;
-
+    const fetchMethodName = isModel ? 'fetchAll' : 'fetch';
+    const targetModel = isModel ? this.constructor : (this.target || this.model);
+    const tableName = targetModel.prototype.tableName;
+    const idAttribute = targetModel.prototype.idAttribute || 'id';
+    const targetIdColumn = [`${tableName}.${idAttribute}`];
     let usingPageSize = false; // usingPageSize = false means offset/limit, true means page/pageSize
     let _page;
     let _pageSize;
@@ -135,12 +139,6 @@ module.exports = function paginationPlugin (bookshelf) {
       _offset = ensureIntWithDefault(offset, DEFAULT_OFFSET);
     }
 
-    const fetchMethodName = isModel ? 'fetchAll' : 'fetch';
-    const targetModel = isModel ? this.constructor : (this.target || this.model);
-    const tableName = targetModel.prototype.tableName;
-    const idAttribute = targetModel.prototype.idAttribute || 'id';
-    const targetIdColumn = `${tableName}.${idAttribute}`;
-
     const paginate = () => {
       const pager = this.clone();
 
@@ -156,6 +154,7 @@ module.exports = function paginationPlugin (bookshelf) {
     const count = () => {
       const notNeededQueries = ['orderByBasic', 'orderByRaw', 'groupByBasic', 'groupByRaw'];
       const counter = this.clone();
+      const groupColumns = [];
 
       return counter.query(qb => {
         Object.assign(qb, this.query().clone());
@@ -164,6 +163,9 @@ module.exports = function paginationPlugin (bookshelf) {
         // for a count, and grouping returns the entire result set
         // What we want instead is to use `DISTINCT`
         _.remove(qb._statements, statement => {
+          if (statement.grouping === 'group')
+            statement.value.forEach(value => groupColumns.push(`${tableName}.${value}`));
+
           return notNeededQueries.indexOf(statement.type) > -1 || statement.grouping === 'columns';
         });
 
@@ -173,7 +175,7 @@ module.exports = function paginationPlugin (bookshelf) {
           counter.relatedData.joinColumns = function() {};
         }
 
-        qb.countDistinct.apply(qb, [targetIdColumn]);
+        qb.countDistinct.apply(qb, groupColumns.length > 0 ? groupColumns : targetIdColumn);
       })[fetchMethodName]({transacting}).then(result => {
         const metadata = usingPageSize ? {page: _page, pageSize: _limit} : {offset: _offset, limit: _limit};
 

test/integration/plugins/pagination.js

@@ -5,7 +5,7 @@ module.exports = function (bookshelf) {
     bookshelf.plugin('pagination');
     var Models = require('../helpers/objects')(bookshelf).Models;
 
-    describe('Model instance fetchPage', function () {
+    describe('Model#fetchPage()', function () {
       it('fetches a single page of results with defaults', function () {
         return Models.Customer.forge().fetchPage().then(function (results) {
           ['models', 'pagination'].forEach(function (prop) {
@@ -76,7 +76,7 @@ module.exports = function (bookshelf) {
       })
     })
 
-    describe('Model static fetchPage', function () {
+    describe('Model.fetchPage()', function () {
       it('fetches a page without calling forge', function () {
         return Models.Customer.fetchPage().then(function (results) {
           ['models', 'pagination'].forEach(function (prop) {
@@ -86,14 +86,15 @@ module.exports = function (bookshelf) {
       })
     })
 
-    describe('Collection fetchPage', function () {
+    describe('Collection#fetchPage()', function () {
       it('fetches a page from a collection', function () {
         return Models.Customer.collection().fetchPage().then(function (results) {
           ['models', 'pagination'].forEach(function (prop) {
             expect(results).to.have.property(prop);
           });
         })
       })
+
       it('fetches a page from a relation collection', function () {
         return Models.User.forge({uid: 1}).roles().fetchPage().then(function (results) {
           expect(results.length).to.equal(1);
@@ -102,6 +103,7 @@ module.exports = function (bookshelf) {
           });
         });
       })
+
       it('fetches a page from a relation collection with additional condition', function () {
         return Models.User.forge({uid: 1}).roles().query(function (query) {
           query.where('roles.rid', '!=', 4);
@@ -114,7 +116,7 @@ module.exports = function (bookshelf) {
       })
     })
 
-    describe('Inside a transaction', function() {
+    describe('inside a transaction', function() {
       it('returns consistent results for rowCount and number of models', function() {
         return bookshelf.transaction(function(t) {
           var options = {transacting: t};
@@ -129,5 +131,24 @@ module.exports = function (bookshelf) {
         });
       })
     })
+
+    describe('with groupBy', function() {
+      it('counts grouped rows instead of total rows', function() {
+        var total
+
+        return Models.Blog.count().then(function(count) {
+          total = parseInt(count, 10);
+
+          return Models.Blog.query(function(qb) {
+            qb.max('id');
+            qb.groupBy('site_id');
+            qb.whereNotNull('site_id');
+          }).fetchPage()
+        }).then(function(blogs) {
+          expect(blogs.pagination.rowCount).to.equal(blogs.length);
+          expect(blogs.length).to.be.below(total);
+        })
+      })
+    })
   });
 };