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: Cache write interceptor should gracefully handle missing cache records #439

Merged
merged 2 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions Tests/ApolloTests/RequestChainTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,56 @@
}
}

func test__error__givenGraphqlError_withoutData_shouldReturnError() {
// given
let client = MockURLSessionClient(
response: .mock(
url: TestURL.mockServer.url,
statusCode: 200,
httpVersion: nil,
headerFields: nil
),
data: """
{
"errors": [{
"message": "Bad request, could not start execution!"
}]
}
""".data(using: .utf8)
)

let interceptorProvider = DefaultInterceptorProvider(client: client, store: ApolloStore())
let interceptors = interceptorProvider.interceptors(for: MockQuery.mock())
let requestChain = InterceptorRequestChain(interceptors: interceptors)

let expectation = expectation(description: "Response received")

let request = JSONRequest(
operation: MockQuery<Hero>(),
graphQLEndpoint: TestURL.mockServer.url,
clientName: "test-client",
clientVersion: "test-client-version"
)

// when + then
requestChain.kickoff(request: request) { result in
defer {
expectation.fulfill()
}

switch (result) {
case let .success(data):
XCTAssertEqual(data.errors, [
GraphQLError("Bad request, could not start execution!")
])
case let .failure(error):
XCTFail("Unexpected failure result - \(error)")
}
}

wait(for: [expectation], timeout: 1)
}

// MARK: Multipart request tests

struct RequestTrapInterceptor: ApolloInterceptor {
Expand Down Expand Up @@ -882,7 +932,7 @@
throw XCTSkip("Flaky test skipped in PR #386- must be refactored or fixed in a separate PR.")

// given
let store = ApolloStore(cache: InMemoryNormalizedCache(records: [

Check warning on line 935 in Tests/ApolloTests/RequestChainTests.swift

View workflow job for this annotation

GitHub Actions / Apollo Unit Tests - macOS

code after 'throw' will never be executed
"QUERY_ROOT": [
"__typename": "Hero",
"name": "R2-D2"
Expand Down
24 changes: 9 additions & 15 deletions apollo-ios/Sources/Apollo/CacheWriteInterceptor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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.")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case noResponseToParse

case missingCacheRecords
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public var errorDescription: String? {
switch self {
case .noResponseToParse:
return "The Cache Write Interceptor was called before a response was received to be parsed. Double-check the order of your interceptors."
case .missingCacheRecords:
return "The Cache Write Interceptor cannot find any cache records. Double-check the order of your interceptors."
}
}
}
Expand All @@ -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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return
}

guard request.cachePolicy != .fetchIgnoringCacheCompletely else {
// If we're ignoring the cache completely, we're not writing to it.
chain.proceedAsync(
Expand All @@ -49,25 +48,20 @@ public struct CacheWriteInterceptor: ApolloInterceptor {
return
}

guard
let createdResponse = response,
let cacheRecords = createdResponse.cacheRecords
else {
guard let createdResponse = response else {
chain.handleErrorAsync(
CacheWriteError.missingCacheRecords,
CacheWriteError.noResponseToParse,
request: request,
response: response,
completion: completion
)
return
}

guard !chain.isCancelled else {
return
if let cacheRecords = createdResponse.cacheRecords {
self.store.publish(records: cacheRecords, identifier: request.contextIdentifier)
}

self.store.publish(records: cacheRecords, identifier: request.contextIdentifier)

chain.proceedAsync(
request: request,
response: createdResponse,
Expand Down
Loading