Moved response error and status checking to `HTTPClient` by NachoSoto · Pull Request #1431 · RevenueCat/purchases-ios

NachoSoto · 2022-03-30T19:23:56Z

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 · 2022-03-30T19:39:33Z

This is basically what asOptionalResult abstracts now.

NachoSoto · 2022-03-30T19:39:51Z

These lines were in every single operation.

NachoSoto · 2022-03-30T19:40:45Z

NachoSoto · 2022-03-30T19:40:48Z

NachoSoto · 2022-03-30T22:37:59Z

+extension Result where Success: OptionalType {
+
+    /// Converts a `Result<Success?, Error>` into `Result<Success, Error>?`
+    var asOptionalResult: Result<Success.Wrapped, Failure>? {


I didn't write tests for this because the compiler is doing all the work checking the types. It's a simple type conversion with no added logic.

I understand the point about the current code being correct, but in practice, writing tests to check that the current code is correct is usually not that valuable in my experience.
The real value in writing tests is future-proofing, so that if in the future another dev wants to modify or refactor this code, they can do so while resting assured that as long as tests pass, the code is still working correctly.

Say that someone was debugging something, and in order to quickly test out what happens in the empty success case, they comment or remove the code in this implementation and replace it with a simple

var asOptionalResult: Result<Success.Wrapped, Failure>? { return nil }

Then they forget about it and never roll it back. Hopefully it would get caught in a PR, but if tests are passing, it might not be caught if it doesn't draw attention.

If there are tests for this, it'll be easy to catch. Even if it doesn't make it to a full PR because other code is actually using the value and so the tests for that code fail, the tests will no longer be specific, and it might be hard to debug.

So I'd always argue for making tests, even if the current code is guaranteed to be correct, in order to future-proof the code and ensure that other devs won't be able to accidentally break it.

Always a great point that I keep forgetting. Added tests!

taquitos · 2022-03-31T21:53:23Z

        self.start(request: request)
    }

+    // - Returns: `nil` if the request must be retried


This is cleaner 🧹

taquitos · 2022-03-31T21:53:57Z

                                                                     request: urlRequest,
                                                                     retried: request.retried)
            }
+            .asOptionalResult?


so functional, wow.

taquitos · 2022-03-31T21:54:56Z

 class CreateAliasOperation: CacheableNetworkOperation {

    private let aliasCallbackCache: CallbackCache<AliasCallback>
-    private let createAliasResponseHandler: NoContentResponseHandler


taquitos · 2022-03-31T22:09:06Z

-                    return Result { try CustomerInfo.from(json: response) }
+        let errorResponse = ErrorResponse.from(response.value?.jsonObject ?? [:])
+
+        let result: Result<CustomerInfo, Error> = response


Visually, this is more confusing to me. Is there a way to structure this so it's easier to parse? Maybe I'm the only one, but that was something I struggled with learning RxSwift/ReactiveCocoa. The many .flatMap after .flatMap with more nested function calls like mapError it's difficult for me to follow the flow of logic/transformations.

Yeah I brought up a similar comment here.
For this particular example this code is going away in that PR since the deserialization will be done by HTTPClient, so this is just temporary.

But the point is still valid, especially in that other PR.

Oh yeah, that's a good comment 😄 Off the top of my head, I don't know a better way of structuring these things.

+1 on the functional code being tougher to understand. We can continue discussion on the other PR if this is temporary

Yeah this is gone later since all the parsing will be done by HTTPClient, but welcoming feedback on the other one.

taquitos

This is a great improvement. I want to keep comments open for other folks to check it out. If people don't feel the same way I do about some of the confusing functional structure, I'm happy to approve 😄

Closes #695. Depends on #1431, #1432, #1433. ## 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`.

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.

Closes #695. Depends on #1431, #1432, #1433. ## 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`.

NachoSoto · 2022-04-04T18:29:56Z

This is the next PR that needs reviewing 🙏🏻

aboedo · 2022-04-05T13:49:03Z

love seeing so much code go away 🪄

NachoSoto · 2022-04-05T16:00:08Z

+    }
+
+    func testWithError() {
+        expect(Data.failure(.error1).asOptionalResult) == OptionalData.some(.failure(.error1))


Testing with explicit types as well.

I updated and removed some tests in #1431 but forgot to update the snapshots for iOS 12.x

Closes #695. Depends on #1431, #1432, #1433. ## 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`.

…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 requested a review from a team March 30, 2022 19:23

NachoSoto mentioned this pull request Mar 30, 2022

HTTPClient now handles response deserialization + Introduced NetworkError and BackendError #1425

Merged

10 tasks

NachoSoto commented Mar 30, 2022

View reviewed changes

NachoSoto force-pushed the error-response-in-http-client branch from c2a8b59 to 86381bb Compare March 30, 2022 19:52

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 HTTPClient.Completion and refactored response handling implementation #1426

Merged

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

NachoSoto force-pushed the error-response-in-http-client branch from 86381bb to 4d1b356 Compare March 30, 2022 21:37

NachoSoto commented Mar 30, 2022

View reviewed changes

taquitos reviewed Mar 31, 2022

View reviewed changes

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

NachoSoto force-pushed the error-response-in-http-client branch from 4d1b356 to c2842b7 Compare April 1, 2022 00:29

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-in-http-client branch from c2842b7 to 9c80081 Compare April 1, 2022 20:52

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

Base automatically changed from error-response-using-it to main April 1, 2022 21:26

NachoSoto requested a review from a team April 1, 2022 21:26

NachoSoto force-pushed the error-response-in-http-client branch from 9c80081 to 31e5bd1 Compare April 1, 2022 21:27

NachoSoto force-pushed the error-response-in-http-client branch from 31e5bd1 to 1e538b3 Compare April 1, 2022 21:30

Added tests for Result.asOptionalResult

f407563

NachoSoto commented Apr 5, 2022

View reviewed changes

NachoSoto requested a review from aboedo April 5, 2022 16:00

aboedo approved these changes Apr 5, 2022

View reviewed changes

NachoSoto merged commit 3f1a527 into main Apr 5, 2022

NachoSoto deleted the error-response-in-http-client branch April 5, 2022 18:39

NachoSoto added a commit that referenced this pull request Apr 5, 2022

Updated snapshots for iOS 12.x

36670d7

I updated and removed some tests in #1431 but forgot to update the snapshots for iOS 12.x

NachoSoto mentioned this pull request Apr 5, 2022

Updated snapshots for iOS 12.x #1453

Merged

NachoSoto added a commit that referenced this pull request Apr 5, 2022

Updated snapshots for iOS 12.x (#1453)

8fc7f8c

I updated and removed some tests in #1431 but forgot to update the snapshots for iOS 12.x

Conversation

NachoSoto commented Mar 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taquitos left a comment

Choose a reason for hiding this comment

Uh oh!

NachoSoto commented Apr 4, 2022

Uh oh!

aboedo commented Apr 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants