Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bigdecimal_as_decimal doesn't work in compat mode #632

Closed
troex opened this issue Dec 24, 2020 · 18 comments
Closed

bigdecimal_as_decimal doesn't work in compat mode #632

troex opened this issue Dec 24, 2020 · 18 comments

Comments

@troex
Copy link

troex commented Dec 24, 2020

As I understood from Modes Matrix compat mode should support setting bigdecimal_as_decimal but it looks like not:

Oj.default_options = { mode: :compat, bigdecimal_as_decimal: true }
puts Oj.dump({time: Time.now, c: BigDecimal('1') / BigDecimal('3')})
# => {"time":"2020-12-24 05:56:55 +0200","c":"0.333333333333333333e0"}

However I expect that decimal be dumped without quotes, like it does in custom mode:

Oj.default_options = { mode: :custom, bigdecimal_as_decimal: true }
puts Oj.dump({time: Time.now, c: BigDecimal('1') / BigDecimal('3')})
# {"time":1608782178.626367000,"c":0.333333333333333333e0}

I basically want :compat mode with decimals dumped as decimals

@ohler55
Copy link
Owner

ohler55 commented Dec 24, 2020

Looks like a bug. I'll get it fixed assuming it does differ from the JSON gem. If not we may need to discuss other options.

@troex
Copy link
Author

troex commented Dec 24, 2020

@ohler55 I think :compat is currently matching JSON gem but since JSON doesn't have bigdecimal_as_decimal option or alternative (or I'm not aware of) I was looking into oj since we need to dump bigdecimals in scientific notation for couple of reasons.

I think for now my option option is to go with :custom and align it to be in format I need, but experimenting with options is a bit scary because we have a huge system that depends on JSON, and it works fine with Oj :compat mode, the only additional piece I want is bigdecimals

@ohler55
Copy link
Owner

ohler55 commented Dec 24, 2020

The JSON gem does have a secret option :decimal_class but I might only be for loading. I can't really make compat mode do something that the JSON gem doesn't;t do as that would break compatibility but you can set up custom mode to be just like compat mode and then turn on the bigdecimal mode you want.

Would it be helpful if I listed the options needed to configure custom mode to be like compat?

@troex
Copy link
Author

troex commented Dec 25, 2020

@ohler55 yes that would be very useful for my case!

I'm already aware of :decimal_class in JSON also tried to outline my case in json gem issue too flori/json#462 please check it, the point is that it's possible to control how to parse scientific notation but not how to produce/dump it.

What about Oj, in default mode it works:

decimal = BigDecimal('1') / BigDecimal('3')
hash = { bc: decimal }

oj_dump_default = Oj.dump(hash) # {":bc":0.333333333333333333e0}
Oj.load(oj_dump_default) == hash # true

# test that `:bigdecimal_as_decimal` works
Oj.dump(hash, bigdecimal_as_decimal: false) # {":bc":"0.333333333333333333e0"}

In the last test it produces string instead of scientific notation as it should with bigdecimal_as_decimal turned off

In compat mode:

oj_dump_compat = Oj.dump(hash, mode: :compat, bigdecimal_as_decimal: true) # {"bc":"0.333333333333333333e0"}
Oj.load(oj_dump_compat, mode: :compat, bigdecimal_as_decimal: true) # {"bc"=>"0.333333333333333333e0"}
Oj.load(oj_dump_compat, mode: :compat, bigdecimal_as_decimal: true) == hash # false

