Skip to content

Converted CustomerInfo and related types to use Codable#1496

Merged
NachoSoto merged 6 commits into
mainfrom
customer-info-decodable
May 24, 2022
Merged

Converted CustomerInfo and related types to use Codable#1496
NachoSoto merged 6 commits into
mainfrom
customer-info-decodable

Conversation

@NachoSoto

@NachoSoto NachoSoto commented Apr 12, 2022

Copy link
Copy Markdown
Contributor

Depends on:

Changes

  • Removed all custom deserialization code thanks to Created DefaultValue and DefaultDecodable #1537, Added @IgnoreDecodeErrors property wrapper #1541, and Created @LossyArray and @LossyDictionary #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.

Other changes:

  • 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.
  • RawDataContainer implementation is changed in Added tests and fix to ensure RawDataContainer includes all data #1565.

TODO:

@NachoSoto NachoSoto force-pushed the customer-info-decodable branch 2 times, most recently from 3ac5e2f to a845599 Compare April 12, 2022 18:41
@NachoSoto NachoSoto force-pushed the offerings-response branch from 7456ec6 to 62eaa0e Compare April 12, 2022 19:46
@NachoSoto NachoSoto force-pushed the customer-info-decodable branch from a845599 to 161c9a0 Compare April 12, 2022 19:48
@NachoSoto NachoSoto force-pushed the offerings-response branch from 62eaa0e to 88eb5be Compare April 12, 2022 21:06
@NachoSoto NachoSoto force-pushed the customer-info-decodable branch from 161c9a0 to 30cdc48 Compare April 12, 2022 21:07
NachoSoto added a commit that referenced this pull request Apr 12, 2022
…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 NachoSoto force-pushed the offerings-response branch from 88eb5be to c4c90a4 Compare April 12, 2022 22:12
@NachoSoto NachoSoto force-pushed the customer-info-decodable branch from 30cdc48 to c5984a9 Compare April 12, 2022 22:17
@NachoSoto NachoSoto force-pushed the offerings-response branch from c4c90a4 to 97fb1d0 Compare April 13, 2022 21:18
@NachoSoto NachoSoto force-pushed the customer-info-decodable branch from c5984a9 to 8d330ad Compare April 13, 2022 21:19
@NachoSoto NachoSoto force-pushed the offerings-response branch from 97fb1d0 to 80e56be Compare April 14, 2022 12:37
@NachoSoto NachoSoto force-pushed the customer-info-decodable branch from 8d330ad to 09160d6 Compare April 14, 2022 12:47
@NachoSoto NachoSoto force-pushed the offerings-response branch 2 times, most recently from 1af39a9 to 7aafc8f Compare April 21, 2022 17:07
@NachoSoto NachoSoto force-pushed the customer-info-decodable branch from 09160d6 to d3b33fd Compare April 21, 2022 17:07
Base automatically changed from offerings-response to main April 22, 2022 18:19
@NachoSoto NachoSoto force-pushed the customer-info-decodable branch 7 times, most recently from df1c24d to e1a9cfc Compare April 26, 2022 16:53
@NachoSoto NachoSoto force-pushed the customer-info-decodable branch from e1a9cfc to df686d2 Compare April 26, 2022 17:40
@NachoSoto NachoSoto force-pushed the customer-info-decodable branch 2 times, most recently from b2b5d55 to 291717b Compare April 28, 2022 20:55
@NachoSoto NachoSoto requested a review from a team April 28, 2022 21:05
@NachoSoto NachoSoto force-pushed the customer-info-decodable branch 2 times, most recently from 66c0454 to 3a50d92 Compare April 28, 2022 21:22
@NachoSoto NachoSoto removed the WIP label Apr 28, 2022
@NachoSoto NachoSoto changed the title [WIP] Replaced CustomerInfo deserialization with Decodable Converted CustomerInfo and related types to use Codable Apr 28, 2022
@NachoSoto NachoSoto marked this pull request as ready for review April 28, 2022 21:23
NachoSoto added a commit that referenced this pull request Apr 28, 2022
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.
@NachoSoto NachoSoto force-pushed the ignore-decode-errors-collections-snake-case branch from c79c596 to b351833 Compare April 28, 2022 21:26
@NachoSoto NachoSoto force-pushed the customer-info-decodable branch 2 times, most recently from 2f34205 to 89e1b33 Compare April 28, 2022 21:32

@NachoSoto NachoSoto left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ready for review now!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This custom deserialization is no longer needed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was removed in #1540.

* it's only meant for debugging purposes or for getting access to future data
* without updating the SDK.
*/
@objc public let rawData: [String: Any]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now provided automatically through RawDataContainer.

return false
}

return NSDictionary(dictionary: self.jsonObjectWithNoDate)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much simpler comparison! CustomerInfoResponse Equatable implementation ignores the date.

Comment thread Sources/Identity/CustomerInfo.swift Outdated
}

static let currentSchemaVersion = "2"
fileprivate init(data: Data) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method now stores each of the properties from the already decoded Data.

Comment on lines -329 to -338
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 }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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] {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@NachoSoto NachoSoto force-pushed the customer-info-decodable branch from 89e1b33 to c37cbda Compare April 29, 2022 19:54

@joshdholtz joshdholtz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving some early comments on my review while I continue reviewing 😊

Comment on lines -329 to -338
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 }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG, the new version is so ❤️ I was in here debugging something a while back and it was hard to follow. Great work!

Comment thread Sources/Identity/CustomerInfoManager.swift Outdated
Comment thread Sources/Misc/RawDataContainer.swift Outdated
Comment on lines 31 to 41

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😇

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh shoot that's a great catch! Let me look into how we could support storing the original data.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to update this here, this is fixed by #1565.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole file is soooooo 💯

Comment thread Sources/Networking/Responses/CustomerInfoResponse.swift Outdated
NachoSoto added a commit that referenced this pull request May 10, 2022
…rty wrappers (#1540)

Follow up to #1537.
`EntitlementInfo.ProductData` and related types will be moved out as part of #1496, to become part of the `CustomerInfo` response. But this is an intermediate change to simplify the decoding of this part of the data.
NachoSoto added a commit that referenced this pull request May 10, 2022
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 aboedo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread Sources/Identity/CustomerInfo.swift Outdated
Comment thread Sources/Identity/CustomerInfo.swift Outdated
Comment thread Sources/Identity/CustomerInfo.swift Outdated
Comment thread Sources/Identity/CustomerInfo.swift Outdated
Comment thread Sources/Identity/CustomerInfo.swift
Comment thread Sources/Misc/RawDataContainer.swift
Comment thread Sources/Purchasing/EntitlementInfos.swift Outdated
Comment on lines +78 to +91
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)
)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I guess it's personal preference, I'm happy to change the data manipulation method back.
What do you mean by "flat" though?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Tests/UnitTests/Networking/Responses/CustomerInfoDecodingTests.swift Outdated
Comment thread Tests/UnitTests/Networking/Responses/CustomerInfoDecodingTests.swift Outdated

}

/// Default implementation that encodes the `underlyingData` for public use.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Default implementation that encodes the `underlyingData` for public use.
/// Default implementation that encodes the ``RawDataContainer/underlyingData`` for public use.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this in #1565 already :)

NachoSoto added 6 commits May 24, 2022 14:14
- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants