8000 Why is this library using JSON2? · Issue #80 · wookay/Bukdu.jl · GitHub
[go: up one dir, main page]
Skip to content

Why is this library using JSON2? #80

New issue
New issue
Open
Open
Why is this library using JSON2?#80

Description

@Amval
Amval
opened on Jan 17, 2020

Hello,

I saw that this library is using JSON2 instead of the standard JSON library. This library hasn't seen updates in the last 9 months (in fact, the author seems to have moved into doing JSON3 and it doesn't seem to respect the JSON standard.

Here there are a couple examples of JSON2 failing at a roundtrip:

# Float
JSON.parse(JSON.json(2.0)) # Float -> Float
JSON2.read(JSON2.write(2.0)) # Float -> Int
# Dict
JSON.parse(JSON.json(Dict("abc" => 1))) # Dict -> Dict
JSON2.read(JSON2.write(Dict("abc" => 1))) # Dict -> NamedTuple

Is there any JSON2 specific behaviour that you are relying on? Would you accept a pull request changing the library?

Thank you in advance.

PS: This behaviour is also maintained in JSON3.

Activity

  1. wookay commented on Jan 17, 2020

    @wookay
    wookay
    on Jan 17, 2020
    Owner

    I can't remember clearly, maybe JSON2 0.1.3 was better than JSON 0.16.1.

    • Bukdu v0.3.0 : deps JSON2 0.1.3
    • Bukdu v0.2.0 : deps JSON 0.16.1

    I'd like to using JSON.
    PR's are welcome!

  2. Amval commented on Jan 17, 2020

    @Amval
    Amval
    on Jan 17, 2020
    Author

    Thank you for the quick response!

    I forked the package and ran into a couple of issues:

    1. There is a clash while defining JSON as a type, as the module is also called JSON now, instead of JSON2. I temporarily solved it by renaming it as asJSON.
    function index(c::WelcomeController)
    render(asJSON, "Hello World")
end

    Not as nice as before :/ But since Julia doesn't seem to admit aliasing imports I couldn't find a nicer workaround. Dispatching on a Symbol instead of the type and then adding a function that deal with each case would solve it, I suppose.

    function index(c::WelcomeController)
    render(:JSON, "Hello World")
end

    You can take a look here: https://github.com/Amval/Bukdu.jl

    1. More importantly: the last test fails, I don't understand exactly why_
    @test_throws HTTP.ExceptionRequest.StatusError HTTP.post("http://127.0.0.1:8190/", ["Content-Type"=>"application/json"]; body=JSON.json(3))

    Stacktrace:

    INFO: POST    AnonymousController create          200 /
System_internal_error MethodError(+, (nothing, 1), 0x0000000000006959)
    (::Main.test_bukdu_server.var"#2#4")(::Conn) at server.jl:14
    (::Bukdu.var"#create#13"{Main.test_bukdu_server.var"#2#4"})(::Bukdu.System.AnonymousController) at routes.jl:173
