`HTTPClient.perform`: changed response to `Result<[String: Any], Error>` by NachoSoto · Pull Request #1387 · RevenueCat/purchases-ios

NachoSoto · 2022-03-11T21:27:59Z

Currently pretty much all closures in the codebase use (Value?, Error?), which is a leftover from Objective-C days. These can represent 4 different cases:

Value / no-error
No-value / error
Value / error
No-value / no-error

Out of those 4, only 2 are really valid, which meant that a lot of code was spent handling the other 2, either creating errors (like UnexpectedBackendResponseSubErrorCode.loginMissingResponse) that weren't possible, or hard to follow code because both a value and an error were possible.

In tests, too, the mocks were initially set up to produce no value and no error, which were in many cases triggering code paths that weren't legal with real code.

This change removes this possibility all together by ensuring at compile time, that one of the two is always present.

This is step 1 in a series of refactors.

Step 2 will move a lot of duplicated code that handles status codes != 20x, error handling, parsing error responses, etc. up to HTTPClient and out of the operations, making the operations a lot simpler.
Step 3 will change HTTPClient to return a Response: Decodable instead of [String: Any], moving more of the response serialization logic to HTTPClient, and leaving the operations to only focus on the "happy path".

NachoSoto · 2022-03-11T22:44:27Z

I'm seeing these failures locally too, but only sometimes 🤔 not when I run each test individually. It's failing to find the active UIWindowScene. I debugged this, and the scene is in "background" mode.

I have an idea for how to improve this for tests by I don't understand what in this PR would make these fail all of a sudden?

NachoSoto · 2022-03-11T23:55:19Z

Tests fixed in #1390

NachoSoto · 2022-03-11T23:56:50Z

There's so much logic here that's going to go away and move to HTTPClient.

NachoSoto · 2022-03-11T23:57:08Z

This line is basically repeated in all operations, so it'll be moved to HTTPClient too.

NachoSoto · 2022-03-11T23:58:12Z

Obviously the public APIs still have the Obj-C style API. This is how the Result is converted back.

I'd love to add Result alternatives down the line for Swift, although tbh I imagine a lot of folks will just migrate to async / await

Yeah exactly, but we could easily offer them too

NachoSoto · 2022-03-12T00:00:03Z

One example of several where tests were using mocks that returned no value and no error by default, which many tests didn't override.

NachoSoto · 2022-03-12T00:01:48Z

These are no longer using the test-only .init(testData:). The actual method in the code .init(data:) already has a throws initializer, so don't need to use XCTUnwrap. I think using this we can probably get rid of init(testData:).

NachoSoto · 2022-03-12T00:02:01Z

This was testing something that couldn't happen.

NachoSoto · 2022-03-12T00:02:41Z

Same here, why have an Optional around when we can make sure at initialization that the data is correct. Th (Value?, Error?) APIs enabled optionals all over the place that shouldn't have been.

NachoSoto · 2022-03-12T00:04:09Z

Same here, this could have been setting nil as the mock without knowing 🤷🏻‍♂️

NachoSoto · 2022-03-12T00:04:25Z

Again testing something that wasn't possible.

aboedo

awesome!!! I love this refactor. I'd love to see pretty much everything in our codebase that requires completion blocks use Result, and this gets us like 90% of the way there 👏

aboedo · 2022-03-14T20:00:55Z

aboedo · 2022-03-14T20:01:19Z

I've been dreaming of this refactor since we first started migrating code to swift

aboedo · 2022-03-14T20:04:05Z

tbh I still feel like the needsToRefresh variable made this slightly more readable, i.e.: it told you why you're going on this path - you need to refresh.

