Skip to content

Introduced NetworkError and BackendError#1437

Merged
NachoSoto merged 2 commits into
wip-decodable-3-rebasedfrom
http-error
Apr 12, 2022
Merged

Introduced NetworkError and BackendError#1437
NachoSoto merged 2 commits into
wip-decodable-3-rebasedfrom
http-error

Conversation

@NachoSoto

@NachoSoto NachoSoto commented Mar 31, 2022

Copy link
Copy Markdown
Contributor

Changes:

  • 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
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:
-        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

Comment thread Sources/Networking/HTTPResponse.swift Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No more untyped Error 🎉

@NachoSoto NachoSoto force-pushed the wip-decodable-3-rebased branch from 4e0dfa8 to c53e94b Compare April 1, 2022 00:31
@NachoSoto NachoSoto force-pushed the wip-decodable-3-rebased branch 3 times, most recently from 0e34e67 to 87a2c90 Compare April 5, 2022 18:57
@NachoSoto NachoSoto force-pushed the wip-decodable-3-rebased branch from 87a2c90 to 27a3ea3 Compare April 6, 2022 21:35
@NachoSoto NachoSoto force-pushed the http-error branch 3 times, most recently from 922a191 to 5bdc29c Compare April 7, 2022 18:14
@NachoSoto NachoSoto changed the title [WIP] Introduced NetworkError to make HTTPClient errors type-safe [WIP] Introduced NetworkError and BackendError Apr 7, 2022
@NachoSoto NachoSoto force-pushed the http-error branch 8 times, most recently from 5406ed7 to fd1021d Compare April 8, 2022 00:52

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No more unknown errors 🤓

Comment thread Sources/Networking/Backend.swift Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is gone in favor of NetworkError.successfullySynced

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

YESSSSSSssss

Comment thread Sources/Error Handling/ErrorUtils.swift Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is gone in favor of NetworkError.finishable

Comment thread Sources/Networking/HTTPResponse.swift Outdated
Comment on lines 74 to 77

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These are gone in favor of the properties in NetworkError that rely on the HTTPStatusCode implementation as well.

Comment thread Sources/Networking/NetworkError.swift Outdated
Comment on lines 138 to 161

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These have extensive tests now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No more hacky introspecting of userInfo 🌮

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🪓

@NachoSoto NachoSoto changed the title [WIP] Introduced NetworkError and BackendError Introduced NetworkError and BackendError Apr 8, 2022
@NachoSoto

Copy link
Copy Markdown
Contributor Author

This still needs reviewing 🙏🏻 😇
It addresses the final comments on #1425.

@aboedo aboedo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks great!! Made a few minor comments

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)

}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yessss

Comment on lines +118 to +119
var finishable: Bool {
return self.networkError?.finishable ?? false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I might cry

case postOfferIdSignature

/// getOffer call failed with an invalid response.
case getOfferUnexpectedResponse

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree. These are the already existing UnexpectedBackendResponseError cases, but I agree it could be simplified.

Comment thread Tests/UnitTests/Networking/Backend/BackendGetCustomerInfoTests.swift Outdated
Comment thread Tests/UnitTests/Networking/Backend/BackendPostOfferForSigningTests.swift Outdated
Comment thread Tests/UnitTests/Networking/NetworkErrorTests.swift Outdated
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

look at all that code go away 😍

Comment thread Tests/UnitTests/SubscriberAttributes/BackendSubscriberAttributesTestBase.swift Outdated
@NachoSoto

Copy link
Copy Markdown
Contributor Author

Addressed all comments.

- 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`
@NachoSoto NachoSoto merged commit 19e6a9e into wip-decodable-3-rebased Apr 12, 2022
@NachoSoto NachoSoto deleted the http-error branch April 12, 2022 20:58
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 Apr 12, 2022
…rrorCodeConvertible` and added tests (#1473)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants