From dfd350bd694cd6d8efcaf4a84f6dd1939117f89b Mon Sep 17 00:00:00 2001 From: Katharina Irrgang Date: Fri, 22 Feb 2019 12:07:34 +0100 Subject: [PATCH] Cleaned up Admin API v2 posts/pages input serializer (#10516) no issue - make use of filter instead of status=all or data.page - nql was designed to filter data on database layer - do not break v0.1 - we just got rid of the "status" query param, you should use the filter instead - get rid of the ugly condition to remove page field if "fields" param was used - allow filtering on model layer for "findOne" - do not allow filtering for "findOne" on API layer for now - the API controller defines what is allowed - the model layer can allow more by default - we can re-use the powerful filter logic without adding hacks --- .../api/v2/utils/serializers/input/pages.js | 54 ++++++------ .../api/v2/utils/serializers/input/posts.js | 64 +++++++------- .../utils/serializers/output/utils/mapper.js | 7 -- core/server/models/base/index.js | 25 +++--- core/server/models/post.js | 13 +-- .../regression/api/v0.1/public_api_spec.js | 12 +++ .../regression/api/v2/content/posts_spec.js | 12 +++ .../v2/utils/serializers/input/pages_spec.js | 83 +++++++++++++++---- .../v2/utils/serializers/input/posts_spec.js | 43 +++++++--- 9 files changed, 212 insertions(+), 101 deletions(-) diff --git a/core/server/api/v2/utils/serializers/input/pages.js b/core/server/api/v2/utils/serializers/input/pages.js index 2353e65e42..1dab290411 100644 --- a/core/server/api/v2/utils/serializers/input/pages.js +++ b/core/server/api/v2/utils/serializers/input/pages.js @@ -25,23 +25,35 @@ function setDefaultOrder(frame) { } } +/** + * CASE: + * + * - the content api endpoints for pages forces the model layer to return static pages only + * - we have to enforce the filter + * + * @TODO: https://github.com/TryGhost/Ghost/issues/10268 + */ +const forcePageFilter = (frame) => { + if (frame.options.filter) { + frame.options.filter = `(${frame.options.filter})+page:true`; + } else { + frame.options.filter = 'page:true'; + } +}; + +const forceStatusFilter = (frame) => { + if (!frame.options.filter) { + frame.options.filter = 'status:[draft,published,scheduled]'; + } else if (!frame.options.filter.match(/status:/)) { + frame.options.filter = `(${frame.options.filter})+status:[draft,published,scheduled]`; + } +}; + module.exports = { browse(apiConfig, frame) { debug('browse'); - /** - * CASE: - * - * - the content api endpoints for pages forces the model layer to return static pages only - * - we have to enforce the filter - * - * @TODO: https://github.com/TryGhost/Ghost/issues/10268 - */ - if (frame.options.filter) { - frame.options.filter = `(${frame.options.filter})+page:true`; - } else { - frame.options.filter = 'page:true'; - } + forcePageFilter(frame); if (localUtils.isContentAPI(frame)) { removeMobiledocFormat(frame); @@ -49,10 +61,7 @@ module.exports = { } if (!localUtils.isContentAPI(frame)) { - // @TODO: remove when we drop v0.1 - if (!frame.options.filter || !frame.options.filter.match(/status:/)) { - frame.options.status = 'all'; - } + forceStatusFilter(frame); } debug(frame.options); @@ -61,7 +70,7 @@ module.exports = { read(apiConfig, frame) { debug('read'); - frame.data.page = true; + forcePageFilter(frame); if (localUtils.isContentAPI(frame)) { removeMobiledocFormat(frame); @@ -69,10 +78,7 @@ module.exports = { } if (!localUtils.isContentAPI(frame)) { - // @TODO: remove when we drop v0.1 - if (!frame.options.filter || !frame.options.filter.match(/status:/)) { - frame.data.status = 'all'; - } + forceStatusFilter(frame); } debug(frame.options); @@ -100,8 +106,8 @@ module.exports = { debug('edit'); - // @NOTE: force not being able to update a page via pages endpoint - frame.options.page = true; + forceStatusFilter(frame); + forcePageFilter(frame); }, destroy(apiConfig, frame) { diff --git a/core/server/api/v2/utils/serializers/input/posts.js b/core/server/api/v2/utils/serializers/input/posts.js index 64270c8fc0..84ec651343 100644 --- a/core/server/api/v2/utils/serializers/input/posts.js +++ b/core/server/api/v2/utils/serializers/input/posts.js @@ -34,23 +34,35 @@ function setDefaultOrder(frame) { } } +/** + * CASE: + * + * - posts endpoint only returns posts, not pages + * - we have to enforce the filter + * + * @TODO: https://github.com/TryGhost/Ghost/issues/10268 + */ +const forcePageFilter = (frame) => { + if (frame.options.filter) { + frame.options.filter = `(${frame.options.filter})+page:false`; + } else { + frame.options.filter = 'page:false'; + } +}; + +const forceStatusFilter = (frame) => { + if (!frame.options.filter) { + frame.options.filter = 'status:[draft,published,scheduled]'; + } else if (!frame.options.filter.match(/status:/)) { + frame.options.filter = `(${frame.options.filter})+status:[draft,published,scheduled]`; + } +}; + module.exports = { browse(apiConfig, frame) { debug('browse'); - /** - * CASE: - * - * - posts endpoint only returns posts, not pages - * - we have to enforce the filter - * - * @TODO: https://github.com/TryGhost/Ghost/issues/10268 - */ - if (frame.options.filter) { - frame.options.filter = `(${frame.options.filter})+page:false`; - } else { - frame.options.filter = 'page:false'; - } + forcePageFilter(frame); /** * ## current cases: @@ -71,10 +83,7 @@ module.exports = { } if (!localUtils.isContentAPI(frame)) { - // @TODO: remove when we drop v0.1 - if (!frame.options.filter || !frame.options.filter.match(/status:/)) { - frame.options.status = 'all'; - } + forceStatusFilter(frame); } debug(frame.options); @@ -83,7 +92,7 @@ module.exports = { read(apiConfig, frame) { debug('read'); - frame.data.page = false; + forcePageFilter(frame); /** * ## current cases: @@ -104,16 +113,13 @@ module.exports = { } if (!localUtils.isContentAPI(frame)) { - // @TODO: remove when we drop v0.1 - if (!frame.options.filter || !frame.options.filter.match(/status:/)) { - frame.data.status = 'all'; - } + forceStatusFilter(frame); } debug(frame.options); }, - add(apiConfig, frame) { + add(apiConfig, frame, options = {add: true}) { debug('add'); if (_.get(frame,'options.source')) { @@ -126,15 +132,17 @@ module.exports = { frame.data.posts[0] = url.forPost(Object.assign({}, frame.data.posts[0]), frame.options); - // @NOTE: force storing post - frame.data.posts[0].page = false; + // @NOTE: force adding post + if (options.add) { + frame.data.posts[0].page = false; + } }, edit(apiConfig, frame) { - this.add(apiConfig, frame); + this.add(apiConfig, frame, {add: false}); - // @NOTE: force that you cannot update pages via posts endpoint - frame.options.page = false; + forceStatusFilter(frame); + forcePageFilter(frame); }, destroy(apiConfig, frame) { diff --git a/core/server/api/v2/utils/serializers/output/utils/mapper.js b/core/server/api/v2/utils/serializers/output/utils/mapper.js index c1d3d9965b..7f0800e926 100644 --- a/core/server/api/v2/utils/serializers/output/utils/mapper.js +++ b/core/server/api/v2/utils/serializers/output/utils/mapper.js @@ -56,13 +56,6 @@ const mapPost = (model, frame) => { }); } - /** - * Remove extra data attributes passed for filtering when used with columns/fields as bookshelf doesn't filter it out - */ - if (frame.options.columns && frame.options.columns.indexOf('page') < 0) { - delete jsonModel.page; - } - return jsonModel; }; diff --git a/core/server/models/base/index.js b/core/server/models/base/index.js index 278a0f4ebc..018f26a49d 100644 --- a/core/server/models/base/index.js +++ b/core/server/models/base/index.js @@ -917,9 +917,16 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ * @return {Promise(ghostBookshelf.Model)} Single Model */ findOne: function findOne(data, unfilteredOptions) { - var options = this.filterOptions(unfilteredOptions, 'findOne'); + const options = this.filterOptions(unfilteredOptions, 'findOne'); data = this.filterData(data); - return this.forge(data).fetch(options); + const model = this.forge(data); + + // @NOTE: The API layer decides if this option is allowed + if (options.filter) { + model.applyDefaultAndCustomFilters(options); + } + + return model.fetch(options); }, /** @@ -936,17 +943,15 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ edit: function edit(data, unfilteredOptions) { const options = this.filterOptions(unfilteredOptions, 'edit'); const id = options.id; - let model; - - if (options.hasOwnProperty('page')) { - model = this.forge({id: id, page: options.page}); - delete options.page; - } else { - model = this.forge({id: id}); - } + const model = this.forge({id: id}); data = this.filterData(data); + // @NOTE: The API layer decides if this option is allowed + if (options.filter) { + model.applyDefaultAndCustomFilters(options); + } + // We allow you to disable timestamps when run migration, so that the posts `updated_at` value is the same if (options.importing) { model.hasTimestamps = false; diff --git a/core/server/models/post.js b/core/server/models/post.js index 3a4ca19b9d..bff7a4d803 100644 --- a/core/server/models/post.js +++ b/core/server/models/post.js @@ -701,11 +701,11 @@ Post = ghostBookshelf.Model.extend({ // whitelists for the `options` hash argument on methods, by method name. // these are the only options that can be passed to Bookshelf / Knex. validOptions = { - findOne: ['columns', 'importing', 'withRelated', 'require'], + findOne: ['columns', 'importing', 'withRelated', 'require', 'filter'], findPage: ['status', 'staticPages'], findAll: ['columns', 'filter'], destroy: ['destroyAll', 'destroyBy'], - edit: ['page'] + edit: ['filter'] }; // The post model additionally supports having a formats option @@ -753,10 +753,11 @@ Post = ghostBookshelf.Model.extend({ * @extends ghostBookshelf.Model.findOne to handle post status * **See:** [ghostBookshelf.Model.findOne](base.js.html#Find%20One) */ - findOne: function findOne(data, options) { - data = _.defaults(data || {}, { - status: 'published' - }); + findOne: function findOne(data = {}, options = {}) { + // @TODO: remove when we drop v0.1 + if (!options.filter && !data.status) { + data.status = 'published'; + } if (data.status === 'all') { delete data.status; diff --git a/core/test/regression/api/v0.1/public_api_spec.js b/core/test/regression/api/v0.1/public_api_spec.js index 93a3d3c5ed..887f9057ef 100644 --- a/core/test/regression/api/v0.1/public_api_spec.js +++ b/core/test/regression/api/v0.1/public_api_spec.js @@ -146,6 +146,18 @@ describe('Public API', function () { }); }); + it('read post with filter', function () { + return request + .get(localUtils.API.getApiQuery(`posts/${testUtils.DataGenerator.Content.posts[0].id}/?client_id=ghost-admin&client_secret=not_available&filter=slug:test`)) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .then((res) => { + // proofs that "filter" does not work for v0.1 + localUtils.API.checkResponse(res.body.posts[0], 'post'); + }); + }); + it('browse posts with inverse filters', function (done) { request.get(localUtils.API.getApiQuery('posts/?client_id=ghost-admin&client_secret=not_available&filter=tag:-[bacon,pollo,getting-started]&include=tags')) .expect('Content-Type', /json/) diff --git a/core/test/regression/api/v2/content/posts_spec.js b/core/test/regression/api/v2/content/posts_spec.js index 7426d63bd3..f486784631 100644 --- a/core/test/regression/api/v2/content/posts_spec.js +++ b/core/test/regression/api/v2/content/posts_spec.js @@ -139,4 +139,16 @@ describe('Posts', function () { .expect('Cache-Control', testUtils.cacheRules.private) .expect(404); }); + + it('can read post with fields', function () { + return request + .get(localUtils.API.getApiQuery(`posts/${testUtils.DataGenerator.Content.posts[0].id}/?key=${validKey}&fields=title,slug`)) + .set('Origin', testUtils.API.getURL()) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .then((res)=> { + localUtils.API.checkResponse(res.body.posts[0], 'post', null, null, ['id', 'title', 'slug']); + }); + }); }); diff --git a/core/test/unit/api/v2/utils/serializers/input/pages_spec.js b/core/test/unit/api/v2/utils/serializers/input/pages_spec.js index 31ec625348..6c8a2f5beb 100644 --- a/core/test/unit/api/v2/utils/serializers/input/pages_spec.js +++ b/core/test/unit/api/v2/utils/serializers/input/pages_spec.js @@ -71,37 +71,92 @@ describe('Unit: v2/utils/serializers/input/pages', function () { }); describe('read', function () { - it('default', function () { + it('content api default', function () { const apiConfig = {}; const frame = { options: { context: {} }, - data: { - status: 'all' - } + data: {} }; serializers.input.pages.read(apiConfig, frame); - frame.data.status.should.eql('all'); - frame.data.page.should.eql(true); + frame.options.filter.should.eql('page:true'); }); - it('overrides page', function () { + it('content api default', function () { const apiConfig = {}; const frame = { + apiType: 'content', options: { - context: {} + context: { + user: 0, + api_key: { + id: 1, + type: 'content' + }, + } }, - data: { - status: 'all', - page: false - } + data: {} }; serializers.input.pages.read(apiConfig, frame); - frame.data.status.should.eql('all'); - frame.data.page.should.eql(true); + frame.options.filter.should.eql('page:true'); + }); + + it('admin api default', function () { + const apiConfig = {}; + const frame = { + apiType: 'admin', + options: { + context: { + user: 0, + api_key: { + id: 1, + type: 'admin' + }, + } + }, + data: {} + }; + + serializers.input.pages.read(apiConfig, frame); + frame.options.filter.should.eql('(page:true)+status:[draft,published,scheduled]'); + }); + + it('custom page filter', function () { + const apiConfig = {}; + const frame = { + options: { + filter: 'page:false', + context: {} + }, + data: {} + }; + + serializers.input.pages.read(apiConfig, frame); + frame.options.filter.should.eql('(page:false)+page:true'); + }); + + it('custom status filter', function () { + const apiConfig = {}; + const frame = { + apiType: 'admin', + options: { + filter: 'status:draft', + context: { + user: 0, + api_key: { + id: 1, + type: 'admin' + }, + } + }, + data: {} + }; + + serializers.input.pages.read(apiConfig, frame); + frame.options.filter.should.eql('(status:draft)+page:true'); }); it('remove mobiledoc option from formats', function () { diff --git a/core/test/unit/api/v2/utils/serializers/input/posts_spec.js b/core/test/unit/api/v2/utils/serializers/input/posts_spec.js index d5a4ce1495..a80f1eba14 100644 --- a/core/test/unit/api/v2/utils/serializers/input/posts_spec.js +++ b/core/test/unit/api/v2/utils/serializers/input/posts_spec.js @@ -35,7 +35,7 @@ describe('Unit: v2/utils/serializers/input/posts', function () { }; serializers.input.posts.browse(apiConfig, frame); - should.equal(frame.options.filter, 'page:false'); + should.equal(frame.options.filter, '(page:false)+status:[draft,published,scheduled]'); }); it('combine filters', function () { @@ -135,7 +135,7 @@ describe('Unit: v2/utils/serializers/input/posts', function () { }); describe('read', function () { - it('with apiType of "content" it sets data.page to false', function () { + it('with apiType of "content" it forces page filter', function () { const apiConfig = {}; const frame = { apiType: 'content', @@ -144,25 +144,24 @@ describe('Unit: v2/utils/serializers/input/posts', function () { }; serializers.input.posts.read(apiConfig, frame); - frame.data.page.should.eql(false); + frame.options.filter.should.eql('page:false'); }); - it('with apiType of "content" it overrides data.page to be false', function () { + it('with apiType of "content" it forces page false filter', function () { const apiConfig = {}; const frame = { apiType: 'content', - options: {}, - data: { - status: 'all', - page: true - } + options: { + filter: 'page:true' + }, + data: {} }; serializers.input.posts.read(apiConfig, frame); - frame.data.page.should.eql(false); + frame.options.filter.should.eql('(page:true)+page:false'); }); - it('with apiType of "admin" it does not set data.page', function () { + it('with apiType of "admin" it forces page & status false filter', function () { const apiConfig = {}; const frame = { apiType: 'admin', @@ -178,7 +177,27 @@ describe('Unit: v2/utils/serializers/input/posts', function () { }; serializers.input.posts.read(apiConfig, frame); - should.equal(frame.data.page, false); + frame.options.filter.should.eql('(page:false)+status:[draft,published,scheduled]'); + }); + + it('with apiType of "admin" it forces page filter & respects custom status filter', function () { + const apiConfig = {}; + const frame = { + apiType: 'admin', + options: { + context: { + api_key: { + id: 1, + type: 'admin' + } + }, + filter: 'status:draft' + }, + data: {} + }; + + serializers.input.posts.read(apiConfig, frame); + frame.options.filter.should.eql('(status:draft)+page:false'); }); it('remove mobiledoc option from formats', function () {