xyncro / chiron

JSON for F#
https://xyncro.tech/chiron
MIT License
175 stars 41 forks source link

JSON parser can't handle " " key #29

Closed haf closed 9 years ago

haf commented 9 years ago
Error in Ln: 1 Col: 54
{"updated":{"description":"a","key":"a","settings":{"   ":"a"}}}
                                                     ^
Expecting: '"' or '\\'

But it's a valid key; isn't it? I haven't seen that JSON requires trimming the keys for whitespace.

kolektiv commented 9 years ago

Spec says it is, i'm not sure why it's not valid... Will check!

kolektiv commented 9 years ago

That's actually particularly odd, as when I run Json.tryParse on that JSON it seems to work fine... Have you got a repro handy?

haf commented 9 years ago

I was just gonna add another test-case:

System.Exception: Error in Ln: 1 Col: 55
{"updated":{"description":"a","key":"a","settings":{""":"a"}}}
                                                      ^
Expecting: ':'

Let me try that thing.

kolektiv commented 9 years ago

For reference, I've just tried a throwaway console app with

open Chiron

[<EntryPoint>]
let main _ =

    let json = """{"updated":{"description":"a","key":"a","settings":{"   ":"a"}}}"""
    let parsed = Json.tryParse json

    0

in it. It seemed happy...

kolektiv commented 9 years ago

Well, that second test case you quote there is as expected I would think. JSON strings can't include unescaped quote marks, so I would expect that to complain...

haf commented 9 years ago

So it probably has something to do with how I serialise. It's a round-trip test with FsCheck, so gg to me. Let's drill down.

haf commented 9 years ago

Ok, so it turns out I'm actually giving it a char 31, or for real, a '' character. Which according to the spec needs to be escaped. Test-case:

#!/usr/bin/env fsharpi

#I "../packages/FParsec/lib/net40-client"
#r "FParsec.dll"
#I "../packages/Aether/lib/net40"
#r "Aether.dll"
open Aether
#I "../packages/Chiron/lib/net40"
#r "Chiron.dll"
open Chiron

let v = Map [ "", "abc" ]
let json = v |> Json.serialize |> Json.format
printfn "%s" json

Should output {"\u0031":"abc"} as far as I understand.

kolektiv commented 9 years ago

Aha, yes, I think I've made a mistake interpreting the RFC (http://tools.ietf.org/html/rfc7159#section-7) here - ALL control characters should be escaped, so I'll update that!

kolektiv commented 9 years ago

Or, in fact, I had interpreted that correctly - but we don't appear to be escaping object keys. This should be, hopefully, a quick fix.

kolektiv commented 9 years ago

I think that if you're giving it char 31 (dec? That would make it the unit separator char? char 0031 hex would be the number 1, and thus not escaped) then it should indeed be escaped here to the hex code, producing this:

{"\u001F":"abc"}

I've just made that change to escape object keys (as they should be) and I'll push that out as 3.1.0 (yes, strictly should be 4.0.0 maybe but...)

kolektiv commented 9 years ago

3.1.0 is out, hopefully happily fixing that issue. Let me know if it seems better! Would love to get any FsCheck tests you have in to the build too if they're covering ground we don't cover so far...

kolektiv commented 9 years ago

Closing this for now, feel free to re-open if you find anything which breaks this :)

haf commented 9 years ago

Works in 4.0! Thanks.