This doesn't work because :bigdecimal_as_decimal option doesn't work (because it didn't turn ON), maybe it shouldn't work by design but then in the matrix table here https://github.com/ohler55/oj/blame/develop/pages/Modes.md#L97 it should not say that :bigdecimal_as_decimal is supported option

@ohler55
Copy link
Owner

ohler55 commented Dec 25, 2020

oops, that was a mistake. It should have been on the bigdecimal_load. I'll fix that but I think the solution for you will be custom mode.

@ohler55
Copy link
Owner

ohler55 commented Dec 26, 2020

V3.10.18 corrects the documentation on bigdecimal. A new constant was also added that can be used to set the default options to be custom mode but matching compat mode. It is Oj::CUSTOM_MIMIC_JSON_OPTIONS. Should be pretty much the same with possibly some deviations with special case error types.

@troex
Copy link
Author

troex commented Dec 26, 2020

Thanks a lot! I already started testing custom mode in our staging environment, CUSTOM_MIMIC_JSON_OPTIONS hash will help a lot, I tried to find these in ruby code but looks like most of it sits in C code logic.

In our system we use JSON to exchange information between different internal APIs but at the same time we use it for JS front-end so we can't use object mode or "additions" from JSON, I have been trying to fight bigdecimals for a long time converting them into floats but our DB calculations usually return bigdecimals so it was pain, now I look forward to move everything to bigdecimal and forget about fighting formats. Thanks once again for quick response!

@ohler55
Copy link
Owner

ohler55 commented Dec 26, 2020

Hope it works out well for you. If not let me know.

@troex
Copy link
Author

troex commented Jan 5, 2021

I think I figured out working scheme for us:

Oj.default_options = Oj::CUSTOM_MIMIC_JSON_OPTIONS.merge(
  { bigdecimal_as_decimal: true, use_to_json: false }
)

use_to_json was messing with decimals so bigdecimal_as_decimal had no effect when use_to_json: true and I suggest it might be true for other options as well.

@ohler55
Copy link
Owner

ohler55 commented Jan 5, 2021

That could be the case since setting the :use_to_json will use the target objects code instead of Oj's code. Depending on the object class that may impact performance but that shouldn't be a surprise.

@ohler55
Copy link
Owner

ohler55 commented Jan 13, 2021

Does the latest set of changes for compat mode solve your issue?

@troex
Copy link
Author

troex commented Jan 13, 2021

@ohler55 YES thanks! I've rolled out the above config as drop-in replacement for JSON gem in production and it fixed issues for us with decimals since we depend on them a lot. Also based on our metrics with JSON gem our system was spending ~5500 second of CPU time on just JSON encoding daily, now with OJ it dropped to 800-1000 seconds daily (these figures are not exact json/oj metrics comparison because we also did some other optimizations as well but OJ definitely played big role there)

I really think JSON gem is broken based on https://json-schema.org/understanding-json-schema/reference/numeric.html#number spec where it describes "Exponential notation also works". I've found other threads online where people complain about JSON gem that it doesn't support exporting decimals properly and it breaks compatibility with JSON based databases and storages in AWS, our case was similar but it was breaking other systems compatibility.

Also it could break JavaScript in cases for example when you need to manipulate JSON parsed number like:
image

So having option to control this is really useful for advanced cases! I wonder why not that many complain about it yet.

UPDATE I didn't test compat mode yet, the above described worked on 3.10.18

@ohler55
Copy link
Owner

ohler55 commented Jan 13, 2021

Let me know when you do test. If it looks good this issue can be closed. Remember compat mode tries to mimic the JSON gem so compat mode may not be what you want. Custom mode may be more to your liking.

@troex
Copy link
Author

troex commented Jan 13, 2021

ok I'll test - I don't promise I could test it this week maybe next one, have pretty big backlog right now, but for us it's more critical to be JSON spec compatible than json gem in a long term.

@ohler55
Copy link
Owner

ohler55 commented Jan 13, 2021

No rush here. Take your time and let me know if you need anything else.

@troex
Copy link
Author

troex commented Mar 12, 2021

@ohler55 hi again, I'm not sure it's related but I came across this issue when figuring what's wrong with Sinatra json encoder which I set like: set :json_encoder, Oj so it does pick Oj but doesn't pick default Oj options. A little digging showed that resolving encoder happens here https://github.com/sinatra/sinatra/blob/ed2add378576ffa0d5225fd541e6a974b2f054a4/sinatra-contrib/lib/sinatra/json.rb#L111

btw there is Sinatra doc for it http://sinatrarb.com/contrib/json

and it probes for two methods encode and generate since Oj only has last one - it's used. So that leads us to:

# configure my default options
Oj.default_options = Oj::CUSTOM_MIMIC_JSON_OPTIONS.merge(
  { bigdecimal_as_decimal: true, use_to_json: false }
)

Oj.generate
# "null"

Oj.dump
# ArgumentError: wrong number of arguments (0 for 1).
# from (pry):2:in `dump'

var = { c: BigDecimal('1') / BigDecimal('3') }
# {:c=>0.333333333333333333e0}

puts Oj.generate var
# {"c":"0.333333333333333333e0"}

puts Oj.dump var
# {"c":0.333333333333333333e0}

I'm pretty sure we should not use Oj.generate since it's not documented but I feel like other meta-gems that use Oj as adapter or similar could be having this issue thou most people probably don't notice it and are not that sensible as in my case where it causes critical errors.

I've been confused because JSON do have both methods dump and generate I'm not sure they are the same but it looks like other authors relay on these methods then they think of JSON compatible gem.

@troex
Copy link
Author

troex commented Mar 12, 2021

btw similar case with MultiJson, it looks like they mess with Oj default options:

Oj.default_options = Oj::CUSTOM_MIMIC_JSON_OPTIONS.merge(
  { bigdecimal_as_decimal: true, use_to_json: false }
)
var = { c: BigDecimal('1') / BigDecimal('3') }

MultiJson.adapter
# MultiJson::Adapters::Oj

puts MultiJson.encode var
# {"c":"0.333333333333333333e0"}

MultiJson.dump_options = Oj.default_options

puts MultiJson.encode var
# {"c":0.333333333333333333e0}

Definitely not an issue of Oj, just leaving it here in case if anyone stumbles on it

@ohler55
Copy link
Owner

ohler55 commented Mar 13, 2021

The generate method is for compatibility with the JSON which forces the precision that gem uses.

Thanks for the notes.

@ohler55 ohler55 closed this as completed Jun 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants