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.

Sign up for GitHub

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

Jump to bottom

fix: Remove managedSelf from InterceptorRequestChain #3070

Merged
merged 14 commits into from
Jun 14, 2023
Merged

fix: Remove managedSelf from InterceptorRequestChain #3070

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c7c75c2
Adds tests for interceptor chain exits
calvincestari Jun 8, 2023
4923c16
Cleanup request chain memory tests
calvincestari Jun 9, 2023
41a4ee8
Adds interceptor chain retry test
calvincestari Jun 9, 2023
9447ee3
Adds cancellation based tests
calvincestari Jun 11, 2023
7f42faf
Removes managedSelf
calvincestari Jun 12, 2023
96a6d22
Adds identification to ApolloInterceptor
calvincestari Jun 12, 2023
d869491
Deprecates proceedAsync
calvincestari Jun 12, 2023
ea357eb
Refactor ApolloInterceptor and proceedAsync
calvincestari Jun 13, 2023
68eff3f
Add missing Foundation import
calvincestari Jun 13, 2023
8b58f86
Renames tests
calvincestari Jun 13, 2023
401165f
Uses immutables where possible
calvincestari Jun 13, 2023
9c8d58a
Adds interceptor index sync tests
calvincestari Jun 13, 2023
883cb4b
Adds 1.3 migration guide
calvincestari Jun 13, 2023
bdc34bb
Changes hash table key to String
calvincestari Jun 14, 2023
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
4 changes: 0 additions & 4 deletions Apollo.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,6 @@
E69BEDA52798B86D00000D10 /* InputObjectTemplate.swift in Sources */ = {isa = PBXBuildFile; fileRef = E69BEDA42798B86D00000D10 /* InputObjectTemplate.swift */; };
E69BEDA72798B89600000D10 /* InputObjectTemplateTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E69BEDA62798B89600000D10 /* InputObjectTemplateTests.swift */; };
E69F436C29B81182006FF548 /* InterceptorRequestChain.swift in Sources */ = {isa = PBXBuildFile; fileRef = E69F436B29B81182006FF548 /* InterceptorRequestChain.swift */; };
E69F436E29B812FC006FF548 /* ApolloInterceptorReentrantWrapper.swift in Sources */ = {isa = PBXBuildFile; fileRef = E69F436D29B812FC006FF548 /* ApolloInterceptorReentrantWrapper.swift */; };
E69F437229BBD958006FF548 /* MultipartResponseParsingInterceptor.swift in Sources */ = {isa = PBXBuildFile; fileRef = E69F437129BBD958006FF548 /* MultipartResponseParsingInterceptor.swift */; };
E6A6866427F63AEF008A1D13 /* FileGeneratorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E6A6866327F63AEF008A1D13 /* FileGeneratorTests.swift */; };
E6A6866627F63BDC008A1D13 /* FileGenerator_ResolvePath_Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E6A6866527F63BDC008A1D13 /* FileGenerator_ResolvePath_Tests.swift */; };
Expand Down Expand Up @@ -1969,7 +1968,6 @@
E69BEDA42798B86D00000D10 /* InputObjectTemplate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = InputObjectTemplate.swift; sourceTree = "<group>"; };
E69BEDA62798B89600000D10 /* InputObjectTemplateTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = InputObjectTemplateTests.swift; sourceTree = "<group>"; };
E69F436B29B81182006FF548 /* InterceptorRequestChain.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = InterceptorRequestChain.swift; sourceTree = "<group>"; };
E69F436D29B812FC006FF548 /* ApolloInterceptorReentrantWrapper.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ApolloInterceptorReentrantWrapper.swift; sourceTree = "<group>"; };
E69F437129BBD958006FF548 /* MultipartResponseParsingInterceptor.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MultipartResponseParsingInterceptor.swift; sourceTree = "<group>"; };
E6A6866327F63AEF008A1D13 /* FileGeneratorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FileGeneratorTests.swift; sourceTree = "<group>"; };
E6A6866527F63BDC008A1D13 /* FileGenerator_ResolvePath_Tests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = FileGenerator_ResolvePath_Tests.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -2327,7 +2325,6 @@
children = (
9BC742AF24D09F880029282C /* DefaultInterceptors */,
9B260BEA245A020300562176 /* ApolloInterceptor.swift */,
E69F436D29B812FC006FF548 /* ApolloInterceptorReentrantWrapper.swift */,
9BC742AB24CFB2FF0029282C /* ApolloErrorInterceptor.swift */,
9B260C07245A437400562176 /* InterceptorProvider.swift */,
);
Expand Down Expand Up @@ -5509,7 +5506,6 @@
DE100B18287F3FB100BE11C2 /* Documentation.docc in Sources */,
9FA6F3681E65DF4700BF8D73 /* GraphQLResultAccumulator.swift in Sources */,
9FF90A651DDDEB100034C3B6 /* GraphQLExecutor.swift in Sources */,
E69F436E29B812FC006FF548 /* ApolloInterceptorReentrantWrapper.swift in Sources */,
9BDE43DF22C6708600FD7C7F /* GraphQLHTTPRequestError.swift in Sources */,
9B1CCDD92360F02C007C9032 /* Bundle+Helpers.swift in Sources */,
9B260BF5245A028D00562176 /* HTTPResponse.swift in Sources */,
Expand Down
2 changes: 2 additions & 0 deletions Sources/Apollo/ApolloInterceptor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import ApolloAPI

