-
Notifications
You must be signed in to change notification settings - Fork 316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Moved response error and status checking to HTTPClient
#1431
Conversation
.flatMap { | ||
$0.map(Result.success) | ||
?? .failure(ErrorUtils.unexpectedBackendResponseError()) | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically what asOptionalResult
abstracts now.
.mapError { ErrorUtils.networkError(withUnderlyingError: $0) } | ||
.flatMap { response in | ||
guard response.statusCode.isSuccessfulResponse else { | ||
return .failure( | ||
ErrorResponse | ||
.from(response.jsonObject) | ||
.asBackendError(with: statusCode) | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines were in every single operation.
|
||
import Foundation | ||
|
||
class SubscriberAttributeHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋🏻
|
||
import Foundation | ||
|
||
class NoContentResponseHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋🏻
c2a8b59
to
86381bb
Compare
c4830a4
to
0f99cc1
Compare
0f99cc1
to
09b1496
Compare
86381bb
to
4d1b356
Compare
extension Result where Success: OptionalType { | ||
|
||
/// Converts a `Result<Success?, Error>` into `Result<Success, Error>?` | ||
var asOptionalResult: Result<Success.Wrapped, Failure>? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always a great point that I keep forgetting. Added tests!
@@ -162,31 +162,29 @@ private extension HTTPClient { | |||
self.start(request: request) | |||
} | |||
|
|||
// - Returns: `nil` if the request must be retried |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cleaner 🧹
.map { HTTPResponse(statusCode: statusCode, jsonObject: $0) } | ||
.map { | ||
return self.eTagManager.httpResultFromCacheOrBackend(with: httpURLResponse, | ||
jsonObject: $0.jsonObject, | ||
request: urlRequest, | ||
retried: request.retried) | ||
} | ||
.asOptionalResult? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so functional, wow.
@@ -16,15 +16,12 @@ import Foundation | |||
class CreateAliasOperation: CacheableNetworkOperation { | |||
|
|||
private let aliasCallbackCache: CallbackCache<AliasCallback> | |||
private let createAliasResponseHandler: NoContentResponseHandler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😻
return Result { try CustomerInfo.from(json: response) } | ||
let errorResponse = ErrorResponse.from(response.value?.jsonObject ?? [:]) | ||
|
||
let result: Result<CustomerInfo, Error> = response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, that's a good comment 😄 Off the top of my head, I don't know a better way of structuring these things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on the functional code being tougher to understand. We can continue discussion on the other PR if this is temporary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is gone later since all the parsing will be done by HTTPClient
, but welcoming feedback on the other one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
09b1496
to
58eac08
Compare
4d1b356
to
c2842b7
Compare
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`.
58eac08
to
9b707ec
Compare
c2842b7
to
9c80081
Compare
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`.
9b707ec
to
3ba81e4
Compare
9c80081
to
31e5bd1
Compare
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.
31e5bd1
to
1e538b3
Compare
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`.
This is the next PR that needs reviewing 🙏🏻 |
love seeing so much code go away 🪄 |
} | ||
|
||
func testWithError() { | ||
expect(Data.failure(.error1).asOptionalResult) == OptionalData.some(.failure(.error1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing with explicit types as well.
I updated and removed some tests in #1431 but forgot to update the snapshots for iOS 12.x
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`.
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`.
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
Follow up to #1429. For #695.
This was duplicated in all operations.
HTTPClient
now only forwardsResult.success
if the request actually succeeded,leaving each
NetworkOperation
to just parse the body (which will become part ofHTTPClient
too).Other changes:
HTTPClient
by using a newResult.asOptionalResult
finishable
checks are already tested as part ofErrorResponse
.ErrorResponse
.HTTPClient
tests now.