Oj: JSON.parse vs Oj.load BigDecimal treatment

[mediafinger]

Hello, and thank you for maintaining Oj!

We are struggling a bit to parse JSON values with high precision correctly as BigDecimal (instead of Float).

• Rails 6.0.3.4
• Oj 3.10.15

_our initializer_

require "oj"

# none of these 4 settings seems to change the behavior
Oj::Rails.set_encoder
Oj::Rails.set_decoder
Oj::Rails.optimize
Oj::Rails.mimic_JSON

Oj.default_options = {
  trace: true, # debugging
  mode: :compat,
  allow_blank: true,
  bigdecimal_as_decimal: false, # dump BigDecimal as a String
  bigdecimal_load: :bigdecimal, # convert all decimal numbers to BigDecimal
  empty_string: false,
  second_precision: 6,
  time_format: :ruby,
}

When using Oj.load it works as expected. We were under the impression Oj overloads JSON.parse, which should therefore behave the same, but it does not:

json = %Q({ "amount": 100000000000000.01 })


JSON.parse(json)

#0:     compat.c: 72:Oj:}: start_hash
#0:     compat.c:157:Oj:-:   set_number Float
#0:     compat.c: 98:Oj:{: hash_end Hash
#0:     strict.c: 36:Oj:-: add_value Hash

# => { "amount" => 100000000000000.02 } # losing precision

JSON.parse(json)["amount"].class

# => Float < Numeric


Oj.load(json)

#0:     compat.c: 72:Oj:}: start_hash
#0:     compat.c:157:Oj:-:   set_number BigDecimal
#0:     compat.c: 98:Oj:{: hash_end Hash
#0:     strict.c: 36:Oj:-: add_value Hash

# => { "amount" => 100000000000000.01 }

Oj.load(json)["amount"].class

# => BigDecimal < Numeric # correctly treating the number as a BigDecimal

So our questions:

1. Is this a config issue, a bug, or do we mis-understand _"drop-in replacement"_?
2. Are we supposed to replace every explicit call to JSON.parse by Oj.load?
3. Is there any way to achieve that JSON.parse behaves the same as Oj.load and respects our configuration to treat decimal numbers as BigDecimals - without passing this parameter explicitly to every call?

[ohler55]

I haven't checked the JSON gem or Rails compatibility recently so maybe they have changed. In previous versions there was no options for either to convert a decimal to a BigDecimal so those options for Oj do not apply to the rails or compat modes. If that has changed then Oj will need to be updated but I did check the JSON gem version 2.3.1 and it still returns a Float. Can you tell me what options are being set to jet the JSON gem to return a BigDecimal?

[mediafinger]

Sure :-)

json = %Q({ "amount": 100000000000000.01 })

JSON.parse(json, decimal_class: BigDecimal)

# => { "amount" => 100000000000000.01 } # precise result, treating value as BigDecimal

[ohler55]

Very helpful, thanks. I'll get that option linked to the Oj option and open it up for use with compat mode.

[ohler55]

I pushed a branch called decimal-class that I supports decimal_class. It needs unit tests but if you want to give it a try that would be awesome.

[mediafinger]

Hey Peter, thanks for the fast reaction. I've tried the branch in our project, added: decimal_class: BigDecimal, to the default_options, but unfortunately the behavior did not change. JSON.parse still treats the number as Float.

[ohler55]

The decimal_class can be used with Oj.load or JSON.parse but for the default options the previous bigdecimal_load should be used. I see that I had not set up the default options yet though so don't bother checking that yet. Will take care of that tonight.

[ohler55]

Pushed an update. Setting :bigdecimal_load to :bigdecimal in default options will now take effect in JSON.parse.

[mediafinger]

Great 😃 with branch decimal-class@b86bdcd it now works as expected.

json = %Q({ "amount": 100000000000000.01 })

JSON.parse(json)
# => { "amount" => 100000000000000.01 }

JSON.parse(json)["amount"].class
# => BigDecimal < Numeric

This also seems to be the default behavior now, only when setting:

Oj.default_options = {
  decimal_class: Float,
  bigdecimal_load: :float # using one of the options is sufficient
}

I could provoke the rounding error / loss of precision / handling of the number as float. In all other cases it returned BigDecimals now. While I think this is the proper way of handling it, I am not sure, if this could be a breaking change.

I did not test with conflicting settings - but that is a weird case and explaining this in the README should be enough.

Thank you for the quick fix!

[ohler55]

I'm claiming it as a bug fix as I missed the decimal_class option. I'm not sure how you found it since it does not appear in the documentation. Thanks though. I have one more bug fix for rails mode and then I'll release unless you need this right away.

[mediafinger]

