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

GetCustomerInfoAPI: avoid making a request if there's any PostReceiptDataOperation in progress #1911

Merged
merged 2 commits into from
Sep 21, 2022
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
40 changes: 37 additions & 3 deletions Sources/Networking/Caching/CustomerInfoCallback.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ struct CustomerInfoCallback: CacheKeyProviding {

typealias Completion = (Result<CustomerInfo, BackendError>) -> Void

let cacheKey: String
let source: NetworkOperation.Type
let completion: Completion
var cacheKey: String
var source: NetworkOperation.Type
var completion: Completion
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add another init instead of making this mutable? Seems safer to me but lmk if you think there are problems with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd recommend reading on Swift's value types. This isn't like having mutable data in other languages, it's only mutable for mutables copies, and even then, because it doesn't have reference semantics, you can't have spooky action at a distance by changing one "instance" and mutating something else. So it's perfectly safe.


init(operation: CacheableNetworkOperation, completion: @escaping Completion) {
self.cacheKey = operation.cacheKey
Expand All @@ -28,3 +28,37 @@ struct CustomerInfoCallback: CacheKeyProviding {
}

}

// MARK: - CallbackCache helpers

extension CallbackCache where T == CustomerInfoCallback {

func addOrAppendToPostReceiptDataOperation(callback: CustomerInfoCallback) -> CallbackCacheStatus {
if let existing = self.callbacks(ofType: PostReceiptDataOperation.self).last {
return self.add(callback: callback.withNewCacheKey(existing.cacheKey))
} else {
return self.add(callback: callback)
}
}

private func callbacks(ofType type: NetworkOperation.Type) -> [T] {
return self
.cachedCallbacksByKey
.value
.lazy
.flatMap(\.value)
.filter { $0.source == type }
}

}

private extension CustomerInfoCallback {

func withNewCacheKey(_ newKey: String) -> Self {
var copy = self
copy.cacheKey = newKey

return copy
}

}
2 changes: 1 addition & 1 deletion Sources/Networking/CustomerAPI.swift
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ final class CustomerAPI {
customerInfoCallbackCache: self.customerInfoCallbackCache)

let callback = CustomerInfoCallback(operation: operation, completion: completion)
let cacheStatus = self.customerInfoCallbackCache.add(callback: callback)
let cacheStatus = self.customerInfoCallbackCache.addOrAppendToPostReceiptDataOperation(callback: callback)
self.backendConfig.addCacheableOperation(operation,
withRandomDelay: randomDelay,
cacheStatus: cacheStatus)
Expand Down
34 changes: 1 addition & 33 deletions Sources/Networking/Operations/GetCustomerInfoOperation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,8 @@ class GetCustomerInfoOperation: CacheableNetworkOperation {
self.customerInfoResponseHandler = customerInfoResponseHandler
self.customerInfoCallbackCache = customerInfoCallbackCache

var individualizedCacheKeyPart = configuration.appUserID

// If there is any enqueued `PostReceiptDataOperation` we don't want this new
// `GetCustomerInfoOperation` to share the same cache key.
// If it did, future `GetCustomerInfoOperation` would receive a cached value
// instead of an up-to-date `CustomerInfo` after those post receipt operations finish.
if customerInfoCallbackCache.hasPostReceiptOperations {
individualizedCacheKeyPart += "-\(customerInfoCallbackCache.numberOfGetCustomerInfoOperations)"
}

super.init(configuration: configuration,
individualizedCacheKeyPart: individualizedCacheKeyPart)
individualizedCacheKeyPart: configuration.appUserID)
}

override func begin(completion: @escaping () -> Void) {
Expand Down Expand Up @@ -72,25 +62,3 @@ private extension GetCustomerInfoOperation {
}

}

private extension CallbackCache where T == CustomerInfoCallback {

var numberOfGetCustomerInfoOperations: Int {
return self.callbacks(ofType: GetCustomerInfoOperation.self)
}

var hasPostReceiptOperations: Bool {
return self.callbacks(ofType: PostReceiptDataOperation.self) > 0
}

private func callbacks(ofType type: NetworkOperation.Type) -> Int {
return self
.cachedCallbacksByKey
.value
.lazy
.flatMap(\.value)
.filter { $0.source == type }
.count
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -432,8 +432,7 @@ class BackendPostReceiptDataTests: BaseBackendTests {

expect(self.httpClient.calls.map { $0.request.path }) == [
getCustomerInfoPath,
.postReceiptData,
getCustomerInfoPath
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🐼

.postReceiptData
]
}

Expand Down