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

Already on GitHub? Sign in to your account

BigDecimal string wrapping in JSON serialization can now be opted-out #6040

Merged
merged 1 commit into from May 2, 2012

Conversation

Projects
None yet
4 participants
Contributor

davout commented Apr 28, 2012

Please see issue #6033

This commit allows one to easily opt-out of the specific BigDecimal JSON serialization (ie. get actual numeric serialization instead of having a string-wrapped number)

I think this should be the default behaviour in a future major release as this breaks the principle of least surprise and assumes that users always have control of the parsing end. It should be made opt-in instead when breaking backwards compatibility is acceptable.

Owner

jeremy commented Apr 29, 2012

How about something like encode_big_decimal_as_string rather than use_standard_json_big_decimal_format?

Owner

rafaelfranca commented Apr 29, 2012

@jeremy if we change this options to be true by default we will close this PR too. #2036 WDYT?

Contributor

josevalim commented Apr 29, 2012

Honestly, if all developers need to do is to override to_json in BigDecimal to get this behavior, I wouldn't even add this option. Because re-opening a class and overriding to_json is the official way anyway to customize JSON behavior.

Contributor

davout commented Apr 29, 2012

@josevalim The "official monkeypatch" doesn't even work on 3.2, see #6033

Contributor

davout commented Apr 29, 2012

@jeremy I copied the use_standard_json_time_format naming, shouldn't we try to keep configuration option names consistent ?

Contributor

davout commented Apr 29, 2012

@rafaelfranca No, because even with this patch the way BDs are serialized is not guaranteed to be quotes-free

Contributor

davout commented Apr 29, 2012

IMHO the way BigDecimal objects are currently serialized as strings is not correct and creates more problems than it tries to solve.

Here's my full reasoning :

The change introduced in c1d7327 states that "a string is the best JSON representation for a BigDecimal". Which is wrong: the JSON specification simply says that a number is : "an integer component that may be prefixed with an optional minus sign, which may be followed by a fraction part and/or an exponent part." it does not say how, and with which precision these should be deserialized.

This behaviour is also based on two other assumptions :

  • The parsing libraries will misbehave and treat these as floats,
  • Users will have control over the parsing end and will be able to post-process string-wrapped BSs

So you can only take advantage of the feature if :

  • you have a parsing library that misbehaves, AND
  • you are not willing/able to fix/report an issue/replace the misbehaving library AND
  • you have control over the parsing end in a way that you can post-process the string

But the price to pay is surprising behaviour when you knowinlgy use BDs for arbitrary precision calculations/storage and you try to talk to JSON APIs, and other cases (see pull request #2036).

So this feature tries to pro-actively and preventively work-around hypothetical parsing bugs in third-party code at the price of breaking the principle of least suprise.

I argue that working around hypothetical third-party bugs is not a reason that's good enough to break the principle of least surprise for all of us who use BDs and talk JSON to third-party APIs.

One might argue that overriding the default is easy, but that's not a valid point for a bunch of reasons :

  • Overriding the default should be required if you want to enable non-standard behaviour, not disable it,
  • Overriding currently requires monkey-patching, which is wrong™

And I'm not just being anal about the monkey-patching part, the recommended official monkey-patch just stopped working as of Rails 3.2, probably because of some lazy/auto loading getting in the way. (See #6033 for a monkey patch that works).

So, what IMHO should be in a future major release is as follows :

  • Rails should serialize BDs as numbers by default in accordance with the JSON spec
  • Parsing libraries should be responsible for their own bugs,
  • String-wrapping could remain a configuration option if it doesn't induce breakage elsewhere, (as in #2036 for example)

Sorry if it feels like a rant, it's not <3

Thoughts welcome!

Contributor

josevalim commented Apr 29, 2012

Given the monkey patch does not work, I am 👍 for this patch. However, I agree with @jeremy that encode_big_decimal_as_string would be a better name for this. :)

@davout thanks for the detailed information!

Contributor

davout commented Apr 29, 2012

Ok, will do, which part of the changelog should I update ?
If unsure how the versioning and branches fit together, I'd like to keep it pretty much as is for a hypothetical 3.x.
But if this only gets into 4.0 I'd also like to make the string-wrapping opt-in instead of opt-out.

Also, if it's relevant, how and where should I put the commented-out configuration setting (I've seen some commented settings in initializers and others in application.rb, not sure where it'd fit best).

Contributor

josevalim commented Apr 29, 2012

Let's discuss the opt-in or opt-out of the feature in another issue, since it is another discussion and we should get more feedback before. Let's leave this pull request for getting the feature and configuration options in (targeting master, aka 4.0)

Contributor

davout commented May 2, 2012

Rebased, updated

@josevalim josevalim added a commit that referenced this pull request May 2, 2012

@josevalim josevalim Merge pull request #6040 from Paymium/issue-6033
BigDecimal string wrapping in JSON serialization can now be opted-out
07438f9

@josevalim josevalim merged commit 07438f9 into rails:master May 2, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment