Fix for number output bug.

[johnjaylward]

java.lang.Number is currently output without any validation. For all java.* Numbers, this is fine, but for custom Number implementations like Complex or Fraction, the resulting JSON output may be invalid.

For example: If a Fraction implementation defines its' toString method as return numerator + "/" + denominator, then the resulting JSON output would be something like this:

{ "fraction" : 1/2 }

This is not valid JSON.

This commit verifies that the string representation of the number is close to a JSON formatted number by use of the BigDecimal constructor. If the constructor throws a NumberFormatException, then the string value is instead quoted as a string. The example above would instead output like the following:

{ "fraction" : "1/2" }

[johnjaylward]

Would it be better to have the check in the numberToString method instead of in the valueToString and writeValue methods? It may make more sense to centralize the check, but I didn't want to change the output of the public API without comment. I wasn't sure if some people may be dependent on that function always returning an unquoted string.

[stleary]

Don't know the answer to that, just use your best judgement.

[stleary]

@johnjaylward have you completed all of the changes for this pull request?

[johnjaylward]

Yes, I think so. It looks like this has a conflict now, so I'll rebase and double check the changes. I don't think I'm going to centralize the check at this time.

[johnjaylward]

ok, I finished the merge with the master branch. I think this is completed now.

[stleary]

What problem does this code solve?
JSONObject.numberToString() should return valid JSON text strings. A user-defined Number class might produce a string that is not valid JSON text. If the string cannot be converted back into a BigDecimal, it is instead returned as a properly quoted and escaped string. Although numberToString() does not advertise that it only returns valid JSON text, it is used internally by methods that do guarantee the returned strings are valid.

Risks
Low risk. User defined Number classes that convert to invalid JSON text are not expected to be in common use. But if they do, it is better to change the behavior to replace the invalid string with a valid JSON string.

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

Will this require a new release?
Can be rolled into the next release

Should the documentation be updated?
Not required.

Does it break the unit tests?
Need to check whether unit tests are complete for this code fix.

Review status
ACCEPTED. Starting 3-day comment window

[stleary]

Leaving this open til I can update the unit tests, hopefully within the next 24 hours.