Handling test client `data` / `query_params`

See Fixed #14611 -- Added query_params to TestClient and RequestFactory. by knyghty · Pull Request #17447 · django/django · GitHub

While adding a query_params parameter to test client and request factory methods, I hit the issue that for GET and HEAD requests we can already pass in a data parameter to pass query parameters. The data parameter is also used for form data in other request methods. Everyone (so far) agrees we shouldn’t have both for these methods.

I suggested deprecating the data parameter for GET and HEAD requests. @felixxm instead prefers not adding query_params to them and sticking with data.

I am going to assume that the reason for this is for backwards compatibility, but I’ll let him tell us if there are some other issues.

But the reasons I prefer the deprecation:

  • All methods will have the same API, I think this is less confusing for beginners (and everyone, honestly).
  • It’s also easier to use them programmatically. if you need to make lots of different types of requests with query parameters to a URL, you can iterate over the method names and call them all with the same parameters.
  • I think we can be a bit more lenient on deprecating things in tests, because we shouldn’t be breaking anyone’s site with this change, only their tests.
  • This kind of change could be done automatically with django-upgrade (though I’m not sure exactly how it works so do correct me if I’m wrong).
  • It feels to me like the code would be easier to read and maintain.

I feel like query_params and data are both correct names so given that data is backwards compatible and already in a lot of folks’ muscle memory I’d probably lean that way too :man_shrugging:

I replied on the PR (…since I had it open :stuck_out_tongue_winking_eye:)

Summary here:

tl;dr: I think having both is fine. It keeps the API consistent. It’s using both at the same time that we should guard against

I think we should warn/error if you use both params on GET/HEAD, but other than that leave it.

Let’s not remove data. As per Aymeric’s comment on the ticket:

The data argument of the test client is designed to hold simulated form data, and is automatically inserted in requests according to these rules.

Ref django-upgrade: I’d love to make it official somehow, so that folks updating would get it automatically, but until such a time we have to assume that (relatively speaking) almost nobody in the user base has heard of it or is using it. :flushed:

I feel strongly about the need to support the same param name (query_params) across all the test client HTTP methods, including GET and HEAD. Having worked with big and complex code bases for a long time, I can assure you there is some amount of “meta-programming” when writing tests[0], and this meta-programming benefits considerably from having consistency in these calls.

I’m in favor of:

  1. Supporting both data and query_params for GET and HEAD, with a goal of eventually deprecating data
  2. A deprecation warning is shown when running the test suite and there are calls that use both
  3. query_params takes precedence over data when both are given (I think this is safe since a- developers defining query_params are reading and using the latest code/docs, and b- the warning will be shown anyways)

@tom Thanks for raising this topic in the forum!

[0] Whether this is or is not a good practice and to which extent, we can discuss separately, but I have good examples where this is very handy and helps readability and maintainability of the test code.

I favour supporting both data and query_params.

I think a TypeError when both are provided sounds sensible. Silent precedence would lead to tests that look like they do something with the data argument but actually they do nothing.

I am against deprecating data. Yes, django-upgrade could rewrite many cases, as it already does for test client headers. But as Carlton says, the use of django-upgrade is still relatively rare, and it is unofficial. (And who knows if Ruff will duplicate it at some point :sweat_smile:.) Keeping support for data is cheap, and many tests use the positional argument format anyway, in which case the name doesn’t matter. Also, a precedent is that we didn’t deprecate support for the WSGI-style header format when adding headers.

Re: django-upgrade and a conversation elsewhere about showcasing third party packages, I feel like it’s at least worth a mention there (when it exists) and also in any documentation (do we have much other than the release notes) around upgrading to newer versions.

Back on topic, for now I will continue with the current approach (keep both, error when using both at the same time), but it looks like there are a lot of differing opinions here, though it seems there’s a lot of support for having query_params everywhere, and perhaps the fate of data doesn’t need to be addressed immediately.