maybe we could have this be guard !needsToRefresh else {

The reason I changed it is so that I could do let customerInfo = instead of cachedCustomerInfo != nil and then force-unwrapping :/

👍 that makes sense

aboedo · 2022-03-15T17:09:35Z

aboedo · 2022-03-15T17:59:31Z

this is such a nice win - now if the completion was called, there's no way that receivedResult could be nil, so need for an extra var

aboedo · 2022-03-15T18:10:48Z

Currently pretty much all closures in the codebase use `(Value?, Error?)`, which is a leftover from Objective-C days. These can represent 4 different cases: - **Value / no-error** - **No-value / error** - _Value / error_ - _No-value / no-error_ Out of those 4, only 2 are really valid, which meant that a lot of code was spent handling the other 2, either creating errors (like `UnexpectedBackendResponseSubErrorCode.loginMissingResponse`) that weren't possible, or hard to follow code because both a value and an error were possible. In tests, too, the mocks were initially set up to produce no value and no error, which were in many cases triggering code paths that weren't legal with real code. This change removes this possibility all together by ensuring **at compile time**, that one of the two is always present.

These have began failing on #1387, even though that doesn't change anything that should affect the `UIWindowScene` or anything related. This could have been a regression from Xcode 13.3. When the test fails (which isn't always), the scene isn't yet `.foregroundActive` . To workaround that, this defaults to any scene, *though only when running tests, on debug, and on the simulator*. So nothing is changed in release or device builds. This also has a small refactor on `BeginRefundRequestHelperTests` to move the test data below and make the tests more readable. Additionally, the `MockSK2BeginRefundRequestHelper` is recreated for every test. I thought initially that this is what was making the test fail, as the mock expectations and state was being shared across tests. It's not, but now that should remove any potential issues with shared state.

… if the response was successful Fixes [SDKONCALL-62]. The behavior in version 3.x was that we would return a `CustomerInfo` AND the attribute errors. [The test in that version](https://github.com/RevenueCat/purchases-ios/blob/3.14.2/PurchasesTests/SubscriberAttributes/BackendSubscriberAttributesTests.swift#L370-L404) however only verified that the error was returned. When we converted `(Value, Error)` callbacks to using `Result` (#1387), I assumed this was an error. Because of that incomplete test, we didn't notice this wasn't the desired behavior. If the backend returns a 200, it's assumed that the errors aren't more important than the correct `CustomerInfo`. The new implementation simply logs the attribute errors (which is now tested), and returns the `CustomerInfo`.

… if the response was successful (#1778) Fixes [SDKONCALL-62]. The behavior in version 3.x was that we would return a `CustomerInfo` AND the attribute errors. [The test in that version](https://github.com/RevenueCat/purchases-ios/blob/3.14.2/PurchasesTests/SubscriberAttributes/BackendSubscriberAttributesTests.swift#L370-L404) however only verified that the error was returned. When we converted `(Value, Error)` callbacks to using `Result` (#1387), I assumed this was an error. Because of that incomplete test, we didn't notice this wasn't the desired behavior. If the backend returns a 200, it's assumed that the errors aren't more important than the correct `CustomerInfo`. The new implementation simply logs the attribute errors (which is now tested), and returns the `CustomerInfo`: ``` 😿‼️ One or more of the attributes sent could not be saved. ["$email": "email is not in valid format"] ``` [SDKONCALL-62]: https://revenuecats.atlassian.net/browse/SDKONCALL-62?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

NachoSoto requested a review from a team March 11, 2022 21:27

NachoSoto mentioned this pull request Mar 11, 2022

[WIP] HTTPClient returns Result<[String: Any], Error>, using Result everywhere #1372

Closed

NachoSoto force-pushed the http-client-result branch from bf3c5c3 to 7b3b9f1 Compare March 11, 2022 23:50

NachoSoto mentioned this pull request Mar 11, 2022

Fixed sporadic BeginRefundRequestHelperTests failures #1390

Merged

NachoSoto commented Mar 11, 2022

View reviewed changes

NachoSoto commented Mar 12, 2022

View reviewed changes

aboedo reviewed Mar 15, 2022

View reviewed changes

NachoSoto force-pushed the http-client-result branch from 7b3b9f1 to af3f089 Compare March 15, 2022 19:33

NachoSoto force-pushed the http-client-result branch from af3f089 to b812108 Compare March 15, 2022 19:56

Addressed comments

d4250ef

NachoSoto requested a review from aboedo March 15, 2022 20:01

aboedo approved these changes Mar 15, 2022

View reviewed changes

NachoSoto merged commit dc8edc4 into main Mar 15, 2022

NachoSoto deleted the http-client-result branch March 15, 2022 20:17

NachoSoto mentioned this pull request Jul 27, 2022

CustomerInfoResponseHandler: return CustomerInfo instead of error if the response was successful #1778

Merged

Uh oh!

Conversation

NachoSoto commented Mar 11, 2022

Uh oh!

NachoSoto commented Mar 11, 2022

Uh oh!

NachoSoto commented Mar 11, 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!

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

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!

aboedo left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NachoSoto Mar 12, 2022 •

edited

Loading