Replaced manual error response deserialization with `ErrorResponse` by NachoSoto · Pull Request #1429 · RevenueCat/purchases-ios

NachoSoto · 2022-03-30T00:15:24Z

Follow up to #1427.

The new way to create errors is like this:

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 Created ErrorResponse to abstract error deserialization #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 · 2022-03-30T00:16:44Z

This decode method mutates the instance it's called on, which isn't a great practice. At least the keyDecodingStrategy is no longer changed and we rely on a single defaultJsonDecoder everywhere. I should look at the date and data decoding strategies, we might be able to just set them on defaultJsonDecoder too.

Done: #1432

NachoSoto · 2022-03-30T00:17:11Z

This was previously handled by UserInfoAttributeParser.

Follow up to #1429. For #695. This was duplicated in all operations. `HTTPClient` now only forwards `Result.success` if the request actually succeeded, leaving each `NetworkOperation` to just parse the body (which will become part of `HTTPClient` too). Other changes: - Simplified logic for when to retry requests in `HTTPClient` by using a new `Result.asOptionalResult` - Removed several response handlers that don't need to do anything anymore - Removed several tests that were basically duplicated: - The `finishable` checks are already tested as part of `ErrorResponse`. - All the checks for the error details are also part of `ErrorResponse`. - No longer need to have a test for each type of failed response since that's part of `HTTPClient` tests now.

Follow up to #1429. The `decode` methods no longer mutate the underlying `JSONDecoder`, and instead the codebase uses a shared `defaultJSONDecoder` with the correct configuration.

