swiftlang / swift-corelibs-foundation

The Foundation Project, providing core utilities, internationalization, and OS independence
swift.org
Apache License 2.0
5.28k stars 1.13k forks source link

[SR-5249] Codable throws error when decoding an Int/Float from a JSON string #4288

Open swift-ci opened 7 years ago

swift-ci commented 7 years ago
Previous ID SR-5249
Radar rdar://problem/32850823
Original Reporter chrismanderson (JIRA User)
Type Bug
Additional Detail from JIRA | | | |------------------|-----------------| |Votes | 9 | |Component/s | Foundation | |Labels | Bug | |Assignee | @itaiferber | |Priority | Medium | md5: 3573648798d9e727b67a246b3e28a7f1

relates to:

Issue Description:

If I have a JSON object such as

  {
    "id": "4yq6txdpfadhbaqnwp3",
    "email": "john.doe@example.com",
    "name":"John Doe",
    "spent":"1.00",
    "visits":"34"
  }

with a Swift object

struct User: Decodable {
  var id: String
  var email: String
  var name: String
  var spent: Float
  var visits: Int
}

I get the error

typeMismatch(Swift.Float, Swift.DecodingError.Context(codingPath: [Optional(__lldb_expr_68.User.(CodingKeys in _FE7989C1DE048E5A32A5007446084784).spent)], debugDescription: "Expected to decode Float but found a string/data instead."))

I would assume Codable is looking for only number types when decoding, but many of the JSON APIs I work with (e.g. Shopify) wrap their integers and prices in strings.

belkadan commented 7 years ago

That's formally-correct JSON, but certainly takes some of the wind out of Decodable. @itaiferber?

itaiferber commented 7 years ago

This is an implementation detail of JSONDecoder, not of Codable, which is format and type agnostic. We can add a strategy to be more lenient in JSON decoding to allow this type of conversion โ€” noted.

itaiferber commented 7 years ago

@swift-ci create

swift-ci commented 7 years ago

Comment by Chris Anderson (JIRA)

Agreed Jordan, it's not formally-correct JSON, but I'd argue 'formally-correct' JSON is much less of a thing than I'd gather we would all like it to be. ๐Ÿ™‚ Glad you are all taking a look at this!

swift-ci commented 7 years ago

Comment by Jesper Lindholm (JIRA)

I came here because I filed the related issue (SR-5278) and thought I'd make an armchair attempt at some minimal API to solve this:

enum JSONDecoderPrimitive {
    case numberLiteral(String)
    case stringLiteral(String)
    case booleanLiteral(Bool)
    case null
}

protocol JSONDecoderPrimitiveCoercing {
    static func coercePrimitive(originalValue: JSONDecodingPrimitive, key: CodingKey) -> JSONDecodingPrimitive
}

In Chris's case, he could adopt it as: (beware of textarea-compiled code)

struct User: Decodable, JSONDecoderPrimitiveCoercing {
  var id: String
  var email: String
  var name: String
  var spent: Float
  var visits: Int

  static func coercePrimitive(originalValue: JSONDecodingPrimitive, key: CodingKey) -> JSONDecodingPrimitive {
    if (key == .spent || key == .visits) && let case .stringLiteral(let str) = originalValue {
      return .numberLiteral(str)
    }
    return originalValue
  }
}

coercePrimitive would only be called as a last resort before parsing failed, and only on "leaf" values (so not on whole objects or arrays).

This would also let null be interposed with a more fitting sentinel value instead of nil, or the opposite. And it would all be opt-in and specify the JSON-specific behavior. I understand the point about wanting the encoding to be agnostic, but I think the problems I ran into with Decimal supporting keyed coding is illustrative of things working out for property lists that may not work so well in other places.

bobergj commented 7 years ago

A lenient option handling String - > Int coercion would be ok, but I wouldn't want the decoder to make a guess at the format of a decimal number. That would just encourage sloppiness. What locale would it assume? "1.00" could as well be "1,00" (France) or "4.294.967.295,000" (Italy).

Also, the API you are working with is using a string representation for prices due to precision issues, so you don't want a Float there.

Disregarding that visits property, you want something like:

let json = """
{
    "id": "4yq6txdpfadhbaqnwp3",
    "email": "john.doe@example.com",
    "name":"John Doe",
    "spent":"1.00"
}
""".data(using: .utf8)!

struct Money: Decodable {
    let decimal: Decimal