Thanks, we have our workaround at the moment. That we needed it, surprised me though. And as it does not encompass all possible places, we will be happy to update to the new version soon.

I didn't find this option myself, my colleague @padde used it in our workaround. He was researching a fix for this exact issue (before we added Oj to the project) and must have found it in the interwebs.

I only found these PRs later on: https://github.com/flori/json/issues/219 which points to https://github.com/flori/json/issues/223 where the decimal_class option is implemented.

[padde]

Hey, not sure how I found it, to be honest probably a case of poking around until it worked.

I also found https://github.com/ruby/ruby/pull/2630 where somebody wanted to add the option to the Ruby docs. They were directed to flori/json, as snapshots of this are added to ruby core apparently and main development takes place there. It looks like the docs were never added there though, at least not on the JSON.parse method options. There is a notion of the option in the underlying parser documentation comments, not sure whether these end up in generated docs at all though.

[ohler55]

Well, thanks to you guys the feature will be in Oj.

[ohler55]

Release 3.10.17 fixes the issue.

[jeffblake]

With the 3.10.17 release I was seeing a bunch of errors pop up in ActionCable and Sidekiq, where strings were showing up in the place of floats, in regards to time, e.g. Time.current.to_f. Still diagnosing this, but a warning to someone out there. I've reverted back to 3.10.16

[ohler55]

Keep me posted. If there is a bug to fix I'll get right on it.

[mediafinger]

I don't think it is a bug. But as I mentioned earlier, it is a breaking change. I would advise not to set the BigDecimal conversion as default, but stick to Float for it.

[jeffblake]

I didn't change any code. My Oj config is pretty basic:

require 'active_support/core_ext'
require 'active_support/json'
require 'oj'

Oj.optimize_rails

ActiveSupport::JSON::Encoding.time_precision = 6

And here are two errors triggered within Sidekiq and Rails. Also seeing similar 'expected Float, got String' errors in my application code.

I know this doesn't help much... all I know at the moment is 3.10.17 caused these two errors without changing anything.

[ohler55]

Interesting. It looks like both we counting on large numbers being loaded as strings or floats. Let me know if you come up with anything needed to give both bigdecimal and also support for sidekick.

[jeffblake]

The problem occurs when Time.current.to_f returns a 17 digit float as opposed to a 16 digit float. When it's 17 digits, JSON.parse returns a BigDecimal, otherwise it returns a Float. And I imagine when BigDecimals get serialized into job arguments in Sidekiq (and whatever Rails is doing) it ends up serializing the Big D's as strings (to_s) on it and breaks those libraries (intermittently). I haven't looked too deep into what is going on in this issue, maybe this new approach is correct, however it seems to be a breaking change for both Sidekiq and Rails.

[ohler55]

What we are running into is the precision limits on a float. For doubles precision varies from 15 to 17 places. The issue is that is the parsing uses too many digits before assuming a BigDecimal then floats will not be accurate and if too few then you get a BigDecimal when a float is expected. There are two competing requirements. One is for BigDecimal in some cases and some for floats for the same number. If anyone has some ideas on how to address this let discuss them.

[ohler55]

Since the Oj defaults didn't change I wonder is one of the packages is modifying the defaults.

[mediafinger]

Since the Oj defaults didn't change I wonder is one of the packages is modifying the defaults.

I think you changed the defaults from Float (an number) to BigDecimal (a string):

https://github.com/ohler55/oj/issues/628#issuecomment-742485671

[ohler55]

That was unintentional. The place for the defaults did not change so there must be some other interaction I missed. I'll find it and get it back to the way it was.

[geoffharcourt]

In case this helps this investigation at all, while trying to upgrade we found that framework code was unexpectedly getting passed BigDecimals returned after updating. The whole flow is internal to Capybara so I'm not sure exactly how it's happening, but the only change we made was the 3.10.16 -> 3.10.17 bump.

Thanks very much for all your efforts here, this gem is fantastic.

[ohler55]

Thanks that might help. If you can recreate the issue easily enough would you be willing to run a debug version so I can see where it is being set?

[geoffharcourt]

Untangling this from our application is difficult, but it appears to be parsing x/y coordinates where there's a large-precision decimal number. I'm trying to see if I can isolate these parts out into something we can share publicly.

I actually think this issue is a Ruby BigDecimal that's being handled differently after being parsed into JSON, which might be the opposite of the issue cited here.

[ohler55]

It would help but I can understand it might not be easy.

[ohler55]

After going over the code it looks like one possible reason for the seemingly change in default is that previously every call to JSON parse forced the BigDecimal load to be float. Now the default option is used but that default option is set to float when setting the mimic mode. I wonder if the default is getting changed somewhere. Any chance that is happening?

[geoffharcourt]

@ohler55 I think that might be correct.

