diff --git a/ghost/core/core/frontend/helpers/get.js b/ghost/core/core/frontend/helpers/get.js index ab6aef45ed5c..85277b625524 100644 --- a/ghost/core/core/frontend/helpers/get.js +++ b/ghost/core/core/frontend/helpers/get.js @@ -11,6 +11,7 @@ const Sentry = require('@sentry/node'); const _ = require('lodash'); const jsonpath = require('jsonpath'); +const nqlLang = require('@tryghost/nql-lang'); const messages = { mustBeCalledAsBlock: 'The {\\{{helperName}}} helper must be called as a block. E.g. {{#{helperName}}}...{{/{helperName}}}', @@ -121,6 +122,84 @@ function parseOptions(globals, data, options) { return options; } +function optimiseFilterCacheability(resource, options) { + const noOptimisation = { + options, + parseResult(result) { + return result; + } + }; + if (resource !== 'posts') { + return noOptimisation; + } + + if (!options.filter) { + return noOptimisation; + } + + try { + if (options.filter.split('id:-').length !== 2) { + return noOptimisation; + } + + const parsedFilter = nqlLang.parse(options.filter); + // Support either `id:blah` or `id:blah+other:stuff` + if (!parsedFilter.$and && !parsedFilter.id) { + return noOptimisation; + } + const queries = parsedFilter.$and || [parsedFilter]; + const query = queries.find((q) => { + return q?.id?.$ne; + }); + + if (!query) { + return noOptimisation; + } + + const idToFilter = query.id.$ne; + + let limit = options.limit; + if (options.limit !== 'all') { + limit = options.limit ? 1 + parseInt(options.limit, 10) : 16; + } + + // We replace with id:-null so we don't have to deal with leading/trailing AND operators + const filter = options.filter.replace(/id:-[a-f0-9A-F]{24}/, 'id:-null'); + + const parseResult = function parseResult(result) { + const filteredPosts = result?.posts?.filter((post) => { + return post.id !== idToFilter; + }) || []; + + const modifiedResult = { + ...result, + posts: limit === 'all' ? filteredPosts : filteredPosts.slice(0, limit - 1) + }; + + modifiedResult.meta = modifiedResult.meta || {}; + modifiedResult.meta.cacheabilityOptimisation = true; + + if (typeof modifiedResult?.meta?.pagination?.limit === 'number') { + modifiedResult.meta.pagination.limit = modifiedResult.meta.pagination.limit - 1; + } + + return modifiedResult; + }; + + return { + options: { + ...options, + limit, + filter + }, + parseResult + }; + } catch (err) { + logging.warn(err); + return noOptimisation; + } +} + /** * * @param {String} resource @@ -131,6 +210,12 @@ function parseOptions(globals, data, options) { */ async function makeAPICall(resource, controllerName, action, apiOptions) { const controller = api[controllerName]; + let makeRequest = options => controller[action](options); + + const { + options, + parseResult + } = optimiseFilterCacheability(resource, apiOptions); let timer; @@ -141,7 +226,7 @@ async function makeAPICall(resource, controllerName, action, apiOptions) { const logLevel = config.get('optimization:getHelper:timeout:level') || 'error'; const threshold = config.get('optimization:getHelper:timeout:threshold'); - const apiResponse = controller[action](apiOptions); + const apiResponse = makeRequest(options).then(parseResult); const timeout = new Promise((resolve) => { timer = setTimeout(() => { @@ -161,7 +246,7 @@ async function makeAPICall(resource, controllerName, action, apiOptions) { response = await Promise.race([apiResponse, timeout]); clearTimeout(timer); } else { - response = await controller[action](apiOptions); + response = await makeRequest(options).then(parseResult); } return response; @@ -279,3 +364,5 @@ module.exports = async function get(resource, options) { }; module.exports.async = true; + +module.exports.optimiseFilterCacheability = optimiseFilterCacheability; diff --git a/ghost/core/test/e2e-frontend/helpers/get.test.js b/ghost/core/test/e2e-frontend/helpers/get.test.js index b45541746f48..b0dd47679a16 100644 --- a/ghost/core/test/e2e-frontend/helpers/get.test.js +++ b/ghost/core/test/e2e-frontend/helpers/get.test.js @@ -1,3 +1,4 @@ +const assert = require('assert/strict'); const should = require('should'); const sinon = require('sinon'); const testUtils = require('../../utils'); @@ -98,6 +99,238 @@ describe('e2e {{#get}} helper', function () { locals = {root: {_locals: {}}}; }); + describe('Filter optimisation', function () { + it('Does not do filter optimisation on OR queries', async function () { + await get.call({}, 'posts', { + hash: { + filter: `tag:-hash-hidden,id:-${publicPost.id}`, + limit: '5' + }, + data: {}, + locals, + fn, + inverse + }); + assert.notEqual(fn.firstCall.args[0].meta.cacheabilityOptimisation, true); + }); + it('Does not do filter optimisation on nested id queries', async function () { + await get.call({}, 'posts', { + hash: { + filter: `tag:-hash-hidden+(id:-${publicPost.id},id:-${membersPost.id})`, + limit: '5' + }, + data: {}, + locals, + fn, + inverse + }); + assert.notEqual(fn.firstCall.args[0].meta.cacheabilityOptimisation, true); + }); + it('Does not do filter optimisation on multiple negated id queries', async function () { + await get.call({}, 'posts', { + hash: { + filter: `id:-${publicPost.id}+id:-${membersPost.id}`, + limit: '5' + }, + data: {}, + locals, + fn, + inverse + }); + assert.notEqual(fn.firstCall.args[0].meta.cacheabilityOptimisation, true); + }); + it('Returns the correct posts with limit as a string', async function () { + await get.call({}, 'posts', { + hash: { + limit: '5' + }, + data: {}, + locals, + fn, + inverse + }); + const firstPostUsually = fn.firstCall.args[0].posts[0]; + const firstLimit = fn.firstCall.args[0].meta.pagination.limit; + assert.equal(firstLimit, 5); + await get.call({}, 'posts', { + hash: { + filter: `id:-${firstPostUsually.id}`, + limit: '5' + }, + data: {}, + locals, + fn, + inverse + }); + assert.equal(fn.secondCall.args[0].meta.cacheabilityOptimisation, true); + const foundFilteredPost = fn.secondCall.args[0].posts.find(post => post.id === firstPostUsually.id); + assert.equal(foundFilteredPost, undefined); + }); + it('Returns the correct posts with default limit', async function () { + await get.call({}, 'posts', { + hash: {}, + data: {}, + locals, + fn, + inverse + }); + const firstPostUsually = fn.firstCall.args[0].posts[0]; + const defaultLimit = fn.firstCall.args[0].meta.pagination.limit; + await get.call({}, 'posts', { + hash: { + filter: `id:-${firstPostUsually.id}` + }, + data: {}, + locals, + fn, + inverse + }); + assert.equal(fn.secondCall.args[0].meta.pagination.limit, defaultLimit); + assert.equal(fn.secondCall.args[0].meta.cacheabilityOptimisation, true); + const foundFilteredPost = fn.secondCall.args[0].posts.find(post => post.id === firstPostUsually.id); + assert.equal(foundFilteredPost, undefined); + }); + it('Returns the correct posts with a limit all', async function () { + await get.call({}, 'posts', { + hash: { + limit: 'all' + }, + data: {}, + locals, + fn, + inverse + }); + const firstPostUsually = fn.firstCall.args[0].posts[0]; + const initialCount = fn.firstCall.args[0].posts.length; + await get.call({}, 'posts', { + hash: { + filter: `id:-${firstPostUsually.id}`, + limit: 'all' + }, + data: {}, + locals, + fn, + inverse + }); + assert.equal(fn.secondCall.args[0].posts.length, initialCount - 1); + assert.equal(fn.secondCall.args[0].meta.pagination.limit, 'all'); + assert.equal(fn.secondCall.args[0].meta.cacheabilityOptimisation, true); + const foundFilteredPost = fn.secondCall.args[0].posts.find(post => post.id === firstPostUsually.id); + assert.equal(foundFilteredPost, undefined); + }); + it('Returns the correct posts with a solo negative filter', async function () { + await get.call({}, 'posts', { + hash: { + limit: 1 + }, + data: {}, + locals, + fn, + inverse + }); + const firstPostUsually = fn.firstCall.args[0].posts[0]; + await get.call({}, 'posts', { + hash: { + filter: `id:-${firstPostUsually.id}`, + limit: 5 + }, + data: {}, + locals, + fn, + inverse + }); + assert.equal(fn.secondCall.args[0].posts.length, 5); + assert.equal(fn.secondCall.args[0].meta.pagination.limit, 5); + assert.equal(fn.secondCall.args[0].meta.cacheabilityOptimisation, true); + const foundFilteredPost = fn.secondCall.args[0].posts.find(post => post.id === firstPostUsually.id); + assert.equal(foundFilteredPost, undefined); + }); + it('Returns the correct posts with a sandwiched negative filter', async function () { + await get.call({}, 'posts', { + hash: { + filter: `tag:-hash-hidden+visibility:public`, + limit: 1 + }, + data: {}, + locals, + fn, + inverse + }); + const firstPostUsually = fn.firstCall.args[0].posts[0]; + await get.call({}, 'posts', { + hash: { + filter: `visibility:public+id:-${firstPostUsually.id}+tag:-hash-hidden`, + limit: 5 + }, + data: {}, + locals, + fn, + inverse + }); + assert.equal(fn.secondCall.args[0].posts.length, 5); + assert.equal(fn.secondCall.args[0].meta.pagination.limit, 5); + assert.equal(fn.secondCall.args[0].meta.cacheabilityOptimisation, true); + const foundFilteredPost = fn.secondCall.args[0].posts.find(post => post.id === firstPostUsually.id); + assert.equal(foundFilteredPost, undefined); + }); + it('Returns the correct posts with a prefix negative filter', async function () { + await get.call({}, 'posts', { + hash: { + filter: `tag:-hash-hidden`, + limit: 1 + }, + data: {}, + locals, + fn, + inverse + }); + const firstPostUsually = fn.firstCall.args[0].posts[0]; + await get.call({}, 'posts', { + hash: { + filter: `id:-${firstPostUsually.id}+tag:-hash-hidden`, + limit: 5 + }, + data: {}, + locals, + fn, + inverse + }); + assert.equal(fn.secondCall.args[0].posts.length, 5); + assert.equal(fn.secondCall.args[0].meta.pagination.limit, 5); + assert.equal(fn.secondCall.args[0].meta.cacheabilityOptimisation, true); + const foundFilteredPost = fn.secondCall.args[0].posts.find(post => post.id === firstPostUsually.id); + assert.equal(foundFilteredPost, undefined); + }); + it('Returns the correct posts with a suffix negative filter', async function () { + await get.call({}, 'posts', { + hash: { + filter: `tag:-hash-hidden`, + limit: 1 + }, + data: {}, + locals, + fn, + inverse + }); + const firstPostUsually = fn.firstCall.args[0].posts[0]; + await get.call({}, 'posts', { + hash: { + filter: `tag:-hash-hidden+id:-${firstPostUsually.id}`, + limit: 5 + }, + data: {}, + locals, + fn, + inverse + }); + assert.equal(fn.secondCall.args[0].posts.length, 5); + assert.equal(fn.secondCall.args[0].meta.pagination.limit, 5); + assert.equal(fn.secondCall.args[0].meta.cacheabilityOptimisation, true); + const foundFilteredPost = fn.secondCall.args[0].posts.find(post => post.id === firstPostUsually.id); + assert.equal(foundFilteredPost, undefined); + }); + }); + describe('{{access}} property', function () { let member; diff --git a/ghost/core/test/unit/frontend/helpers/get.test.js b/ghost/core/test/unit/frontend/helpers/get.test.js index 5ecfa6a586f4..3ed0a61428cc 100644 --- a/ghost/core/test/unit/frontend/helpers/get.test.js +++ b/ghost/core/test/unit/frontend/helpers/get.test.js @@ -39,6 +39,45 @@ describe('{{#get}} helper', function () { sinon.restore(); }); + describe('cacheability optimisation', function () { + it('Ignores non posts', function () { + const apiOptions = { + filter: 'id:-abcdef1234567890abcdef12' + }; + const { + options, + parseResult + } = get.optimiseFilterCacheability('not-posts', apiOptions); + assert.equal(options.filter, 'id:-abcdef1234567890abcdef12'); + assert.deepEqual(parseResult({not: 'modified'}), {not: 'modified'}); + }); + it('Changes the filter for simple id negations', function () { + const apiOptions = { + filter: 'id:-abcdef1234567890abcdef12', + limit: 1 + }; + const { + options, + parseResult + } = get.optimiseFilterCacheability('posts', apiOptions); + assert.equal(options.filter, 'id:-null'); + assert.deepEqual(parseResult({ + posts: [{ + id: 'abcdef1234567890abcdef12' + }, { + id: '1234567890abcdef12345678' + }] + }), { + posts: [{ + id: '1234567890abcdef12345678' + }], + meta: { + cacheabilityOptimisation: true + } + }); + }); + }); + describe('context preparation', function () { const meta = {pagination: {}};