Optimized requests with identical get helpers#26487
Conversation
ref https://linear.app/ghost/issue/ONC-1431 #get helpers from the frontend are processed as individual API requests, although there may be duplicates. This change creates a per-request cache that prevents duplicative queries hitting the database, while allowing theme creators to continue using a simple syntax.
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds per-request query deduplication to the Get helper by introducing 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ghost/core/test/unit/frontend/helpers/get.test.js (2)
835-855: Test title is misleading — it doesn't actually test different property orderings.Both hashes are
{filter: 'featured:true', limit: 5}with identical key order. The comment on line 846 acknowledges this ("hash object properties are in same order in JS"). The test verifies deduplication of literally identical option objects, which is already covered by the earlier test on line 724. Consider either removing this test as a duplicate, or actually varying the scenario (e.g., a numeric vs. stringlimit).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/frontend/helpers/get.test.js` around lines 835 - 855, The test titled "should deduplicate queries with same parameters in different order" is misleading because both calls pass identical hash objects; update the test in get.test.js to actually vary property order or types so it verifies deduplication across different representations: modify the second call to pass the hash with properties in a different insertion order (or change limit from number to string) when invoking get (same arguments shape used in the first call) and assert browseStub.callCount === 1; reference the get helper being invoked and the browseStub assertion so you change only the test inputs and keep the deduplication config (optimization:getHelper:deduplication) and locals/_queryCache setup intact.
857-888: Good concurrent deduplication test — consider also asserting on the response content.The test correctly validates that only one API call is made and both helpers render (
fn.callCount === 2). Given that both calls share the same response object (see shared-mutation concern onget.js), asserting that the data passed tofnin both calls is correct (e.g.,fn.firstCall.args[0].postsandfn.secondCall.args[0].postsboth have the expected content) would catch double-processing regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/frontend/helpers/get.test.js` around lines 857 - 888, Add assertions to the concurrent deduplication test to verify the actual response passed to the template function is correct for both invocations: after the Promise.all and the existing asserts on browseStub.callCount and fn.callCount, assert that fn.firstCall.args[0].posts (and fn.secondCall.args[0].posts) contain the expected content (e.g., an item with id 'post1' or match the meta/response shape returned by the browseStub). This ensures get (the helper under test) and the browseStub slow response are not being double-processed or mutated between concurrent calls.ghost/core/core/frontend/helpers/get.js (1)
57-73: Cache key delimiter|could cause collisions if any field value contains it.The
|-joined key is position-based, but if a value itself contains|(e.g., a filter string, slug, or order clause), two distinct queries could produce the same key. Example:filter='a|b', limit=''vs.filter='a', limit='b'would both yield...a|b|....In practice this is unlikely with Ghost's current API surface, but a safer approach would be to use a delimiter that cannot appear in any of the option values, or encode each part.
Safer alternative using JSON.stringify per part
function generateCacheKey(resource, apiOptions) { const parts = [ resource, apiOptions.filter || '', apiOptions.limit || '', apiOptions.include || '', apiOptions.fields || '', apiOptions.formats || '', apiOptions.page || '', apiOptions.order || '', apiOptions.id || '', apiOptions.slug || '', apiOptions.context?.member?.uuid || '' ]; - return parts.join('|'); + return JSON.stringify(parts); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/frontend/helpers/get.js` around lines 57 - 73, The generateCacheKey function builds a pipe-delimited key which can collide if any option value contains '|' — fix by encoding or escaping each part before joining (e.g., JSON.stringify or base64-encoding each element of parts) so the delimiter cannot be confused with data; update generateCacheKey to map parts through an encoder and then join with the delimiter, ensuring you still use the same deterministic field order (resource, apiOptions.filter, apiOptions.limit, etc.) and preserve empty-string fallbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/core/frontend/helpers/get.js`:
- Around line 341-365: The shared mutable response causes double-wrapping
because renderResponse calls prepareContextResource which mutates
response[resource]; fix it by shallow-cloning the response at the start of
renderResponse and replacing response[resource] with a shallow copy of that
array so prepareContextResource operates on the clone (preserving the
cached/original response for future calls); update renderResponse to work with
the clonedResponse and cloned array before running prepareContextResource,
leaving SafeString-wrapped values in the original untouched.
---
Nitpick comments:
In `@ghost/core/core/frontend/helpers/get.js`:
- Around line 57-73: The generateCacheKey function builds a pipe-delimited key
which can collide if any option value contains '|' — fix by encoding or escaping
each part before joining (e.g., JSON.stringify or base64-encoding each element
of parts) so the delimiter cannot be confused with data; update generateCacheKey
to map parts through an encoder and then join with the delimiter, ensuring you
still use the same deterministic field order (resource, apiOptions.filter,
apiOptions.limit, etc.) and preserve empty-string fallbacks.
In `@ghost/core/test/unit/frontend/helpers/get.test.js`:
- Around line 835-855: The test titled "should deduplicate queries with same
parameters in different order" is misleading because both calls pass identical
hash objects; update the test in get.test.js to actually vary property order or
types so it verifies deduplication across different representations: modify the
second call to pass the hash with properties in a different insertion order (or
change limit from number to string) when invoking get (same arguments shape used
in the first call) and assert browseStub.callCount === 1; reference the get
helper being invoked and the browseStub assertion so you change only the test
inputs and keep the deduplication config (optimization:getHelper:deduplication)
and locals/_queryCache setup intact.
- Around line 857-888: Add assertions to the concurrent deduplication test to
verify the actual response passed to the template function is correct for both
invocations: after the Promise.all and the existing asserts on
browseStub.callCount and fn.callCount, assert that fn.firstCall.args[0].posts
(and fn.secondCall.args[0].posts) contain the expected content (e.g., an item
with id 'post1' or match the meta/response shape returned by the browseStub).
This ensures get (the helper under test) and the browseStub slow response are
not being double-processed or mutated between concurrent calls.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/core/frontend/helpers/get.js (1)
408-418: Cache-hitcatchis overly broad — catchesrenderResponsefailures too.If the cached promise resolves successfully but
renderResponsethrows (e.g., template error), the catch on line 414 will delete a valid cache entry and fall through to make a redundant API call. Consider narrowing the try/catch to only theawait, or re-throwing non-promise errors:Suggested narrowing
if (queryCache.has(cacheKey)) { + let cachedResponse; try { // Await cached promise (handles both resolved and in-flight) - const cachedResponse = await queryCache.get(cacheKey); - returnedRowsCount = cachedResponse[resource] && cachedResponse[resource].length; - return renderResponse(cachedResponse, resource, options, data); + cachedResponse = await queryCache.get(cacheKey); } catch (error) { // Cached promise rejected - fall through to make new request queryCache.delete(cacheKey); } + + if (cachedResponse) { + returnedRowsCount = cachedResponse[resource] && cachedResponse[resource].length; + return renderResponse(cachedResponse, resource, options, data); + } }Low severity since template errors in production are rare and the per-request cache limits blast radius.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/frontend/helpers/get.js` around lines 408 - 418, The try/catch around both awaiting the cached promise and calling renderResponse causes renderResponse failures to be treated as cache-miss errors and delete the valid cache entry; narrow the scope so only the await of queryCache.get(cacheKey) is inside the try (or catch and re-throw non-promise errors), e.g. await the cached promise in a try to handle rejected/in-flight promises, then after a successful cachedResponse assignment call renderResponse(resource, options, data) outside that try and only delete queryCache for errors thrown by the await; keep the returnedRowsCount assignment tied to the cachedResponse variable and do not delete cache when renderResponse throws.ghost/core/test/unit/frontend/helpers/get.test.js (1)
871-902: Good concurrent dedup test — also consider testing timeout+dedup interaction.The concurrent test correctly validates that in-flight dedup works via
Promise.all. One gap: there's no test for whenoptimization:getHelper:timeout:thresholdis configured alongside deduplication. If the first request is aborted via timeout, both concurrent waiters would receive the@@ABORTED_GET_HELPER@@response. This may be worth a test to verify the aborted marker renders correctly for both callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/frontend/helpers/get.test.js` around lines 871 - 902, Add a new test that verifies dedup + timeout interaction: enable deduplication via configUtils.set('optimization:getHelper:deduplication', true) and set a low timeout using configUtils.set('optimization:getHelper:timeout:threshold', <small ms>) then make browseStub return after a longer delay so the initial in-flight request times out; invoke get.call twice concurrently (same args as existing test using locals, fn, inverse) and assert browseStub.callCount is 1, fn.callCount is 2, and both fn invocations were rendered with the aborted marker '@@ABORTED_GET_HELPER@@' (or equivalent aborted output) to ensure both waiters receive the aborted response.Ensure you reference the same symbols used in the file: browseStub, get.call, locals, fn, inverse, and the aborted marker when adding assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/frontend/helpers/get.js`:
- Around line 408-418: The try/catch around both awaiting the cached promise and
calling renderResponse causes renderResponse failures to be treated as
cache-miss errors and delete the valid cache entry; narrow the scope so only the
await of queryCache.get(cacheKey) is inside the try (or catch and re-throw
non-promise errors), e.g. await the cached promise in a try to handle
rejected/in-flight promises, then after a successful cachedResponse assignment
call renderResponse(resource, options, data) outside that try and only delete
queryCache for errors thrown by the await; keep the returnedRowsCount assignment
tied to the cachedResponse variable and do not delete cache when renderResponse
throws.
In `@ghost/core/test/unit/frontend/helpers/get.test.js`:
- Around line 871-902: Add a new test that verifies dedup + timeout interaction:
enable deduplication via configUtils.set('optimization:getHelper:deduplication',
true) and set a low timeout using
configUtils.set('optimization:getHelper:timeout:threshold', <small ms>) then
make browseStub return after a longer delay so the initial in-flight request
times out; invoke get.call twice concurrently (same args as existing test using
locals, fn, inverse) and assert browseStub.callCount is 1, fn.callCount is 2,
and both fn invocations were rendered with the aborted marker
'@@ABORTED_GET_HELPER@@' (or equivalent aborted output) to ensure both waiters
receive the aborted response.Ensure you reference the same symbols used in the
file: browseStub, get.call, locals, fn, inverse, and the aborted marker when
adding assertions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ghost/core/test/unit/frontend/helpers/get.test.js (1)
8-9: Combine the tworequirecalls into one.The same module is required twice.
querySimplePathandgenerateCacheKeyare properties of the same module object already held inget, so the secondrequireis redundant.♻️ Proposed consolidation
-const get = require('../../../../core/frontend/helpers/get'); -const {querySimplePath, generateCacheKey} = require('../../../../core/frontend/helpers/get'); +const get = require('../../../../core/frontend/helpers/get'); +const {querySimplePath, generateCacheKey} = get;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/frontend/helpers/get.test.js` around lines 8 - 9, You have two requires for the same module; remove the redundant require and obtain querySimplePath and generateCacheKey from the already-imported get object (e.g. keep const get = require('../../../../core/frontend/helpers/get'); then destructure const {querySimplePath, generateCacheKey} = get) so only one require is used and the helpers are referenced from get.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/test/unit/frontend/helpers/get.test.js`:
- Around line 715-735: The test currently sets locals = {root: {_locals: {}},
_queryCache: {}} which uses a plain object and doesn't exercise the Map-based
cache path; change the test to provide a real Map for _queryCache (e.g., locals
= {root: {_locals: {}}, _queryCache: new Map()}) so that get.call(...) runs with
a Map-backed cache while the deduplication config flag remains absent/disabled,
then assert browseStub.callCount === 2 to verify duplicate API calls occur;
update references to locals and _queryCache in this test case only.
- Around line 684-694: The assertion key.includes('2') is unreliable because it
can match other fields (like id: '123'); update the assertion that verifies the
serialized cache key to specifically assert that the page field is present and
equals 2 (e.g. check for the JSON field '"page":2' or use a regex like
/"page"\s*:\s*2/ or parse the key and assert parsed.page === 2). Change the
assertion around the variable key in get.test.js (the block of includes
assertions) to use a field-scoped check for the page property instead of the
loose '2' substring check.
---
Nitpick comments:
In `@ghost/core/test/unit/frontend/helpers/get.test.js`:
- Around line 8-9: You have two requires for the same module; remove the
redundant require and obtain querySimplePath and generateCacheKey from the
already-imported get object (e.g. keep const get =
require('../../../../core/frontend/helpers/get'); then destructure const
{querySimplePath, generateCacheKey} = get) so only one require is used and the
helpers are referenced from get.
EvanHahn
left a comment
EvanHahn
left a comment
There was a problem hiding this comment.
LGTM overall, but a number of nits.
ghost/core/core/frontend/services/theme-engine/middleware/update-local-template-options.js
Outdated
Show resolved
Hide resolved
ghost/core/core/frontend/services/theme-engine/middleware/update-local-template-options.js
Show resolved
Hide resolved
| try { | ||
| // Await cached promise (handles both resolved and in-flight) | ||
| const cachedResponse = await queryCache.get(cacheKey); | ||
| returnedRowsCount = cachedResponse[resource] && cachedResponse[resource].length; |
There was a problem hiding this comment.
nit: I believe this line does nothing.
| returnedRowsCount = cachedResponse[resource] && cachedResponse[resource].length; |
There was a problem hiding this comment.
Currently yes, because of the early exit you mentioned below. Though if we still check the timing, we should include the rows.
| // Await cached promise (handles both resolved and in-flight) | ||
| const cachedResponse = await queryCache.get(cacheKey); | ||
| returnedRowsCount = cachedResponse[resource] && cachedResponse[resource].length; | ||
| return renderResponse(cachedResponse, resource, options, data); |
There was a problem hiding this comment.
- issue: This early return prevents all the logging below. Cached requests will probably be below the notify threshold, but I don't think we should assume that.
- issue: If
cachedResponseresolves butrenderResponsefails, we will needlessly delete the cache key.
There was a problem hiding this comment.
Oh, yep! Good catch. Fixed. I was mostly thinking about skipping the logging in the event of a cached response, but it feels more reasonable to keep these on the same path. Plus, it's entirely possible they take forever for some reason.
There was a problem hiding this comment.
It still looks like we have an early return preventing the logging. Could you fix that?
EvanHahn
left a comment
EvanHahn
left a comment
There was a problem hiding this comment.
Two non-blocking comments. LGTM.
| }; | ||
|
|
||
| /** | ||
| * @typedef {Object.<string, unknown>} GetHelperAPIOptions |
There was a problem hiding this comment.
nit: I think this should be a Record, but I might be wrong.
| * @typedef {Object.<string, unknown>} GetHelperAPIOptions | |
| * @typedef {Record<string, unknown>} GetHelperAPIOptions |
| // Await cached promise (handles both resolved and in-flight) | ||
| const cachedResponse = await queryCache.get(cacheKey); | ||
| returnedRowsCount = cachedResponse[resource] && cachedResponse[resource].length; | ||
| return renderResponse(cachedResponse, resource, options, data); |
There was a problem hiding this comment.
It still looks like we have an early return preventing the logging. Could you fix that?
2 participants
ref https://linear.app/ghost/issue/ONC-1431
#get helpers from the frontend are processed as individual API requests, although there may be duplicates. This change creates a per-request cache that prevents duplicative queries hitting the database, while allowing theme creators to continue using a simple syntax.
This is configured to be used with a new config setting:
optimization:getHelper:deduplication.