Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
rhbvkleef opened this issue Nov 3, 2021 · 5 comments

Comments

@rhbvkleef
Copy link

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
Copy link
Collaborator

jpd236 commented Nov 3, 2021

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
Copy link

Zablas commented Nov 29, 2021

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
Copy link
Collaborator

jpd236 commented Nov 29, 2021

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
Copy link
Author

@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
Copy link
Collaborator

jpd236 commented Dec 13, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants