Converted CustomerInfo and related types to use Codable#1496
Conversation
3ac5e2f to
a845599
Compare
7456ec6 to
62eaa0e
Compare
a845599 to
161c9a0
Compare
62eaa0e to
88eb5be
Compare
161c9a0 to
30cdc48
Compare
…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
88eb5be to
c4c90a4
Compare
30cdc48 to
c5984a9
Compare
c4c90a4 to
97fb1d0
Compare
c5984a9 to
8d330ad
Compare
97fb1d0 to
80e56be
Compare
8d330ad to
09160d6
Compare
1af39a9 to
7aafc8f
Compare
09160d6 to
d3b33fd
Compare
df1c24d to
e1a9cfc
Compare
e1a9cfc to
df686d2
Compare
b2b5d55 to
291717b
Compare
66c0454 to
3a50d92
Compare
CustomerInfo deserialization with DecodableCustomerInfo and related types to use Codable
The new test illustrates the behavior change that adding `@LossyDictionary` to a `[String: Decodable]` property had. Keys were being converted to camel case, which is inconsistent from the default behavior that `container.decode([String: Value].self, forKey: key)` would have. The reason for this is some implementation detail on Foundation that ignores the `keyDecodingStrategy` when decoding "raw" dictionaries versus property names. To deal with this, we decode the strings first as "raw", and then match them to the corresponding converted key. This was necessary to make sure that keys [like these](https://github.com/RevenueCat/purchases-ios/blob/4.3.0/Tests/UnitTests/Purchasing/CustomerInfoTests.swift#L58) aren't incorrectly converted to camel case, which is required for #1496.
c79c596 to
b351833
Compare
2f34205 to
89e1b33
Compare
NachoSoto
left a comment
There was a problem hiding this comment.
This is ready for review now!
There was a problem hiding this comment.
This custom deserialization is no longer needed
| * it's only meant for debugging purposes or for getting access to future data | ||
| * without updating the SDK. | ||
| */ | ||
| @objc public let rawData: [String: Any] |
There was a problem hiding this comment.
This is now provided automatically through RawDataContainer.
| return false | ||
| } | ||
|
|
||
| return NSDictionary(dictionary: self.jsonObjectWithNoDate) |
There was a problem hiding this comment.
Much simpler comparison! CustomerInfoResponse Equatable implementation ignores the date.
| } | ||
|
|
||
| static let currentSchemaVersion = "2" | ||
| fileprivate init(data: Data) { |
There was a problem hiding this comment.
This method now stores each of the properties from the already decoded Data.
| let latestNonSubscriptionTransactionsByProductId = [String: [String: Any]]( | ||
| uniqueKeysWithValues: nonSubscriptionsByProductId.map { productId, transactionsArray in | ||
| (productId, transactionsArray.last ?? [:]) | ||
| }) | ||
|
|
||
| self.allTransactionsByProductId = latestNonSubscriptionTransactionsByProductId | ||
| .merging(subscriptionTransactionsByProductId, strategy: .keepOriginalValue) | ||
|
|
||
| self.allPurchases = latestNonSubscriptionTransactionsByProductId | ||
| .merging(subscriptionTransactionsByProductId) { (current, _) in current } |
There was a problem hiding this comment.
This was super hacky and hard to follow. It was combining the dictionaries inside of "subscriptions" and "non_subscriptions", despite them having different formats. This is now made explicit through type conversions between the two.
There was a problem hiding this comment.
OMG, the new version is so ❤️ I was in here debugging something a while back and it was hard to follow. Great work!
|
|
||
| extension CustomerInfoResponse.Subscriber { | ||
|
|
||
| var allTransactionsByProductId: [String: CustomerInfoResponse.Transaction] { |
There was a problem hiding this comment.
CustomerInfo was combining the dictionaries inside of "subscriptions" and "non_subscriptions", despite them having different formats. This is now made explicit through type conversions between the two.
89e1b33 to
c37cbda
Compare
joshdholtz
left a comment
There was a problem hiding this comment.
Leaving some early comments on my review while I continue reviewing 😊
| let latestNonSubscriptionTransactionsByProductId = [String: [String: Any]]( | ||
| uniqueKeysWithValues: nonSubscriptionsByProductId.map { productId, transactionsArray in | ||
| (productId, transactionsArray.last ?? [:]) | ||
| }) | ||
|
|
||
| self.allTransactionsByProductId = latestNonSubscriptionTransactionsByProductId | ||
| .merging(subscriptionTransactionsByProductId, strategy: .keepOriginalValue) | ||
|
|
||
| self.allPurchases = latestNonSubscriptionTransactionsByProductId | ||
| .merging(subscriptionTransactionsByProductId) { (current, _) in current } |
There was a problem hiding this comment.
OMG, the new version is so ❤️ I was in here debugging something a while back and it was hard to follow. Great work!
There was a problem hiding this comment.
Will this rawData not contain any data that was stripped away by the new lossy array and dictionary types? So like.. this won't show the raw response that this customer info was created from, right? It instead is just a dictionary representation of this object?
I'm not sure if that is a change from what this previously did but just wanted to understand and call that out just in case 😇
There was a problem hiding this comment.
Oh shoot that's a great catch! Let me look into how we could support storing the original data.
There was a problem hiding this comment.
Forgot to update this here, this is fixed by #1565.
There was a problem hiding this comment.
This whole file is soooooo 💯
Inspired by https://github.com/marksands/BetterCodable/blob/master/Sources/BetterCodable but supporting nested collections and with simplified code. These property wrappers allows decoding `Array`s and `Dictionary`s and ignore elements that fail to decode. For example, this will ignore any elements that aren't numbers, instead of failing to decode altogether. ```swift struct Data: Decodable { @LossyArray var list: [Int] @LossyDictionary var map: [String: Int] } ``` This does require that the values are the right type, but these wrappers can be composed with `@DefaultDecodable.EmptyArray` and `@DefaultDecodable.EmptyDictionary` introduced in #1537 to make it produce an empty array in case of any other type error: ```swift struct Data: Decodable { @DefaultDecodable.EmptyArray @LossyArray var list: [Int] @DefaultDecodable.EmptyDictionary @LossyDictionary var map: [String: Int] } ``` Because of limitations of the property wrappers in Swift, an extra type `LossyArrayDictionary` allows lossy decoding of types `[String: [Decodable]`. This will be used to vastly simplify #1496.
aboedo
left a comment
There was a problem hiding this comment.
This looks amazing!! It's great to see the CustomerInfo decoding code go away, that was a pain to read.
I left a few comments
| let entitlements: [String: EntitlementInfo] = Dictionary( | ||
| uniqueKeysWithValues: entitlements.compactMap { identifier, entitlement in | ||
| guard let subscription = purchases[entitlement.productIdentifier] else { | ||
| return nil | ||
| } | ||
|
|
||
| return ( | ||
| identifier, | ||
| EntitlementInfo(identifier: identifier, | ||
| entitlement: entitlement, | ||
| subscription: subscription, | ||
| requestDate: requestDate) | ||
| ) | ||
| } |
There was a problem hiding this comment.
this may just be me, but I found the previous implementation quite a bit easier to read, since it's flat and goes one step at a time
There was a problem hiding this comment.
Yeah I guess it's personal preference, I'm happy to change the data manipulation method back.
What do you mean by "flat" though?
There was a problem hiding this comment.
well, not really flat, it's just that the new implementation does the filtering and matching between entitlementInfos and purchases as an operation within the constructor of the entitlementsByKey dictionary, which seems like a lot to do at once.
I tend to find it easier to keep track of things when they're done one at a time in a top-to-bottom way. It also took me a bit to realize that the entitlements in the signature and the one in declared in the method are different objects
There was a problem hiding this comment.
well, not really flat, it's just that the new implementation does the filtering and matching between entitlementInfos and purchases as an operation within the constructor of the entitlementsByKey dictionary, which seems like a lot to do at once.
Yeah, one is imperative and this is functional. It's kind of a philosophical argument more than anything :P
It also took me a bit to realize that the entitlements in the signature and the one in declared in the method are different objects
Good point, I'll get rid of the unnecessary variable.
|
|
||
| } | ||
|
|
||
| /// Default implementation that encodes the `underlyingData` for public use. |
There was a problem hiding this comment.
| /// Default implementation that encodes the `underlyingData` for public use. | |
| /// Default implementation that encodes the ``RawDataContainer/underlyingData`` for public use. |
- Removed all custom deserialization code thanks to #1537, #1541, and #1543. - The format of the customer info response is now very concise and only in one place (`CustomerInfoResponse`) instead of being passed through dictionaries and magic strings everywhere. - `CustomerInfo` was combining the dictionaries inside of `"subscriptions"` and `"non_subscriptions"`, despite them having different formats. This is now made explicit through type conversions between the two, (see `CustomerInfoResponse.allTransactionsByProductId`). - Removed all custom `CustomerInfo` errors since deserialization is automatic, and the underlying `Error` information will be provided thanks to `ErrorUtils.logDecodingError`. - `CustomerInfo` deserialization always wraps errors into `ErrorCode.customerInfoError`, which is now also covered in a new test. - Added a new snapshot test that covers `CustomerInfo` serialization by storing the result in a JSON file. - `CustomerInfo` equality is now automatic through the `CustomerInfoResponse` `Equatable` conformance. - Simplified `schemaVersion` handling - All tests still use `CustomerInfo(data: [String: Any])`, but that method is only visible for tests now, and it uses the underlying `Decodable` implementation (see `CustomerInfo+TestExtensions.swift`). - Added extra tests for `CustomerInfoResponse` that use a fixture `CustomerInfo.json`, which is easier to maintain than having JSON in code. - Improved definition of the protocol so that our types only have to implement one method: ```swift extension CustomerInfo: RawDataContainer { @available(iOS 13.0, macOS 10.15, tvOS 13.0, watchOS 6.2, *) public var underlyingData: some Encodable { return self.data.response } } ``` - The protocol will provide the `rawData` public method through the default implementation automatically. - The only downside is that this needs to be iOS 13+ only because of `some Encodable`, which allows us to implement this without needing to leak the underlying type.
Depends on:
DefaultValueandDefaultDecodable#1537EntitlementInfodecoding using new@DefaultValueproperty wrappers #1540@IgnoreDecodeErrorsproperty wrapper #1541@LossyArrayand@LossyDictionary#1543@LossyDictionary: workaround for decoding snake case dictionary keys #1546EntitlementInfosTests#1547RawDataContainerconformances to APITesters #1550PeriodType/PurchaseOwnershipType/Store: conform toEncodable#1551,RawDataContainerincludes all data #1565Changes
DefaultValueandDefaultDecodable#1537, Added@IgnoreDecodeErrorsproperty wrapper #1541, and Created@LossyArrayand@LossyDictionary#1543.CustomerInfoResponse) instead of being passed through dictionaries and magic strings everywhere.CustomerInfowas combining the dictionaries inside of"subscriptions"and"non_subscriptions", despite them having different formats. This is now made explicit through type conversions between the two, (seeCustomerInfoResponse.allTransactionsByProductId).CustomerInfoerrors since deserialization is automatic, and the underlyingErrorinformation will be provided thanks toErrorUtils.logDecodingError.CustomerInfodeserialization always wraps errors intoErrorCode.customerInfoError, which is now also covered in a new test.CustomerInfoserialization by storing the result in a JSON file.Other changes:
CustomerInfoequality is now automatic through theCustomerInfoResponseEquatableconformance.schemaVersionhandlingCustomerInfo(data: [String: Any]), but that method is only visible for tests now, and it uses the underlyingDecodableimplementation (seeCustomerInfo+TestExtensions.swift).CustomerInfoResponsethat use a fixtureCustomerInfo.json, which is easier to maintain than having JSON in code.RawDataContainerimplementation is changed in Added tests and fix to ensureRawDataContainerincludes all data #1565.TODO:
RawValueContainerCustomerInfoErrorCustomerInfoResponseTestsRawDataContaineractually store all the original data: Added tests and fix to ensureRawDataContainerincludes all data #1565