Simplify Number comparison during wrap

[johnjaylward]

What problem does this code solve?
Simplifies the comparison in the wrap method to only compare against the Parent Class Number instead of individual Number sub types. This should speed up code as well as allow for other number types to remain unwrapped like AtomicInteger. Classes like AtomicInteger used to be wrapped into a JSONObject and treated like a Bean instead of a number.

Risks
Low risk. Possibly a bug fix. One caveat is that if the AtomicInteger is updated after insert into a JSONObject or JSONArray, but before the JSON is serialized, then the updated value is written, not the original.

AtomicInteger ai = new AtomicInteger(1);
JSONObject jo = new JSONObject();
jo.put("value", ai); // value is 1
ai.getAndIncrement();

assert (new JSONObject(jo.toString())).optInt("value") == 2
    : "Value should have been 2!";

This may lead to unexpected results, however, users of AtomicInteger should already be aware of this and know the risks.

Currently types like java.util.concurrent.atomic.AtomicInteger will be converted to either string or a number inconsistently depending on how it's inserted into a JSONObject or JSONArray.

Changes to the API?
No, but there are functional changes

Will this require a new release?
Can be rolled into the next release, pending comments.

Should the documentation be updated?
Possibly. Maybe just updated javadocs. Will wait on comments before updating.

Does it break the unit tests?
No, new unit tests could possibly be added though.

[johnjaylward]

Updated the "problem solved" section

[stleary]

What about the case where the wrapped object is a user-defined Number? For example,

public class MyNumber extends Number {
    public Number getNumber() { return 42; }
}
public class MyNumberContainer {
    public Number getMyNumber() { return new MyNumber(); }
}
JSONObject jsonObject = new JSONObject(new MyNumberContainer());
String s = jsonObject.toString();

s before change:
{"myNumber":{"number":42}}

s after change:
{"myNumber":org.json.junit.MyNumber@3fd7a715}

This might happen in other cases as well.

[johnjaylward]

looking at tutorials for overriding the Number class (https://docs.oracle.com/javase/tutorial/java/data/numberclasses.html and https://docs.oracle.com/javase/tutorial/java/data/numberformat.html) it looks like the number formaters expect you to provide basic functionality such as overriding toString and equals. Java just doesn't enforce this since it already provides default implementations for those methods. In general all numbers are also expected to implement a Comparable interface as well, but that is also not enforced by the java language.

I'm not sure I'd expect someone who is creating their own number subclass to not override the toString method. I'd also expect them to implement the proper comparable equals and hashcode methods.

[johnjaylward]

hmm, yeah, looking at a Fraction class I created some years ago, I ended up doing all those items:

• Implemented Comparable
• Overrode hashcode and equals
• Overrode toString

I wouldn't have been able to have a usable Fraction without each of those components.

[johnjaylward]

ok, I see the error of this now.

Fraction is a great example. For instance if we are outputting the fraction using my toString override, we'd see something like this:

{
"myFraction": 4/5
}

which is invalid JSON. If we do want to use numbers generically, then we need to verify that their toString output matches the JSON spec before writing it. If it does not match, then what should we do? My thought is to fall back on string quoting (the existing method), so my Fraction would look like this:

{
"myFraction": "4/5"
}

If someone implemented an Imaginary number class, then the output would also probably fall back to string output (i.e. "5i").

[johnjaylward]

I will write some tests against the current master to verify functionality of certain existing features first before exploring further on this PR.

I'll include your case for the custom number (but with a toString method) as well as a very basic fraction class to see what the existing output is and then we can go from there.

[stleary]

We seem to have a minor refactoring that has exposed a bug in Number processing, is that correct? If so, perhaps we should fix the Number bug separately from the refactoring.

[johnjaylward]

I can file a separate PR for the fix if you like. Are you OK with the fix I came up with?

• Using a regex to verify the number
• If the regex matches, use the output as a number
• If the regex does not match, output as a string.

I'm not sure I like using a regex to verify every number, but I'm not sure of a faster way to check. We could use the BigDecimal constructor to parse it, but that is more lenient than the regex.

[stleary]

Yes, but please include a concise statement describing the bug, to make sure we are on the same page and for others reading the pull request. Prefer BigDecimal constructor over regex, understood that it is more lenient. Not concerned about speed at this time.

[stleary]

Are you planning to replace the regex?

[johnjaylward]

I'll re-look at this after we merge #274. I think that one would take precedence as it's fixing an actual bug, where this PR is mostly just trying to clean up code.

[stleary]

@johnjaylward I think in #274 you have included the essential bug fix from this pull request, but let me know if I missed something.

[johnjaylward]

Yes, they were both adjusting the same bug but using different approaches. This PR did have one other change to accept any Number as a JSONNumber instead of just BigDecimal, Long, etc.

I had needed more tests implemented to verify that change though. Now that #274 is merged, I can look at this cleanup again.