-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add no-cache headers to PersistedQuery error responses #2452
Add no-cache headers to PersistedQuery error responses #2452
Conversation
@ryandrewjohnson: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
c7559b2
to
da24aa8
Compare
thx Ryan, that's exactly what I need! |
@abernix @martijnwalraven any chance this can make it in for the next release? This addresses the bug we discussed on Spectrum https://spectrum.chat/apollo/apollo-server/cloudfront-caching-persistedquerynotfound-error-responses~b615361e-53f2-4771-a3d2-d04fb9349246. We have an app in production that we've had to remove Persisted Query functionality due to this bug. In the interim we've had to revert back to plain GET requests, with the plan to activate persisted queries again once this is merged/released. |
@abernix really hoping to get some feedback on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delays! I've left a couple comments, but this seems generally correct to me. I'll do a bit more looking into it before merging, but I'd be keen on putting this into an alpha of Apollo Server 2.5.0 for testing (see #2482).
@abernix thanks for taking a look at this! That would be amazing if it could make it in for the alpha release of alpha 2.5.0. |
@abernix just pushed latest fix. This should be good to merge now - thanks! |
Thanks for this PR! It's been published in |
@abernix any guesstimate on when this release will officially drop? |
The more feedback about the alpha the more confident we can be with promoting it to “latest”. Anything that would be keeping you from trying the alpha? |
@abernix I think we could probably put the alpha version in, and see how it goes. Are there any known breaking changes? |
@ryanmaxwell Nope! These are the changes at the current time: https://github.com/apollographql/apollo-server/milestone/17?closed=1 |
Issue:
Since both
PersistedQueryNotSupportedError
andPersistedQueryNotFoundError
are considered HttpGraphQL errors they will respond with an http status code of 200 with no cache-control headers set. This causes issues when you have a CDN like CloudFront sitting in front of your Apollo Server as it will hold onto these errors, which results in subsequent responses being served the cached error response.Fix:
When either
PersistedQueryNotSupportedError
orPersistedQueryNotFoundError
is thrown the following cache-control header is added to the response preventing it from being cached:For more details please see the Spectrum chat with discussion on the issue:
https://spectrum.chat/apollo/apollo-server/cloudfront-caching-persistedquerynotfound-error-responses~b615361e-53f2-4771-a3d2-d04fb9349246
TODO: