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

fix: add headers to cache key #5740

Conversation

alexandernanberg
Copy link

@alexandernanberg alexandernanberg commented Sep 22, 2021

Fixes #4486
Fixes #5608

Requests with different headers should not be memoized or use the same cache key. This PR add all the headers to the cache key

@apollo-cla
Copy link

@alexandernanberg: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@glasser
Copy link
Member

glasser commented Sep 28, 2021

(First note: apollo-datasource-rest is not being very actively maintained at the moment. You may want to investigate alternatives such as the newer apollo-datasource-http.)

This change seems to be unnecessarily aggressive. Using all headers in the cache key is likely to result in few cache hits.

#4486 can be fixed by overriding cacheKeyFor yourself, as the comments above it suggest. So I think the following could be solutions:

  • Better documentation encouraging you to override cacheKeyFor
  • Making it easier to take headers into account by providing a place to pass a list of relevant headers instead of requiring you to override cacheKeyFor
  • Maybe including a few specific headers by default (such as authorization and cookie) like we do in Studio usage reporting

That said #4486 is also based on a misunderstanding of the data source lifecycle, and I've closed it.

@glasser glasser closed this Sep 28, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants