From bbcc83dadb8cbd337d7fcd3a27dfa08b744e2521 Mon Sep 17 00:00:00 2001 From: naz Date: Thu, 24 Sep 2020 13:32:40 +1200 Subject: [PATCH] Added support for ordering Post API resources by fields coming form posts_meta table (#12226) refs #11729 - When ordering is done by fields from a relation (like post's `meta_title` that comes form `posts_meta` table), Bookshelf does not include those relations in the original query which caused errors. To support this usecase added a mechanism to detect fields from a relation and load those relations into query. - Extended ordering to include table name in ordered field name. The information about the table name is needed to avoid using `tableName` within pagination plugin and gives path to having other than original table ordering fields (e.g. order by posts_meta table fields) - Added test case to check ordering on posts_meta fields - Added support for "eager loading" relations. Allows to extend query builder object with joins to related tables, which could be used in ordering (possibly in filtering later). Bookshelf does not support ordering/filtering by proprieties coming from relations, that's why this kind of plugin and query expansion is needed - Added note about lack of support for child relations with same property names. --- core/server/models/base/index.js | 5 +- core/server/models/plugins/eager-load.js | 79 +++++++++++++++++++ core/server/models/plugins/index.js | 1 + core/server/models/plugins/order.js | 12 ++- core/server/models/plugins/pagination.js | 26 +++++- core/server/models/post.js | 16 ++++ .../regression/api/canary/admin/posts_spec.js | 32 ++++++++ test/regression/api/canary/admin/utils.js | 3 +- test/unit/models/plugins/pagination_spec.js | 2 +- 9 files changed, 170 insertions(+), 6 deletions(-) create mode 100644 core/server/models/plugins/eager-load.js diff --git a/core/server/models/base/index.js b/core/server/models/base/index.js index 4871e07a87..d2bbf504a0 100644 --- a/core/server/models/base/index.js +++ b/core/server/models/base/index.js @@ -34,6 +34,8 @@ ghostBookshelf = bookshelf(db.knex); // Load the Bookshelf registry plugin, which helps us avoid circular dependencies ghostBookshelf.plugin('registry'); +ghostBookshelf.plugin(plugins.eagerLoad); + // Add committed/rollback events. ghostBookshelf.plugin(plugins.transactionEvents); @@ -175,7 +177,8 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ // Ghost ordering handling, allows to order by permitted attributes by default and can be overriden on specific model level orderAttributes: function orderAttributes() { - return this.permittedAttributes(); + return Object.keys(schema.tables[this.tableName]) + .map(key => `${this.tableName}.${key}`); }, // When loading an instance, subclasses can specify default to fetch diff --git a/core/server/models/plugins/eager-load.js b/core/server/models/plugins/eager-load.js new file mode 100644 index 0000000000..174fe191e4 --- /dev/null +++ b/core/server/models/plugins/eager-load.js @@ -0,0 +1,79 @@ +const _ = require('lodash'); +const _debug = require('ghost-ignition').debug._base; +const debug = _debug('ghost-query'); + +/** + * Enchances knex query builder with a join to relation configured in + * + * @param {Bookshelf.Model} model instance of Bookshelf model + * @param {String[]} relationsToLoad relations to be included in joins + */ +function withEager(model, relationsToLoad) { + const tableName = _.result(model.constructor.prototype, 'tableName'); + + return function (qb) { + if (!model.relationsMeta) { + return qb; + } + + for (const [key, config] of Object.entries(model.relationsMeta)) { + if (relationsToLoad.includes(key)) { + const innerQb = qb + .leftJoin(config.targetTableName, `${tableName}.id`, `${config.targetTableName}.${config.foreignKey}`); + + debug(`QUERY has posts: ${innerQb.toSQL().sql}`); + } + } + + return qb; + }; +} + +function load(options) { + if (!options) { + return; + } + + if (this.eagerLoad) { + if (!options.columns && options.withRelated && _.intersection(this.eagerLoad, options.withRelated).length) { + this.query(withEager(this, this.eagerLoad)); + } + } +} + +/** + * ## Pagination + * Extends `bookshelf.Model` native `fetch` and `fetchAll` methods with + * a join to "eager loaded" relation. An exaple of such loading is when + * there is a need to order by fields in the related table. + * + */ +module.exports = function eagerLoadPlugin(Bookshelf) { + const modelPrototype = Bookshelf.Model.prototype; + + Bookshelf.Model = Bookshelf.Model.extend({ + initialize: function () { + return modelPrototype.initialize.apply(this, arguments); + }, + + fetch: function () { + load.apply(this, arguments); + + if (_debug.enabled('ghost-query')) { + debug('QUERY', this.query().toQuery()); + } + + return modelPrototype.fetch.apply(this, arguments); + }, + + fetchAll: function () { + load.apply(this, arguments); + + if (_debug.enabled('ghost-query')) { + debug('QUERY', this.query().toQuery()); + } + + return modelPrototype.fetchAll.apply(this, arguments); + } + }); +}; diff --git a/core/server/models/plugins/index.js b/core/server/models/plugins/index.js index f7a12d9acc..7dd6cacd22 100644 --- a/core/server/models/plugins/index.js +++ b/core/server/models/plugins/index.js @@ -1,4 +1,5 @@ module.exports = { + eagerLoad: require('./eager-load'), filter: require('./filter'), order: require('./order'), customQuery: require('./custom-query'), diff --git a/core/server/models/plugins/order.js b/core/server/models/plugins/order.js index dff7da49cc..0ab1f7b02e 100644 --- a/core/server/models/plugins/order.js +++ b/core/server/models/plugins/order.js @@ -31,11 +31,19 @@ const order = function order(Bookshelf) { field = match[1].toLowerCase(); direction = match[2].toUpperCase(); - if (orderAttributes.indexOf(field) === -1) { + const matchingOrderAttribute = orderAttributes.find((orderAttribute) => { + // NOTE: this logic assumes we use different field names for "parent" and "child" relations. + // E.g.: ['parent.title', 'child.title'] and ['child.title', 'parent.title'] - would not + // distinguish on which relation to sort neither which order to pick the fields on. + // For more context see: https://github.com/TryGhost/Ghost/pull/12226#discussion_r493085098 + return orderAttribute.endsWith(field); + }); + + if (!matchingOrderAttribute) { return; } - result[field] = direction; + result[matchingOrderAttribute] = direction; }); return result; diff --git a/core/server/models/plugins/pagination.js b/core/server/models/plugins/pagination.js index 2f3d0a78c6..e7a6312a37 100644 --- a/core/server/models/plugins/pagination.js +++ b/core/server/models/plugins/pagination.js @@ -90,6 +90,27 @@ paginationUtils = { } return pagination; + }, + + /** + * + * @param {Bookshelf.Model} model instance of Bookshelf model + * @param {string} propertyName property to be inspected and included in the relation + */ + handleRelation: function handleRelation(model, propertyName) { + const tableName = _.result(model.constructor.prototype, 'tableName'); + + const targetTable = propertyName.includes('.') && propertyName.split('.')[0]; + + if (targetTable && targetTable !== tableName) { + if (!model.eagerLoad) { + model.eagerLoad = []; + } + + if (!model.eagerLoad.includes(targetTable)) { + model.eagerLoad.push(targetTable); + } + } } }; @@ -182,7 +203,9 @@ pagination = function pagination(bookshelf) { if (property === 'count.posts') { self.query('orderBy', 'count__posts', direction); } else { - self.query('orderBy', tableName + '.' + property, direction); + self.query('orderBy', property, direction); + + paginationUtils.handleRelation(self, property); } }); } else if (options.orderRaw) { @@ -199,6 +222,7 @@ pagination = function pagination(bookshelf) { // Setup the promise to do a fetch on our collection, running the specified query // @TODO: ensure option handling is done using an explicit pick elsewhere + return self.fetchAll(_.omit(options, ['page', 'limit'])) .then(function (fetchResult) { if (options.limit === 'all') { diff --git a/core/server/models/post.js b/core/server/models/post.js index 7f9d53ae66..fd3504df6b 100644 --- a/core/server/models/post.js +++ b/core/server/models/post.js @@ -68,6 +68,13 @@ Post = ghostBookshelf.Model.extend({ posts_meta: 'posts_meta' }, + relationsMeta: { + posts_meta: { + targetTableName: 'posts_meta', + foreignKey: 'post_id' + } + }, + /** * The base model keeps only the columns, which are defined in the schema. * We have to add the relations on top, otherwise bookshelf-relations @@ -83,6 +90,15 @@ Post = ghostBookshelf.Model.extend({ return filteredKeys; }, + orderAttributes: function orderAttributes() { + let keys = ghostBookshelf.Model.prototype.orderAttributes.apply(this, arguments); + + // extend ordered keys with post_meta keys + let postsMetaKeys = _.without(ghostBookshelf.model('PostsMeta').prototype.orderAttributes(), 'posts_meta.id', 'posts_meta.post_id'); + + return [...keys, ...postsMetaKeys]; + }, + emitChange: function emitChange(event, options = {}) { let eventToTrigger; let resourceType = this.get('type'); diff --git a/test/regression/api/canary/admin/posts_spec.js b/test/regression/api/canary/admin/posts_spec.js index 123f9ce24b..6c7f8b47d5 100644 --- a/test/regression/api/canary/admin/posts_spec.js +++ b/test/regression/api/canary/admin/posts_spec.js @@ -89,6 +89,38 @@ describe('Posts API', function () { done(); }); }); + + it('can order by fields coming from posts_meta table', function (done) { + request.get(localUtils.API.getApiQuery('posts/?order=meta_description%20ASC')) + .set('Origin', config.get('url')) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + should.not.exist(res.headers['x-cache-invalidate']); + const jsonResponse = res.body; + should.exist(jsonResponse.posts); + localUtils.API.checkResponse(jsonResponse, 'posts'); + jsonResponse.posts.should.have.length(13); + + should.equal(jsonResponse.posts[0].meta_description, null); + jsonResponse.posts[12].slug.should.equal('short-and-sweet'); + jsonResponse.posts[12].meta_description.should.equal('test stuff'); + + localUtils.API.checkResponse( + jsonResponse.posts[0], + 'post' + ); + + localUtils.API.checkResponse(jsonResponse.meta.pagination, 'pagination'); + + done(); + }); + }); }); describe('Read', function () { diff --git a/test/regression/api/canary/admin/utils.js b/test/regression/api/canary/admin/utils.js index a0f271133a..f52b7cf101 100644 --- a/test/regression/api/canary/admin/utils.js +++ b/test/regression/api/canary/admin/utils.js @@ -26,11 +26,12 @@ const expectedProperties = { .without('locale') .without('page') .without('author_id', 'author') + .without('type') // always returns computed properties // primary_tag and primary_author properties are included // only because authors and tags are always included .concat('url', 'primary_tag', 'primary_author', 'excerpt') - .concat('authors', 'tags') + .concat('authors', 'tags', 'email') // returns meta fields from `posts_meta` schema .concat( ..._(schema.posts_meta).keys().without('post_id', 'id') diff --git a/test/unit/models/plugins/pagination_spec.js b/test/unit/models/plugins/pagination_spec.js index 54247a2865..b6e5acb3a1 100644 --- a/test/unit/models/plugins/pagination_spec.js +++ b/test/unit/models/plugins/pagination_spec.js @@ -266,7 +266,7 @@ describe('pagination', function () { model.prototype.query.calledTwice.should.be.true(); model.prototype.query.firstCall.calledWith().should.be.true(); - model.prototype.query.secondCall.calledWith('orderBy', 'undefined.id', 'DESC').should.be.true(); + model.prototype.query.secondCall.calledWith('orderBy', 'id', 'DESC').should.be.true(); mockQuery.clone.calledOnce.should.be.true(); mockQuery.clone.firstCall.calledWith().should.be.true();