-
Notifications
You must be signed in to change notification settings - Fork 349
Use fallback API hosts when receiving server down response #4970
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
Tests/BackendIntegrationTests/StoreKitIntegrationTests.swift:991
- This line still references 'serverHostURL' even though the API now uses 'serverHostURLs'; update it to 'HTTPRequest.Path.serverHostURLs.first?.host' for consistency.
let host = try XCTUnwrap(HTTPRequest.Path.serverHostURL.first?.host)
Tests/BackendIntegrationTests/StoreKitIntegrationTests.swift:1012
- This line should also be updated to use 'serverHostURLs' instead of 'serverHostURL' to ensure consistency with the new fallback API host implementation.
let host = try XCTUnwrap(HTTPRequest.Path.serverHostURL.first?.host)
retryScheduled = self.retryRequestWithNextHostIfNeeded(request: request, | ||
httpURLResponse: httpURLResponse) | ||
if !retryScheduled { | ||
retryScheduled = self.retryRequestIfNeeded(request: request, | ||
httpURLResponse: httpURLResponse) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines are the actual changes in this method. The other changes in this method are to accommodate the linter (function_body_length
).
@@ -16,7 +16,7 @@ import Foundation | |||
extension HTTPRequest.DiagnosticsPath: HTTPRequestPath { | |||
|
|||
// swiftlint:disable:next force_unwrapping | |||
static let serverHostURL = URL(string: "https://api-diagnostics.revenuecat.com")! | |||
static let serverHostURLs = [URL(string: "https://api-diagnostics.revenuecat.com")!] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: add final hosts list
static let serverHostURLs = [ | ||
"https://api.revenuecat.com", | ||
"https://api2.revenuecat.com", // TODO: Add real values | ||
"https://api3.revenuecat.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: add final hosts list
@@ -16,7 +16,7 @@ import Foundation | |||
extension HTTPRequest.PaywallPath: HTTPRequestPath { | |||
|
|||
// swiftlint:disable:next force_unwrapping | |||
static let serverHostURL = URL(string: "https://api-paywalls.revenuecat.com")! | |||
static let serverHostURLs = [URL(string: "https://api-paywalls.revenuecat.com")!] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: add final hosts list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
Sources/Networking/HTTPClient/HTTPClient.swift:296
- [nitpick] Consider adding a clarifying comment in the requestWithNextFallbackPath() method to indicate that the safe subscript on fallbackPaths intentionally returns nil when the fallback index is out of bounds.
func requestWithNextFallbackPath() -> Self? {
Sources/Networking/HTTPClient/HTTPClient.swift:288
- [nitpick] The retriedRequest() method preserves the fallbackPathIndex when creating a retried request. Adding a comment to clarify this intentional behavior would improve code readability.
func retriedRequest() -> Self {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
path: nextRequest.path | ||
)) | ||
self.state.modify { | ||
$0.queuedRequests.insert(nextRequest, at: 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm nothing to do for this PR since I believe this could actually happen already, but I wonder if we should dedupe any queued requests here... In android we do something like that, and just "hook" the callbacks to the existing queued/in progress request if it exists.
/// - request: The original `HTTPClient.Request` that may need to be retried. | ||
/// - httpURLResponse: An optional `HTTPURLResponse` that contains the status code of the response. | ||
/// - Returns: A Boolean value indicating whether the request was retried. | ||
internal func retryRequestWithNextFallbackPathIfNeeded( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, nothing to do in this PR, but I was thinking it would be great to have a way to test this more easily. One thing that came to mind would be to add an option to our test proxy to block only these endpoints in the main host...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I like that! In fact I'm trying to add these fallback endpoints to our backend tests and I'm really struggling so far 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just have some general questions about naming consistency.
private func getCurrentPath() -> HTTPRequestPath? { | ||
if let fallbackPathIndex = self.fallbackPathIndex { | ||
return self.httpRequest.path.fallbackPaths[safe: fallbackPathIndex] | ||
} else { | ||
return self.httpRequest.path | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So only the path changes in a fallback scenario? I'm a bit confused as this PR title mentions hosts (and the Diagnostics PR does too), but I don't see hosts being changed. Maybe I'm misunderstanding something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that. Originally, the host would only change, but we realized that the fallback backend had different paths. In the end, after talking with COIN, they've added a mapping in the fallback backend which means that the original premise was correct: only the host will change.
I'll put this back as a draft until I've made the changes back. Sorry again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries at all! Thanks for explaining 😄
…equests support a fallback host For those requests that support a fallback host, the path, headers and response structure remain the same
📸 Snapshot Test192 unchanged
🛸 Powered by Emerge Tools |
<!-- Thank you for contributing to Purchases! Before pressing the "Create Pull Request" button, please provide the following: --> ### Checklist - [x] If applicable, unit tests - [ ] If applicable, create follow-up issues for `purchases-ios` and hybrids ### Description This PR implements a retry mechanism using fallback URL hosts for supported requests that fail with a server down error ### Notes * Only some requests support a fallback API host. Currently only getCustomerInfo and getProductEntitlementMapping support it. * `AppConfig` now has a `fallbackBaseURLs` containing the list of hosts, in order of preference, to use for supported requests. * When a request's response is a server error (5XX), the same request is immediately retried with the next host of the array. * The logic is implemented per-request: that is, if a request fails with a server-down error for host 1 and then falls back and succeeds for host 2, then all future requests will use again host 1 initially. No state is held between different requests. * No fallback retries happen when using a proxy URL iOS PR counterpart --> RevenueCat/purchases-ios#4970
<!-- Thank you for contributing to Purchases! Before pressing the "Create Pull Request" button, please provide the following: --> ### Checklist - [x] If applicable, unit tests - [ ] If applicable, create follow-up issues for `purchases-ios` and hybrids ### Description This PR implements a retry mechanism using fallback URL hosts for supported requests that fail with a server down error ### Notes * Only some requests support a fallback API host. Currently only getCustomerInfo and getProductEntitlementMapping support it. * `AppConfig` now has a `fallbackBaseURLs` containing the list of hosts, in order of preference, to use for supported requests. * When a request's response is a server error (5XX), the same request is immediately retried with the next host of the array. * The logic is implemented per-request: that is, if a request fails with a server-down error for host 1 and then falls back and succeeds for host 2, then all future requests will use again host 1 initially. No state is held between different requests. * No fallback retries happen when using a proxy URL iOS PR counterpart --> RevenueCat/purchases-ios#4970
Checklist
purchases-android
and hybridsDescription
This PR implements a retry mechanism using fallback URL hosts for supported requests that fail with a server down error
Notes
getCustomerInfo
andgetProductEntitlementMapping
support it.HTTPRequestPath
sserverHostURLs
is now an array containing the list of hosts, in order of preference, to use for supported requests.Android PR counterpart --> RevenueCat/purchases-android#2368