fix: Cache write interceptor should gracefully handle missing cache records #439
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
* Move chain cancellation check earlier * Do not expect cache records to be returned * Only write to cache if cache records were returned
✅ Deploy Preview for eclectic-pie-88a2ba canceled.
|
✅ Deploy Preview for apollo-ios-docc canceled.
|
calvincestari
commented
Jul 24, 2024
| @@ -7,17 +7,12 @@ import ApolloAPI | |||
| public struct CacheWriteInterceptor: ApolloInterceptor { | |||
|
|
|||
| public enum CacheWriteError: Error, LocalizedError { | |||
| @available(*, deprecated, message: "Will be removed in a future version.") | |||
There was a problem hiding this comment.
This was deprecated because in the defer execution PR it was no longer being used. Had we caught this bug then it would never have been deprecated.
calvincestari
commented
Jul 24, 2024
| case noResponseToParse | ||
|
|
||
| case missingCacheRecords |
There was a problem hiding this comment.
Arguably this is a breaking change but given the short availability of release 1.14.0 I don't see this as being too impactful. Like the deprecation of noResponseToParse it would not have been introduced if we'd caught the bug earlier.
calvincestari
commented
Jul 24, 2024
| @@ -37,7 +32,11 @@ public struct CacheWriteInterceptor: ApolloInterceptor { | |||
| request: HTTPRequest<Operation>, | |||
| response: HTTPResponse<Operation>?, | |||
| completion: @escaping (Result<GraphQLResult<Operation.Data>, any Error>) -> Void) { | |||
|
|
|||
|
|
|||
| guard !chain.isCancelled else { | |||
There was a problem hiding this comment.
I've moved this earlier in the interceptAsync logic because it makes sense that if the chain has already been cancelled you would not want to receive any response, nor should the interceptor chain proceed forward.
AnthonyMDev
approved these changes
Jul 24, 2024
BobaFetters
pushed a commit
that referenced
this pull request
Jul 24, 2024
BobaFetters
pushed a commit
to apollographql/apollo-ios
that referenced
this pull request
Jul 24, 2024
BobaFetters
pushed a commit
that referenced
this pull request
Jul 24, 2024
39098c59 fix: Cache write interceptor should gracefully handle missing cache records (#439) git-subtree-dir: apollo-ios git-subtree-split: 39098c59162100bcc5a1483ca1a86b6f12e68c01
BobaFetters
pushed a commit
that referenced
this pull request
Jul 24, 2024
…cefully handle missing cache records git-subtree-dir: apollo-ios git-subtree-mainline: 0f86e27 git-subtree-split: 39098c59162100bcc5a1483ca1a86b6f12e68c01
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The defer execution changes merged from PR #413 introduced a bug where the cache write interceptor would throw if no cache records were returned. This is incorrect as it is a valid response for the
datakey to be missing and only anerrorskey returned, such as in the case of a request error.This changeset is to allow the cache write interceptor to gracefully handle missing cache records and only publish records to the cache if they were returned from parsing.
noResponseToParseerrormissingCacheRecordssince it's no longer valid (breaking change)interceptAsyncofCacheWriteInterceptormethod to return at the earliest pointThis was first reported as a result of #428 not getting a clean CI build.