google / volley

https://google.github.io/volley
Apache License 2.0
3.37k stars 751 forks source link

The JsonObjectRequest and JsonArrayRequest don't support 204: No Content #431

Open rhbvkleef opened 2 years ago

rhbvkleef commented 2 years ago

If the server sends a 204 No Content, Volley still attempts to read the message body, and decode it. It shouldn't, or it should permit an empty body somehow.

jpd236 commented 2 years ago

Since these classes have to return a valid JSONObject/JSONArray object per the API contract, I suppose the question here is whether an empty body should be treated as equivalent to a body containing "{}" (for objects) or "[]" for arrays. The semantics here inherently depend on the application. I could easily see a behavior change here making things worse - if a server starts sending 204s unexpectedly, would it be better to have a client crash due to unexpected response, where the issue might be caught in earlier testing, or silently treat the response as empty, where it could result in data loss (if the app now thinks there is no server-side data available)?

Given how long this class has been around with its existing behavior, I'm hesitant to change it, though I could be convinced. To be honest, though, I'd generally recommend against using these classes either way; JSONObject/JSONArray are low-level, untyped primitives, and you'd likely be better off using an efficient strongly-typed JSON parser like Moshi in a real application. They're also fairly small and easy enough to adapt to suit your needs (e.g. you can always override it and change parseNetworkResponse to check for an empty response before delegating to the regular implementation, if you want to handle it differently.

Will keep this open for now in case there is other feedback.

Zablas commented 2 years ago

Ran into this issue as well. It seems to assume that 204 is an error and you can't even check for the status code in the error listener because networkResponse is null. It would be nice to have it handled in the regular listener at least.

jpd236 commented 2 years ago

I don't think it's assuming that 204 is an error; I believe what's happening is that the parsing logic (e.g. at https://github.com/google/volley/blob/master/core/src/main/java/com/android/volley/toolbox/JsonObjectRequest.java#L103) is trying to parse the empty string as Json and failing to do so, which results in the error callback being invoked with a ParseError.

The workaround is to extend JsonObjectRequest/JsonArrayRequest and override parseNetworkResponse to catch this case however you see fit. For example, something like:


@Override
protected Response<JSONObject> parseNetworkResponse(NetworkResponse response) {
    if (response.statusCode == 204) {
        return Response.success(new JSONObject(), HttpHeaderParser.parseCacheHeaders(response));
    }
    return super.parseNetworkResponse(response);
}
rhbvkleef commented 2 years ago

@jpd236 I would call the success callback with null. Whether to choose an Object or an Array could be perceived as an arbitrary choice while I thin everyone can agree on null.

jpd236 commented 2 years ago

That would have its own drawbacks (see e.g. https://www.lucidchart.com/techblog/2015/08/31/the-worst-mistake-of-computer-science/), especially if has written code assuming a non-null response, as was guaranteed before, and now starts getting a null.

It might be reasonable here, though. Response.success's constructor notes that the result may be null, as does Response.result's Javadoc (although Response.Listener's onResponse does not note this possibility). And in this case, the app would have crashed before when parsing the empty string (I believe?), so in all likelihood, this wouldn't introduce a new crash, but would shift it to later in the response flow.

That said, there's always some risk, however remote, that the crash was preventing response logic from running that doesn't properly handle this case, and rather than crash, does something more dangerous.