Skip to content

Created ErrorResponse to abstract error deserialization#1427

Merged
NachoSoto merged 1 commit into
mainfrom
error-response
Apr 1, 2022
Merged

Created ErrorResponse to abstract error deserialization#1427
NachoSoto merged 1 commit into
mainfrom
error-response

Conversation

@NachoSoto

@NachoSoto NachoSoto commented Mar 29, 2022

Copy link
Copy Markdown
Contributor

This is duplicated in most of the NetworkOperations. This new shared implementation will be used by HTTPClient.

For #695.
See #1429 for how this is used.

@NachoSoto NachoSoto requested a review from a team March 29, 2022 20:55
@NachoSoto NachoSoto changed the base branch from main to http-client-refactor-4 March 29, 2022 20:57
@NachoSoto NachoSoto force-pushed the error-response branch 3 times, most recently from 3dee0c7 to e2af00b Compare March 29, 2022 21:00
@@ -17,11 +17,8 @@ enum UserInfoAttributeParser {

static func attributesUserInfoFromResponse(response: [String: Any], statusCode: HTTPStatusCode) -> [String: Any] {

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 whole class will go away soon, will be superseded by the new ErrorResponse.

Comment thread Sources/Networking/HTTPResponse.swift Outdated

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.

nice!!! this will also help us get rid of some hacky stuff we have like the finishableKey and RCSuccessfullySyncedKey, which were just awkward ways to communicate information through the same existing dictionary back to other layers

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.

👍🏻

Comment thread Sources/Networking/HTTPStatusCode.swift Outdated
Comment on lines 86 to 88

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.

❤️

@NachoSoto NachoSoto force-pushed the http-client-refactor-4 branch from 36cf996 to b1ebd78 Compare March 30, 2022 21:31
NachoSoto added a commit that referenced this pull request Mar 30, 2022
Follow up to #1427.

Other changes:
- Removed `UserInfoAttributeParser`
- Simplified `JSONDecoder.decode` implementation by simply relying on `defaultJsonDecoder`
- Added support for "wrapped" error responses. This was previously handled by `UserInfoAttributeParser`.
- Removed all duplicated code parsing error responses
- The new `ErrorResponse` introduced in #1427 correctly decodes the format sent by the backend. Note that some tests had the wrong format, and `UserInfoAttributeParser` was parsing that wrong as well.

An upcoming PR will actually move these calls to `ErrorResponse` to `HTTPClient`.
@NachoSoto NachoSoto force-pushed the http-client-refactor-4 branch from 101a97a to cd7a551 Compare March 31, 2022 23:55
NachoSoto added a commit that referenced this pull request Apr 1, 2022
Follow up to #1427.

Other changes:
- Removed `UserInfoAttributeParser`
- Simplified `JSONDecoder.decode` implementation by simply relying on `defaultJsonDecoder`
- Added support for "wrapped" error responses. This was previously handled by `UserInfoAttributeParser`.
- Removed all duplicated code parsing error responses
- The new `ErrorResponse` introduced in #1427 correctly decodes the format sent by the backend. Note that some tests had the wrong format, and `UserInfoAttributeParser` was parsing that wrong as well.

An upcoming PR will actually move these calls to `ErrorResponse` to `HTTPClient`.
@NachoSoto NachoSoto force-pushed the http-client-refactor-4 branch from cd7a551 to 7b42026 Compare April 1, 2022 20:50
NachoSoto added a commit that referenced this pull request Apr 1, 2022
Follow up to #1427.

Other changes:
- Removed `UserInfoAttributeParser`
- Simplified `JSONDecoder.decode` implementation by simply relying on `defaultJsonDecoder`
- Added support for "wrapped" error responses. This was previously handled by `UserInfoAttributeParser`.
- Removed all duplicated code parsing error responses
- The new `ErrorResponse` introduced in #1427 correctly decodes the format sent by the backend. Note that some tests had the wrong format, and `UserInfoAttributeParser` was parsing that wrong as well.

An upcoming PR will actually move these calls to `ErrorResponse` to `HTTPClient`.
Base automatically changed from http-client-refactor-4 to main April 1, 2022 21:22
This is duplicated in most of the `NetworkOperation`s. This new shared implementation will be used by `HTTPClient`.
@NachoSoto NachoSoto merged commit ddb5a05 into main Apr 1, 2022
@NachoSoto NachoSoto deleted the error-response branch April 1, 2022 21:24
NachoSoto added a commit that referenced this pull request Apr 1, 2022
Follow up to #1427.

Other changes:
- Removed `UserInfoAttributeParser`
- Simplified `JSONDecoder.decode` implementation by simply relying on `defaultJsonDecoder`
- Added support for "wrapped" error responses. This was previously handled by `UserInfoAttributeParser`.
- Removed all duplicated code parsing error responses
- The new `ErrorResponse` introduced in #1427 correctly decodes the format sent by the backend. Note that some tests had the wrong format, and `UserInfoAttributeParser` was parsing that wrong as well.

An upcoming PR will actually move these calls to `ErrorResponse` to `HTTPClient`.
NachoSoto added a commit that referenced this pull request Apr 1, 2022
…1429)

Follow up to #1427.

The new way to create errors is like this:
```swift
ErrorResponse
    .from(response)
    .asBackendError(with: statusCode)
```

## Other changes:
- Removed `UserInfoAttributeParser`
- Simplified `JSONDecoder.decode` implementation by simply relying on `defaultJsonDecoder`
- Added support for "wrapped" error responses. This was previously handled by `UserInfoAttributeParser`.
- Removed all duplicated code parsing error responses
- The new `ErrorResponse` introduced in #1427 correctly decodes the format sent by the backend. Note that some tests had the wrong format, and `UserInfoAttributeParser` was parsing that wrong as well.

An upcoming PR will actually move these calls to `ErrorResponse` to `HTTPClient`.
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
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.

2 participants