Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed get helper cache optimizations #19865

Merged
merged 1 commit into from Mar 14, 2024
Merged

Fixed get helper cache optimizations #19865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
91 changes: 89 additions & 2 deletions ghost/core/core/frontend/helpers/get.js
View file
Open in desktop
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
Open in desktop
@@ -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 () {
allouis marked this conversation as resolved.
Show resolved Hide resolved
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