taquitos · 2022-03-31T22:19:22Z

                    : .failure(
-                        ErrorUtils.backendError(withBackendCode: BackendErrorCode(code: response["code"]),
-                                                backendMessage: response["message"] as? String)
+                        ErrorResponse


This is SO much better. I hacked on the errors a bunch just to get it working and surfacing what we wanted to surface, but this is a nice API 🧡

taquitos

YES. This is good.

It's not Friday, yet, but I'm still dropping a gif.

Follow up to #1429. For #695. This was duplicated in all operations. `HTTPClient` now only forwards `Result.success` if the request actually succeeded, leaving each `NetworkOperation` to just parse the body (which will become part of `HTTPClient` too). Other changes: - Simplified logic for when to retry requests in `HTTPClient` by using a new `Result.asOptionalResul` - Removed several response handlers that don't need to do anything anymore - Removed several tests that were basically duplicated: - The `finishable` checks are already tested as part of `ErrorResponse`. - All the checks for the error details are also part of `ErrorResponse`. - No longer need to have a test for each type of failed response since that's part of `HTTPClient`tests now.

Follow up to #1429. The `decode` methods no longer mutate the underlying `JSONDecoder`, and instead the codebase uses a shared `defaultJSONDecoder` with the correct configuration.

Follow up to #1429. For #695. This was duplicated in all operations. `HTTPClient` now only forwards `Result.success` if the request actually succeeded, leaving each `NetworkOperation` to just parse the body (which will become part of `HTTPClient` too). Other changes: - Simplified logic for when to retry requests in `HTTPClient` by using a new `Result.asOptionalResul` - Removed several response handlers that don't need to do anything anymore - Removed several tests that were basically duplicated: - The `finishable` checks are already tested as part of `ErrorResponse`. - All the checks for the error details are also part of `ErrorResponse`. - No longer need to have a test for each type of failed response since that's part of `HTTPClient`tests now.

Follow up to #1429. The `decode` methods no longer mutate the underlying `JSONDecoder`, and instead the codebase uses a shared `defaultJSONDecoder` with the correct configuration.

This is duplicated in most of the `NetworkOperation`s. This new shared implementation will be used by `HTTPClient`. For #695. See #1429 for how this is used.

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`.

Follow up to #1429. The `decode` methods no longer mutate the underlying `JSONDecoder`, and instead the codebase uses a shared `defaultJSONDecoder` with the correct configuration.

Follow up to #1429. For #695. This was duplicated in all operations. `HTTPClient` now only forwards `Result.success` if the request actually succeeded, leaving each `NetworkOperation` to just parse the body (which will become part of `HTTPClient` too). Other changes: - Simplified logic for when to retry requests in `HTTPClient` by using a new `Result.asOptionalResul` - Removed several response handlers that don't need to do anything anymore - Removed several tests that were basically duplicated: - The `finishable` checks are already tested as part of `ErrorResponse`. - All the checks for the error details are also part of `ErrorResponse`. - No longer need to have a test for each type of failed response since that's part of `HTTPClient`tests now.

Follow up to #1429. For #695. This was duplicated in all operations. `HTTPClient` now only forwards `Result.success` if the request actually succeeded, leaving each `NetworkOperation` to just parse the body (which will become part of `HTTPClient` too). Other changes: - Simplified logic for when to retry requests in `HTTPClient` by using a new `Result.asOptionalResult` - Removed several response handlers that don't need to do anything anymore - Removed several tests that were basically duplicated: - The `finishable` checks are already tested as part of `ErrorResponse`. - All the checks for the error details are also part of `ErrorResponse`. - No longer need to have a test for each type of failed response since that's part of `HTTPClient` tests now.

NachoSoto requested review from a team, aboedo and taquitos March 30, 2022 00:15

NachoSoto commented Mar 30, 2022

View reviewed changes

NachoSoto mentioned this pull request Mar 30, 2022

Created ErrorResponse to abstract error deserialization #1427

Merged

NachoSoto force-pushed the error-response branch from f78f584 to f1efdc8 Compare March 30, 2022 16:29

NachoSoto force-pushed the error-response-using-it branch 2 times, most recently from 0f4ef59 to c4830a4 Compare March 30, 2022 19:06

NachoSoto mentioned this pull request Mar 30, 2022

Moved response error and status checking to HTTPClient #1431

Merged

NachoSoto force-pushed the error-response-using-it branch from c4830a4 to 0f99cc1 Compare March 30, 2022 20:29

NachoSoto mentioned this pull request Mar 30, 2022

Simplified JSONDecoder.decode decoding strategies #1432

Merged

NachoSoto force-pushed the error-response branch from 8f5e21d to 3ce751a Compare March 30, 2022 21:36

NachoSoto force-pushed the error-response-using-it branch from 0f99cc1 to 09b1496 Compare March 30, 2022 21:37

taquitos reviewed Mar 31, 2022

View reviewed changes

taquitos approved these changes Mar 31, 2022

View reviewed changes

NachoSoto force-pushed the error-response branch from 3ce751a to 8c7435c Compare March 31, 2022 23:57

NachoSoto force-pushed the error-response-using-it branch from 09b1496 to 58eac08 Compare April 1, 2022 00:04

NachoSoto mentioned this pull request Apr 1, 2022

Created SubscriberAttributesManagerIntegrationTests #1441

Merged

NachoSoto force-pushed the error-response branch from 8c7435c to 37d47e9 Compare April 1, 2022 20:51

NachoSoto force-pushed the error-response-using-it branch from 58eac08 to 9b707ec Compare April 1, 2022 20:51

NachoSoto force-pushed the error-response branch from 37d47e9 to f3946de Compare April 1, 2022 21:23

Base automatically changed from error-response to main April 1, 2022 21:24

NachoSoto force-pushed the error-response-using-it branch from 9b707ec to 3ba81e4 Compare April 1, 2022 21:25

NachoSoto merged commit f084b25 into main Apr 1, 2022

NachoSoto deleted the error-response-using-it branch April 1, 2022 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaced manual error response deserialization with `ErrorResponse`#1429

Replaced manual error response deserialization with `ErrorResponse`#1429
NachoSoto merged 1 commit into
mainfrom
error-response-using-it

NachoSoto commented Mar 30, 2022 •

edited

Loading

Uh oh!

NachoSoto Mar 30, 2022

Uh oh!

NachoSoto Mar 30, 2022

Uh oh!

NachoSoto Mar 30, 2022

Uh oh!

taquitos Mar 31, 2022

Uh oh!

taquitos left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

NachoSoto commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Other changes:

Uh oh!

NachoSoto Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

NachoSoto Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

NachoSoto Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

taquitos Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

taquitos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NachoSoto commented Mar 30, 2022 •

edited

Loading