changes number parsing to use BigDecimal as the backing type

[johnjaylward]

What problem does this code solve?
See issue #441 (use BigDecimal for underlying storage of decimal number types)

Risks
Medium

Changes to the API?
No, but it changes how certain API calls work for decimal/floating-point types work.

for example a JSON document list this:

{ "float":1234 }

would parse like this:

JSONObject jo = new JSONObject("{\"float\":1234}");
jo.get("float"); // used to return a Double, now returns a BigDecimal
jo.getDouble("float"); // still returns the value/object as in the old version (Double)

In general, the raw "get/opt" methods with no type information (like getDouble, getInt, optDouble, etc) are not guaranteed to be useful as the backing types were never guaranteed. However, people may be using them without the knowledge that the backing types are semi-dynamic (i.e. number types have always been backed differently if an Integer was parsed vs a float). So even a "standard" document like this:

{
"items": [
 { "floatValue": 123.4 },
 { "floatValue": 321 }
]
}

Would have a Double stored for the first floatValue and an Integer for the second floatValue before the change. After the change they would be stored as BigDecimal and Integer respectively.

Will this require a new release?
Yes.

Should the documentation be updated?
Yes. we should probably mention that the backing type has changed and how that may affect existing code.

Does it break the unit tests?
Yes, many of the tests depend on the data type stored for numbers. The tests have been updated to reflect the new backing type.

Was any code refactored in this commit?
Yes, changes were made to the number parsing methods for XML and JSONObject (they need to be kept in sync for Android support)

Review status
APPROVED

[stleary]

Breaks a lot of unit tests. Aren't users likely to see the same breakages?

[johnjaylward]

Yes, I'm not sure how impactful that would be though. The APIs that are affected are the Object get/opt and not the typed ones (i.e. getString and getDouble still work the same way). The only reason the raw ones are affected is because the type changed, not the value. A programmer calling the Object get/opt methods would hopefully not be depending on a certain type like we do in the test cases.

[stleary]

Is there any way to restrict this so that just values that are added as BigDecimal get the new behavior?

[johnjaylward]

Values added as big decimal are already handled as big decimal. This is strictly to change how the parsing Of JSON text works, not how JSONObject.{put,get*,opt*} work.

[PeerHeijnen]

The behavior for this conversion has changed from 20180813 to 20190722.

System.out.println( new JSONObject( new JSONObject().put( "test", new BigDecimal( "348.8" ) ).toString() ).getBigDecimal( "test" ) );

Output for 20180813: 384.8

Output for 20190722: 348.80000000000001136868377216160297393798828125

Instead of chaging this behavior by default, would it be possible to choose a method for parsing that is suitable to the application?

[johnjaylward]

@PeerHeijnen, the main issue here is that you do the following steps:

1. Create a JSONObject with a single BigDecimal field
2. Convert the JSONObject to a JSON Text
3. Convert the JSON Text back to a JSONObject. The parser will read the number as a double
4. Read a double value as a BigDecimal.

The conversion from double to BigDecimal uses the exact double value which is the output you see now. If you set the BigDecimal scale to 1 or 2, you would lose the extra info you don't want from the double.

This PR is meant to fix the round tripping that causes the issue you are seeing. Currently in master the backing store is always an int, long, or double which makes round trips more difficult as the double is inaccurate as a storage means for decimal floating point.

Another option would be to add another getter with a scale parameter so you can configure the scale you wish to read right away as one call.

BigDecimal bd = JSONObject.getBigDecimal("test", 4);
System.out.println(bd); // would print 348.8000 abbreviated to 348.8

Using today's API, this would look like:

BigDecimal bd = JSONObject.getBigDecimal("test").setScale(4, RoundingMode.HALF_EVEN);
System.out.println(bd); // would print 348.8000 abbreviated to 348.8

As a side note, it's usually recommended to set a scale on all your BigDecimal uses to prevent issues like this popping up.

[PeerHeijnen]

Sure, it was not ment as criticism. I just noted the change in behavior, be it intentional or not.

The code I shared was not actual production code, just a short version of how the conversion happens as you explained.

I was just wondering whether people may experience side effect from the PR. For me, it solves my problems I have now perfectly, so thanks for the effort! :)

[johnjaylward]

rebased to support the new project structure

[stleary]

Starting 3 day comment window

[johnjaylward]

@TheMatthew Do you still have the benchmark tests you were running for the other improvements we made? I was wondering if you could spin them up for this change... I should have probably thought of doing that in the last 1.5 years this has been open, but it just occurred to me now