[Django] #33298: get_object_or_404 / get_list_or_404 don't document/test *args
Django
------------------------------------------------+------------------------
Reporter: Jaap Roes | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
------------------------------------------------+------------------------
During code review I came across the following (slightly redacted)
snippet:
{{{
doc = get_object_or_404(
Document.objects.all(),
Q(is_public=True) | Q(owner=request.user.id),
id=data['document_id']
)
}}}
This looked odd to me, but the accompanying tests prove that it works. I
wanted to comment that the first argument could just be `Document` but
just to make sure I headed to the Django documentation of
[https://docs.djangoproject.com/en/3.2/topics/http/shortcuts/#get-object-
or-404 get_object_or_404].
To my surprise the usage of `*args` is not documented. While the function
signature is shown as `(klass, *args, **kwargs)`, only the `klass` and
`**kwargs` arguments are documented and explained (same goes for
[https://docs.djangoproject.com/en/3.2/topics/http/shortcuts/#get-list-
or-404 get_list_or_404]).
I then dug a bit deeper and looked at the
[https://github.com/django/django/blob/69b0736fad1d1f0197409ca025b7bcdf5666ae62/tests/get_object_or_404/tests.py
tests]. There I can only see tests that exercise the `klass` and
`**kwargs` arguments, there are no tests that pass in `*args` or a
combination `*args` and `**kwargs`.
This made me look at the actual
[https://github.com/django/django/blob/69b0736fad1d1f0197409ca025b7bcdf5666ae62/django/shortcuts.py
implementation]. There the docstring state:
klass may be a Model, Manager, or QuerySet object. All other passed
arguments and keyword arguments are used in the get() query.
I then dug deeper, and found that in the end the initial code snippet is
converted to something like
`Document.objects.all().filter(Q(Q(is_public=True) |
Q(owner=request.user.id), id=data['document_id'])).get()`.
So, in conclusion, is passing `*args` to `get_object_or_404` and
`get_list_or_404` an expected use-case that should be tested/documented?
--
Ticket URL: <https://code.djangoproject.com/ticket/33298>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Unreviewed => Accepted
Comment:
Thanks for the report! Agreed, we should document and add a small test for
`*args`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33298#comment:1>
Django
Reporter: Jaap Roes | Owner: Marcelo
Type: | Galigniana
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Marcelo Galigniana):
* owner: nobody => Marcelo Galigniana
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33298#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: Marcelo
Type: | Galigniana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Marcelo Galigniana):
* has_patch: 0 => 1
Comment:
I update the "Has patch" flag and link the PR to the ticket
https://github.com/django/django/pull/15112
--
Ticket URL: <https://code.djangoproject.com/ticket/33298#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: Marcelo
Type: | Galigniana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33298#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: Marcelo
Type: | Galigniana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Jaap Roes):
Nice! Thanks Marcelo and Mariusz :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/33298#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: Marcelo
Type: | Galigniana
Component: Utilities | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"7f8f69fb38247827220805f18c2e0f08406d8a3b" 7f8f69f]:
{{{
#!CommitTicketReference repository=""
revision="7f8f69fb38247827220805f18c2e0f08406d8a3b"
Fixed #33298 -- Added docs and tests for using Q objects with
get_object_or_404()/get_list_or_404().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33298#comment:6>