From e89a27f3ab6f934c9c05fdaea6158555d15ee50c Mon Sep 17 00:00:00 2001 From: Katharina Irrgang Date: Thu, 15 Nov 2018 15:53:24 +0100 Subject: [PATCH] Replaced `options.where` GQL statements with filter notation (#10160) refs #10105 - `options.where` is an older deprecated logic - before the filter language was invented, Ghost generates statements for knex - if we want to replace GQL with NQL, we can't generate these statements - they are not understood from NQL, because NQL uses mongo JSON - go through usages and rewrite the statements - invent `extraFilters` for now - we need to keep the support for `status` or `staticPages` for now (API requirement) - IMO both shortcuts in the extra filters should be removed in the future This commit is required for https://github.com/TryGhost/Ghost/pull/10159! --- core/server/models/base/index.js | 12 --- core/server/models/invite.js | 4 - core/server/models/plugins/filter.js | 4 +- core/server/models/post.js | 85 +++++++++++--------- core/server/models/subscriber.js | 6 -- core/server/models/tag.js | 7 -- core/server/models/user.js | 62 +++++++------- core/test/unit/models/plugins/filter_spec.js | 24 +++--- core/test/unit/models/post_spec.js | 12 +-- 9 files changed, 92 insertions(+), 124 deletions(-) diff --git a/core/server/models/base/index.js b/core/server/models/base/index.js index 33f63e9f73..1b44f7f13d 100644 --- a/core/server/models/base/index.js +++ b/core/server/models/base/index.js @@ -646,11 +646,6 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ var options = this.filterOptions(unfilteredOptions, 'findAll'), itemCollection = this.forge(); - // transforms fictive keywords like 'all' (status:all) into correct allowed values - if (this.processOptions) { - this.processOptions(options); - } - // @TODO: we can't use order raw when running migrations (see https://github.com/tgriesser/knex/issues/2763) if (this.orderDefaultRaw && !options.migrating) { itemCollection.query((qb) => { @@ -702,13 +697,6 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ // Set this to true or pass ?debug=true as an API option to get output itemCollection.debug = options.debug && config.get('env') !== 'production'; - // This applies default properties like 'staticPages' and 'status' - // And then converts them to 'where' options... this behaviour is effectively deprecated in favour - // of using filter - it's only be being kept here so that we can transition cleanly. - if (this.processOptions) { - this.processOptions(options); - } - // Add Filter behaviour itemCollection.applyDefaultAndCustomFilters(options); diff --git a/core/server/models/invite.js b/core/server/models/invite.js index 59ccfcfa49..a67c3ba814 100644 --- a/core/server/models/invite.js +++ b/core/server/models/invite.js @@ -24,10 +24,6 @@ Invite = ghostBookshelf.Model.extend({ return {}; }, - processOptions: function processOptions(options) { - return options; - }, - add: function add(data, unfilteredOptions) { const options = Invite.filterOptions(unfilteredOptions, 'add'); data = data || {}; diff --git a/core/server/models/plugins/filter.js b/core/server/models/plugins/filter.js index 7d2e36af6a..657161ee9a 100644 --- a/core/server/models/plugins/filter.js +++ b/core/server/models/plugins/filter.js @@ -83,6 +83,8 @@ filter = function filter(Bookshelf) { }, defaultFilters: function defaultFilters() { }, + extraFilters: function extraFilters() { + }, preProcessFilters: function preProcessFilters() { this._filters.statements = gql.json.replaceStatements(this._filters.statements, {prop: /primary_tag/}, function (statement) { @@ -175,7 +177,7 @@ filter = function filter(Bookshelf) { this.enforcedFilters(options), this.defaultFilters(options), options.filter, - options.where + this.extraFilters(options) ); return this; diff --git a/core/server/models/post.js b/core/server/models/post.js index f21f34266e..40c7f25477 100644 --- a/core/server/models/post.js +++ b/core/server/models/post.js @@ -11,6 +11,7 @@ const config = require('../config'); const converters = require('../lib/mobiledoc/converters'); const relations = require('./relations'); const MOBILEDOC_REVISIONS_COUNT = 10; +const ALL_STATUSES = ['published', 'draft', 'scheduled']; let Post; let Posts; @@ -517,6 +518,51 @@ Post = ghostBookshelf.Model.extend({ } return options.context && options.context.public ? 'page:false' : 'page:false+status:published'; + }, + + /** + * You can pass an extra `status=VALUES` or "staticPages" field. + * Long-Term: We should deprecate these short cuts and force users to use the filter param. + */ + extraFilters: function extraFilters(options) { + if (!options.staticPages && !options.status) { + return null; + } + + let filter = null; + + // CASE: "staticPages" is passed + if (options.staticPages && options.staticPages !== 'all') { + // CASE: convert string true/false to boolean + if (!_.isBoolean(options.staticPages)) { + options.staticPages = _.includes(['true', '1'], options.staticPages); + } + + filter = `page:${options.staticPages}`; + } else if (options.staticPages === 'all') { + filter = 'page:[true, false]'; + } + + // CASE: "status" is passed, combine filters + if (options.status && options.status !== 'all') { + options.status = _.includes(ALL_STATUSES, options.status) ? options.status : 'published'; + + if (!filter) { + filter = `status:${options.status}`; + } else { + filter = `${filter}+status:${options.status}`; + } + } else if (options.status === 'all') { + if (!filter) { + filter = `status:[${ALL_STATUSES}]`; + } else { + filter = `${filter}+status:[${ALL_STATUSES}]`; + } + } + + delete options.status; + delete options.staticPages; + return filter; } }, { allowedFormats: ['mobiledoc', 'html', 'plaintext'], @@ -547,45 +593,6 @@ Post = ghostBookshelf.Model.extend({ return order; }, - /** - * @deprecated in favour of filter - */ - processOptions: function processOptions(options) { - if (!options.staticPages && !options.status) { - return options; - } - - // This is the only place that 'options.where' is set now - options.where = {statements: []}; - - // Step 4: Setup filters (where clauses) - if (options.staticPages && options.staticPages !== 'all') { - // convert string true/false to boolean - if (!_.isBoolean(options.staticPages)) { - options.staticPages = _.includes(['true', '1'], options.staticPages); - } - options.where.statements.push({prop: 'page', op: '=', value: options.staticPages}); - delete options.staticPages; - } else if (options.staticPages === 'all') { - options.where.statements.push({prop: 'page', op: 'IN', value: [true, false]}); - delete options.staticPages; - } - - // Unless `all` is passed as an option, filter on - // the status provided. - if (options.status && options.status !== 'all') { - // make sure that status is valid - options.status = _.includes(['published', 'draft', 'scheduled'], options.status) ? options.status : 'published'; - options.where.statements.push({prop: 'status', op: '=', value: options.status}); - delete options.status; - } else if (options.status === 'all') { - options.where.statements.push({prop: 'status', op: 'IN', value: ['published', 'draft', 'scheduled']}); - delete options.status; - } - - return options; - }, - /** * Returns an array of keys permitted in a method's `options` hash, depending on the current method. * @param {String} methodName The name of the method to check valid options for. diff --git a/core/server/models/subscriber.js b/core/server/models/subscriber.js index 4ee93ffa5e..0134a964aa 100644 --- a/core/server/models/subscriber.js +++ b/core/server/models/subscriber.js @@ -35,12 +35,6 @@ Subscriber = ghostBookshelf.Model.extend({ orderDefaultOptions: function orderDefaultOptions() { return {}; }, - /** - * @deprecated in favour of filter - */ - processOptions: function processOptions(options) { - return options; - }, permittedOptions: function permittedOptions(methodName) { var options = ghostBookshelf.Model.permittedOptions.call(this, methodName), diff --git a/core/server/models/tag.js b/core/server/models/tag.js index f55554d813..9249da2374 100644 --- a/core/server/models/tag.js +++ b/core/server/models/tag.js @@ -75,13 +75,6 @@ Tag = ghostBookshelf.Model.extend({ return {}; }, - /** - * @deprecated in favour of filter - */ - processOptions: function processOptions(options) { - return options; - }, - permittedOptions: function permittedOptions(methodName) { var options = ghostBookshelf.Model.permittedOptions.call(this, methodName), diff --git a/core/server/models/user.js b/core/server/models/user.js index 2b5ecf0abe..f10ada381c 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -278,6 +278,35 @@ User = ghostBookshelf.Model.extend({ } return options.context && options.context.public ? null : 'status:[' + allStates.join(',') + ']'; + }, + + /** + * You can pass an extra `status=VALUES` field. + * Long-Term: We should deprecate these short cuts and force users to use the filter param. + */ + extraFilters: function extraFilters(options) { + if (!options.status) { + return null; + } + + let filter = null; + + // CASE: Check if the incoming status value is valid, otherwise fallback to "active" + if (options.status !== 'all') { + options.status = allStates.indexOf(options.status) > -1 ? options.status : 'active'; + } + + if (options.status === 'active') { + filter = `status:[${activeStates}]`; + } else if (options.status === 'all') { + filter = `status:[${allStates}]`; + } else { + filter = `status:${options.status}`; + } + + delete options.status; + + return filter; } }, { orderDefaultOptions: function orderDefaultOptions() { @@ -288,39 +317,6 @@ User = ghostBookshelf.Model.extend({ }; }, - /** - * @deprecated in favour of filter - */ - processOptions: function processOptions(options) { - if (!options.status) { - return options; - } - - // This is the only place that 'options.where' is set now - options.where = {statements: []}; - - var value; - - // Filter on the status. A status of 'all' translates to no filter since we want all statuses - if (options.status !== 'all') { - // make sure that status is valid - options.status = allStates.indexOf(options.status) > -1 ? options.status : 'active'; - } - - if (options.status === 'active') { - value = activeStates; - } else if (options.status === 'all') { - value = allStates; - } else { - value = options.status; - } - - options.where.statements.push({prop: 'status', op: 'IN', value: value}); - delete options.status; - - return options; - }, - /** * Returns an array of keys permitted in a method's `options` hash, depending on the current method. * @param {String} methodName The name of the method to check valid options for. diff --git a/core/test/unit/models/plugins/filter_spec.js b/core/test/unit/models/plugins/filter_spec.js index 5ff7e567ea..b6ea7cd90d 100644 --- a/core/test/unit/models/plugins/filter_spec.js +++ b/core/test/unit/models/plugins/filter_spec.js @@ -109,9 +109,9 @@ describe('Filter', function () { }); it('should call combineFilters with old-style custom filters if set', function () { - var result = ghostBookshelf.Model.prototype.fetchAndCombineFilters({ - where: 'author:cameron' - }); + sandbox.stub(ghostBookshelf.Model.prototype, 'extraFilters').returns('author:cameron'); + + const result = ghostBookshelf.Model.prototype.fetchAndCombineFilters({}); filterUtils.combineFilters.calledOnce.should.be.true(); filterUtils.combineFilters.firstCall.args.should.eql([undefined, undefined, undefined, 'author:cameron']); @@ -135,19 +135,17 @@ describe('Filter', function () { }); it('should call combineFilters with all values if set', function () { - var filterSpy = sandbox.stub(ghostBookshelf.Model.prototype, 'enforcedFilters') - .returns('status:published'), - filterSpy2 = sandbox.stub(ghostBookshelf.Model.prototype, 'defaultFilters') - .returns('page:false'), - result; + sandbox.stub(ghostBookshelf.Model.prototype, 'defaultFilters').returns('page:false'); + sandbox.stub(ghostBookshelf.Model.prototype, 'enforcedFilters').returns('status:published'); + sandbox.stub(ghostBookshelf.Model.prototype, 'extraFilters').returns('author:cameron'); - result = ghostBookshelf.Model.prototype.fetchAndCombineFilters({ - filter: 'tag:photo', - where: 'author:cameron' + const result = ghostBookshelf.Model.prototype.fetchAndCombineFilters({ + filter: 'tag:photo' }); - filterSpy.calledOnce.should.be.true(); - filterSpy2.calledOnce.should.be.true(); + ghostBookshelf.Model.prototype.enforcedFilters.calledOnce.should.be.true(); + ghostBookshelf.Model.prototype.defaultFilters.calledOnce.should.be.true(); + filterUtils.combineFilters.calledOnce.should.be.true(); filterUtils.combineFilters.firstCall.args .should.eql(['status:published', 'page:false', 'tag:photo', 'author:cameron']); diff --git a/core/test/unit/models/post_spec.js b/core/test/unit/models/post_spec.js index be7673bdc3..a774e8c56b 100644 --- a/core/test/unit/models/post_spec.js +++ b/core/test/unit/models/post_spec.js @@ -272,7 +272,7 @@ describe('Unit: models/post', function () { }); }); - describe('processOptions', function () { + describe('extraFilters', function () { it('generates correct where statement when filter contains unpermitted values', function () { const options = { filter: 'status:[published,draft]', @@ -280,14 +280,8 @@ describe('Unit: models/post', function () { status: 'published' }; - models.Post.processOptions(options); - - options.where.statements.should.be.an.Array().with.lengthOf(1); - options.where.statements[0].should.deepEqual({ - prop: 'status', - op: '=', - value: 'published' - }); + const filter = new models.Post().extraFilters(options); + filter.should.eql('status:published'); }); });