Investigate feasibility of enhancing JSON-Java to support BigDecimal and BigInteger

[FritzMock]

Following test-code will not produce expected results, the array-elements of type BigDecimal will be incorrectly quoted. I guess other Number types like BigInteger will fail too.

Number[] foo = new Number[5];
foo[0] = BigDecimal.ZERO;
foo[1] = new Integer( 1 );
foo[2] = new BigDecimal( 1.2 );
foo[3] = new Integer( 3 );
foo[4] = new BigDecimal( 4 );
JSONStringer s = new JSONStringer();
s.object();
s.key( "bar" );
s.value( foo );
s.endObject();
System.out.println( s.toString() );

I located the problem in the JSONObject.wrap(Object object) method line 1547 (ff) where the half dozen "instanceof " comparision should be replaced by a single "intanceof Number".

(The original code where i stumbled into this bug produces "highcharts" from various oracle database queries and of course, highcharts fails to produce charts from strings. Using Doubles is also no option ...)

[FritzMock]

One more comment about the condition in question, NULL is an instanceof JSONObject:

object instanceof JSONObject || [...] || NULL.equals(object)

If the instanceof is false, the call to the equals method just adds pointless computation overhead.
(Calling equals first will also -most certainly- not provide any performance benefit.)

[johnjaylward]

And this is why Sean is asking for help on code coverage tests :D Thanks for that.

[johnjaylward]

I just checked the source and Null is not an instance of JSONObject. It is an Internal class of JSONObject. The instanceof check will fail if object is of type JSONObject.Null.

[FritzMock]

Sorry, you are right! I cannot recall, how I came on that wrong track ...

[stleary]

To support BigDecimal and BigInteger, what would have to occur in terms of:

• API changes
• Behavior changes
• Files touched
• Effects on existing applications
• Interoperability
• Unit test changes

Constraints:

• Must not break or change the output of existing applications

[douglascrockford]

The package was written before Big was invented. I am not opposed to adding Big
support if we can absolutely guarantee that there is no possibility that any
existing application will be broken or inconvenienced by the change.

On June 12, 2015 at 10:28 AM Sean Leary notifications@github.com wrote:

To support BigDecimal and BigInteger, what would have to occur in terms of:

• API changes
• Behavior changes
• Files touched
• Effects on existing applications
• Interoperability
• Unit test changes

---

Reply to this email directly or view it on GitHub:
#126 (comment)

[stleary]

Thanks for clarifying, added a constraint to the issue statement.

[stleary]

Investigation results:

• JSONObject(BigInteger|BigDecimal) not meaningful since these types are not structured as beans.
• jsonObject.get|optBigInteger(String), jsonObject.get|optBigDecimal(String) should be implemented.
• jsonObject.increment(String) throws an exception when incrementing big numbers. It should support BigInteger, BigDecimal
• jsonObject.put(String, BigInteger|BigDecimal) works correctly, serializes to number.
• JSONObject.numberToString(BigInteger|BigDecimal) works correctly.
• JSONObject.stringToValue(BigInteger|BigDecimal) is used when deserializing. It does not account for BigInteger|BigDecimal. It is possible users have come to expect this behavior. Adding support would change the way deserialization works.
• JSONObject.wrap(BigInteger|BigDecimal) turns the value into a string. It is public static and is used to initialize a JSONObject or JSONArray from an array, a map, or a bean. To be consistent, wrap() should return BigInteger|BigDecimal as an object. Currently serializes to String.
• JSONArray.get|optBigInteger(int), JSONArray.get|optBigDecimal(int) should be implemented.

API changes:

• No changes for ctor for JSONObject(BigInteger|BigDecimal)
• Implement the missing get() and opt() methods. increment() should be updated.

Changes to existing behavior:

• wrap() will return BigInteger|BigDecimal as object instead of String
• increment() will no longer throw an exception when incrementing a BigDecimal|BigInteger.

While working on the unit tests, it became apparent that the behavior of some user applications is
going to change. Normally this would prevent the update from going forward. But if the underlying
bug in wrap() was found in a dev shop, I think that it would be flagged as a moderate severity bug,
and it would be a given that the bug had to be fixed. The only question would be to determine the
best fix. To align wrap(big number) behavior with other number types seems to me to be the best
solution. So, despite the behavior change, I think in this situation, the change is justified and
recommend committing the code.

Files touched:
JSONObject.java
JSONArray.java

Effect on existing apps:

• Maps, arrays, and objects which contain BigInteger|BigDecimal will now store those values as the object, not the string. This changes serialization, which was inconsistent between values added
  by put() and values added inside a map, array, or Bean. Users who call wrap() directly for BigInteger|BigDecimal will see a behavior change.

Interoperability:
There will be some effect on interoperability. Serialization is changed. It was always possible to store BigInteger|BigDecimal via put(String, Object) and serialize them as numbers,
or store them as strings via ctor with map, array, or Bean parameter which contains a big number. The change will make serialization consistent - big numbers will be serialized to numbers, not strings.

Unit test changes:
JSONObjectTest bigNumberOperations() tracks actual behavior, and will be modified to test the new behavior after this code is committed. There are a few other asserts in this file that will need to be updated.

Next steps:
The proposed changes are on the big-numbers branch. A pull request will be created and left up for some time for comments. Please let me know if you notice anything I missed, or miss-stated.

[Updated 20 June 2015] Removed possible changes to ctor, updated wrap(), added opt() and increment() method discussion, added next steps.

[Updated 6 Jul 2015] Added support on the JSONArray side, which I had earlier missed, and updated the comments to the behavior change section. Will leave this up a few more days in case of comments
and then proceed with the next step.