LossyDictionary: added failing test and fix for key conversion issue
#51
Conversation
| let reencodedFixture = try decoder.decode(Fixture.self, from: encoder.encode(fixture)) | ||
|
|
||
| XCTAssertEqual(reencodedFixture, fixture) | ||
| XCTAssertEqual(Set(reencodedFixture.stringToInt.keys), ["snake_case"]) |
There was a problem hiding this comment.
This becomes snakeCase. Removing @LossyDictionary from Fixture's definition makes the test pass.
NachoSoto
changed the title
LossyDictionary: added failing test for key convertion issueLossyDictionary: added failing test and fix for key conversion issue
| intToString: [:] | ||
| ) | ||
|
|
||
| let reencodedFixture = try decoder.decode(Fixture.self, from: encoder.encode(fixture)) |
There was a problem hiding this comment.
Encode and decode to ensure that fixture == reencodedFixture.
|
Ideally that manual key conversion wouldn't be necessary, but I haven't been able to figure out how to avoid it. |
NachoSoto
force-pushed
the
idempotent-keys
branch
from
060c3e3
to
f5a86a7
Compare
|
I've changed the workaround to not use that method and instead look at the original unconverted keys. |
| "snake_case.with.dots_99.and_numbers": 1, | ||
| "dots.and_2.00.1_numbers": 2, | ||
| "key.1": 3 |
There was a problem hiding this comment.
These were failing tests before and/or with the other workaround.
NachoSoto
force-pushed
the
idempotent-keys
branch
from
f5a86a7
to
f1ec126
Compare
The property wrapper `LossyDictionary` incorrectly decodes keys with `KeyDecodingStrategy.convertFromSnakeCase`. The test simply encodes and decodes a sample data, to illustrate that the conversion isn't idempotent. 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 in `Foundation` that ignores the `keyDecodingStrategy` when decoding "raw" dictionaries versus property names. To deal with this, this decodes the strings first as "raw", and then matches them to the corresponding converted key.
NachoSoto
force-pushed
the
idempotent-keys
branch
from
f1ec126
to
e0c3e48
Compare
|
|
||
| return zip( | ||
| container.allKeys.sorted(by: { $0.stringValue < $1.stringValue }), | ||
| keys.sorted() |
There was a problem hiding this comment.
I understand why this is necessary, but how robust is it? Can you think of a scenario where the sorting would be different when converting from snake case? I'd hate for there to be an edge case that's missed.
I think the answer is "no", but is it possible to route through this only when the key strategy isn't the default?
There was a problem hiding this comment.
I couldn't think of a way, but I agree it's not ideal.
Unfortunately the Decoder protocol doesn't expose that. I went through the whole Swift implementation to understand how it uses private types in a way that we can't.
There was a problem hiding this comment.
Thanks for the due diligence. I'll play around with this tonight and try and button up some other in flight work.
There was a problem hiding this comment.
I'm going to merge this because I haven't come up with a better solution. If this exposes problems, then hopefully they're reported and then I can take a closer look at it. I have an outstanding fix for a DefaultCodable encoding bug that I need to add tests for. If you would prefer a release or tag for this, though, let me know so you're not blocked.
The property wrapper
LossyDictionaryincorrectly decodes keys withKeyDecodingStrategy.convertFromSnakeCase.The test simply encodes and decodes a sample data, to illustrate that the conversion isn't idempotent.
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 in
Foundationthat ignores thekeyDecodingStrategywhen decoding "raw" dictionaries versus property names.To deal with this, this decodes the strings first as "raw", and then matches them to the corresponding converted key.