Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Fix response from cache metadata with tests #381

Merged
merged 2 commits into from
Dec 6, 2024
+49 −32
Merged

Fix response from cache metadata with tests #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f74fba3
Fix response from cache metadata with tests
smyrick Dec 5, 2024
e7c1086
Add changeset
smyrick Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fluffy-news-cry.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@apollo/datasource-rest': patch
---

Add responseFromCache to DataSourceFetchResult
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion src/HTTPCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ interface SneakyCachePolicy extends CachePolicy {

interface ResponseWithCacheWritePromise {
response: FetcherResponse;
responseFromCache?: Boolean;
responseFromCache?: boolean;
cacheWritePromise?: Promise<void>;
}

Expand Down Expand Up @@ -247,6 +247,7 @@ export class HTTPCache<CO extends CacheOptions = CacheOptions> {
const returnedResponse = response.clone();
return {
response: returnedResponse,
responseFromCache: false,
cacheWritePromise: this.readResponseAndWriteToCache({
response,
policy,
Expand Down
11 changes: 5 additions & 6 deletions src/RESTDataSource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ export interface DataSourceFetchResult<TResult> {
response: FetcherResponse;
requestDeduplication: RequestDeduplicationResult;
httpCache: HTTPCacheResult;
responseFromCache?: boolean;
}

// RESTDataSource has two layers of caching. The first layer is purely in-memory
Expand Down Expand Up @@ -536,16 +537,13 @@ export abstract class RESTDataSource<CO extends CacheOptions = CacheOptions> {
? outgoingRequest.cacheOptions
: this.cacheOptionsFor?.bind(this);
try {
const { response, cacheWritePromise } = await this.httpCache.fetch(
url,
outgoingRequest,
{
const { response, responseFromCache, cacheWritePromise } =
await this.httpCache.fetch(url, outgoingRequest, {
cacheKey,
cacheOptions,
httpCacheSemanticsCachePolicyOptions:
outgoingRequest.httpCacheSemanticsCachePolicyOptions,
},
);
});

if (cacheWritePromise) {
this.catchCacheWritePromiseErrors(cacheWritePromise);
Expand All @@ -566,6 +564,7 @@ export abstract class RESTDataSource<CO extends CacheOptions = CacheOptions> {
httpCache: {
cacheWritePromise,
},
responseFromCache,
};
} catch (error) {
this.didEncounterError(error as Error, outgoingRequest, url);
Expand Down
58 changes: 35 additions & 23 deletions src/__tests__/RESTDataSource.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1720,12 +1720,14 @@ describe('RESTDataSource', () => {
'cache-control': 'public, max-age=31536000, immutable',
});
nock(apiUrl).get('/foo/2').reply(200);
const { httpCache } = await dataSource.getFoo(1);
expect(httpCache.cacheWritePromise).toBeDefined();
await httpCache.cacheWritePromise;
const firstResponse = await dataSource.getFoo(1);
expect(firstResponse.responseFromCache).toBeFalsy();
expect(firstResponse.httpCache.cacheWritePromise).toBeDefined();
await firstResponse.httpCache.cacheWritePromise;

// Call a second time which should be cached
await dataSource.getFoo(1);
const secondResponse = await dataSource.getFoo(1);
expect(secondResponse.responseFromCache).toBe(true);
});

it('does not cache 302 responses', async () => {
Expand All @@ -1741,16 +1743,18 @@ describe('RESTDataSource', () => {
'cache-control': 'public, max-age=31536000, immutable',
});
nock(apiUrl).get('/foo/2').reply(200);
const { httpCache } = await dataSource.getFoo(1);
expect(httpCache.cacheWritePromise).toBeUndefined();
const firstResponse = await dataSource.getFoo(1);
expect(firstResponse.responseFromCache).toBeFalsy();
expect(firstResponse.httpCache.cacheWritePromise).toBeUndefined();

// Call a second time which should NOT be cached (it's a temporary redirect!).
nock(apiUrl).get('/foo/1').reply(302, '', {
location: 'https://api.example.com/foo/2',
'cache-control': 'public, max-age=31536000, immutable',
});
nock(apiUrl).get('/foo/2').reply(200);
await dataSource.getFoo(1);
const secondResponse = await dataSource.getFoo(1);
expect(secondResponse.responseFromCache).toBeFalsy();
});

it('allows setting cache options for each request', async () => {
Expand All @@ -1773,12 +1777,14 @@ describe('RESTDataSource', () => {
})();

nock(apiUrl).get('/foo/1').reply(200);
const { httpCache } = await dataSource.getFoo(1);
expect(httpCache.cacheWritePromise).toBeDefined();
await httpCache.cacheWritePromise;
const firstResponse = await dataSource.getFoo(1);
expect(firstResponse.responseFromCache).toBeFalsy();
expect(firstResponse.httpCache.cacheWritePromise).toBeDefined();
await firstResponse.httpCache.cacheWritePromise;

// Call a second time which should be cached
await dataSource.getFoo(1);
const secondResponse = await dataSource.getFoo(1);
expect(secondResponse.responseFromCache).toBe(true);
});

it('allows setting custom cache options for each request', async () => {
Expand All @@ -1800,16 +1806,17 @@ describe('RESTDataSource', () => {
const spyOnHttpFetch = jest.spyOn(dataSource['httpCache'], 'fetch');

nock(apiUrl).get('/foo/1').reply(200);
const { httpCache } = await dataSource.getFoo(1);
expect(httpCache.cacheWritePromise).toBeDefined();
await httpCache.cacheWritePromise;
const firstResponse = await dataSource.getFoo(1);
expect(firstResponse.httpCache.cacheWritePromise).toBeDefined();
await firstResponse.httpCache.cacheWritePromise;
expect(spyOnHttpFetch.mock.calls[0][2]).toEqual({
cacheKey: 'GET https://api.example.com/foo/1',
cacheOptions: { ttl: 1000000, tags: ['foo', 'bar'] },
});

// Call a second time which should be cached
await dataSource.getFoo(1);
const secondResponse = await dataSource.getFoo(1);
expect(secondResponse.responseFromCache).toBe(true);
});

it('allows setting a short TTL for the cache', async () => {
Expand All @@ -1835,9 +1842,10 @@ describe('RESTDataSource', () => {
})();

nock(apiUrl).get('/foo/1').reply(200);
const { httpCache } = await dataSource.getFoo(1);
expect(httpCache.cacheWritePromise).toBeDefined();
await httpCache.cacheWritePromise;
const firstResponse = await dataSource.getFoo(1);
expect(firstResponse.responseFromCache).toBeFalsy();
expect(firstResponse.httpCache.cacheWritePromise).toBeDefined();
await firstResponse.httpCache.cacheWritePromise;

// expire the cache (note: 999ms, just shy of the 1s ttl, will reliably fail this test)
jest.advanceTimersByTime(1000);
Expand All @@ -1864,12 +1872,15 @@ describe('RESTDataSource', () => {
'set-cookie':
'whatever=blah; expires=Mon, 01-Jan-2050 00:00:00 GMT; path=/; domain=www.example.com',
});
const { httpCache } = await dataSource.getFoo(1, false);
expect(httpCache.cacheWritePromise).toBeDefined();
await httpCache.cacheWritePromise;
const firstResponse = await dataSource.getFoo(1, false);
expect(firstResponse.responseFromCache).toBeFalsy();
expect(firstResponse.httpCache.cacheWritePromise).toBeDefined();
await firstResponse.httpCache.cacheWritePromise;

// Call a second time which should be cached despite `set-cookie` due to
// `shared: false`.
await dataSource.getFoo(1, false);
const secondResponse = await dataSource.getFoo(1, false);
expect(secondResponse.responseFromCache).toBe(true);

nock(apiUrl).get('/foo/2').times(2).reply(200, '{}', {
'Cache-Control': 'max-age=60,must-revalidate',
Expand All @@ -1883,7 +1894,8 @@ describe('RESTDataSource', () => {
).toBeUndefined();
// Call a second time which should be not be cached because of
// `set-cookie` with `shared: true`. (Note the `.times(2)` above.)
await dataSource.getFoo(2, true);
const cookieResponse = await dataSource.getFoo(2, true);
expect(cookieResponse.responseFromCache).toBeFalsy();
});

it('should not crash in revalidation flow header handling when sending non-array non-string headers', async () => {
Expand Down