@LossyDictionary: workaround for decoding snake case dictionary keys#1546
Conversation
LossyDictionary: workaround for decoding snake case dictionary keys@LossyDictionary: workaround for decoding snake case dictionary keys
There was a problem hiding this comment.
This was becoming snakeCase, which was breaking the deserialization of these tests for example.
fe05e62 to
d2d4592
Compare
d2d4592 to
d90d5d0
Compare
292a53b to
c79c596
Compare
1819dc6 to
f7315e1
Compare
c79c596 to
b351833
Compare
joshdholtz
left a comment
There was a problem hiding this comment.
Looks good! Just some questions for my clarity but 💯 LGTM
| container.allKeys.sorted(by: { $0.stringValue < $1.stringValue }), | ||
| keys.sorted() |
There was a problem hiding this comment.
Should we put the same sort function on keys here too? I mean... I'm going to assume its always the same but just in case? 🤷♂️
There was a problem hiding this comment.
It relies on String: Comparable's implementation. It would be the same as doing keys.sorted(<)
There was a problem hiding this comment.
So could...
container.allKeys.sorted(by: { $0.stringValue < $1.stringValue })also be
container.allKeys.map { $0.stringValue }.sorted( )? 🤔 My Swift is feeling rusty this week so just 😛
There was a problem hiding this comment.
The method needs to return DictionaryCodingKey (which is what try container.decode(Value.self, forKey: key) needs), so we can't map those to string value.
| container.allKeys.sorted(by: { $0.stringValue < $1.stringValue }), | ||
| keys.sorted() | ||
| ) | ||
| .map { ($0, $1) } |
There was a problem hiding this comment.
So just to make sure I'm understanding this correctly... $0 is the original key that may be converted to camel case or whatever default decoded behavior is? And $1 is the forced snake case / original non-formatted string?
There was a problem hiding this comment.
Exactly.
Don't get me wrong, I don't think this is pretty at all. I contributed this fix to the library I took inspiration from for these property wrappers (marksands/BetterCodable#51) and the maintainer merged it, we both agreed this was the best solution 🤷🏻
There was a problem hiding this comment.
Yeah yeah, all good! Can we maybe add that as a comment saying that is what this does? 😇 Just so its a little easier to understand from a quick glance a few months from now
There was a problem hiding this comment.
Yup good point, this .map isn't clear at all.
It was an ugly way to convert from ZipSequence to Array, but I don't need that. I just changed it to return AnySequence so we even avoid copying the whole array in memory :)
f7315e1 to
10b1caf
Compare
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.
b351833 to
6351a31
Compare
### Depends on: - #1537 - #1540 - #1541 - #1543 - #1546 - #1547 - #1550 - #1551, - #1565 ### Changes - 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. ### 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 #1565. ### TODO: - [x] Fix all tests - [x] Update `RawValueContainer` - [x] Consider recovering `CustomerInfoError` - [x] Look at all previously logged errors - [x] Finish `CustomerInfoResponseTests` - [x] Handle all TODOs - [x] Make `RawDataContainer` actually store all the original data: #1565
The new test illustrates the behavior change that adding
@LossyDictionaryto a[String: Decodable]property had. Keys were being converted to camel case, which is inconsistent from the default behavior thatcontainer.decode([String: Value].self, forKey: key)would have.The reason for this is some implementation detail on Foundation that ignores the
keyDecodingStrategywhen 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 aren't incorrectly converted to camel case, which is required for #1496.