INFO: POST    SystemController    internal_error  500 /
┌ Error: error handling request
│   exception =
│    Unexpected end of input
│     ...when parsing byte with value '51'
│    Stacktrace:
│     [1] _error(::String, ::JSON.Parser.StreamingParserState{Base.GenericIOBuffer{Array{UInt8,1}}}) at /home/amval/.julia/packages/JSON/d89fA/src/Parser.jl:150
│     [2] byteat(::JSON.Parser.StreamingParserState{Base.GenericIOBuffer{Array{UInt8,1}}}) at /home/amval/.julia/packages/JSON/d89fA/src/Parser.jl:59
│     [3] current at /home/amval/.julia/packages/JSON/d89fA/src/Parser.jl:72 [inlined]
│     [4] parse_number(::JSON.Parser.ParserContext{Dict{String,Any},Int64,true,nothing}, ::JSON.Parser.StreamingParserState{Base.GenericIOBuffer{Array{UInt8,1}}}) at /home/amval/.julia/packages/JSON/d89fA/src/Parser.jl:400
│     [5] parse_value(::JSON.Parser.ParserContext{Dict{String,Any},Int64,true,nothing}, ::JSON.Parser.StreamingParserState{Base.GenericIOBuffer{Array{UInt8,1}}}) at /home/amval/.julia/packages/JSON/d89fA/src/Parser.jl:166
│     [6] #parse#2(::Type, ::Type{Int64}, ::Bool, ::Nothing, ::typeof(JSON.Parser.parse), ::Base.GenericIOBuffer{Array{UInt8,1}}) at /home/amval/.julia/packages/JSON/d89fA/src/Parser.jl:494
│     [7] parse at /home/amval/.julia/packages/JSON/d89fA/src/Parser.jl:492 [inlined]
│     [8] parse(::Type{Bukdu.Plug.ContentParsers.MergedJSON}, ::Base.GenericIOBuffer{Array{UInt8,1}}) at /home/amval/.julia/dev/Bukdu/src/plugs/ContentParsers.jl:127
│     [9] fetch_body_params(::HTTP.Messages.Request) at /home/amval/.julia/dev/Bukdu/src/plugs/ContentParsers.jl:137
│     [10] handle_request(::HTTP.Messages.Request, ::HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}) at /home/amval/.julia/dev/Bukdu/src/server.jl:7
│     [11] handle_stream(::HTTP.Streams.Stream{HTTP.Messages.Request,HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}}) at /home/amval/.julia/dev/Bukdu/src/server.jl:39
│     [12] macro expansion at /home/amval/.julia/packages/HTTP/lZVI1/src/Servers.jl:360 [inlined]
│     [13] (::HTTP.Servers.var"#13#14"{typeof(Bukdu.handle_stream),HTTP.ConnectionPool.Transaction{Sockets.TCPSocket},HTTP.Streams.Stream{HTTP.Messages.Request,HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}}})() at ./task.jl:333
└ @ HTTP.Servers ~/.julia/packages/HTTP/lZVI1/src/Servers.jl:364
: Test Failed at /home/amval/.julia/dev/Bukdu/test/bukdu/server.jl:29
  Expression: HTTP.post("http://127.0.0.1:8190/", ["Content-Type" => "application/json"]; body=JSON.json(3))
    Expected: HTTP.ExceptionRequest.StatusError
      Thrown: HTTP.IOExtras.IOError
Stacktrace:
 [1] top-level scope at /home/amval/.julia/dev/Bukdu/test/bukdu/server.jl:29
 [2] include at ./boot.jl:328 [inlined]
 [3] include_relative(::Module, ::String) at ./loading.jl:1105
 [4] include(::Module, ::String) at ./Base.jl:31
 [5] include(::String) at ./client.jl:424
 [6] macro expansion at ./util.jl:212 [inlined]
 [7] run(::String, ::Array{String,1}, ::Int64) at /home/amval/.julia/packages/Jive/mDqIC/src/runtests.jl:232
 [8] #runtests#5(::Array{Any,1}, ::Array{String,1}, ::Array{String,1}, ::Bool, ::typeof(runtests), ::String) at /home/amval/.julia/packages/Jive/mDqIC/src/runtests.jl:79
 [9] (::Jive.var"#kw##runtests")(::NamedTuple{(:node1,),Tuple{Array{String,1}}}, ::typeof(runtests), ::String) at ./tuple.jl:0
 [10] top-level scope at /home/amval/.julia/dev/Bukdu/test/runtests.jl:7
 [11] include at ./boot.jl:328 [inlined]
 [12] include_relative(::Module, ::String) at ./loading.jl:1105
 [13] include(::Module, ::String) at ./Base.jl:31
 [14] include(::String) at ./client.jl:424
 [15] top-level scope at none:6
 [16] eval(::Module, ::Any) at ./boot.jl:330
 [17] exec_options(::Base.JLOptions) at ./client.jl:263
 [18] _start() at ./client.jl:460
Bukdu has stopped.

  3. Amval commented on Jan 17, 2020

    @Amval
    Amval
    on Jan 17, 2020
    Author

    Additionally, this method can probably be simplified from:

    function parse(::Type{MergedJSON}, buf::IOBuffer)::Vector{Pair{String,Any}}
    nt = JSON.parse(buf)
    return [Pair(string(k),v) for (k,v) in pairs(nt)]
end

    nt should now be a dictionary, by removing pairs() the behaviour seems identical.

  4. wookay commented on Jan 17, 2020

    @wookay
    wookay
    on Jan 17, 2020
    Owner

    hmm.
    JSON.parse returns Dict
    JSON2.read returns NamedTuple
    JSON3.read returns JSON3.Object
    Can we use JSON3?

    JSON3.read(JSON3.write((k=1,))).k
