Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#35316 closed Uncategorized (wontfix)

Support scalars as query parameters in admin changelist filters (for backward compatibility).

Reported by: Manolis Stamatogiannakis Owned by: nobody
Component: contrib.admin Version: 5.0
Severity: Normal Keywords:
Cc: Sarah Boyce Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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.

Change History (4)

comment:1 by Mariusz Felisiak, 9 months ago

Cc: Sarah Boyce added
Resolution: invalid
Status: newclosed

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().

in reply to:  1 comment:2 by Manolis Stamatogiannakis, 9 months ago

Resolution: invalid
Status: closednew

Replying to 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

Version 0, edited 9 months ago by Manolis Stamatogiannakis (next)

comment:3 by Mariusz Felisiak, 9 months ago

Resolution: wontfix
Status: newclosed

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?

self.used_parameters[self.lookup_kwarg] = True

Please don't reopen closed ticket. You can follow the triaging guidelines with regards to wontfix tickets and take this to DevelopersMailingList, if you don't agree.

in reply to:  3 comment:4 by Manolis Stamatogiannakis, 9 months ago

Replying to Mariusz Felisiak:

Why don't you set a value directly?

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, because it still runs before build_q_object_from_lookup_parameters() processes the values.

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

Last edited 9 months ago by Manolis Stamatogiannakis (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top