diff --git a/resources/RequestTarget.ts b/resources/RequestTarget.ts index 21e68e802d..aac5b10839 100644 --- a/resources/RequestTarget.ts +++ b/resources/RequestTarget.ts @@ -56,7 +56,8 @@ export class RequestTarget extends URLSearchParams { declare previousResidency?: string[]; // Action tracking - /** Cache disposition of a get on a caching table; also mirrored onto the request Context. */ + /** Cache disposition of this get on a caching table: true if loaded from the source, false if + * served from cache. Set per-get; read it on the RequestTarget you passed to the get. */ declare loadedFromSource?: boolean; declare createdNewId?: string; diff --git a/resources/ResourceInterface.ts b/resources/ResourceInterface.ts index 1c3abdd76e..4ce2515d2e 100644 --- a/resources/ResourceInterface.ts +++ b/resources/ResourceInterface.ts @@ -94,12 +94,6 @@ export interface Context { sourceApply?: boolean; originatingOperation?: OperationFunctionName; previousResidency?: string[]; - /** Cache disposition of the most recent get on a caching table in this context: true if the get - * fetched from source — including when a source error fell back to a stale record (staleIfError); - * false if served from cache — including stale-while-revalidate responses (the source fetch - * continues in the background) and waits on another request's in-flight source fetch. - * Subsequent gets in the same context overwrite it. */ - loadedFromSource?: boolean; nodeName?: string; resourceCache?: Map; _freezeRecords?: boolean; // until v5, we conditionally freeze records for back-compat diff --git a/resources/Table.ts b/resources/Table.ts index 5ef1c3a5d4..c4c7b5d90e 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -727,7 +727,7 @@ export function makeTable(options) { // return 504 (rather than 404) if there is no content and the cache-control header // dictates not to go to source if (!this.doesExist()) throw new ServerError('Entry is not cached', 504); - if (hasSourceGet) setLoadedFromSource(target, request, false); // mark it as cached + if (hasSourceGet) setLoadedFromSource(target, false); // mark it as cached } else if (resourceOptions?.ensureLoaded) { const loadingFromSource = ensureLoadedFromSource( (this.constructor as any).source, @@ -743,7 +743,7 @@ export function makeTable(options) { TableResource._updateResource(this, entry); return this; }); - } else if (hasSourceGet) setLoadedFromSource(target, request, false); // mark it as cached + } else if (hasSourceGet) setLoadedFromSource(target, false); // mark it as cached } return this; } @@ -776,7 +776,7 @@ export function makeTable(options) { this.#record = entry.value; this.#version = entry.version; }); - } else if (hasSourceGet) setLoadedFromSource(undefined, this.getContext(), false); // mark it as cached + } } // #section: lifecycle-admin static getNewId(): any { @@ -1224,7 +1224,7 @@ export function makeTable(options) { // return 504 (rather than 404) if there is no content and the cache-control header // dictates not to go to source if (!entry?.value) throw new ServerError('Entry is not cached', 504); - if (hasSourceGet) setLoadedFromSource(target, context, false); // mark it as cached + if (hasSourceGet) setLoadedFromSource(target, false); // mark it as cached } else if (ensureLoaded) { const loadingFromSource = ensureLoadedFromSource( constructor.source, @@ -1237,7 +1237,7 @@ export function makeTable(options) { if (loadingFromSource) { txn?.disregardReadTxn(); // this could take some time, so don't keep the transaction open if possible return loadingFromSource.then((entry) => entry?.value); - } else if (hasSourceGet) setLoadedFromSource(target, context, false); // mark it as cached + } else if (hasSourceGet) setLoadedFromSource(target, false); // mark it as cached } return entry?.value; }); @@ -4555,16 +4555,10 @@ export function makeTable(options) { } } - function setLoadedFromSource( - target: RequestTarget | undefined, - context: Context | undefined, - loadedFromSource: boolean - ) { - // mirror the flag onto the context: callers that pass a plain id get an internal - // RequestTarget they never see, so the context is their only way to observe cache disposition (#1571) + function setLoadedFromSource(target: RequestTarget | undefined, loadedFromSource: boolean) { + // cache disposition is a per-get result, recorded on the RequestTarget of the get (#1576) // target may be a primitive id on instance-API calls, which can't hold the flag if (target && typeof target === 'object') target.loadedFromSource = loadedFromSource; - if (context) context.loadedFromSource = loadedFromSource; } function ensureLoadedFromSource(source: typeof TableResource, id, entry, context, resource?, target?) { if (context?.onlyIfCached) { @@ -4798,7 +4792,7 @@ export function makeTable(options) { whenResolved(getFromSource(source, id, primaryStore.getEntry(id), context, target)); else { // served from cache after waiting for another request to resolve - setLoadedFromSource(target, context, false); + setLoadedFromSource(target, false); whenResolved(entry); } }; @@ -4813,7 +4807,7 @@ export function makeTable(options) { }); } // lock acquired — this request will actually load from source - setLoadedFromSource(target, context, true); + setLoadedFromSource(target, true); const existingRecord = existingEntry?.value; // it is important to remember that this is _NOT_ part of the current transaction; nothing is changing diff --git a/unitTests/resources/caching.test.js b/unitTests/resources/caching.test.js index 1cbaf9cc9f..4193323964 100644 --- a/unitTests/resources/caching.test.js +++ b/unitTests/resources/caching.test.js @@ -133,39 +133,24 @@ describe('Caching', () => { assert.equal(target23.loadedFromSource, true); }); - it('loadedFromSource is observable on the context with a plain id', async function () { - // with a plain id, the static get dispatch mints an internal RequestTarget the caller - // never sees, so the flag must be mirrored onto the context (#1571) - CachingTable.setTTLExpiration(30); - await CachingTable.invalidate(31); - let context = {}; - let result = await CachingTable.get(31, context); - assert.equal(result.id, 31); - assert.equal(context.loadedFromSource, true); - context = {}; - result = await CachingTable.get(31, context); - assert.equal(result.id, 31); - assert.equal(context.loadedFromSource, false); - context = { onlyIfCached: true }; - result = await CachingTable.get(31, context); - assert.equal(result.id, 31); - assert.equal(context.loadedFromSource, false); - }); - - it('loadedFromSource is observable on the context with loadAsInstance = false', async function () { + it('loadedFromSource is observable on the target with loadAsInstance = false (#1576)', async function () { + // disposition is recorded on the RequestTarget of the get; verify the loadAsInstance=false + // value path marks it on both cache miss (true) and cache hit (false) const previousLoadAsInstance = CachingTable.loadAsInstance; try { CachingTable.loadAsInstance = false; CachingTable.setTTLExpiration(30); await CachingTable.invalidate(32); - let context = {}; - let result = await CachingTable.get(32, context); + let target = new RequestTarget(); + target.id = 32; + let result = await CachingTable.get(target); assert.equal(result.id, 32); - assert.equal(context.loadedFromSource, true); - context = {}; - result = await CachingTable.get(32, context); + assert.equal(target.loadedFromSource, true); + target = new RequestTarget(); + target.id = 32; + result = await CachingTable.get(target); assert.equal(result.id, 32); - assert.equal(context.loadedFromSource, false); + assert.equal(target.loadedFromSource, false); } finally { CachingTable.loadAsInstance = previousLoadAsInstance; } @@ -266,10 +251,11 @@ describe('Caching', () => { events = []; await new Promise((resolve) => setTimeout(resolve, 10)); // should be stale but not evicted - const swrContext = {}; - let result = await CachingTableStaleWhileRevalidate.get(23, swrContext); + const swrTarget = new RequestTarget(); + swrTarget.id = 23; + let result = await CachingTableStaleWhileRevalidate.get(swrTarget); assert(result); // should exist in database even though it is stale - assert.equal(swrContext.loadedFromSource, false); // stale value served from cache while revalidating + assert.equal(swrTarget.loadedFromSource, false); // stale value served from cache while revalidating assert.equal(sourceRequests, 1); // the source request should be started assert.equal(sourceResponses, 0); // the source request should not be completed yet // the source request should be completed