Introduced NetworkError and BackendError#1437
Conversation
There was a problem hiding this comment.
No more untyped Error 🎉
4e0dfa8 to
c53e94b
Compare
0e34e67 to
87a2c90
Compare
87a2c90 to
27a3ea3
Compare
922a191 to
5bdc29c
Compare
NetworkError to make HTTPClient errors type-safeNetworkError and BackendError
5406ed7 to
fd1021d
Compare
There was a problem hiding this comment.
No more unknown errors 🤓
There was a problem hiding this comment.
This is gone in favor of NetworkError.successfullySynced
There was a problem hiding this comment.
This is gone in favor of NetworkError.finishable
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.
There was a problem hiding this comment.
These have extensive tests now.
There was a problem hiding this comment.
No more hacky introspecting of userInfo 🌮
NetworkError and BackendErrorNetworkError and BackendError
7445957 to
8e0c0be
Compare
|
This still needs reviewing 🙏🏻 😇 |
aboedo
left a comment
There was a problem hiding this comment.
looks great!! Made a few minor comments
| 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.
| var successfullySynced: Bool { | ||
| return self.networkError?.successfullySynced ?? false |
| 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.
| 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 |
|
Addressed all comments. |
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`
…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
…rrorCodeConvertible` and added tests Follow up to #1437. Fixes [CF-323]
… 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.
… 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.
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