-
-
Notifications
You must be signed in to change notification settings - Fork 249
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
JSON.parse vs Oj.load BigDecimal treatment #628
Comments
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? |
Sure :-) json = %Q({ "amount": 100000000000000.01 })
JSON.parse(json, decimal_class: BigDecimal)
# => { "amount" => 100000000000000.01 } # precise result, treating value as BigDecimal |
Very helpful, thanks. I'll get that option linked to the Oj option and open it up for use with compat mode. |
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. |
Hey Peter, thanks for the fast reaction. I've tried the branch in our project, added: |
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. |
Pushed an update. Setting :bigdecimal_load to :bigdecimal in default options will now take effect in JSON.parse. |
Great 😃 with branch 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! |
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. |
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: flori/json#219 which points to flori/json#223 where the |
Hey, not sure how I found it, to be honest probably a case of poking around until it worked. I also found ruby/ruby#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 |
Well, thanks to you guys the feature will be in Oj. |
Release 3.10.17 fixes the issue. |
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. |
Keep me posted. If there is a bug to fix I'll get right on it. |
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. |
I didn't change any code. My Oj config is pretty basic:
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. |
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. |
The problem occurs when |
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. |
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): |
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. |
In case this helps this investigation at all, while trying to upgrade we found that framework code was unexpectedly getting passed Thanks very much for all your efforts here, this gem is fantastic. |
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? |
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 |
It would help but I can understand it might not be easy. |
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? |
@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: 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 |
Let me know what the outcome of your tests are. One way or another we'll get this to work. |
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. |
A PoC would be very helpful. |
@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
Reviewing our codebase, I think we've been unknowningly relying on 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 My suggestion would be to restore the previous behavior and add a separate version of 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 Whatever you decide, thanks for your years of work on oj. It makes a very big performance difference! |
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.
Correct? |
Yes, I believe that's correct. |
I'll get started on that then. |
Branch "compact-decimal" is ready for a bit of testing if you would like to run it though the paces. |
@ohler55 the |
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. |
@ohler55, our tests also pass with |
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).
our initializer
When using
Oj.load
it works as expected. We were under the impression Oj overloadsJSON.parse
, which should therefore behave the same, but it does not:So our questions:
JSON.parse
byOj.load
?JSON.parse
behaves the same asOj.load
and respects our configuration to treat decimal numbers as BigDecimals - without passing this parameter explicitly to every call?The text was updated successfully, but these errors were encountered: