[Django] #35316: Support scalars as query parameters in admin changelist filters (for backward compatibility).

17 views
Skip to first unread message

Django

unread,
Mar 18, 2024, 2:02:55 PM3/18/24
to django-...@googlegroups.com
#35316: Support scalars as query parameters in admin changelist filters (for
backward compatibility).
-----------------------------------------+------------------------
Reporter: m000 | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 5.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-----------------------------------------+------------------------
Ticket #1873 added support for multi-valued query parameters in admin
changelist filters. Because only list types are supported, older code that
e.g. extends SimpleListFilter or BooleanFieldListFilter will break.

The latter (extending BooleanFieldListFilter) may even be completely
unsupported. See notes in PR for #1873.

Overall, user code needs to introduce version conditionals if it is
required to support both Django 4.x and Django 5. The reason is that
Django 4.x only supports scalar types as query parameter values, while
Django 5 only supports lists.

Since supporting both scalars and lists should be possible without adding
complexity to the Django codebase, it would be good to continue supporting
scalars, perhaps with a deprecation warning, if necessary.
--
Ticket URL: <https://code.djangoproject.com/ticket/35316>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,
Mar 18, 2024, 2:35:35 PM3/18/24
to django-...@googlegroups.com
#35316: Support scalars as query parameters in admin changelist filters (for
backward compatibility).
-------------------------------+--------------------------------------
Reporter: m000 | Owner: nobody
Type: Uncategorized | Status: closed
Component: contrib.admin | Version: 5.0
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+--------------------------------------
Changes (by Mariusz Felisiak):

* cc: Sarah Boyce (added)
* resolution: => invalid
* status: new => closed

Comment:

`build_q_object_from_lookup_parameters()` is a private API and all built-
in filter use `GET.lists()` so I don't see anything breaking in the #1873.
If you extend built-in filters and really want to use
`build_q_object_from_lookup_parameters()` you should also use
`GET.lists()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/35316#comment:1>

Django

unread,
Mar 18, 2024, 7:19:58 PM3/18/24
to django-...@googlegroups.com
#35316: Support scalars as query parameters in admin changelist filters (for
backward compatibility).
-------------------------------------+-------------------------------------
Reporter: Manolis | Owner: nobody
Stamatogiannakis |
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 5.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Manolis Stamatogiannakis):

* resolution: invalid =>
* status: closed => new

Comment:

Replying to [comment:1 Mariusz Felisiak]:
> `build_q_object_from_lookup_parameters()` is a private API and all
built-in filter use `GET.lists()` so I don't see anything breaking in the
#1873. If you extend built-in filters and really want to use
`build_q_object_from_lookup_parameters()` you should also use
`GET.lists()`.


I believe you may have misunderstood the case described. Of course
`build_q_object_from_lookup_parameters()` is not directly used, since I am
referring to code that pre-dates Django 5, when the function was
introduced. The problem is that this code now needs version conditionals
in order to work with both Django 4.x and Django 5.x.

This isn't anything super fancy. Just a subclass of BooleanFieldListFilter
that needs to have True as the default selection instead of All. It looks
like this ATM:

{{{
class BooleanDefaultTrueFilter(BooleanFieldListFilter):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
if self.lookup_kwarg2 not in self.used_parameters and
self.lookup_kwarg not in self.used_parameters:
self.`[self.lookup_kwarg] = prepare_lookup_value(
self.lookup_kwarg,
True if django.VERSION < (5,0) else (True,)
)
self.lookup_val = '1'
}}}

Without the conditional, an exception will be raised in
`build_q_object_from_lookup_parameters()`, which is why the specific
function was updated.

Of course one may still argue that `used_parameters` is internal state.
But overriding `__init__()` like above is still the most elegant way I
could find to achive the desired effect (see [1]). Is there a
better/"sanctioned" way to achieve this effect? The documentation for
BooleanFieldListFilter/FieldListFilter is pretty sparse.

Also, a quick GH search shows a number of repositories modifying
`used_parameters` [2]. So, although not thorougly documented, this is not
something super-exotic for users customizing admin filters, and one would
expect that there will be more codebases that will have to implement
similar workarounds.

Overall, the sparse documentation for
BooleanFieldListFilter/FieldListFilter should not be a reason for
breakage, when that is easily avoidable.

Along this line, I think that updating
`build_q_object_from_lookup_parameters()` as proposed in PR #17990 will be
a useful addition, as long as there are no side-effects that I missed and
would outweigh the benefits.



[1] https://forum.djangoproject.com/t/django-admin-filter-how-to-default-
to-no-or-yes-rather-than-all/6684
[2]
https://github.com/search?q=self.used_parameters%5B+lang%3APython+&type=code
--
Ticket URL: <https://code.djangoproject.com/ticket/35316#comment:2>

Django

unread,
Mar 19, 2024, 12:32:22 AM3/19/24
to django-...@googlegroups.com
#35316: Support scalars as query parameters in admin changelist filters (for
backward compatibility).
-------------------------------------+-------------------------------------
Reporter: Manolis | Owner: nobody
Stamatogiannakis |
Type: Uncategorized | Status: closed
Component: contrib.admin | Version: 5.0
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):

* resolution: => wontfix
* status: new => closed

Comment:

Are you maintaining a 3rd-party package? it's rather rare that you have to
support multiple Django versions in a project. Moreover,
`prepare_lookup_value()` is another private API that you don't have to
use. Why don't you set a value directly?
{{{#!python
self.used_parameters[self.lookup_kwarg] = True
}}}

Please don't reopen closed ticket. You can
[https://docs.djangoproject.com/en/stable/internals/contributing/triaging-
tickets/#closing-tickets follow the triaging guidelines with regards to
wontfix tickets] and take this to DevelopersMailingList, if you don't
agree.
--
Ticket URL: <https://code.djangoproject.com/ticket/35316#comment:3>

Django

unread,
Mar 19, 2024, 4:49:08 AM3/19/24
to django-...@googlegroups.com
#35316: Support scalars as query parameters in admin changelist filters (for
backward compatibility).
-------------------------------------+-------------------------------------
Reporter: Manolis | Owner: nobody
Stamatogiannakis |
Type: Uncategorized | Status: closed
Component: contrib.admin | Version: 5.0
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Manolis Stamatogiannakis):

Replying to [comment:3 Mariusz Felisiak]:
> Why don't you set a value directly?
> {{{#!python
> self.used_parameters[self.lookup_kwarg] = True
> }}}

For the benefit of anyone else that may arrive here having a similar
problem, this still needs a version-based conditional.

If `prepare_lookup_value()` is ever made a sanctioned API, maybe it would
be a more appropriate location to provide a fix for keeping existing
Django 4 filter customization code working in Django 5.

EOT
--
Ticket URL: <https://code.djangoproject.com/ticket/35316#comment:4>
Reply all
Reply to author
Forward
0 new messages