do not return 'null' for toString

[paulmillar]

Motivation:

null is not a valid return value for Object#toString; for example, see:

https://stackoverflow.com/questions/32378264/when-we-override-the-tostring-method-we-should-always-return-a-string-represen

Additionally, catching Exception means that any RuntimeException (which is likely a bug in the underlying code) will be supressed, making it harder to figure out what is going wrong.

Modification:

The JSONObject#toString contract requires toString to return valid JSON.

The code could return a valid JSON by returning a textural description of the JSONException as a quoted string; however, this would not make it clear than an error has occurred, and would likely be incompatible with the expected structure of the JSON (the schema).

As JSONException is a RuntimeException, the code simply allow it to propagate.

Result:

JSONObject#toString no longer returns null under exceptional circumstances.

[stleary]

What problem does this code solve? Fixes an error handling issue in JSONObject toString() method. Currently null is returned when an exception is thrown. This is arguably not the best implementation. Instead, the exception will not be handled in this method, but must be caught elsewhere. Changing existing behavior is a concern. This is believed to be an edge case that rarely occurs. Changing the method signature by adding 'throws' is another concern. If user feedback reveals any issues, the change can be backed out. Extending the comment period to 14 days for this change.

Risks Low

Changes to the API? JSONObject.toString() now may throw an exception

Will this require a new release? No

Should the documentation be updated? No

Does it break the unit tests? No, a new unit test was added

Was any code refactored in this commit? No

Review status Under discussion

[stleary]

After thinking more about this change, I have a concern that it could break existing applications.

[paulmillar]

Indeed.

For me, there's two ways of looking at this.

It is true that it is an API change. Therefore, it has the possibility to break existing applications.

However, equally, this patch is "simply" a bug-fix since null is not a valid value for Object#toString to return.

The underlying problem is that the documented API is incompatible with existing Java behaviour: the JSONObject#toString JavaDoc describes how the method may return null.

As the project doesn't use semantic versioning, I don't think there is any good way out of this problem.

The choice is between keeping the buggy behaviour of toString (patch is updated so it adds a new method) or toString is fixed (potentially breaking existing applications).

My preference would be for the former option, but I'm happy to update the patch if you wish to go for the latter option.

[stleary]

@paulmillar A section of the project FAQ mentions cases where a bug fix must be balanced against a behavior change:

Does it change the behavior of the lib? If so, it will not be accepted, unless it fixes an egregious bug. 
This is happening less frequently now.

In this case I don't think the severity of the bug rises to the level where it justifies changing the behavior.

[paulmillar]

OK.

Should I update the patch to introduce a new method (similar to JSONObject#toString but that never returns null), or should I drop this pull-request?

[stleary]

If returning null breaks your application, a PR that instead returned an empty string would be reasonable. Otherwise, it's probably better to consider this behavior a known problem in the code and not fix it.