/// A protocol to set up a chainable unit of networking work.
public protocol ApolloInterceptor {

var id: String { get set }
Copy link
Member Author

Choose a reason for hiding this comment

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

Identifiable is only available on iOS 13+ and macOS 15+, we support lower versions of both. This imposes the same 'identifiable' requirement for interceptors.

Copy link

Choose a reason for hiding this comment

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

  1. It would be really helpful to document this method and explain what the purpose and scope of this identifier is, so that custom implementers can decide what the best identifier would be for their specific use case.
  2. In order to avoid the breaking change introduced by this, have you considered a default implementation of this requirement using a protocol extension?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be really helpful to document this method and explain what the purpose and scope of this identifier is, so that custom implementers can decide what the best identifier would be for their specific use case.

Documentation can be added to clarify it's purpose, I'll get that done. As long as it's unique to other identifiers and a String it shouldn't matter. This was required as a workaround to the interceptor architecture while fixing the memory leak and maintaining the ability to support HTTP-based subscriptions.

In order to avoid the breaking change introduced by this, have you considered a default implementation of this requirement using a protocol extension?

Yes I did, but Swift extensions cannot hold stored properties and the identifier needs to be constant during the lifetime of the interceptor. Static constants don't work here either.

Copy link

Choose a reason for hiding this comment

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

Documentation can be added to clarify it's purpose, I'll get that done.

Thanks! Perhaps the scope is the interceptor instance? What does "other identifiers" mean concretely? Unique within the set of installed interceptors active during the processing of one request?

Swift extensions cannot hold stored properties and the identifier needs to be constant during the lifetime of the interceptor.

Ahh, makes sense. Perhaps an alternative might've been an id { ObjectIdentifier(self) }, but it would incur a protocol requirement for class-semantics by the implementer.

Copy link
Member Author

Choose a reason for hiding this comment

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

What does "other identifiers" mean concretely? Unique within the set of installed interceptors active during the processing of one request?

Correct. The interceptor provider passed into RequestChainNetworkTransport during initialization is used to build the request chain for each request. They need to be unique amongst that array of interceptors.


/// Called when this interceptor should do its work.
///
Expand Down
107 changes: 0 additions & 107 deletions Sources/Apollo/ApolloInterceptorReentrantWrapper.swift
View file
Open in desktop

This file was deleted.

42 changes: 27 additions & 15 deletions Sources/Apollo/AutomaticPersistedQueryInterceptor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ public struct AutomaticPersistedQueryInterceptor: ApolloInterceptor {
}
}
}

public var id: String = UUID().uuidString

