Merge pull request #5432 from ErisDS/api-pagination2

Refactor pagination count query

core/server/models/post.js

@@ -355,57 +355,13 @@ Post = ghostBookshelf.Model.extend({
     findPage: function findPage(options) {
         options = options || {};
 
-        var tagInstance = options.tag !== undefined ? ghostBookshelf.model('Tag').forge({slug: options.tag}) : false,
+        // -- Part 0 --
+        // Step 1: Setup filter models
+        var self = this,
+            tagInstance = options.tag !== undefined ? ghostBookshelf.model('Tag').forge({slug: options.tag}) : false,
             authorInstance = options.author !== undefined ? ghostBookshelf.model('User').forge({slug: options.author}) : false;
 
-        if (options.limit && options.limit !== 'all') {
+        // Step 2: Setup filter model promises
-            options.limit = parseInt(options.limit, 10) || 15;
-        }
-
-        if (options.page) {
-            options.page = parseInt(options.page, 10) || 1;
-        }
-
-        options = this.filterOptions(options, 'findPage');
-
-        // Set default settings for options
-        options = _.extend({
-            page: 1, // pagination page
-            limit: 15,
-            staticPages: false, // include static pages
-            status: 'published',
-            where: {}
-        }, options);
-
-        if (options.staticPages !== 'all') {
-            // convert string true/false to boolean
-            if (!_.isBoolean(options.staticPages)) {
-                options.staticPages = options.staticPages === 'true' || options.staticPages === '1' ? true : false;
-            }
-            options.where.page = options.staticPages;
-        }
-
-        if (options.featured) {
-            // convert string true/false to boolean
-            if (!_.isBoolean(options.featured)) {
-                options.featured = options.featured === 'true' || options.featured === '1' ? true : false;
-            }
-            options.where.featured = options.featured;
-        }
-
-        // Unless `all` is passed as an option, filter on
-        // the status provided.
-        if (options.status !== 'all') {
-            // make sure that status is valid
-            options.status = _.indexOf(['published', 'draft'], options.status) !== -1 ? options.status : 'published';
-            options.where.status = options.status;
-        }
-
-        // Add related objects
-        options.withRelated = _.union(options.withRelated, options.include);
-
-        // If a query param for a tag is attached
-        // we need to fetch the tag model to find its id
         function fetchTagQuery() {
             if (tagInstance) {
                 return tagInstance.fetch();
@@ -420,92 +376,132 @@ Post = ghostBookshelf.Model.extend({
             return false;
         }
 
-        return Promise.join(fetchTagQuery(), fetchAuthorQuery())
+        // Step 3: Prefetch filter models
-            // Set the limit & offset for the query, fetching
+        return Promise.join(fetchTagQuery(), fetchAuthorQuery()).then(function setupCollectionPromises() {
-            // with the opts (to specify any eager relations, etc.)
+            // -- Part 1 --
-            // Omitting the `page`, `limit`, `where` just to be sure
+            var postCollection = Posts.forge(),
-            // aren't used for other purposes.
+                collectionPromise,
-            .then(function then() {
+                countPromise;
-                var postCollection = Posts.forge(),
-                    collectionPromise,
-                    countPromise,
-                    qb;
 
-                // If there are where conditionals specified, add those
+            // Step 1: Setup pagination options
-                // to the query.
+            if (options.limit && options.limit !== 'all') {
-                if (options.where) {
+                options.limit = parseInt(options.limit, 10) || 15;
-                    postCollection.query('where', options.where);
+            }
+
+            if (options.page) {
+                options.page = parseInt(options.page, 10) || 1;
+            }
+
+            // Step 2: Filter options
+            options = self.filterOptions(options, 'findPage');
+
+            // Step 3: Extend defaults
+            options = _.extend({
+                page: 1, // pagination page
+                limit: 15,
+                staticPages: false, // include static pages
+                status: 'published',
+                where: {}
+            }, options);
+
+            // Step 4: Setup filters (where clauses)
+            if (options.staticPages !== 'all') {
+                // convert string true/false to boolean
+                if (!_.isBoolean(options.staticPages)) {
+                    options.staticPages = options.staticPages === 'true' || options.staticPages === '1' ? true : false;
                 }
-                // If we have a tag instance we need to modify our query.
+                options.where.page = options.staticPages;
-                // We need to ensure we only select posts that contain
+            }
-                // the tag given in the query param.
+
-                if (tagInstance) {
+            if (options.featured) {
-                    postCollection
+                // convert string true/false to boolean
-                        .query('join', 'posts_tags', 'posts_tags.post_id', '=', 'posts.id')
+                if (!_.isBoolean(options.featured)) {
-                        .query('where', 'posts_tags.tag_id', '=', tagInstance.id);
+                    options.featured = options.featured === 'true' || options.featured === '1' ? true : false;
                 }
+                options.where.featured = options.featured;
+            }
 
-                if (authorInstance) {
+            // Unless `all` is passed as an option, filter on
-                    postCollection
+            // the status provided.
-                        .query('where', 'author_id', '=', authorInstance.id);
+            if (options.status !== 'all') {
+                // make sure that status is valid
+                options.status = _.indexOf(['published', 'draft'], options.status) !== -1 ? options.status : 'published';
+                options.where.status = options.status;
+            }
+
+            // If there are where conditionals specified, add those to the query.
+            if (options.where) {
+                postCollection.query('where', options.where);
+            }
+
+            // Step 5: Setup joins
+            if (tagInstance) {
+                postCollection
+                    .query('join', 'posts_tags', 'posts_tags.post_id', '=', 'posts.id')
+                    .query('where', 'posts_tags.tag_id', '=', tagInstance.id);
+            }
+
+            if (authorInstance) {
+                postCollection
+                    .query('where', 'author_id', '=', authorInstance.id);
+            }
+
+            // Step 6: Setup the counter to fetch the number of items in the set
+            // @TODO abstract this out
+            // tableName = _.result(postCollection, 'tableName'),
+            // idAttribute = _.result(postCollection, 'idAttribute');
+            countPromise = postCollection.query().clone().count('posts.id as aggregate');
+
+            // -- Part 2 --
+            // Add limit, offset and other query changes which aren't required when performing a count
+
+            // Step 1: Add related objects
+            options.withRelated = _.union(options.withRelated, options.include);
+
+            // Step 2: Add pagination options if needed
+            if (_.isNumber(options.limit)) {
+                postCollection
+                    .query('limit', options.limit)
+                    .query('offset', options.limit * (options.page - 1));
+            }
+
+            // Step 3: add order parameters
+            postCollection
+                .query('orderBy', 'status', 'ASC')
+                .query('orderBy', 'published_at', 'DESC')
+                .query('orderBy', 'updated_at', 'DESC')
+                .query('orderBy', 'id', 'DESC');
+
+            // Step 4: Setup the promise
+            collectionPromise = postCollection.fetch(_.omit(options, 'page', 'limit'));
+
+            // -- Part 3 --
+            // Step 1: Fetch the data
+            return Promise.join(collectionPromise, countPromise);
+        }).then(function formatResponse(results) {
+            var postCollection = results[0],
+                data = {};
+
+            // Step 2: Format the data
+            data.posts = postCollection.toJSON(options);
+            data.meta = {pagination: paginateResponse(results[1][0].aggregate, options)};
+
+            if (tagInstance) {
+                data.meta.filters = {};
+                if (!tagInstance.isNew()) {
+                    data.meta.filters.tags = [tagInstance.toJSON(options)];
                 }
+            }
 
-                if (_.isNumber(options.limit)) {
+            if (authorInstance) {
-                    postCollection
+                data.meta.filters = {};
-                        .query('limit', options.limit)
+                if (!authorInstance.isNew()) {
-                        .query('offset', options.limit * (options.page - 1));
+                    data.meta.filters.author = authorInstance.toJSON(options);
                 }
+            }
 
-                collectionPromise = postCollection
+            return data;
-                    .query('orderBy', 'status', 'ASC')
+        }).catch(errors.logAndThrowError);
-                    .query('orderBy', 'published_at', 'DESC')
-                    .query('orderBy', 'updated_at', 'DESC')
-                    .query('orderBy', 'id', 'DESC')
-                    .fetch(_.omit(options, 'page', 'limit'));
-
-                // Find the total number of posts
-
-                qb = ghostBookshelf.knex('posts');
-
-                if (options.where) {
-                    qb.where(options.where);
-                }
-
-                if (tagInstance) {
-                    qb.join('posts_tags', 'posts_tags.post_id', '=', 'posts.id');
-                    qb.where('posts_tags.tag_id', '=', tagInstance.id);
-                }
-                if (authorInstance) {
-                    qb.where('author_id', '=', authorInstance.id);
-                }
-
-                countPromise = qb.count('posts.id as aggregate');
-
-                return Promise.join(collectionPromise, countPromise);
-            }).then(function then(results) {
-                var postCollection = results[0],
-                    data = {};
-
-                data.posts = postCollection.toJSON(options);
-                data.meta = {pagination: paginateResponse(results[1][0].aggregate, options)};
-
-                if (tagInstance) {
-                    data.meta.filters = {};
-                    if (!tagInstance.isNew()) {
-                        data.meta.filters.tags = [tagInstance.toJSON(options)];
-                    }
-                }
-
-                if (authorInstance) {
-                    data.meta.filters = {};
-                    if (!authorInstance.isNew()) {
-                        data.meta.filters.author = authorInstance.toJSON(options);
-                    }
-                }
-
-                return data;
-            })
-            .catch(errors.logAndThrowError);
     },
 
     /**

core/server/models/tag.js

@@ -110,10 +110,17 @@ Tag = ghostBookshelf.Model.extend({
     findPage: function findPage(options) {
         options = options || {};
 
+        // -- Part 0 --
+        // Step 1: Setup filter models
+        // Step 2: Setup filter model promises
+        // Step 3: Prefetch filter models
+
+        // -- Part 1 --
         var tagCollection = Tags.forge(),
             collectionPromise,
-            qb;
+            countPromise;
 
+        // Step 1: Setup pagination options
         if (options.limit && options.limit !== 'all') {
             options.limit = parseInt(options.limit, 10) || 15;
         }
@@ -122,42 +129,60 @@ Tag = ghostBookshelf.Model.extend({
             options.page = parseInt(options.page, 10) || 1;
         }
 
+        // Step 2: Filter options
         options = this.filterOptions(options, 'findPage');
-        // Set default settings for options
+
+        // Step 3: Extend defaults
         options = _.extend({
             page: 1, // pagination page
             limit: 15,
             where: {}
         }, options);
 
-        // only include a limit-query if a numeric limit is provided
+        // Step 4: Setup filters (where clauses)
+        // If there are where conditionals specified, add those to the query.
+        if (options.where) {
+            tagCollection.query('where', options.where);
+        }
+
+        // Step 5: Setup joins
+
+        // Step 6: Setup the counter to fetch the number of items in the set
+        // @TODO abstract this out
+        // tableName = _.result(postCollection, 'tableName'),
+        // idAttribute = _.result(postCollection, 'idAttribute');
+        countPromise = tagCollection.query().clone().count('tags.id as aggregate');
+
+        // -- Part 2 --
+        // Add limit, offset and other query changes which aren't required when performing a count
+
+        // Step 1: Add related objects
+        addPostCount(options, tagCollection);
+        options.withRelated = _.union(options.withRelated, options.include);
+
+        // Step 2: Add pagination options if needed
         if (_.isNumber(options.limit)) {
             tagCollection
                 .query('limit', options.limit)
                 .query('offset', options.limit * (options.page - 1));
         }
 
-        addPostCount(options, tagCollection);
+        // Step 3: add order parameters
 
+        // Step 4: Setup the promise
         collectionPromise = tagCollection.fetch(_.omit(options, 'page', 'limit'));
 
-        // Find total number of tags
+        // -- Part 3 --
-        qb = ghostBookshelf.knex('tags');
+        // Step 1: Fetch the data
-
+        return Promise.join(collectionPromise, countPromise).then(function formatResponse(results) {
-        if (options.where) {
-            qb.where(options.where);
-        }
-
-        return Promise.join(collectionPromise, qb.count('tags.id as aggregate')).then(function then(results) {
             var tagCollection = results[0],
                 data = {};
-
+            // Step 2: Format the data
             data.tags = tagCollection.toJSON(options);
             data.meta = {pagination: paginateResponse(results[1][0].aggregate, options)};
 
             return data;
-        })
+        }).catch(errors.logAndThrowError);
-        .catch(errors.logAndThrowError);
     },
     destroy: function destroy(options) {
         var id = options.id;

core/server/models/user.js

@@ -225,65 +225,12 @@ User = ghostBookshelf.Model.extend({
     findPage: function findPage(options) {
         options = options || {};
 
-        var userCollection = Users.forge(),
+        // -- Part 0 --
+        // Step 1: Setup filter models
+        var self = this,
             roleInstance = options.role !== undefined ? ghostBookshelf.model('Role').forge({name: options.role}) : false;
 
-        if (options.limit && options.limit !== 'all') {
+        // Step 2: Setup filter model promises
-            options.limit = parseInt(options.limit, 10) || 15;
-        }
-
-        if (options.page) {
-            options.page = parseInt(options.page, 10) || 1;
-        }
-
-        options = this.filterOptions(options, 'findPage');
-
-        // Set default settings for options
-        options = _.extend({
-            page: 1, // pagination page
-            limit: 15,
-            status: 'active',
-            where: {},
-            whereIn: {}
-        }, options);
-
-        // TODO: there are multiple statuses that make a user "active" or "invited" - we a way to translate/map them:
-        // TODO (cont'd from above): * valid "active" statuses: active, warn-1, warn-2, warn-3, warn-4, locked
-        // TODO (cont'd from above): * valid "invited" statuses" invited, invited-pending
-
-        // Filter on the status.  A status of 'all' translates to no filter since we want all statuses
-        if (options.status && options.status !== 'all') {
-            // make sure that status is valid
-            // TODO: need a better way of getting a list of statuses other than hard-coding them...
-            options.status = _.indexOf(
-                ['active', 'warn-1', 'warn-2', 'warn-3', 'warn-4', 'locked', 'invited', 'inactive'],
-                options.status) !== -1 ? options.status : 'active';
-        }
-
-        if (options.status === 'active') {
-            userCollection.query().whereIn('status', activeStates);
-        } else if (options.status === 'invited') {
-            userCollection.query().whereIn('status', invitedStates);
-        } else if (options.status !== 'all') {
-            options.where.status = options.status;
-        }
-
-        // If there are where conditionals specified, add those
-        // to the query.
-        if (options.where) {
-            userCollection.query('where', options.where);
-        }
-
-        // Add related objects
-        options.withRelated = _.union(options.withRelated, options.include);
-
-        // only include a limit-query if a numeric limit is provided
-        if (_.isNumber(options.limit)) {
-            userCollection
-                .query('limit', options.limit)
-                .query('offset', options.limit * (options.page - 1));
-        }
-
         function fetchRoleQuery() {
             if (roleInstance) {
                 return roleInstance.fetch();
@@ -291,62 +238,116 @@ User = ghostBookshelf.Model.extend({
             return false;
         }
 
-        return Promise.resolve(fetchRoleQuery())
+        // Step 3: Prefetch filter models
-            .then(function then() {
+        return Promise.resolve(fetchRoleQuery()).then(function setupCollectionPromises() {
-                function fetchCollection() {
+            // -- Part 1 --
-                    if (roleInstance) {
+            var userCollection = Users.forge(),
-                        userCollection
+                collectionPromise,
-                            .query('join', 'roles_users', 'roles_users.user_id', '=', 'users.id')
+                countPromise;
-                            .query('where', 'roles_users.role_id', '=', roleInstance.id);
-                    }
 
-                    return userCollection
+            // Step 1: Setup pagination options
-                        .query('orderBy', 'last_login', 'DESC')
+            if (options.limit && options.limit !== 'all') {
-                        .query('orderBy', 'name', 'ASC')
+                options.limit = parseInt(options.limit, 10) || 15;
-                        .query('orderBy', 'created_at', 'DESC')
+            }
-                        .fetch(_.omit(options, 'page', 'limit'));
+
+            if (options.page) {
+                options.page = parseInt(options.page, 10) || 1;
+            }
+
+            // Step 2: Filter options
+            options = self.filterOptions(options, 'findPage');
+
+            // Step 3: Extend defaults
+            options = _.extend({
+                page: 1, // pagination page
+                limit: 15,
+                status: 'active',
+                where: {},
+                whereIn: {}
+            }, options);
+
+            // Step 4: Setup filters (where clauses)
+            // TODO: there are multiple statuses that make a user "active" or "invited" - we a way to translate/map them:
+            // TODO (cont'd from above): * valid "active" statuses: active, warn-1, warn-2, warn-3, warn-4, locked
+            // TODO (cont'd from above): * valid "invited" statuses" invited, invited-pending
+
+            // Filter on the status.  A status of 'all' translates to no filter since we want all statuses
+            if (options.status && options.status !== 'all') {
+                // make sure that status is valid
+                // TODO: need a better way of getting a list of statuses other than hard-coding them...
+                options.status = _.indexOf(
+                    ['active', 'warn-1', 'warn-2', 'warn-3', 'warn-4', 'locked', 'invited', 'inactive'],
+                    options.status) !== -1 ? options.status : 'active';
+            }
+
+            if (options.status === 'active') {
+                userCollection.query().whereIn('status', activeStates);
+            } else if (options.status === 'invited') {
+                userCollection.query().whereIn('status', invitedStates);
+            } else if (options.status !== 'all') {
+                options.where.status = options.status;
+            }
+
+            // If there are where conditionals specified, add those to the query.
+            if (options.where) {
+                userCollection.query('where', options.where);
+            }
+
+            // Step 5: Setup joins
+            if (roleInstance) {
+                userCollection
+                    .query('join', 'roles_users', 'roles_users.user_id', '=', 'users.id')
+                    .query('where', 'roles_users.role_id', '=', roleInstance.id);
+            }
+
+            // Step 6: Setup the counter to fetch the number of items in the set
+            // @TODO abstract this out
+            // tableName = _.result(userCollection, 'tableName'),
+            // idAttribute = _.result(userCollection, 'idAttribute');
+            countPromise = userCollection.query().clone().count('users.id as aggregate');
+
+            // -- Part 2 --
+            // Add limit, offset and other query changes which aren't required when performing a count
+
+            // Step 1: Add related objects
+            options.withRelated = _.union(options.withRelated, options.include);
+
+            // Step 2: Add pagination options if needed
+            if (_.isNumber(options.limit)) {
+                userCollection
+                    .query('limit', options.limit)
+                    .query('offset', options.limit * (options.page - 1));
+            }
+
+            // Step 3: add order parameters
+            userCollection
+                .query('orderBy', 'last_login', 'DESC')
+                .query('orderBy', 'name', 'ASC')
+                .query('orderBy', 'created_at', 'DESC');
+
+            // Step 4: Setup the promise
+            collectionPromise = userCollection.fetch(_.omit(options, 'page', 'limit'));
+
+            // -- Part 3 --
+            // Step 1: Fetch the data
+            return Promise.join(collectionPromise, countPromise);
+        }).then(function formatResponse(results) {
+            var userCollection = results[0],
+                data = {};
+
+            // Step 2: Format the data
+            data.users = userCollection.toJSON(options);
+            data.meta = {pagination: paginateResponse(results[1][0].aggregate, options)};
+
+            if (roleInstance) {
+                data.meta.filters = {};
+                if (!roleInstance.isNew()) {
+                    data.meta.filters.roles = [roleInstance.toJSON(options)];
                 }
+            }
 
-                function fetchPaginationData() {
+            return data;
-                    var qb,
+        }).catch(errors.logAndThrowError);
-                        tableName = _.result(userCollection, 'tableName'),
-                        idAttribute = _.result(userCollection, 'idAttribute');
-
-                    // After we're done, we need to figure out what
-                    // the limits are for the pagination values.
-                    qb = ghostBookshelf.knex(tableName);
-
-                    if (options.where) {
-                        qb.where(options.where);
-                    }
-
-                    if (roleInstance) {
-                        qb.join('roles_users', 'roles_users.user_id', '=', 'users.id');
-                        qb.where('roles_users.role_id', '=', roleInstance.id);
-                    }
-
-                    return qb.count(tableName + '.' + idAttribute + ' as aggregate');
-                }
-
-                return Promise.join(fetchCollection(), fetchPaginationData());
-            })
-            // Format response of data
-            .then(function then(results) {
-                var data = {};
-
-                data.users = userCollection.toJSON(options);
-                data.meta = {pagination: paginateResponse(results[1][0].aggregate, options)};
-
-                if (roleInstance) {
-                    data.meta.filters = {};
-                    if (!roleInstance.isNew()) {
-                        data.meta.filters.roles = [roleInstance.toJSON(options)];
-                    }
-                }
-
-                return data;
-            })
-            .catch(errors.logAndThrowError);
     },
 
     /**
@@ -370,6 +371,7 @@ User = ghostBookshelf.Model.extend({
 
         options = options || {};
         options.withRelated = _.union(options.withRelated, options.include);
+        data = this.filterData(data);
 
         // Support finding by role
         if (lookupRole) {
@@ -386,8 +388,6 @@ User = ghostBookshelf.Model.extend({
             query = this.forge(data, {include: options.include});
         }
 
-        data = this.filterData(data);
-
         if (status === 'active') {
             query.query('whereIn', 'status', activeStates);
         } else if (status === 'invited') {