Bukdu.jl icon indicating copy to clipboard operation
Bukdu.jl copied to clipboard

Why is this library using JSON2?

Open Amval opened this issue 5 years ago • 17 comments

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.

Amval avatar Jan 17 '20 14:01 Amval

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!

wookay avatar Jan 17 '20 15:01 wookay

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.

Amval avatar Jan 17 '20 15:01 Amval

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.

Amval avatar Jan 17 '20 15:01 Amval

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"]

wookay avatar Jan 17 '20 15:01 wookay

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.

Amval avatar Jan 17 '20 17:01 Amval

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?

Amval avatar Jan 20 '20 10:01 Amval

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

wookay avatar Jan 20 '20 11:01 wookay

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 :)

Amval avatar Jan 20 '20 13:01 Amval

thanks for your passionate contribution!

wookay avatar Jan 20 '20 13:01 wookay

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

wookay avatar Jan 20 '20 14:01 wookay

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

Amval avatar Jan 20 '20 14:01 Amval

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}

wookay avatar Jan 20 '20 14:01 wookay

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

Amval avatar Jan 20 '20 15:01 Amval

I have commented 2 things in that PR. thanks.

wookay avatar Jan 20 '20 15:01 wookay

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

Amval avatar Jan 21 '20 08:01 Amval

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

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

haydenfree avatar Jan 23 '20 15:01 haydenfree

@haydenfree released it today. thanks!

wookay avatar Jan 24 '20 05:01 wookay