/// Designated initializer
public init() {}
Expand All @@ -36,35 +38,46 @@ public struct AutomaticPersistedQueryInterceptor: ApolloInterceptor {
guard let jsonRequest = request as? JSONRequest,
jsonRequest.autoPersistQueries else {
// Not a request that handles APQs, continue along
chain.proceedAsync(request: request,
response: response,
completion: completion)
chain.proceedAsync(
request: request,
response: response,
interceptor: self,
completion: completion
)
return
}

guard let result = response?.parsedResponse else {
// This is in the wrong order - this needs to be parsed before we can check it.
chain.handleErrorAsync(APQError.noParsedResponse,
request: request,
response: response,
completion: completion)
chain.handleErrorAsync(
calvincestari marked this conversation as resolved.
Show resolved Hide resolved
APQError.noParsedResponse,
request: request,
response: response,
completion: completion
)
return
}

guard let errors = result.errors else {
// No errors were returned so no retry is necessary, continue along.
chain.proceedAsync(request: request,
response: response,
completion: completion)
chain.proceedAsync(
request: request,
response: response,
interceptor: self,
completion: completion
)
return
}

let errorMessages = errors.compactMap { $0.message }
guard errorMessages.contains("PersistedQueryNotFound") else {
// The errors were not APQ errors, continue along.
chain.proceedAsync(request: request,
response: response,
completion: completion)
chain.proceedAsync(
request: request,
response: response,
interceptor: self,
completion: completion
)
return
}

Expand Down Expand Up @@ -93,7 +106,6 @@ public struct AutomaticPersistedQueryInterceptor: ApolloInterceptor {

// We need to retry this query with the full body.
jsonRequest.isPersistedQueryRetry = true
chain.retry(request: jsonRequest,
completion: completion)
chain.retry(request: jsonRequest, completion: completion)
}
}
77 changes: 52 additions & 25 deletions Sources/Apollo/CacheReadInterceptor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import Foundation
#if !COCOAPODS
import ApolloAPI
#endif
Expand All @@ -6,6 +7,8 @@ import ApolloAPI
public struct CacheReadInterceptor: ApolloInterceptor {

private let store: ApolloStore

public var id: String = UUID().uuidString

/// Designated initializer
///
Expand All @@ -24,62 +27,86 @@ public struct CacheReadInterceptor: ApolloInterceptor {
case .mutation,
.subscription:
// Mutations and subscriptions don't need to hit the cache.
chain.proceedAsync(request: request,
response: response,
completion: completion)
chain.proceedAsync(
request: request,
response: response,
interceptor: self,
completion: completion
)

case .query:
switch request.cachePolicy {
case .fetchIgnoringCacheCompletely,
.fetchIgnoringCacheData:
// Don't bother with the cache, just keep going
chain.proceedAsync(request: request,
response: response,
completion: completion)
chain.proceedAsync(
request: request,
response: response,
interceptor: self,
completion: completion
)

case .returnCacheDataAndFetch:
self.fetchFromCache(for: request, chain: chain) { cacheFetchResult in
switch cacheFetchResult {
case .failure:
// Don't return a cache miss error, just keep going
break
case .success(let graphQLResult):
chain.returnValueAsync(for: request,
value: graphQLResult,
completion: completion)
chain.returnValueAsync(
for: request,
value: graphQLResult,
completion: completion
)
}

// In either case, keep going asynchronously
chain.proceedAsync(request: request,
response: response,
completion: completion)
chain.proceedAsync(
request: request,
response: response,
interceptor: self,
completion: completion
)
}
case .returnCacheDataElseFetch:
self.fetchFromCache(for: request, chain: chain) { cacheFetchResult in
switch cacheFetchResult {
case .failure:
// Cache miss, proceed to network without returning error
chain.proceedAsync(request: request,
response: response,
completion: completion)
chain.proceedAsync(
request: request,
response: response,
interceptor: self,
completion: completion
)

case .success(let graphQLResult):
// Cache hit! We're done.
chain.returnValueAsync(for: request,
value: graphQLResult,
completion: completion)
chain.returnValueAsync(
for: request,
value: graphQLResult,
completion: completion
)
}
}
case .returnCacheDataDontFetch:
self.fetchFromCache(for: request, chain: chain) { cacheFetchResult in
switch cacheFetchResult {
case .failure(let error):
// Cache miss - don't hit the network, just return the error.
chain.handleErrorAsync(error,
request: request,
response: response,
completion: completion)
chain.handleErrorAsync(
error,
request: request,
response: response,
completion: completion
)

case .success(let result):
chain.returnValueAsync(for: request,
value: result,
completion: completion)
chain.returnValueAsync(
for: request,
value: result,
completion: completion
)
}
}
}
Expand Down
Loading