fix(gateway): Don't cache 302 responses from the registry. #4325
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This affects only the communication with the registry, not with anything related to sending requests to the implementer's graph or its downstream services. A `HEAD` request has no body to cache and a 304 response could have only been negotiated (with the server) by means of a conditional request header (e.g., `if-none-match` or `if-modified-since`). We do NOT want to write to the cache in either of these cases! Without avoiding this cache write, we will invalidate the cache, thus causing subsequent conditional requests (e.g., `If-None-Match: "MD%") to be lacking content to conditionally request against and necessitating a full request/response.
abernix
added a commit
that referenced
this pull request
Jun 29, 2020
This is a similar treatment as to what was applied to `@apollo/gateway` in #3783. This replaces a "fetcher" implementation we'd been maintaining (which continues to grow in complexity) which was built on [`node-fetch`][1]. Instead, it switches to an off-the-shelf [Fetch API][2]-compliant implementation called [`make-fetch-happen`][3] which leverages [`minipass-fetch`][4] under the hood. It's also what `npm` uses internally! Whereas `node-fetch` is relatively bare-bones and necessitated us writing our own [conditional request][5], `make-fetch-happen` just does these things (and also other things, like supporting `HTTP_PROXY` out of the box!). It does, however, need us to provide a cache store since we cannot use its (default) file-system based cache (which leverages the powerful [`cacache`][6] implementation used by `npm`) since file-systems are not available in all of our supported integrations. Therefore, this duplicates the same cache implementation used in `@apollo/gateway`. If there was a suitable place to depend on this code from both of these packages, we could depend on this implementation from that location, but I didn't immediately see a great place for that to live. Also, rule of threes or something? (Also worth noting that it _already includes_ the fix which I opened in #4325 / 2f29c60 which needed to be applied to the gateway implementation.) This does REMOVE A TEST which was previously valuable but should no longer be nearly as valuable. Specifically, since we now have an HTTP implementation that handles conditional requests, a cache, and retries itself, we do handle intermediary retries within that layer. We still test the polling (i.e, the literal existence of an interval which re-fires) in other tests, but it was too tricky to try to re-jigger this test with the abstraction. I think this is a good thing to not need to worry about, but we can consider re-adding it in the event of regressions. [1]: https://npm.im/node-fetch [2]: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API [3]: https://npm.im/make-fetch-happen [4]: https://npm.im/minipass-fetch [5]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Conditional_requests [6]: https://npm.im/cacache Do not attempt to cache requests with no response body. A `HEAD` request has no body to cache and a 304 response could have only been negotiated by using a cached body that was still valid. Therefore, we do NOT write to the cache in either of these cases. Without avoiding this, we will invalidate the cache, thus causing subsequent conditional requests (e.g., `If-None-Match: "MD%") to be lacking content to conditionally request against and necessitating a full request/response.
abernix
added a commit
that referenced
this pull request
Jun 29, 2020
This is a similar treatment as to what was applied to `@apollo/gateway` in #3783. This replaces a "fetcher" implementation we'd been maintaining (which continues to grow in complexity) which was built on [`node-fetch`][1]. Instead, it switches to an off-the-shelf [Fetch API][2]-compliant implementation called [`make-fetch-happen`][3] which leverages [`minipass-fetch`][4] under the hood. It's also what `npm` uses internally! Whereas `node-fetch` is relatively bare-bones and necessitated us writing our own [conditional request][5], `make-fetch-happen` just does these things (and also other things, like supporting `HTTP_PROXY` out of the box!). It does, however, need us to provide a cache store since we cannot use its (default) file-system based cache (which leverages the powerful [`cacache`][6] implementation used by `npm`) since file-systems are not available in all of our supported integrations. Therefore, this duplicates the same cache implementation used in `@apollo/gateway`. If there was a suitable place to depend on this code from both of these packages, we could depend on this implementation from that location, but I didn't immediately see a great place for that to live. Also, rule of threes or something? (Also worth noting that it _already includes_ the fix which I opened in #4325 / 2f29c60 which needed to be applied to the gateway implementation.) This does REMOVE A TEST which was previously valuable but should no longer be nearly as valuable. Specifically, since we now have an HTTP implementation that handles conditional requests, a cache, and retries itself, we do handle intermediary retries within that layer. We still test the polling (i.e, the literal existence of an interval which re-fires) in other tests, but it was too tricky to try to re-jigger this test with the abstraction. I think this is a good thing to not need to worry about, but we can consider re-adding it in the event of regressions. [1]: https://npm.im/node-fetch [2]: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API [3]: https://npm.im/make-fetch-happen [4]: https://npm.im/minipass-fetch [5]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Conditional_requests [6]: https://npm.im/cacache
abernix
added a commit
that referenced
this pull request
Jun 30, 2020
This is a similar treatment as to what was applied to `@apollo/gateway` in #3783. This replaces a "fetcher" implementation we'd been maintaining (which continues to grow in complexity) which was built on [`node-fetch`][1]. Instead, it switches to an off-the-shelf [Fetch API][2]-compliant implementation called [`make-fetch-happen`][3] which leverages [`minipass-fetch`][4] under the hood. It's also what `npm` uses internally! Whereas `node-fetch` is relatively bare-bones and necessitated us writing our own [conditional request][5], `make-fetch-happen` just does these things (and also other things, like supporting `HTTP_PROXY` out of the box!). It does, however, need us to provide a cache store since we cannot use its (default) file-system based cache (which leverages the powerful [`cacache`][6] implementation used by `npm`) since file-systems are not available in all of our supported integrations. Therefore, this duplicates the same cache implementation used in `@apollo/gateway`. If there was a suitable place to depend on this code from both of these packages, we could depend on this implementation from that location, but I didn't immediately see a great place for that to live. Also, rule of threes or something? (Also worth noting that it _already includes_ the fix which I opened in #4325 / 2f29c60 which needed to be applied to the gateway implementation.) This does REMOVE A TEST which was previously valuable but should no longer be nearly as valuable. Specifically, since we now have an HTTP implementation that handles conditional requests, a cache, and retries itself, we do handle intermediary retries within that layer. We still test the polling (i.e, the literal existence of an interval which re-fires) in other tests, but it was too tricky to try to re-jigger this test with the abstraction. I think this is a good thing to not need to worry about, but we can consider re-adding it in the event of regressions. [1]: https://npm.im/node-fetch [2]: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API [3]: https://npm.im/make-fetch-happen [4]: https://npm.im/minipass-fetch [5]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Conditional_requests [6]: https://npm.im/cacache
abernix
added a commit
to apollographql/federation
that referenced
this pull request
Sep 4, 2020
…lographql/apollo-server#4326) This is a similar treatment as to what was applied to `@apollo/gateway` in apollographql/apollo-server#3783. This replaces a "fetcher" implementation we'd been maintaining (which continues to grow in complexity) which was built on [`node-fetch`][1]. Instead, it switches to an off-the-shelf [Fetch API][2]-compliant implementation called [`make-fetch-happen`][3] which leverages [`minipass-fetch`][4] under the hood. It's also what `npm` uses internally! Whereas `node-fetch` is relatively bare-bones and necessitated us writing our own [conditional request][5], `make-fetch-happen` just does these things (and also other things, like supporting `HTTP_PROXY` out of the box!). It does, however, need us to provide a cache store since we cannot use its (default) file-system based cache (which leverages the powerful [`cacache`][6] implementation used by `npm`) since file-systems are not available in all of our supported integrations. Therefore, this duplicates the same cache implementation used in `@apollo/gateway`. If there was a suitable place to depend on this code from both of these packages, we could depend on this implementation from that location, but I didn't immediately see a great place for that to live. Also, rule of threes or something? (Also worth noting that it _already includes_ the fix which I opened in apollographql/apollo-server#4325 / 5b2d2d89e4ca6 which needed to be applied to the gateway implementation.) This does REMOVE A TEST which was previously valuable but should no longer be nearly as valuable. Specifically, since we now have an HTTP implementation that handles conditional requests, a cache, and retries itself, we do handle intermediary retries within that layer. We still test the polling (i.e, the literal existence of an interval which re-fires) in other tests, but it was too tricky to try to re-jigger this test with the abstraction. I think this is a good thing to not need to worry about, but we can consider re-adding it in the event of regressions. [1]: https://npm.im/node-fetch [2]: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API [3]: https://npm.im/make-fetch-happen [4]: https://npm.im/minipass-fetch [5]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Conditional_requests [6]: https://npm.im/cacache Apollo-Orig-Commit-AS: apollographql/apollo-server@0830b66
abernix
added a commit
to apollographql/federation
that referenced
this pull request
Sep 4, 2020
…phql/apollo-server#4325) This affects only the communication with the registry, not with anything related to sending requests to the implementer's graph or its downstream services. A `HEAD` request has no body to cache and a 304 response could have only been negotiated (with the server) by means of a conditional request header (e.g., `if-none-match` or `if-modified-since`). We do NOT want to write to the cache in either of these cases! Without avoiding this cache write, we will invalidate the cache, thus causing subsequent conditional requests (e.g., `If-None-Match: "MD%") to be lacking content to conditionally request against and necessitating a full request/response. Apollo-Orig-Commit-AS: apollographql/apollo-server@4736c18
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This affects only the communication with the registry, not with anything related to sending requests to the implementer's graph or its downstream services.
A
HEADrequest has no body to cache and a 304 response could have only been negotiated (with the server) by means of a conditional request header (e.g.,if-none-matchorif-modified-since).We do NOT want to write to the cache in either of these cases! Without avoiding this cache write, we will invalidate the cache, thus causing subsequent conditional requests (e.g.,
If-None-Match: "MD%") to be lacking content to conditionally request against and necessitating a full request/response.