I'm still working on extracting, but the points where this seems to be failing for us are here:
https://github.com/twalpole/apparition/blob/ba431173024c56354a86ef8369c3493e8a1a39bb/lib/capybara/apparition/driver/chrome_client.rb

This is a driver for running browser tests. This piece of code is running some JavaScript in the browser, then passing JSON (as a string) from the browser to the driver for parsing. (It ends up being used to figure out what x/y coordinates on the screen need to be "clicked" by the mouse to interact with an element like a button or link, etc.) As of 3.10.16 this behaved as usual, but in 3.10.17 and 3.10.18 it blows up because the data type of the x/y coordinates has changed. From the Chrome CDP docs, it appears that these coordinate simply need to be numbers (not any specific type of number), but they are now coming through as strings:

https://chromedevtools.github.io/devtools-protocol/tot/Input/#method-dispatchMouseEvent

I'm going to hopefully have a few minutes to build a small app with a test to recreate this, but running through the failure trace in our CI and looking at the code for this gem, these JSON.parse calls look like they are the ones that have been affected.

[ohler55]

Let me know what the outcome of your tests are. One way or another we'll get this to work.

[geoffharcourt]

Update here: we moved from Apparition to use a Ferrum-based Chrome driver (Cuprite, different library but very similar communication style) and we're seeing the same kinds of errors around coordinates despite completely different code in the library. Hopefully helps somewhat in that it's not something specific to one library.

We have an R&D day coming up and I'm going to try building a very small PoC for this issue.

[ohler55]

A PoC would be very helpful.

[zarqman]

@ohler55, some feedback on this change... In short, this bit us too and we've had to revert to 3.10.16 for the moment.

With bigdecimal_load: :bigdecimal, we're seeing 2-3 issues with mongoid:

1) mongoid doesn't handle BigDecimals well for Time fields (eg: field :updated_at, type: Time). Arguably it's a long-standing bug in mongoid, but this change in oj triggers it.

2) We're also seeing issues with mongoid very similar to the described sidekiq issues, where Time#to_f sometimes returns 17 digits which causes mongoid to wrongly convert Time objects into Strings.

3) .17+ also results in mongoid persisting a couple BigDecimals as Strings elsewhere (not Time fields), but that might be a limitation of those particular fields (which can hold multiple types and thus can't force the type on reload).

Reviewing our codebase, I think we've been unknowningly relying on Oj.load parsing as :bigdecimal, while simultaneously expecting Rails and JSON.parse to parse as :float. The key issue seems to be that other libraries and gems also rely on JSON.parse consistently producing a float.

I submit that the prior split behavior might actually have been desirable, despite it seeming at first glance like a bug that should be fixed. With 3.10.17+, given the interaction with external gems (especially with Time objects, but not just those), it seems Oj.mimic_JSON effectively cannot be used in combination with bigdecimal_load: :bigdecimal (via default_options anyway). Instead, the default must be set to :float for compatibility with various gems and we need to selectively override to :bigdecimal on each call to parse/load. Arguably this is correct, but it's a bummer to have to make such a change on a patch release.

My suggestion would be to restore the previous behavior and add a separate version of bigdecimal_load that only affects mimic-mode/JSON.parse and defaults to :float. This way folks like the OP can selectively have universal :bigdecimal loading, while the default behavior would preserve proper operation with sidekiq, mongoid, capybara, etc.

If you're committed to keeping the "fixed" behavior, then I'd encourage adding a big warning to both the changelog and regular docs about using bigdecimal_load: :bigdecimal in combination with Oj.mimic_JSON.

Whatever you decide, thanks for your years of work on oj. It makes a very big performance difference!

[ohler55]

That is a great explanation. I've like to keep Oj as compatible as possible so work with me on this and we can get it resolved.

Let me see if I can rephrase to make sure I understand the issue correctly.

1. The JSON gem always returns a float unless the secret :decimal_class option is used.
2. By reusing the Oj :bigdecimal_load that JSON gem option was being set when not desired.
3. A fix would be to not link the Oj :bigdecimal_load to the JSON :decimal_class but instead have a separate Oj option to represent that option.

Correct?

[zarqman]

Yes, I believe that's correct.

[ohler55]

I'll get started on that then.

[ohler55]

Branch "compact-decimal" is ready for a bit of testing if you would like to run it though the paces.

[geoffharcourt]

@ohler55 the compat-decimal branch resolved the failures in our test suite. Thanks for your efforts on this despite my inability to provide much telemetry!

[ohler55]

Between all the folks that chimed in it was enough to sold the issue. Anyone else have results to share? If not I'll release in the next day or two.

[zarqman]

@ohler55, our tests also pass with compat-decimal. Yay!