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

Fix for DecodingError "Expected to decode Double but found Int instead." #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrewash
Copy link

@andrewash andrewash commented Dec 1, 2023

Related Issue
#21

Description
Int literals cannot be cast as a Double using as? so Double(<Int>) must be used instead.
JSON has only one Number type, and allows "5" to be considered a Double, not just "5.0"

However MoreCodable's DictionaryDecoder always decoded "3" as an Int so we would need a way to convert it to a Double to support JSON's flexible number formats.

Testing
I've included a new unit test that fails before this PR, and passes once the change is made to func castOrThrow<T>()

References
This railroad from json.org shows that 3 is totally valid as a JSON number

railroad diagram

Int literals cannot be cast as a Double using `as?` so `Double(<Int>)` must be used instead.
JSON has only one Number type, and allows "5" to be considered a Double, not just "5.0"
Copy link

@shanecowherd shanecowherd left a comment

Choose a reason for hiding this comment

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

Do we need to address any other types?

@andrewash
Copy link
Author

Thank you Shane! @tattn it's my colleague @shanecowherd commenting above. Shane, I won't need anything beyond Double that I can foresee right now. Float would probably be a good choice for the library more generally. @tattn probably knows what types are otherwise supported by the library.

@tattn
Copy link
Owner

tattn commented Dec 3, 2023

Thank you for your PR 👍

I created DictionaryDecoder not as a JSON decoder, but to decode Swift dictionaries. Therefore, in the test case ["double": 5], it is correct behavior for it to be decoded as 5 of Int type in Swift, which is the current behavior.

Could you please share the use case?
I believe that the types should be what you intend to work with at the stage of being a dictionary object.
For situations like network communication or file reading, where types like string are not strict and need to be decoded as JSON, the standard JSONDecoder is more suitable.

@andrewash
Copy link
Author

Thank you for your feedback @tattn
Sorry it took me a week to get back to you. Been busy with some other stories at work.

I'm happy to share our use case which may be surprising.
Use Case: Hybrid React Native / Swift apps.
Are you familiar with React Native apps?

I'll try to explain the use case simply:

  • React Native is written in JS, but allows for interop with Swift code via something called a React Native (RN) "NativeModule"
  • At PBSCO, where I work, we have NativeModules for things like talking to the camera, or bluetooth, etc. Things that Swift handles much better.
  • This lets JS "call" a Swift function
  • It turns out, when that call is made, if RN wants to pass a JSON object as a parameter to Swift (which is very common), then it always does so using a [String: Any] dictionary (and not a JSON string).
  • Of course I'm a Swift engineer, so I prefer to work with Codable strongly typed objects, not [String: Any] dictionaries.
  • DictionaryDecodable happens to be perfect for doing this kind of conversion.

Unfortunately in JSON, a Double like "1.00" can be written as "1" and so RN sends { "price": 1 } to Swift, instead of { "price": 1.00 }

@tattn Would you be open to supporting this use case in your library if it's not a lot of extra work?
It would really help us out, and I'm happy to contribute PRs to maintain this support.
Good news: 90% of the time, DictionaryDecodable just works for this use case, so I don't expect many PRs to be needed.

If you do decide to support this use case, this PR would be needed for that.

If you don't want to support this use case, then I can maintain a fork of your repo at my company's organization and keep the fix localized to that fork. That's ok too.

Example: Here is the function definition, in Swift, of a RN NativeModule function. This is what's called by JS.
FYI - In JavaScript the code to call this Swift function looks like BluetoothNativeModule.disconnectFrom({ deviceId: id });

  /// Disconnects from a connected BLE device
  ///
  /// - Parameters:
  ///   - rawRequest: JSON: `{ deviceId: <String> }`
  ///   - resolver: The Promise resolver function that returns `true` if the file exists, `false` otherwise.
  ///   - rejecter: The Promise rejecter function, not used here as the function doesn't throw errors.
  @objc func disconnectFrom(_ rawRequest: [String: Any],
                            resolver: @escaping RCTPromiseResolveBlock,
                            rejecter: @escaping RCTPromiseRejectBlock) { }

@tattn
Copy link
Owner

tattn commented Dec 7, 2023

@andrewash
Thank you for the detailed explanation!

I haven't used React Native in a production environment, so I wasn't aware of the issue where Double is converted to Int when treated as JSON in data transmission. This seems like a problematic specification 😢

Using DictionaryDecoder as a JSON decoder is an interesting idea, but overly flexible decoding might delay the detection of implementation errors.

Here's what I suggest:

  1. Introduce a decodingStrategy property similar to dateDecodingStrategy in JSONDecoder.
  2. Create a type capable of decoding both Int and Double.

For 1:
Here's an example usage:

let decoder = DictionaryDecoder()
decoder.decodingStrategy = .json
try decoder.decode(JSON.self, from: jsonDictionary)

Setting .dictionary as the default for decodingStrategy keeps the current behavior.
However, implementing .json should not be done half-heartedly. It needs to be developed to a point where it can satisfactorily meet JSON specifications, which might take some time.

For 2:
This isn't about adding a feature to this library, but a suggestion for your project.
Consider a numeric type like the following:

struct RNDouble: Decodable, Equatable {
    var value: Double

    init(_ value: Double) {
        self.value = value
    }

    init(from decoder: Decoder) throws {
        let container = try decoder.singleValueContainer()
        do {
            value = try container.decode(Double.self)
        } catch {
            value = Double(try container.decode(Int.self))
        }
    }
}

struct Model: Decodable, Equatable {
    let double: RNDouble
}

This approach directly addresses the issue but might be slightly cumbersome due to type nesting.

What are your thoughts on these proposals?

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