JSON.parse(JSON.json((k=1,)))["k"]

  5. Amval commented on Jan 17, 2020

    @Amval
    Amval
    on Jan 17, 2020
    Author

    The issue with that is that the wrong behaviour is still present in JSON3. I will open an issue in that repository. It was an open issue on the older package as well

    I am not sure how experimental is it. Standard JSON is probably more batttle tested.

  6. Amval commented on Jan 20, 2020

    @Amval
    Amval
    on Jan 20, 2020
    Author

    Ok, so I took another look and it was very easy to preserve your external API (so no external changes needed).

    I got all tests passing except for two:

    @test ContentParsers.parse(ContentParsers.JSONDecoder, buf) == Pair{String,Any}["json"=>(k=1,)]

    JSON doesn't have any concept of Tuples, and therefore it can be just parse back as a Pair or Dictionary. This test is relying on JSON2 "clever" conversions.

    And the one I mentioned before:

    # @test_throws HTTP.ExceptionRequest.StatusError HTTP.post("http://127.0.0.1:8190/", ["Content-Type"=>"application/json"]; body=JSON.json(3))

    Which throws HTTP.IOExtras.IOError because it fails at parsing the IOBuffer

    function parse(::Type{MergedJSON}, buf::IOBuffer)::Vector{Pair{String,Any}}
    nt = JSON.parse(buf) # fails here
    return [Pair(string(k),v) for (k,v) in pairs(nt)]
end

    Perhaps it was also relying on some non-standard JSON2 behaviour? All other 155 tests are passing.

    Shall I make a pull request or do you think these changes might be too disruptive?

  7. wookay commented on Jan 20, 2020

    @wookay
    wookay
    on Jan 20, 2020
    Owner

    the problem is when JSON.parse with IOBuffer("3")

    julia> JSON.parse(IOBuffer("3"))
ERROR: Unexpected end of input
 ...when parsing byte with value '51'

    compare to the others

    julia> JSON2.read(IOBuffer("3"))
3
julia> JSON3.read(IOBuffer("3"))
3

  8. Amval commented on Jan 20, 2020

    @Amval
    Amval
    on Jan 20, 2020
    Author

    After reading the documentation, this solves the issue:

    buf = IOBuffer("3")
JSON.parse(read(buf, String))

    I made a PR. Thank you for your time and quick responses. Let me know if there's something I can help with :)

  9. wookay commented on Jan 20, 2020

    @wookay
    wookay
    on Jan 20, 2020
    Owner

    thanks for your passionate contribution!

  10. wookay commented on Jan 20, 2020

    @wookay
    wookay
    on Jan 20, 2020
    Owner

    providing namedtuple like Dict{String} also useful.

    function Base.getproperty(dict::T, key::Symbol) where {T <: Dict{String}}
    key in fieldnames(T) ? getfield(dict, key) : getindex(dict, String(key))
end

dict = Dict("a"=>Dict("b"=>3))
@info dict.a.b == 3

  11. Amval commented on Jan 20, 2020

    @Amval
    Amval
    on Jan 20, 2020
    Author

    I am not sure I understand what do you mean with the last message.

  12. wookay commented on Jan 20, 2020

    @wookay
    wookay
    on Jan 20, 2020
    Owner

    when parse JSON object,
    JSON2.jl returns NamedTuple (not Dict), that is sometimes useful like indexing by . (dot).
    that's for JSON.jl indexing by dot for Dict{String}

  13. Amval commented on Jan 20, 2020

    @Amval
    Amval
    on Jan 20, 2020
    Author

    Ah, I see. So is there something else I should do for the Pull Request to be accepted? :)

  14. wookay commented on Jan 20, 2020

    @wookay
    wookay
    on Jan 20, 2020
    Owner

    I have commented 2 things in that PR. thanks.

  15. Amval commented on Jan 21, 2020

    @Amval
    Amval
    on Jan 21, 2020
    Author

    Alright. I incorporated the changes and now all checks are passing. Thank you for the support.

  16. haydenfree commented on Jan 23, 2020

    @haydenfree
    haydenfree
    on Jan 23, 2020

    @Amval Thanks for your work updating the package to remove JSON2.

    @wookay Any idea when this change will be released in the next version?

  17. wookay commented on Jan 24, 2020

    @wookay
    wookay
    on Jan 24, 2020
    Owner

    @haydenfree released it today. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions
      0