Introduced NetworkError and BackendError
#1437
Merged
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
NachoSoto
commented
Mar 31, 2022
| @@ -16,7 +16,7 @@ import Foundation | |||
|
|
|||
| struct HTTPResponse<Body: HTTPResponseBody> { | |||
|
|
|||
| typealias Result = Swift.Result<Self, Error> | |||
| typealias Result = Swift.Result<Self, NetworkError> | |||
There was a problem hiding this comment.
No more untyped Error 🎉
NachoSoto
force-pushed
the
wip-decodable-3-rebased
branch
from
4e0dfa8
to
c53e94b
Compare
Merged
10 tasks
NachoSoto
force-pushed
the
wip-decodable-3-rebased
branch
3 times, most recently
from
0e34e67
to
87a2c90
Compare
NachoSoto
force-pushed
the
wip-decodable-3-rebased
branch
from
87a2c90
to
27a3ea3
Compare
NachoSoto
force-pushed
the
http-error
branch
3 times, most recently
from
922a191
to
5bdc29c
Compare
NachoSoto
changed the title
NetworkError to make HTTPClient errors type-safeNetworkError and BackendError
NachoSoto
force-pushed
the
http-error
branch
8 times, most recently
from
5406ed7
to
fd1021d
Compare
NachoSoto
commented
Apr 8, 2022
| @@ -75,10 +74,6 @@ extension ReceiptStrings: CustomStringConvertible { | |||
| case .unable_to_load_receipt: | |||
| return "Unable to load receipt, ensure you are logged in to a valid Apple account." | |||
|
|
|||
| case .unknown_backend_error: | |||
There was a problem hiding this comment.
No more unknown errors 🤓
NachoSoto
commented
Apr 8, 2022
|
|
||
| class Backend { | ||
|
|
||
| static let RCSuccessfullySyncedKey: NSError.UserInfoKey = "rc_successfullySynced" |
There was a problem hiding this comment.
This is gone in favor of NetworkError.successfullySynced
NachoSoto
commented
Apr 8, 2022
| fileName: String = #fileID, functionName: String = #function, line: UInt = #line | ||
| ) -> Error { | ||
| let extraUserInfo: [NSError.UserInfoKey: Any] = [ | ||
| ErrorDetails.finishableKey: finishable |
There was a problem hiding this comment.
This is gone in favor of NetworkError.finishable
NachoSoto
commented
Apr 8, 2022
Comment on lines
-74
to
-77
| var userInfo: [NSError.UserInfoKey: Any] = [ | ||
| ErrorDetails.finishableKey: !statusCode.isServerError, | ||
| Backend.RCSuccessfullySyncedKey: statusCode.isSuccessfullySynced | ||
| ] |
There was a problem hiding this comment.
These are gone in favor of the properties in NetworkError that rely on the HTTPStatusCode implementation as well.
NachoSoto
commented
Apr 8, 2022
Comment on lines
138
to
161
| extension NetworkError { | ||
|
|
||
| var successfullySynced: Bool { | ||
| return self.errorStatusCode?.isSuccessfullySynced ?? false | ||
| } | ||
|
|
||
| var finishable: Bool { | ||
| if let statusCode = self.errorStatusCode { | ||
| return !statusCode.isServerError | ||
| } else { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
These have extensive tests now.
NachoSoto
commented
Apr 8, 2022
| @@ -584,36 +585,42 @@ private extension PurchasesOrchestrator { | |||
|
|
|||
| let completion = self.getAndRemovePurchaseCompletedCallback(forTransaction: transaction) | |||
| let error = result.error | |||
| let nsError = error as NSError? | |||
| let finishable = (nsError?.userInfo[ErrorDetails.finishableKey as String] as? NSNumber)?.boolValue ?? false | |||
| let finishable = error?.finishable ?? false | |||
There was a problem hiding this comment.
No more hacky introspecting of userInfo 🌮
NachoSoto
changed the title
NetworkError and BackendErrorNetworkError and BackendError
NachoSoto
force-pushed
the
wip-decodable-3-rebased
branch
from
7445957
to
8e0c0be
Compare
|
This still needs reviewing 🙏🏻 😇 |
aboedo
reviewed
Apr 11, 2022
Comment on lines
+19
to
+28
| enum BackendError: Error, Equatable { | ||
|
|
||
| case networkError(NetworkError) | ||
| case missingAppUserID(Source) | ||
| case emptySubscriberAttributes(Source) | ||
| case missingReceiptFile(Source) | ||
| case missingTransactionProductIdentifier(Source) | ||
| case unexpectedBackendResponse(UnexpectedBackendResponseError, extraContext: String?, Source) | ||
|
|
||
| } |
There was a problem hiding this comment.
this was one of the biggest things I was looking forward to when we started the Swift migration ❤️
| case networkError(NetworkError) | ||
| case missingAppUserID(Source) | ||
| case emptySubscriberAttributes(Source) | ||
| case missingReceiptFile(Source) |
There was a problem hiding this comment.
[not for this PR] This feels like it shouldn't be the responsibility of Backend, digging up a receipt, since it's even performed by an entirely different class
There was a problem hiding this comment.
Yeah this is sent by PurchasesOrchestrator. I didn't want to make this even bigger than it already was, but a nice follow up would be to add something like PurchaseError for PurchaseOrchestrator level errors.
Comment on lines
+112
to
+113
| var successfullySynced: Bool { | ||
| return self.networkError?.successfullySynced ?? false |
Comment on lines
+118
to
+119
| var finishable: Bool { | ||
| return self.networkError?.finishable ?? false |
| case postOfferIdSignature | ||
|
|
||
| /// getOffer call failed with an invalid response. | ||
| case getOfferUnexpectedResponse |
There was a problem hiding this comment.
feels like the naming for "unexpected thing came from the backend" is inconsistent:
loginResponseDecoding, postOfferIdBadResponse, getOfferUnexpectedResponse, customerInfoResponseParsing.
I think we should settle on naming and make them the same if they represent the same concept of "the backend returned an object that we didn't expect"
There was a problem hiding this comment.
Yeah I agree. These are the already existing UnexpectedBackendResponseError cases, but I agree it could be simplified.
Tests/UnitTests/Networking/Backend/BackendGetCustomerInfoTests.swift
Outdated
Show resolved
Hide resolved
Tests/UnitTests/Networking/Backend/BackendPostOfferForSigningTests.swift
Outdated
Show resolved
Hide resolved
Comment on lines
-133
to
-145
| let nsError = try XCTUnwrap(receivedResult?.error as NSError?) | ||
| expect(nsError.domain) == RCPurchasesErrorCodeDomain | ||
| expect(nsError.code) == ErrorCode.unknownBackendError.rawValue | ||
|
|
||
| let underlyingError = try XCTUnwrap(nsError.userInfo[NSUnderlyingErrorKey] as? NSError) | ||
|
|
||
| expect(underlyingError.domain) == "RevenueCat.UnexpectedBackendResponseSubErrorCode" | ||
| expect(underlyingError.code) == UnexpectedBackendResponseSubErrorCode.customerInfoResponseParsing.rawValue | ||
|
|
||
| let parsingError = try XCTUnwrap(underlyingError.userInfo[NSUnderlyingErrorKey] as? NSError) | ||
|
|
||
| expect(parsingError.domain) == "RevenueCat.CustomerInfoError" | ||
| expect(parsingError.code) == CustomerInfoError.missingJsonObject.rawValue |
Tests/UnitTests/SubscriberAttributes/BackendSubscriberAttributesTestBase.swift
Outdated
Show resolved
Hide resolved
|
Addressed all comments. |
NachoSoto
force-pushed
the
wip-decodable-3-rebased
branch
from
8e0c0be
to
1f28630
Compare
- Each layer (only a few for now) has its own error type: `NetworkError`, `BackendError`.
- `HTTPClient` for example has to produce `NetworkError`, operations produce `BackendError`
```diff
struct HTTPResponse<Body: HTTPResponseBody> {
- typealias Result = Swift.Result<Self, Error>
+ typealias Result = Swift.Result<Self, NetworkError>
}
```
- The parent `BackendError` can have specific errors like `.missingAppUserID`, but also be simply a child error `case networkError(NetworkError)`
- All of these conform to a `ErrorCodeConvertible`, so there is a single point of code that converts from simple and readable errors (like `BackendError.emptySubscriberAttributes`, `.unexpectedBackendResponse(.loginResponseDecoding)`) into errors with all the context using `ErrorUtils`
- Tests also become simpler:
```diff
- expect(receivedError?.domain).toEventually(equal(RCPurchasesErrorCodeDomain))
- expect(receivedError?.code).toEventually(
- equal(ErrorCode.unexpectedBackendResponseError.rawValue))
- expect(receivedUnderlyingError?.code).toEventually(
- equal(UnexpectedBackendResponseSubErrorCode.postOfferIdMissingOffersInResponse.rawValue))
+
+ expect(receivedError) == .unexpectedBackendResponse(.postOfferIdMissingOffersInResponse)
```
- Converted `DNSError` into `NetworkError.dnsError`. Its functionality remains unchanged.
- Removed `Backend.RCSuccessfullySyncedKey` and `ErrorDetails.finishableKey` in favor of tested properties on `NetworkError`
aboedo
approved these changes
Apr 12, 2022
NachoSoto
added a commit
that referenced
this pull request
Apr 12, 2022
…rkError` and `BackendError` (#1425) Closes #695 and finishes [CF-195]. Depends on ~~#1431, #1432, #1433, #1437~~. ## Goals: - [x] Handle all requests / response / deserialization errors in `HTTPClient`. Clients of `HTTPClient` will only have to handle the "happy" path: #1431 - [x] `HTTPClient` shouldn't return `Result.success` with failed responses, forcing clients to verify the response is actually a failure: #1431 - [x] Abstract error response deserialization / error creation: #1427 - [x] Abstract attribute error parsing: #1427 - [x] Move response deserialization to `HTTPClient` based on a `ResponseType: Decodable` type, so the completion block will simply return a `Result<HTTPResponse<ResponseType>, Error>` - [x] `ETagManager` should store response `Data` instead of a deserialized `[String: Any]` - [x] Improved error types in `HTTPClient` to fix tests: #1437 - [x] Dealt with non-backwards compatible `ETagManager: #1438 ## Changes: - Replaced `HTTPResponse`'s body from `[String: Any]` to a generic `HTTPResponseBody`. - Created `HTTPResponseBody` to abstract `Decodable` and provide some default implementations for types like `Data,` `[String: Any]` (for backwards compatibility to types that aren't `Decodable` yet), and `Decodable` itself. - New `HTTPResponse.Result` typealias (`Result<HTTPResponse<HTTPResponseBody>, Error>`) used everywhere. This will allow replacing `Error` with a more specific `Error` so we can forward known typed errors, and make sure that we don't end up with the wrong error type, or with a very complex error hierarchy and the details buried in `underlyingError`. - Each layer (only a few for now) has its own error type: `NetworkError`, `BackendError`, `OfferingsManager.Error`. - `HTTPClient` for example has to produce `NetworkError`, operations produce `BackendError` - The parent `BackendError` can have specific errors like `.missingAppUserID`, but also be simply a child error `case networkError(NetworkError)` - All of these conform to a `ErrorCodeConvertible`, so there is a single point of code that converts from simple and readable errors (like `BackendError.emptySubscriberAttributes`, `.unexpectedBackendResponse(.loginResponseDecoding)`) into errors with all the context using `ErrorUtils` - Converted `DNSError` into `NetworkError.dnsError`. Its functionality remains unchanged. - Removed `Backend.RCSuccessfullySyncedKey` and `ErrorDetails.finishableKey` in favor of tested properties on `NetworkError` ### Leftovers There's a few things I've decided to not finish for now: - [ ] `CustomerInfo` still does't conform to `Decodable` (manual deserialization is still supported by the current system): #1496 - `SubscriberAttributeDict` could also be `Codable` - [x] Replace `OfferingsFactory` with `Decodable`: #1435 [CF-195]: https://revenuecats.atlassian.net/browse/CF-195?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
NachoSoto
added a commit
that referenced
this pull request
Apr 12, 2022
…rrorCodeConvertible` and added tests Follow up to #1437. Fixes [CF-323]
NachoSoto
added a commit
that referenced
this pull request
Sep 4, 2022
… returned error types This is the last change from what was started with #1437. That PR introduced a few private error types, but for the errors returned in the public `APIs` we were still passing around `Error`s everywhere, which didn't provide any compile-time guarantees. - Ensure that public APIs *only* return `ErrorCode`s wrapped with the additional `userInfo`, and not accidentally return other types like `SKError`, `BackendError`, `ErrorCode` directly, etc. - Guarantee type-safety when passing errors around within our internal implementations instead of assuming certain thrown errors are of a particular type. - Created `PurchasesError` for wrapping an `ErrorCode` with the additional `userInfo`. - Changed returned errors from `Error` to `NSError`. This is a change in the API, but the new type is a more specific `Error` so it shouldn't lead to any changes in client apps. - Because `PurchasesError` isn't implicitly convertible to `NSError`, this ensures that we don't return those values directly, and instead use `PurchasesError.asPublicError`. - Changed `ErrorCodeConvertible` to a now more precise `PurchasesErrorConvertible`. Thanks to this new type-safety this PR fixes at least 2 bugs where we were returning private errors instead of `ErrorCode`s. Thanks to #1871 we know that the returned errors are still convertible to `ErrorCode` so users can `switch` over them.
NachoSoto
added a commit
that referenced
this pull request
Sep 5, 2022
… returned error types (#1879) This is the last change from what was started with #1437. That PR introduced a few private error types, but for the errors returned in the public `APIs` we were still passing around `Error`s everywhere, which didn't provide any compile-time guarantees. ### Goals: - Ensure that public APIs *only* return `ErrorCode`s wrapped with the additional `userInfo`, and not accidentally return other types like `SKError`, `BackendError`, `ErrorCode` directly, etc. - Guarantee type-safety when passing errors around within our internal implementations instead of assuming certain thrown errors are of a particular type. ### Changes: - Created `PurchasesError` for wrapping an `ErrorCode` with the additional `userInfo`. - Changed returned errors from `Error` to `NSError`. This is a change in the API, but the new type is a more specific `Error` so it shouldn't lead to any changes in client apps. - Because `PurchasesError` isn't implicitly convertible to `NSError`, this ensures that we don't return those values directly, and instead use `PurchasesError.asPublicError`. - Changed `ErrorCodeConvertible` to a now more precise `PurchasesErrorConvertible`. Thanks to this new type-safety this PR fixes at least 3 bugs where we were returning private errors instead of `ErrorCode`s. ### Testing: Thanks to #1871 we know that the returned errors are still convertible to `ErrorCode` so users can `switch` over them.
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.
Changes:
NetworkError,BackendError,OfferingsManager.Error.HTTPClientfor example has to produceNetworkError, operations produceBackendErrorstruct HTTPResponse<Body: HTTPResponseBody> { - typealias Result = Swift.Result<Self, Error> + typealias Result = Swift.Result<Self, NetworkError> }BackendErrorcan have specific errors like.missingAppUserID, but also be simply a child errorcase networkError(NetworkError)ErrorCodeConvertible, so there is a single point of code that converts from simple and readable errors (likeBackendError.emptySubscriberAttributes,.unexpectedBackendResponse(.loginResponseDecoding)) into errors with all the context usingErrorUtilsDNSErrorintoNetworkError.dnsError. Its functionality remains unchanged.Backend.RCSuccessfullySyncedKeyandErrorDetails.finishableKeyin favor of tested properties onNetworkError