apollographql / apollographql/datasource-rest

304 throws if last-modified doesn't match across requests

Open
#320 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
48
Forks
21
PR merge metrics
No merged PRs in 30d

Description

Hello,

I have not had a chance to build a repro (or PR), but at first glance it appears HTTPCache in the 304 validation flow does not properly handle the policy.revalidatedPolicy() return when the resulting status is 304 (and it returns modified)

[HTTPCache.ts](https://github.com/apollographql/datasource-rest/blob/df6aef14c07b34cdc1392fef5e8b613524df34df/src/HTTPCache.ts#L156C1-L160C1)
```
const { policy: revalidatedPolicy, modified } = policy.revalidatedPolicy(
policyRequestFrom(urlString, revalidationRequest),
policyResponseFrom(revalidationResponse),
) as unknown as { policy: SneakyCachePolicy; modified: boolean };
```

If the response is a 304, but hcs (http-cache-semantics) doesn't think the result matches it will adopt the current response (which in my case has status 304), which will result in[ RESTDataSource.ts](https://github.com/apollographql/datasource-rest/blob/3f3fe4cc927176386dbbbe839574496bb3a4acaf/src/RESTDataSource.ts#L355) throwing.

Here's what hcs has to say about this case:
[http-cache-semantics index.js](https://github.com/kornelski/http-cache-semantics/blob/eefc7266f0a0614d3762ef89c21e0b8c6bf82020/index.js#L644)
```
revalidatedPolicy(request, response) {
...
if (!matches) {
return {
policy: new this.constructor(request, response),
// Client receiving 304 without body, even if it's invalid/mismatched has no option
// but to reuse a cached body. We don't have a good way to tell clients to do
// error recovery in such case.
modified: response.status != 304,
matches: false,
};
}
...
```

They also call out the option of using the cached value, or doing another fresh request in the [readme](https://github.com/kornelski/http-cache-semantics/tree/main?tab=readme-ov-file#revalidatedpolicyrevalidationrequest-revalidationresponse)

```
revalidatedPolicy(revalidationRequest, revalidationResponse)

Use this method to update the cache after receiving a new response from the origin server. It returns an object with two keys:

policy — A new CachePolicy with HTTP headers updated from revalidationResponse. You can always replace the old cached CachePolicy with the new one.
modified — Boolean indicating whether the response body has changed.
If false, then a valid 304 Not Modified response has been received, and you can reuse the old cached response body. This is also affected by stale-if-error.
If true, you should use new response's body (if present), or make another request to the origin server without any conditional headers (i.e. don't use revalidationHeaders() this time) to get the new resource.
```

I suspect, in the error case I was seeing, cloudflare edge cache was adding a `last-modified` header based on the time when it was added to the cache, but subsequent calls would get different values depending in which node got hit causing hcs to discard the 304. It's possible it was upstream of cloudflare, but regardless it's probably inconsistent caches returning their local guess at `last-modified` time.

Seems like if there is a desire to fix this, HTTPCache can honor the 304, using the cached result and convert the 304 to a 200, or it can do another fresh request without the revalidation headers and get a fresh response.

hcs suggests using the body on the 304 if present, but as the [spec](https://www.rfc-editor.org/rfc/rfc9110.html#name-304-not-modified) precludes a 304 having content it's probably safer to not (unless there is some convention to do so I am not aware of).

```
A 304 response is terminated by the end of the header section; it cannot contain content or trailers.
```

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by reading the 304 validation flow in src/HTTPCache.ts around lines 156-160 and the response handling in src/RESTDataSource.ts around line 355. Investigate how a mismatched last-modified value from policy.revalidatedPolicy() produces a 304 response, then determine whether the intended recovery is to reuse the cached result or make a fresh request. Done means this case no longer causes RESTDataSource to throw.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.