google / volley

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

Remove new constructors from JsonRequests which are breaking builds. #420

Closed jpd236 closed 3 years ago

jpd236 commented 3 years ago

These conflict with pre-existing constructors - since the request argument is nullable, existing code which passes in null requests is no longer invoking an unambiguous constructor.

See #419 for more details and a workaround.

These constructors were added in #406 and have not yet been included in a public release, so they are safe to remove.

jpd236 commented 3 years ago

@paulsmithkc FYI - sorry to have to revert this part of #406; this issue came up when testing the latest version of Volley against Google's internal applications, which means it likely also impacts other applications in the wild. We'll keep the request open for the longer term if/when we decide to make API breaking changes.

paulsmithkc commented 3 years ago

@jpd236 In that case the constructors could easily be combined into a single constructor. It would just need to accept a parent class of both JSONObject and JSONArray. That would prevent the constructor from being ambiguous.

jpd236 commented 3 years ago

Unfortunately, both of those classes extend directly from Object. So while it wouldn't be ambiguous, it wouldn't offer sufficient type safety to merit the API change.

paulsmithkc commented 3 years ago

The problem is that the existing system expects that the return type and the input type of the service will always be the same.

So at present you cannot:

paulsmithkc commented 3 years ago

If you look into the base request class (JsonRequest) you'll see that it accepts the body of the request as a string. And doesn't have any expectations about the content of that string. Which means that in JsonObjectRequest and JsonArrayRequest the only expectation is that body parameter has the toString() method.

Thankfully toString() is defined by Object, so this can be accepted without any issues.

Thus overall, allowing any object to be passed as the body actually doesn't create any type safety issues.

jpd236 commented 3 years ago

I don't think we want to open the door here for arbitrary objects in the request. It is compile-time safe and will not crash at runtime, but can result in invalid requests because the Content-Type specified in JsonRequest is application/json, but the request cannot be guaranteed to actually be that type.

Arguably this is also a flaw in JsonRequest since it takes an arbitrary String. Overall, I would say these APIs are not great! But the classes themselves are very simple and if you really want to use them, you can use the workaround noted in #419. If/when we decide to make breaking changes, we can clean this up properly and make a coherent set of APIs.

Personally, I'd avoid using these request classes anyway, and would implement a custom Request subclass using a strongly-typed, reflection-free deserialization library like Moshi or Kotlin Serialization, rather than reading directly from a raw JSONObject.

paulsmithkc commented 3 years ago

The issue for me here is that these are introduced and recommended in the documentation as the way that you should be sending JSON data with Volley. I know the documentation changes are pending, but this would be an important thing to address when the docs are migrated to Github.

Volley is one the technologies that I've been introducing my students to in my Android course. (College level.) Unfortunately this issue is one that they run into immediately, when they are getting started with Volley and reading the official docs.

jpd236 commented 3 years ago

Agreed that we should clarify recommendations around how best to make/structure JSON requests in the docs. I'm curious about the scenario that leads to this issue, though - if students are solving a canned problem for academic purposes, wouldn't it be simplest to just use JSONObjects on both sides? If you really wanted to just use an array on either side, you could just wrap it in an object.

(BTW, we haven't forgotten about the pending doc update here)

paulsmithkc commented 3 years ago

Let's say your building an ecommerce website. And you want to implement a product search. You need to send a few fields to the server:

The back-end for this service is happy to accept these parameters as query parameters, form fields, or a json object.

However the results are returned as an array of products. Which is a perfectly reasonable and sensical thing to do here.

JsonObjectRequest cannot handle this because it expects the results to be an object.

JsonArrayRequest is the obvious answer since the result type is an array. So this is what most students try. But they can't send the data the server, as obviously the server cannot accept these parameters as an array.

jpd236 commented 3 years ago

If it were up to me, I'd advise students in that situation to return the search response as an object including a result array. It's more future-proof in case there's additional metadata beyond just the result list - such as a status code to indicate that the result of the request, or some debugging information like how long the request took. And it happens to work neatly around the limitation here, since your request and response would both be objects. On the other hand, if you build the service around returning a JSONArray directly, it'd be impossible to add additional metadata to the response later without breaking older clients.