    init(from decoder: Decoder) throws {
        let decimalString = try decoder.singleValueContainer().decode(String.self)
        if let decimal = Decimal(string: decimalString, locale: Locale(identifier: "en_US")) {
            self.decimal = decimal
        } else {
            throw DecodingError.typeMismatch(Money.self, .init(codingPath: decoder.codingPath, debugDescription: "\(decimalString) is not in the expected format"))
        }
    }
}

struct User: Decodable {
    var id: String
    var email: String
    var name: String
    var spent: Money
}

let decoder = JSONDecoder()
let user = try! decoder.decode(User.self, from: json)

print("The user spent \(user.spent.decimal)")
swift-ci commented 7 years ago

Comment by Chris Anderson (JIRA)

Yeah, using Float for a 'money' type in this case was just an example, not something I would actually do. Point still stands though, I would expect JSONDecoder to either handle or provide a simple option to handle e.g. "1.3" to 1.3, etc.

swift-ci commented 7 years ago

Comment by Chris Anderson (JIRA)

Curious as to any update about this ticket?

itaiferber commented 7 years ago

chrismanderson (JIRA User) Adding a new strategy to JSONEncoder/JSONDecoder to support this type of conversion would require going through internal and external API review. At this point in the release we can't properly vet this, but this will come in a future version of Swift.

In the meantime, there are a few workarounds that you can employ โ€” custom encoding/decoding code; a private var whose type is String which is what gets decoded, and a public lazy var whose type is Double which is parsed from the String on first access; etc.

swift-ci commented 7 years ago

Comment by Chris Anderson (JIRA)

Bummer but understandable.

Sort of a hypothetical, but is it possible to subclass JSONEncoder and change the strategy myself? This could be useful for a lot of cases where a date format changes from API to API, number formats change, to handle some custom data conversion, etc. Some kind of API to define a custom strategy for a type would be amazing, as I think you all are being a bit optimistic about how stable JSON 'types' are ๐Ÿ™‚.

itaiferber commented 7 years ago

chrismanderson (JIRA User) It's possible for you to subclass JSONEncoder and JSONDecoder, but you'll have to rewrite the implementation to suit your needs as the bulk of it is present in private types internally.

The "custom strategy for a type" is really just writing a custom encode/decode โ€” if you've got changing data over time, you're going to have to customize how you handle this to be able to support multiple formats for your types. It's easily supported without needing a strategy on JSONEncoder and JSONDecoder.

swift-ci commented 7 years ago

Comment by Peter (JIRA)

Here the same issue. I have a JSON API that returns Int's wrapped in Strings. All other fields automatically fill correctly when decoding, but it gives error on the Int field, since it has a String in the JSON. Would really like a way to easily allow String values to be parsed to Int's.

Am I correct that writing a custom encode/decode involves writing a encode/decode for the full object? Or can I manually decode the string-ints and let the decoder pick up from there? I.E. not having to decode everything manually?

itaiferber commented 7 years ago

