Skip to content
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

[SR-6629] JSONEncoder with convertToSnakeCase encodes myURLProperty into my_url_property, but JSONDecoder with convertFromSnakeCase does not decode myURLProperty from my_url_property. #3752

Open
norio-nomura opened this issue Dec 17, 2017 · 9 comments

Comments

@norio-nomura
Copy link
Contributor

Previous ID SR-6629
Radar rdar://problem/41038908
Original Reporter @norio-nomura
Type Bug
Environment

swift-4.1-DEVELOPMENT-SNAPSHOT-2017-12-15-a

Additional Detail from JIRA
Votes 1
Component/s Foundation
Labels Bug, Codable
Assignee @itaiferber
Priority Medium

md5: fee6142a597261d7125a4f80b8b8ceb2

Issue Description:

Because JSONDecoder coverts my_url_property to myUrlProperty.

import Foundation

struct S: Codable {
    var myURLProperty: String
}

let encoder = JSONEncoder()
encoder.keyEncodingStrategy = .convertToSnakeCase
let data = try encoder.encode(S(myURLProperty: "property"))
let decoder = JSONDecoder()
decoder.keyDecodingStrategy = .convertFromSnakeCase
do {
    let decoded = try decoder.decode(S.self, from: data)
} catch {
    print(error)
// keyNotFound(CodingKeys(stringValue: "myURLProperty", intValue: nil), Swift.DecodingError.Context(codingPath: [], debugDescription: "No value associated with key CodingKeys(stringValue: \"myURLProperty\", intValue: nil) (\"myURLProperty\"), converted to my_url_property.", underlyingError: nil))
}
@norio-nomura
Copy link
Contributor Author

Perhaps JSONDecoder should not convert keys of JSON from snake_case to camelCase, but should query JSON with CodingKeys that converted to snake_case.

@belkadan
Copy link

JSONDecoder doesn't have a list of words that are normally written as initials in Swift style, so the camel-ification of my_url_property would be myUrlProperty. @itaiferber, what's the recommendation here?

@norio-nomura
Copy link
Contributor Author

I'm trying to implement my idea on YAMLDecoder in jpsim/Yams#84
However, since the implementation of JSONDecoder.KeyDecodingStrategy.convertFromSnakeCase supports too many variations, my implementation can not pass the same test cases. 🙁

@norio-nomura
Copy link
Contributor Author

Rather than supporting various notation fluctuations, I would like to prioritize that data encoded with convertToSnakeCase can be decoded reliably with convertFromSnakeCase.

@itaiferber
Copy link
Contributor

Sorry for the delay in responding — I was on vacation. My recommendation for values like this would be to provide a key name of something like myUrlProperty (manually in the CodingKeys enum) so the value can round-trip. There are indeed edge cases like this where we couldn't possibly tell what the original key was, so we can't round-trip. @norio-nomura what tests are you unable to pass?

@itaiferber
Copy link
Contributor

(In any case, I don't think this is necessarily a bug, just a behavior that exists here because we can't know what the original key was; it's an imperfect conversion.)

@norio-nomura
Copy link
Contributor Author

Sorry for delay.
My test implementation on Yams is here.
The YAMLDecoder converts CodingKey.stringValue into snake_case on accessing YAML contents, instead of converting keys in YAML contents from snake_case to camelCase.

what tests are you unable to pass?

Since used String conversion is snakecased only, the value can round-trip.
Instead, it does not pass tests that require special logic when converting snake_case to camelCase.

@itaiferber
Copy link
Contributor

@norio-nomura I don't currently have the time to read through the full implementation of Yams, but how do you handle custom conversions on decode? The .custom block goes from key in JSON to CodingKey, but it sounds like your snake case conversion goes the other way. We do our conversion in the same direction both ways, which is why we end up with cases like this.

@norio-nomura
Copy link
Contributor Author

I opened a PR that implemented my idea.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants