Repository navigation
Investigate feasibility of enhancing JSON-Java to support BigDecimal and BigInteger #126
Description
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 ...)
Activity
referencing stleary#126 : uses superclass Number instead of specific …
FritzMock commentedon May 20, 2015
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 commentedon May 20, 2015
And this is why Sean is asking for help on code coverage tests :D Thanks for that.
johnjaylward commentedon May 20, 2015
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 commentedon May 20, 2015
Sorry, you are right! I cannot recall, how I came on that wrong track ...
[-]Erroneous JSON from Number[] filled with java.math.BigDecimal[/-][+]Investigate feasibility of enhancing JSON-Java to support BigDecimal and BigInteger [/+]stleary commentedon Jun 12, 2015
To support BigDecimal and BigInteger, what would have to occur in terms of:
Constraints:
douglascrockford commentedon Jun 12, 2015
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.
stleary commentedon Jun 13, 2015
Thanks for clarifying, added a constraint to the issue statement.
stleary commentedon Jun 18, 2015
Investigation results:
API changes:
Changes to existing behavior:
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:
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.
referencing stleary#126 : uses superclass Number instead of specific …
referencing stleary#126 : uses superclass Number instead of specific …