de Kraker (JIRA User) Yes, when you provide an implementation of init(from๐Ÿ™‚ and encode(to๐Ÿ™‚, you are responsible for encoding and decoding all of the fields that you need for the object.

swift-ci commented 7 years ago

Comment by Peter (JIRA)

@itaiferber That's too bad. Will look for other ways to handle this then. Or maybe just make my object use Strings instead of Ints.

itaiferber commented 7 years ago

de Kraker (JIRA User) One option that you have is writing an adaptor type which can perform decoding from a String but exposes an Int, as so:

import Foundation

struct CoercingInt : Codable, RawRepresentable {
    let rawValue: Int

    init(rawValue: Int) {
        self.rawValue = rawValue
    }

    public init(from decoder: Decoder) throws {
        let container = try decoder.singleValueContainer()
        let stringValue = try container.decode(String.self)
        guard let value = Int(stringValue) else {
            throw DecodingError.dataCorruptedError(in: container, debugDescription: "Invalid integer string.")
        }

        self.rawValue = value
    }

    public func encode(to encoder: Encoder) throws {
        var container = encoder.singleValueContainer()
        try container.encode("\(self.rawValue)")
    }
}

struct ThingWithAnInt : Codable {
    let myValue: CoercingInt
}

let json = """
{
    "myValue": "42"
}
""".data(using: .utf8)!

let decoder = JSONDecoder()
let value = try decoder.decode(ThingWithAnInt.self, from: json)
print(value.myValue.rawValue)
swift-ci commented 7 years ago

Comment by Peter (JIRA)

@itaiferber that's an awesome solution if it works indeed. Saves me from manually parsing lots of JSON objects from a legacy API. Thanks, will try it out!

swift-ci commented 7 years ago

Comment by Chris Anderson (JIRA)

@itaiferber that's not a bad suggestion but it's still not ideal to have to deal with a wrapper struct. It's also tricky for JSON values that can be both strings and ints (it happens ๐Ÿ™). With the structure of Codable as is, there's a lot of manual work which (sort of) negates the benefit of Codable in the first place.

Has there been any thought to allowing a user to custom decode one attribute but let Codable handle all the others? Or, allow a user to write their own decoding strategies ala https://developer.apple.com/documentation/foundation/jsonencoder/2895199-nonconformingfloatencodingstrate?

itaiferber commented 7 years ago

chrismanderson (JIRA User) I agree that this is suboptimal, but it's workable in the meantime. There's often a tradeoff between convenience and correctness/safety, so we need to find the right place to land on this. Allowing arbitrary coercions is likely a big (& unsafe) hammer for solving a problem like this; perhaps adaptors are a better solution. But, this requires thought and careful design that we haven't yet had time to do.

swift-ci commented 7 years ago

Comment by Chris Anderson (JIRA)

Makes sense for sure. I think adapters could be an amazing solution cause it could maintain as much strictness (an int for an int and only and int) or looseness (could be a string, could be a decimal, could be an image, etc into an int) as the user desires.

swift-ci commented 6 years ago

Comment by Chris Anderson (JIRA)

Sorry to ping an old issue, but has there been any work on providing a method to automatically and safely convert Strings into Ints? Or, allowing folks to conform to a custom adapter? Still crossing my fingers that it is a possibility for the future as it's really tough for me (and while I don't know, but am assuming) others to use Codable for JSON in its current state.

itaiferber commented 6 years ago

chrismanderson (JIRA User) Sorry, Chris, but unfortunately, we haven't had the bandwidth to work on this in the meantime. ๐Ÿ™ This is still being considered and planned for the future, though. Custom adaptors could be very powerful, but we need to have the time to design the API appropriately with a lot of consideration.

swift-ci commented 6 years ago

Comment by Chris Anderson (JIRA)

Bummer nothing is imminent, but glad to hear it could be on the horizon one day. Thanks!

swift-ci commented 6 years ago

Comment by Artem Goncharov (JIRA)

@itaiferber I can understand why you add the support of parsing `strings` as `Float`, but do you think that it's a correct from integrity of JSONEncoder point of view. Because encoding of this object produce a different json - without strings as a container. As user of JSONEncoder and JSONDecoder, I expect that conversion `json1 -> codable object -> json2` produce equal `json1` and `json2`. Please, correct me if I'm wrong, but I still think that json from the ticket is not "valid", of course, it has a valid json structure, but not valid for particular codable object. It should be a problem of particular implementation of API and client, and shouldn't be a problem of Foundation and shouldn't be exposed for all Swift users. Do these changes really should be applied, what do you think?

swift-ci commented 6 years ago

Comment by Chris Anderson (JIRA)

FWIW I don't think parsing a string into a float should be the default case; but should be enabled as an opt-in either through an adapter, method on JSONDecoder/JSONEncoder, or some other method. Also, that example is sort of cribbed from the Shopify API (https://help.shopify.com/api/reference/order), a heavily used API from a major company demonstrating that it's not just little one off APIs that different in their JSON formats.

JSON is inherently a super flexible yet widely inconsistent format. As much as I'd love to have every single float be a float and not as a string (as just one example), consuming so many JSON APIs over the years has proven that is not a given. Different JSON APIs can have so many different data formats that building in the flexibility into Codable to handle these cases in a safe and opt-in manner (without resorting to having to manually handle everything) is a good balance to handle the wild west that is JSON.

itaiferber commented 6 years ago

artem (JIRA User) We don't intend to ever enable this sort of implicit conversion by default โ€” this would be strictly opt-in. Enabling a "lenient" mode, too, wouldn't indicate anything like this on encode, either. Regardless, the goal is to likely offer a strongly-typed solution that allows you to do this on a case-by-case basis with out falling off the "cliff" into having to implement all of encode(to๐Ÿ™‚ and init(from๐Ÿ™‚ for the benefit of one property; the solution is likely nontrivial and would require a lot of API discussion to figure out how to do well, hence why we haven't been able to do this yet.

swift-ci commented 5 years ago

Comment by Christopher Prince (JIRA)

I've only recent started using `Codable` so perhaps my comment is too ill-informed. It does seem there's a lot of "without resorting to having to manually handle everything" behavior. That is, some customization that in my mind should be pretty easy results in manually having to handle everything.