Inconsistencies in how null is handled from implementations of JSONString

[johnjaylward]

This is related to #273, but could be fixed separate from it if needed. It was first discovered by @run2000 while working on PR #241.

Currently output from the library is generated using 2 different methods JSONObject.valueToString (used mostly when inserting new objects to a JSONObject or JSONArray) and JSONObject.writeValue (used by toString and other write operations). Each of these methods have their own implementation for how values are processed and for most things the implementation matches. However, for JSONString implementations, we see two different approaches used for null checks from the JSONString.toJSONString.

In valueToString there is an instanceof String check that occurs and if it fails, a JSONException is thrown.

In writeValue if value.toJSONString() returns null, then the quoted value of value.toString() is used instead. No exception is thrown.

The Java Documentation on JSONString does not explicitly dictate how null values returned from implementations should be handled, but it does seem to imply that the value.toString() is correct and not the Exception.

This issue is to discuss what type of handling should be used to make this API call consistent as well as to note any possible change to the documentation.

[stleary]

Thanks, running a bit behind schedule, will try to take a look at this today.

[stleary]

My thinking is that this does not rise to the level of a problem that should be fixed by changing existing behavior.

• JSONObject.valueToString() is a static method, so any user can call it with any value, and the expectation that it will either return strictly conforming JSON text, or throw an exception. If the object implements JSONString, but fails to return a valid string from toJSONString(), then it seems reasonable in this case to throw an exception. The Javadoc is a little unclear, "throws JSONException If the value is or contains an invalid number."
• It may be called from JSONWriter.value(Object). Although this method is public, it is not commented as if it is intended to be used externally. Internally, JSONWriter.value(Object) is only called by JSONWriter.value(double), which is also public, but which will never pass along a JSONString. The Javadoc mentions throwing a JSONException on invalid number, which is only true for internal usage.
• It may be called from JSONArray.join(), also public. The Javadoc only mentions invalid numbers, which seems incomplete.

I would prefer to fix this by correcting the Javadoc. But - please let me know if I missed something in the analysis.

[johnjaylward]

My issue isn't necessarily the exception, it's that valueToString throws the exception while writeValue does not. Both places in the API call the toJSONString() method, but handle the possible null value differently.

If we agree that returning null from toJSONString() is illegal and should result in an exception, then I believe that writeValue should be modified to also throw an exception on null.

I actually prefer the exception myself, and think that null should never be allowed as a return value from toJSONString.

I do agree that the javadoc needs to be updated in either case.

[stleary]

Agreed that the code should not have been written this way, so far as I can tell. But can't proceed with a code change without a use case to demonstrate that fixing the problem, and possibly breaking existing code, is better than leaving it unchanged. Will leave this issue open for a week or so in case someone can come up with that, In the meanwhile, will document the behavior.

[johnjaylward]

ok, I'll try to write a few test cases to document when the exception occurs vs silent failure. Not sure I'll have time this week, but I'd like to at least document it through test cases so we can see the use cases of each code path. That may make fixing it a higher priority or show that it's a non-issue.

[stleary]

Thanks, I think that gives us a path forward.

[stleary]

Do you still want to keep this issue open, @johnjaylward

[johnjaylward]

Yes, I've just been super busy at work lately and haven't had the free time to look at this again in more detail.

[stleary]

No objection to proceeding with a pull request. Let's make sure it has a credible use case where an exception is thrown/not thrown, depending on how a given input is processed, and how the code change fixes the problem.