#19182 closed Bug (fixed)
ModelAdmin filtering when list_filter = (('fieldname', ClassName),) throws SuspiciousOperation exception
Reported by: | Andrey Gusev | Owned by: | nobody |
---|---|---|---|
Component: | contrib.admin | Version: | 1.4 |
Severity: | Normal | Keywords: | modeladmin lookup_allowed |
Cc: | r1chardj0n3s, schillingt@…, Paul Garner, chris+django@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Admin raises SuspiciousOperation on using complex query like ghi__jkl__contains
in custom list filter described by ('fieldpath', ClassName )
syntax.
class MyModelAdmin(ModelAdmin): ... list_filter = ( 'abc', 'def', ( 'ghi__jkl__contains', MyListFilter ) ) ...
Simple but ugly fix by replacing last line of ModelAdmin.lookup_allowed
(around line 270 in contrib/admin/options.py):
clean_lookup = LOOKUP_SEP.join(parts) --- return clean_lookup in self.list_filter or clean_lookup == self.date_hierarchy +++ return any([ ( clean_lookup == ( i[0] if isinstance(i, tuple) else i ) ) for i in self.list_filter ]) or clean_lookup == self.date_hierarchy
Change History (25)
comment:1 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 11 years ago
Cc: | added |
---|
comment:5 by , 11 years ago
Hm, there's no tests at all for lookup_allowed that I can see, unless I'm missing something.
comment:6 by , 11 years ago
Most tests for list_filter
should normally be in https://github.com/django/django/blob/master/tests/admin_filters/tests.py
After a quick look it seems there are also a few in admin_views
, for example: https://github.com/django/django/blob/1b47508ac88c1b1ee6ae71f032abb268395ac00e/tests/admin_views/tests.py#L411-L480
It would be very helpful to write a failing test reproducing the SuspiciousOperation
error.
comment:7 by , 11 years ago
Cc: | added |
---|
I'm not sure this is a bug. The list_filter
accepts tuples in it as long as the filter class passed in inherits from django.contrib.admin.FieldListFilter
Documentation: https://docs.djangoproject.com/en/dev/ref/contrib/admin/#django.contrib.admin.ModelAdmin.list_filter
comment:8 by , 11 years ago
I just hit this, I'm on 1.5
@CodenameTim I think you misunderstood, the bug is in admin.ModelAdmin
...it doesn't handle tuples in the list_filter
attr. Everything works until you try to actually filter on that field
The code seems to only handle strings - it doesn't look like it would be able to handle the other form shown in the docs list_filter = (DecadeBornListFilter,)
either but I haven't tried it.
comment:9 by , 11 years ago
@anentropic I must be misunderstanding the problem. I just pulled stable/1.5.x and implemented what is in the documentation and it worked fine. Can you post what you tried to use and what you expected it to do?
comment:10 by , 11 years ago
Yes I'm a bit puzzled, there's a case in the tests that uses the tuple form and appears to work fine. I haven't yet found the part of Django that makes it work - any pointers? (because reading the source of lookup_allowed
method it seems obvious why it wouldn't work...)
Am trying to narrow down what part of my code my be causing it, to make a reproduceable case
comment:11 by , 11 years ago
@CodenameTim it seems to be related to use of AllValuesFieldListFilter
Here is a simple test case which fails:
https://github.com/anentropic/django/commit/5d32f4e09e68a101ccd4202438504296ac68a02b
DisallowedModelAdminLookup: Filtering by author__email not allowed
comment:12 by , 11 years ago
Cool, thanks for that. I was able to reproduce it now. Not sure if it matters, but a workaround for that case would be to just pass 'author__email'
rather than ('author__email', AllValuesFieldListFilter)
comment:13 by , 11 years ago
I think the whole idea that admin code should know what a lookup targets is wrong. Instead methods for checking if lookup exists at all, and what models the lookup traverses should be added to QuerySet's public API, then admin should use those methods.
As is, the code is very fragile. In fact, to me it seems that if you are using GIS it will be possible to query non-allowed models by gis lookups. Why? Use a lookup that isn't in default QUERY_TERMS, but is in GeoQuery's lookups. The lookup part will not be popped in L335 in admin/options.py. Later on we get to FieldDoesNotExist on L346 for the GIS lookup. But the comment's logic doesn't apply here - the lookup isn't actually going to produce field error later on. End result is lookup allowed, even if it targets non-allowed related field. (I haven't tested this.)
I don't object to improving lookup_allowed for this specific cases - but trying to repeat the logic of how QuerySet treats lookups is non-DRY for a complex piece of code. This means that lookup_allowed method *will* fail when things are changed in QuerySet.
comment:14 by , 11 years ago
@akaariai My problem is the opposite one: lookups being disallowed when they shouldn't be
@CodenameTime in my real code I have a more complex filter class that inherits from AllValuesFieldListFilter
which I need to cause to be used
This is what I've done to get things working for me:
https://github.com/anentropic/django/commit/1cc56634b729fe498c4ecf237c3c322c2fead919
comment:15 by , 11 years ago
Actually it has nothing to do with AllValuesFieldListFilter
, test also fails if you substitute BooleanFieldListFilter
The problem is with relation-spanning filters such as 'author__email'
, in conjunction with the tuple form
I'm not sure whether my lookup is even supposed to fall through this check:
if len(parts) == 1: return True
(which is why the existing test case containing ('is_best_seller', BooleanFieldListFilter)
filter does not fail)
but I think it is supposed to, as there is a comment that the code above is just a special case for trimming foo__id
comment:16 by , 11 years ago
OK, lets deal with the more generic lookup_allowed() refactoring problem separately.
comment:17 by , 11 years ago
I'm sorry, I'm a little bit lost. What issue are we trying to solve with this ticket?
comment:18 by , 11 years ago
The problem is as described by the OP:
"Admin raises SuspiciousOperation on using complex query like ghi__jkl__contains
in custom list filter described by ('fieldpath', ClassName )
syntax."
comment:19 by , 11 years ago
After understanding the code a bit better last night I think my fix above is basically correct.
I have added another test case, for (SimpleListFilter,)
class-only syntax... if the parameter_name
attribute included double-underscores it was also failing. Have also tidied up the naming of previous test case and put some comments in.
https://github.com/anentropic/django/commit/91a6cd4895b269b01b872799001587ee34a25855
Is this enough to make a pull request? Do you want the new tests and the fix separately, at the moment they're in the same branch on my fork.
comment:20 by , 11 years ago
Cc: | added |
---|---|
Has patch: | set |
comment:21 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:22 by , 11 years ago
Summary: | ModelAdmin.lookup_allowed should also check for ('fieldname', ClassName) syntax → ModelAdmin filtering when list_filter = (('fieldname', ClassName),) throws SuspiciousOperation exception |
---|
Update the summary to help people to find this ticket.
comment:23 by , 11 years ago
I'd like to request that this fix be backported to 1.6.x, since the use of ('fieldname', SimpleListFilter) is documented to work for 1.6.x but doesn't.
comment:24 by , 11 years ago
This doesn't qualify for a backport to 1.6.x since it's not a new regression (the bug has been present since 1.4 according to the ticket).
comment:25 by , 11 years ago
Cc: | added |
---|
Accepted, although a clearer implementation than the one proposed in the ticket description would be preferable.