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.

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

[apollo-datasource-rest] HTTPCache incorrectly caches 302 redirects #39

Closed
gburgett opened this issue Jul 31, 2020 · 1 comment · Fixed by #100
Closed

[apollo-datasource-rest] HTTPCache incorrectly caches 302 redirects #39

gburgett opened this issue Jul 31, 2020 · 1 comment · Fixed by #100

Comments

@gburgett
Copy link

gburgett commented Jul 31, 2020

In package apollo-datasource-rest, the datasource is incorrectly storing a 302 redirect with the caching headers supplied by the final resource. Then when the request is checked, the original URL is assumed to be fresh and it does not check the original redirect again. This goes against the HTTP spec for 302 redirects (and caused a problem for me in my app)

I've created an integration spec that demonstrates the problem in my fork: https://github.com/gburgett/apollo-server/blob/rest-caching/packages/apollo-datasource-rest/src/__tests__/integration.test.ts

To reproduce:

  • Set up a REST API with a 302 redirect to some static resource with cache-control: public, max-age=31536000, immutable
  • Issue a GraphQL query that performs a GET request to this 302 redirect path
  • Change the 302 redirect to point to another resource
  • Issue the same GraphQL query again

At this point, the expected behavior is that the new resource is returned. The actual behavior is that the old resource is served from the HTTP cache.

Our current workaround is to set the TTL to 0 in the caching options for this data source.

@glasser glasser transferred this issue from apollographql/apollo-server Oct 11, 2022
glasser added a commit that referenced this issue Nov 24, 2022
RESTDataSource contained a memoization cache (separate from the
HTTP-header-sensitive shared cache) which caches all GET requests
forever. This wasn't even documented for the first four years of
RESTDataSource's existence and led to a lot of confusion.

In this new major version, this cache will instead by default only
de-duplicate *concurrent* GET requests.

This also introduces `deduplicationPolicyFor()` which lets you tune how
de-duplication works, including restoring v4 semantics (see the
changeset for code snippets).

Fixes #40.
Fixes #72.
Fixes #73.
Fixes #46.

Maybe fi-xes #39 but we could use a unit test.

Somewhat addresses #65 but only for the de-duplication cache (we should
still allow direct control over the cache key for the HTTP).
@glasser
Copy link
Member

glasser commented Nov 28, 2022

There's two things going on here.

The first issue is that there are two layers of caching in Apollo Server and one of them is basically a sledgehammer that caches all GETs forever. We are going to fix that in #100.

However, this is going to reveal the opposite issue — we won't cache any redirects at all (assuming you don't change the redirect Fetch option to manual) because we will only see the final response, not the pre-redirect response. So we also won't cache 301 responses. I'll open a new issue.

glasser added a commit that referenced this issue Nov 28, 2022
RESTDataSource contained a memoization cache (separate from the
HTTP-header-sensitive shared cache) which caches all GET requests
forever. This wasn't even documented for the first four years of
RESTDataSource's existence and led to a lot of confusion.

In this new major version, this cache will instead by default only
de-duplicate *concurrent* GET requests.

This also introduces `deduplicationPolicyFor()` which lets you tune how
de-duplication works, including restoring v4 semantics (see the
changeset for code snippets).

Fixes #40.
Fixes #72.
Fixes #73.
Fixes #46.

Maybe fi-xes #39 but we could use a unit test.

Somewhat addresses #65 but only for the de-duplication cache (we should
still allow direct control over the cache key for the HTTP).
glasser added a commit that referenced this issue Nov 28, 2022
glasser added a commit that referenced this issue Nov 28, 2022
RESTDataSource contained a memoization cache (separate from the
HTTP-header-sensitive shared cache) which caches all GET requests
forever. This wasn't even documented for the first four years of
RESTDataSource's existence and led to a lot of confusion.

In this new major version, this cache will instead by default only
de-duplicate *concurrent* GET requests.

This also introduces `deduplicationPolicyFor()` which lets you tune how
de-duplication works, including restoring v4 semantics (see the
changeset for code snippets).

Fixes #40.
Fixes #72.
Fixes #73.
Fixes #46.
Fixes #39 (though it "introduces" #102).

Somewhat addresses #65 but only for the de-duplication cache (we should
still allow direct control over the cache key for the HTTP).
glasser added a commit that referenced this issue Nov 28, 2022
glasser added a commit that referenced this issue Nov 29, 2022
RESTDataSource contained a memoization cache (separate from the
HTTP-header-sensitive shared cache) which caches all GET requests
forever. This wasn't even documented for the first four years of
RESTDataSource's existence and led to a lot of confusion.

In this new major version, this cache will instead by default only
de-duplicate *concurrent* GET requests.

This also introduces `deduplicationPolicyFor()` which lets you tune how
de-duplication works, including restoring v4 semantics (see the
changeset for code snippets).

Fixes #40.
Fixes #72.
Fixes #73.
Fixes #46.
Fixes #39 (though it "introduces" #102).

Somewhat addresses #65 but only for the de-duplication cache (we should
still allow direct control over the cache key for the HTTP).
glasser added a commit that referenced this issue Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants