Opened 7 years ago

Closed 13 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.

(Asked on django-developers)

Change History (5)

comment:1 by Tim Graham, 7 years ago

Summary: Admin changelist turns QueryDict into dictAdmin changelist turns GET QueryDict into dict which may lose parameters
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Sounds reasonable, if it can be done in a backwards-compatible way.

comment:2 by Ling-Xiao Yang, 7 years ago

Cc: Ling-Xiao Yang added
Owner: changed from nobody to Ling-Xiao Yang
Status: newassigned

comment:3 by Ling-Xiao Yang, 7 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 Ling-Xiao Yang, 7 years ago

Owner: Ling-Xiao Yang removed
Status: assignednew

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:

  1. The admin filters are single-valued by definition (example) and this is semantically shown as self.value() in the code example of related doc.
  1. The dict-typed GET parameters are further passed from `ChangeList` view to filter classes, which would require a change in both sides.
  1. There is a ChangeList.get_query_string method that accepts an additional dict-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 every dict to QueryDict, it would be more of a systematic change.
  1. 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 its MultipleChoiceFilter, see its doc. So a multi-valued GET parameter such as state=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 Mariusz Felisiak, 13 months ago

Resolution: duplicate
Status: newclosed
Triage Stage: AcceptedUnreviewed

Duplicate of #1873.

Note: See TracTickets for help on using tickets.
Back to Top