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

[davout]

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.

[jeremy]

How about something like encode_big_decimal_as_string rather than use_standard_json_big_decimal_format?

[rafaelfranca]

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

[josevalim]

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.

[davout]

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

[davout]

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

[davout]

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

[davout]

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 TagHelper creates invalid data attributes when value is a BigDecimal #2036 for example)

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

Thoughts welcome!

[josevalim]

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!

[davout]

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).

[josevalim]

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)

[davout]

Rebased, updated