Opened 8 years ago
Closed 21 months ago
#27559 closed Cleanup/optimization (duplicate)
Admin changelist turns GET QueryDict into dict which may lose parameters
Reported by: | Jonas von Poser | Owned by: | |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Ling-Xiao Yang | Triage Stage: | Unreviewed |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
We implemented a custom SimpleListFilter for the admin which presents the choices as a list of checkboxes. Pressing "submit" sends the list to the backend for filtering.
Unfortunately, this doesn't really work well and it took us a while to find out why: the ChangeList
view turns request.GET
(a QueryDict
) into a dict.
This means, a query string in the form of ?q=123&state=1&state=2
simply loses all state values except the last one. It still works in principle but as soon as you e.g. click on a column heading to re-sort, the link only shows the last value instead of all of them.
I checked the git log and traced this re-casting back to the "NEW ADMIN MERGE" in 2005. It was probably done to be able to manipulate the parameters but seems to be a very lossy operation for this purpose.
Shouldn't self.params
rather be kept as a QueryDict
? We'll get started on a pull request if there's interest.
Change History (5)
comment:1 by , 8 years ago
Summary: | Admin changelist turns QueryDict into dict → Admin changelist turns GET QueryDict into dict which may lose parameters |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
comment:2 by , 8 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:3 by , 8 years ago
This may also require a change in django.contrib.admin.filters.ListFilter.__init__()
. This init method now accepts the argument params
that has to be a dict
object and is passed from ChangeList.get_filter()
, and eventually from ChangeList.params
. Even if we replace ChangeList.params
as a QueryDict
, it is still difficult to pass the multiple GET values to the admin filter classes while having their init params
as a dict
object.
It should require a change in the admin filter __init__()
definition, unless it would be safe to iterate through all these GET values that have the same key, pass one value to the filter every time (while not exposing other values), and use Q() | Q()
to link them in ChangeList.get_filter()
or ChangeList.get_queryset()
, in order to keep the filter part unchanged. I think this would be the minimal drawback to fix this issue right now, as developers are more supposed to access GET values in filter's argument request
in documented methods lookups(self, request, model_admin)
and queryset(self, request, queryset)
but not __init__()
.
comment:4 by , 8 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
I didn't find a good way of ensuring the backward compatibility, as this turned out as a conceptual problem that has existed for a long time:
- The admin filters are single-valued by definition (example) and this is semantically shown as
self.value()
in the code example of related doc.
- The
dict
-typed GET parameters are further passed from `ChangeList` view to filter classes, which would require a change in both sides.
- There is a
ChangeList.get_query_string
method that accepts an additionaldict
-typed parameter (new_params
) to allow modification of GET parameters, see code. And this method has been used in many places like 1, 2, 3 and so on. If we change everydict
toQueryDict
, it would be more of a systematic change.
- I considered an implicit OR connection of SQL queries in my last comment, in order to restrict changes in
ChangeList
only, but this may not be a good idea. First, RFC 3986 has no definition on the meaning of multi-valued GET parameter. Also, in the other extension package django-filters, both OR and AND logic are allowed in itsMultipleChoiceFilter
, see its doc. So a multi-valued GET parameter such asstate=1&state=2
does not necessarily mean an OR relationship in the filtering process.
I'm deassigning myself here. More people are welcomed to look at this issue.
comment:5 by , 21 months ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
Triage Stage: | Accepted → Unreviewed |
Duplicate of #1873.
Sounds reasonable, if it can be done in a backwards-compatible way.