ohler55 / oj

Optimized JSON
http://www.ohler.com/oj
MIT License
3.14k stars 251 forks source link

Does `max_nesting` work? #894

Closed ioquatix closed 1 year ago

ioquatix commented 1 year ago
irb(main):005:0> a = [[[[[]]]]]
=> [[[[[]]]]]

irb(main):006:0> Oj.dump(a, max_nesting: 2)
=> "[[[[[]]]]]"

irb(main):008:0> Oj.dump(a, mode: :compat, max_nesting: 2)
=> "[[[[[]]]]]"

irb(main):009:0> Oj.default_options = {mode: :compat, max_nesting: 2}
=> {:mode=>:compat, :max_nesting=>2}

irb(main):010:0> Oj.dump(a)
=> "[[[[[]]]]]"

If max_nesting is set to 2, shouldn't this raise an error (since the nesting of array is more than 2).

ohler55 commented 1 year ago

max_nesting is a boolean. http://www.ohler.com/oj/doc/Oj.html#to_json-class_method

ioquatix commented 1 year ago

Ohhhh, sorry, I totally missed this, I expected it to behave the same as the JSON gem. Would it make sense to accept an integer?

ioquatix commented 1 year ago

For more context, for the purpose of logging, we want a relatively low nesting level - e.g. 8 - to avoid deeply nested logs. If the level is exceeded, we do custom filtering and mark the log message as "truncated" - which requires further investigation (as to why the log is so deeply nested).

ohler55 commented 1 year ago

I think at the time I added that the JSON gem did not have the option. I could be wrong though. If it does I'll gladly support the old behaviour as well as assigning an integer.

ioquatix commented 1 year ago

This is the interface AFAIK:

irb(main):002:0> JSON.dump([[[]]], 2)
/home/samuel/.rubies/ruby-3.2.1/lib/ruby/3.2.0/json/common.rb:649:in `rescue in dump': exceed depth limit (ArgumentError)

irb(main):003:0> JSON.generate([[[]]], max_nesting: 2)
/home/samuel/.rubies/ruby-3.2.1/lib/ruby/3.2.0/json/common.rb:312:in `generate': nesting of 2 is too deep (JSON::NestingError)
ohler55 commented 1 year ago

Well, I was incorrect regarding the code. Oj.to_json did honour the max_nesting as fixnum. Oj.dump did not. That has been fixed in the "more-options" branch. Please give it a try and see what you think. There is some inconsistency between using JSON.generate and JSON.dump in regards to what is considered the max but that's a JSON gem oddness that Oj attempts to match.

ioquatix commented 1 year ago

I tried it out.

It might be a little sensitive.

Oj.dump([], mode: :compat, max_nesting: 1) # Ok.
Oj.dump(['x'], mode: :compat, max_nesting: 1) # Fails - max nesting exceeded.

JSON.dump([], 1) # Ok.
JSON.dump(['x'], 1) # Ok.
JSON.dump([[]], 1) # Fails

I'm okay with either interpretation, but I suspect "nesting" means nesting with a construct that can contain other things, rather than primitive types (strings, integers, etc).

ohler55 commented 1 year ago

I'll try the different JSON encoding methods. Years ago they acted differently. There is code in place to handle that but maybe the behavior of the JSON has changed. I'll get them to match though.

ohler55 commented 1 year ago

It looks like JSON.generate is now consistent with in regard to max_nesting and instead of dump generating an ArgError it now generates a NestingError. I'll remove the special case code and update the imported JSON tests which I suspect have been update in the json project. Give me a day or two to get that and the other changes in the branch double checked.

ohler55 commented 1 year ago

sigh, nope, in some case JSON.dump does indeed still return an Argument error. More detective work ahead.

ioquatix commented 1 year ago

Yes, I agree, the error handling of JSON.dump is bad.

ohler55 commented 1 year ago

Release v3.16.0 with the fix.

ioquatix commented 1 year ago

I'll test it today and close the issue with confirmation that it's working as expected!

ioquatix commented 1 year ago

It looks good to me, thanks!