Skip to content
This repository has been archived by the owner on Mar 23, 2021. It is now read-only.

Incorrect String->Int map detection #52

Closed
remko opened this issue Sep 27, 2016 · 8 comments
Closed

Incorrect String->Int map detection #52

remko opened this issue Sep 27, 2016 · 8 comments

Comments

@remko
Copy link
Contributor

remko commented Sep 27, 2016

When I run the following code:

let value = ["foo": 42]
print(value)
if let data = try? Jay().dataFromJson(any: value) {
print(String(bytes: data, encoding: String.Encoding.utf8))
}

it outputs

["foo": 42]
Optional("{\"foo\":true}")

Is it expected that Jay interprets the number as a boolean?

@czechboy0
Copy link
Collaborator

czechboy0 commented Sep 27, 2016

Err, yeah this is tricky. I've been considering removing the Any API, as it relies on optional casting in detecting types, which was inconsistent across macOS and Linux a few months ago.

For now, could you 1) fork the repo and add a failing test case? And in the meantime, you can switch to type-safe JSON, which should work well. So that'd be

let value: JSON = .object(["foo": .number(.integer(42))])
print(value)
if let data = try? Jay().dataFromJson(json: value) {
    print(data.string())
}

@endocrimes
Copy link
Owner

Hmm, this could potentially be solved by changing priorities here: https://github.com/DanToml/Jay/blob/master/Sources/Jay/NativeTypeConversions.swift#L162

as Bool values shouldn't evaluate to Integer types? - Although this would need good test coverage.

@czechboy0
Copy link
Collaborator

@dantoml Try it, I have a feeling that breaks other tests though - the NSNumber weirdness in bridging on macOS vs Linux makes type-unsafe JSON really hard to keep consistent across platforms.

But we definitely need a failing test here & then see what can be done. I put most of my energy into the handling of type-safe JSON, so I suspect there are many potential improvements in the type-safe <-> type-unsafe JSON.

@endocrimes
Copy link
Owner

@czechboy0 true, NSNumber/NSValue bridging will make this super weird

@remko
Copy link
Contributor Author

remko commented Sep 28, 2016

PR created.

Changing the order indeed breaks the boolean conversion :(

I like the Any API, because I can then use it as a drop-in replacement of JSONSerialization for data that comes from a JSON-agnostic API.

@remko
Copy link
Contributor Author

remko commented Sep 30, 2016

It seems the problem is with:

 if let nsdict = json as? NSDictionary {

On OS X, this is converting the type to some native type where you can't tell the difference between integers and booleans.

I added a test

     if let nativeDict = json as? [String: Any] {

before this (and a similar test for Arrays). On OS X, it takes this path, and doesn't lose any type information, and all the tests pass. On Linux, it doesn't take this path, but somehow the NSDictionary path works fine there, and the tests pass as well ¯\_(ツ)_/¯

@endocrimes
Copy link
Owner

:( - It might be worth us trying to come up with a tiny repro case for this and filing a Swift JIRA/Radar

@remko
Copy link
Contributor Author

remko commented Sep 30, 2016

Actually, what's happening is this:

  • If you create an [Any] array, on OS X it will cast to both an [Any] and an NSArray. On Linux only an [Any].
  • If you create an NSArray, on OS X it will cast to both an [Any] and an NSArray. On Linux only an NSArray.

I'm guessing the cross-casting on OS X is a backwards compatibility / bridging feature. However, during cross-casting, type information seems to get lost of the elements. Without knowing what's behind this, this might be unavoidable because e.g. in Objective-C you can't tell the difference between both (wild guess, don't actually know Objective-C or the bridging details enough for this).

So actually, this isn't that strange behavior, and the solution is what the patch does: first try the native type, and if that fails, it means the user passed in an NSArray/NSDictionary (instead of a native array/dictionary), and they'll have to live with losing type information.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants