Skip to content

Commit

Permalink
Fixed get helper cache optimizations (#19865)
Browse files Browse the repository at this point in the history 
ref [ENG-747](https://linear.app/tryghost/issue/ENG-747/)
ref https://linear.app/tryghost/issue/ENG-747

H'okay - so what we're trying to do here is make get helper queries more
cacheable. The way we're doing that is by modifying the filter used when
we're trying to remove a single post from the query.

The idea is that we can remove that restriction on the filter, increase
the number of posts fetched by 1 and then filter the fetched posts back
down, this means that the same query, but filtering different posts,
will be updated to make _exactly_ the same query, and so share a cache!

We've been purposefully restrictive in the types of filters we
manipulate, so that we only deal with the simplest cases and the code is
easier to understand.
  • Loading branch information
@allouis @royalfig
allouis authored and royalfig committed Mar 25, 2024
1 parent e326f30 commit cd8385b
Show file tree
Hide file tree
Showing 3 changed files with 361 additions and 2 deletions.
91 changes: 89 additions & 2 deletions ghost/core/core/frontend/helpers/get.js
View file
Expand Up @@ -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}}}',
Expand Down Expand Up @@ -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
Expand All @@ -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;

Expand All @@ -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(() => {
Expand All @@ -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;
Expand Down Expand Up @@ -279,3 +364,5 @@ module.exports = async function get(resource, options) {
};

module.exports.async = true;

module.exports.optimiseFilterCacheability = optimiseFilterCacheability;
233 changes: 233 additions & 0 deletions ghost/core/test/e2e-frontend/helpers/get.test.js
View file
@@ -1,3 +1,4 @@
const assert = require('assert/strict');
const should = require('should');
const sinon = require('sinon');
const testUtils = require('../../utils');
Expand Down Expand Up @@ -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;

Expand Down

0 comments on commit cd8385b

Please sign in to comment.