Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Disentangled some parts of the admin ChangeList and ListFilter's inte…

…rnals. With this refactoring, the query string lookups are now processed once instead of twice and some bugs (in particular the SimpleListFilter parameter name being mistaken for a model field in some cases) are avoided.

Refs #17091.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@17145 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information...
commit a89b15628429da79dd47c04bd191de566ef7eee6 1 parent e71f336
@jphalip jphalip authored
View
139 django/contrib/admin/filters.py
@@ -13,13 +13,15 @@
from django.utils.translation import ugettext_lazy as _
from django.contrib.admin.util import (get_model_from_relation,
- reverse_field_path, get_limit_choices_to_from_path)
+ reverse_field_path, get_limit_choices_to_from_path, prepare_lookup_value)
class ListFilter(object):
title = None # Human-readable title to appear in the right sidebar.
def __init__(self, request, params, model, model_admin):
- self.params = params
+ # This dictionary will eventually contain the request's query string
+ # parameters actually used by this filter.
+ self.used_parameters = {}
if self.title is None:
raise ImproperlyConfigured(
"The list filter '%s' does not specify "
@@ -27,7 +29,7 @@ def __init__(self, request, params, model, model_admin):
def has_output(self):
"""
- Returns True if some choices would be output for the filter.
+ Returns True if some choices would be output for this filter.
"""
raise NotImplementedError
@@ -43,15 +45,14 @@ def queryset(self, request, queryset):
"""
raise NotImplementedError
- def used_params(self):
+ def expected_parameters(self):
"""
- Return a list of parameters to consume from the change list
- querystring.
+ Returns the list of parameter names that are expected from the
+ request's query string and that will be used by this filter.
"""
raise NotImplementedError
-
class SimpleListFilter(ListFilter):
# The parameter that should be used in the query string for that filter.
parameter_name = None
@@ -67,16 +68,20 @@ def __init__(self, request, params, model, model_admin):
if lookup_choices is None:
lookup_choices = ()
self.lookup_choices = list(lookup_choices)
+ if self.parameter_name in params:
+ value = params.pop(self.parameter_name)
+ self.used_parameters[self.parameter_name] = value
def has_output(self):
return len(self.lookup_choices) > 0
def value(self):
"""
- Returns the value given in the query string for this filter,
- if any. Returns None otherwise.
+ Returns the value (in string format) provided in the request's
+ query string for this filter, if any. If the value wasn't provided then
+ returns None.
"""
- return self.params.get(self.parameter_name, None)
+ return self.used_parameters.get(self.parameter_name, None)
def lookups(self, request, model_admin):
"""
@@ -84,7 +89,7 @@ def lookups(self, request, model_admin):
"""
raise NotImplementedError
- def used_params(self):
+ def expected_parameters(self):
return [self.parameter_name]
def choices(self, cl):
@@ -111,15 +116,18 @@ def __init__(self, field, request, params, model, model_admin, field_path):
self.field = field
self.field_path = field_path
self.title = getattr(field, 'verbose_name', field_path)
- super(FieldListFilter, self).__init__(request, params, model, model_admin)
+ super(FieldListFilter, self).__init__(
+ request, params, model, model_admin)
+ for p in self.expected_parameters():
+ if p in params:
+ value = params.pop(p)
+ self.used_parameters[p] = prepare_lookup_value(p, value)
def has_output(self):
return True
def queryset(self, request, queryset):
- for p in self.used_params():
- if p in self.params:
- return queryset.filter(**{p: self.params[p]})
+ return queryset.filter(**self.used_parameters)
@classmethod
def register(cls, test, list_filter_class, take_priority=False):
@@ -144,20 +152,20 @@ def create(cls, field, request, params, model, model_admin, field_path):
class RelatedFieldListFilter(FieldListFilter):
def __init__(self, field, request, params, model, model_admin, field_path):
- super(RelatedFieldListFilter, self).__init__(
- field, request, params, model, model_admin, field_path)
other_model = get_model_from_relation(field)
- if hasattr(field, 'verbose_name'):
- self.lookup_title = field.verbose_name
- else:
- self.lookup_title = other_model._meta.verbose_name
rel_name = other_model._meta.pk.name
- self.lookup_kwarg = '%s__%s__exact' % (self.field_path, rel_name)
- self.lookup_kwarg_isnull = '%s__isnull' % (self.field_path)
+ self.lookup_kwarg = '%s__%s__exact' % (field_path, rel_name)
+ self.lookup_kwarg_isnull = '%s__isnull' % field_path
self.lookup_val = request.GET.get(self.lookup_kwarg, None)
self.lookup_val_isnull = request.GET.get(
self.lookup_kwarg_isnull, None)
self.lookup_choices = field.get_choices(include_blank=False)
+ super(RelatedFieldListFilter, self).__init__(
+ field, request, params, model, model_admin, field_path)
+ if hasattr(field, 'verbose_name'):
+ self.lookup_title = field.verbose_name
+ else:
+ self.lookup_title = other_model._meta.verbose_name
self.title = self.lookup_title
def has_output(self):
@@ -169,7 +177,7 @@ def has_output(self):
extra = 0
return len(self.lookup_choices) + extra > 1
- def used_params(self):
+ def expected_parameters(self):
return [self.lookup_kwarg, self.lookup_kwarg_isnull]
def choices(self, cl):
@@ -206,14 +214,14 @@ def choices(self, cl):
class BooleanFieldListFilter(FieldListFilter):
def __init__(self, field, request, params, model, model_admin, field_path):
- super(BooleanFieldListFilter, self).__init__(field,
- request, params, model, model_admin, field_path)
- self.lookup_kwarg = '%s__exact' % self.field_path
- self.lookup_kwarg2 = '%s__isnull' % self.field_path
+ self.lookup_kwarg = '%s__exact' % field_path
+ self.lookup_kwarg2 = '%s__isnull' % field_path
self.lookup_val = request.GET.get(self.lookup_kwarg, None)
self.lookup_val2 = request.GET.get(self.lookup_kwarg2, None)
+ super(BooleanFieldListFilter, self).__init__(field,
+ request, params, model, model_admin, field_path)
- def used_params(self):
+ def expected_parameters(self):
return [self.lookup_kwarg, self.lookup_kwarg2]
def choices(self, cl):
@@ -243,12 +251,12 @@ def choices(self, cl):
class ChoicesFieldListFilter(FieldListFilter):
def __init__(self, field, request, params, model, model_admin, field_path):
+ self.lookup_kwarg = '%s__exact' % field_path
+ self.lookup_val = request.GET.get(self.lookup_kwarg)
super(ChoicesFieldListFilter, self).__init__(
field, request, params, model, model_admin, field_path)
- self.lookup_kwarg = '%s__exact' % self.field_path
- self.lookup_val = request.GET.get(self.lookup_kwarg)
- def used_params(self):
+ def expected_parameters(self):
return [self.lookup_kwarg]
def choices(self, cl):
@@ -260,7 +268,8 @@ def choices(self, cl):
for lookup, title in self.field.flatchoices:
yield {
'selected': smart_unicode(lookup) == self.lookup_val,
- 'query_string': cl.get_query_string({self.lookup_kwarg: lookup}),
+ 'query_string': cl.get_query_string({
+ self.lookup_kwarg: lookup}),
'display': title,
}
@@ -269,25 +278,19 @@ def choices(self, cl):
class DateFieldListFilter(FieldListFilter):
def __init__(self, field, request, params, model, model_admin, field_path):
- super(DateFieldListFilter, self).__init__(
- field, request, params, model, model_admin, field_path)
-
- self.field_generic = '%s__' % self.field_path
+ self.field_generic = '%s__' % field_path
self.date_params = dict([(k, v) for k, v in params.items()
if k.startswith(self.field_generic)])
-
today = datetime.date.today()
one_week_ago = today - datetime.timedelta(days=7)
today_str = str(today)
- if isinstance(self.field, models.DateTimeField):
+ if isinstance(field, models.DateTimeField):
today_str += ' 23:59:59'
-
- self.lookup_kwarg_year = '%s__year' % self.field_path
- self.lookup_kwarg_month = '%s__month' % self.field_path
- self.lookup_kwarg_day = '%s__day' % self.field_path
- self.lookup_kwarg_past_7_days_gte = '%s__gte' % self.field_path
- self.lookup_kwarg_past_7_days_lte = '%s__lte' % self.field_path
-
+ self.lookup_kwarg_year = '%s__year' % field_path
+ self.lookup_kwarg_month = '%s__month' % field_path
+ self.lookup_kwarg_day = '%s__day' % field_path
+ self.lookup_kwarg_past_7_days_gte = '%s__gte' % field_path
+ self.lookup_kwarg_past_7_days_lte = '%s__lte' % field_path
self.links = (
(_('Any date'), {}),
(_('Today'), {
@@ -307,31 +310,22 @@ def __init__(self, field, request, params, model, model_admin, field_path):
self.lookup_kwarg_year: str(today.year),
}),
)
+ super(DateFieldListFilter, self).__init__(
+ field, request, params, model, model_admin, field_path)
- def used_params(self):
+ def expected_parameters(self):
return [
- self.lookup_kwarg_year, self.lookup_kwarg_month, self.lookup_kwarg_day,
- self.lookup_kwarg_past_7_days_gte, self.lookup_kwarg_past_7_days_lte
+ self.lookup_kwarg_year, self.lookup_kwarg_month,
+ self.lookup_kwarg_day, self.lookup_kwarg_past_7_days_gte,
+ self.lookup_kwarg_past_7_days_lte
]
- def queryset(self, request, queryset):
- """
- Override the default behaviour since there can be multiple query
- string parameters used for the same date filter (e.g. year + month).
- """
- query_dict = {}
- for p in self.used_params():
- if p in self.params:
- query_dict[p] = self.params[p]
- if len(query_dict):
- return queryset.filter(**query_dict)
-
def choices(self, cl):
for title, param_dict in self.links:
yield {
'selected': self.date_params == param_dict,
'query_string': cl.get_query_string(
- param_dict, [self.field_generic]),
+ param_dict, [self.field_generic]),
'display': title,
}
@@ -344,13 +338,12 @@ def choices(self, cl):
# more appropriate, and the AllValuesFieldListFilter won't get used for it.
class AllValuesFieldListFilter(FieldListFilter):
def __init__(self, field, request, params, model, model_admin, field_path):
- super(AllValuesFieldListFilter, self).__init__(
- field, request, params, model, model_admin, field_path)
- self.lookup_kwarg = self.field_path
- self.lookup_kwarg_isnull = '%s__isnull' % self.field_path
+ self.lookup_kwarg = field_path
+ self.lookup_kwarg_isnull = '%s__isnull' % field_path
self.lookup_val = request.GET.get(self.lookup_kwarg, None)
- self.lookup_val_isnull = request.GET.get(self.lookup_kwarg_isnull, None)
- parent_model, reverse_path = reverse_field_path(model, self.field_path)
+ self.lookup_val_isnull = request.GET.get(self.lookup_kwarg_isnull,
+ None)
+ parent_model, reverse_path = reverse_field_path(model, field_path)
queryset = parent_model._default_manager.all()
# optional feature: limit choices base on existing relationships
# queryset = queryset.complex_filter(
@@ -358,10 +351,14 @@ def __init__(self, field, request, params, model, model_admin, field_path):
limit_choices_to = get_limit_choices_to_from_path(model, field_path)
queryset = queryset.filter(limit_choices_to)
- self.lookup_choices = queryset.distinct(
- ).order_by(field.name).values_list(field.name, flat=True)
+ self.lookup_choices = (queryset
+ .distinct()
+ .order_by(field.name)
+ .values_list(field.name, flat=True))
+ super(AllValuesFieldListFilter, self).__init__(
+ field, request, params, model, model_admin, field_path)
- def used_params(self):
+ def expected_parameters(self):
return [self.lookup_kwarg, self.lookup_kwarg_isnull]
def choices(self, cl):
View
27 django/contrib/admin/util.py
@@ -12,6 +12,33 @@
from django.utils.translation import ungettext
from django.core.urlresolvers import reverse
+def lookup_needs_distinct(opts, lookup_path):
+ """
+ Returns True if 'distinct()' should be used to query the given lookup path.
+ """
+ field_name = lookup_path.split('__', 1)[0]
+ field = opts.get_field_by_name(field_name)[0]
+ if ((hasattr(field, 'rel') and
+ isinstance(field.rel, models.ManyToManyRel)) or
+ (isinstance(field, models.related.RelatedObject) and
+ not field.field.unique)):
+ return True
+ return False
+
+def prepare_lookup_value(key, value):
+ """
+ Returns a lookup value prepared to be used in queryset filtering.
+ """
+ # if key ends with __in, split parameter into separate values
+ if key.endswith('__in'):
+ value = value.split(',')
+ # if key ends with __isnull, special case '' and false
+ if key.endswith('__isnull'):
+ if value.lower() in ('', 'false'):
+ value = False
+ else:
+ value = True
+ return value
def quote(s):
"""
View
145 django/contrib/admin/views/main.py
@@ -1,6 +1,6 @@
import operator
-from django.core.exceptions import SuspiciousOperation
+from django.core.exceptions import SuspiciousOperation, ImproperlyConfigured
from django.core.paginator import InvalidPage
from django.db import models
from django.utils.datastructures import SortedDict
@@ -10,7 +10,8 @@
from django.contrib.admin import FieldListFilter
from django.contrib.admin.options import IncorrectLookupParameters
-from django.contrib.admin.util import quote, get_fields_from_path
+from django.contrib.admin.util import (quote, get_fields_from_path,
+ lookup_needs_distinct, prepare_lookup_value)
# Changelist settings
ALL_VAR = 'all'
@@ -28,14 +29,6 @@
# Text to display within change-list table cells if the value is blank.
EMPTY_CHANGELIST_VALUE = ugettext_lazy('(None)')
-def field_needs_distinct(field):
- if ((hasattr(field, 'rel') and
- isinstance(field.rel, models.ManyToManyRel)) or
- (isinstance(field, models.related.RelatedObject) and
- not field.field.unique)):
- return True
- return False
-
class ChangeList(object):
def __init__(self, request, model, list_display, list_display_links,
@@ -84,14 +77,33 @@ def __init__(self, request, model, list_display, list_display_links,
self.title = title % force_unicode(self.opts.verbose_name)
self.pk_attname = self.lookup_opts.pk.attname
- def get_filters(self, request, use_distinct=False):
+ def get_filters(self, request):
+ lookup_params = self.params.copy() # a dictionary of the query string
+ use_distinct = False
+
+ # Remove all the parameters that are globally and systematically
+ # ignored.
+ for ignored in IGNORED_PARAMS:
+ if ignored in lookup_params:
+ del lookup_params[ignored]
+
+ # Normalize the types of keys
+ for key, value in lookup_params.items():
+ if not isinstance(key, str):
+ # 'key' will be used as a keyword argument later, so Python
+ # requires it to be a string.
+ del lookup_params[key]
+ lookup_params[smart_str(key)] = value
+
+ if not self.model_admin.lookup_allowed(key, value):
+ raise SuspiciousOperation("Filtering by %s not allowed" % key)
+
filter_specs = []
- cleaned_params, use_distinct = self.get_lookup_params(use_distinct)
if self.list_filter:
for list_filter in self.list_filter:
if callable(list_filter):
# This is simply a custom list filter class.
- spec = list_filter(request, cleaned_params,
+ spec = list_filter(request, lookup_params,
self.model, self.model_admin)
else:
field_path = None
@@ -106,11 +118,26 @@ def get_filters(self, request, use_distinct=False):
if not isinstance(field, models.Field):
field_path = field
field = get_fields_from_path(self.model, field_path)[-1]
- spec = field_list_filter_class(field, request, cleaned_params,
+ spec = field_list_filter_class(field, request, lookup_params,
self.model, self.model_admin, field_path=field_path)
+ # Check if we need to use distinct()
+ use_distinct = (use_distinct or
+ lookup_needs_distinct(self.lookup_opts,
+ field_path))
if spec and spec.has_output():
filter_specs.append(spec)
- return filter_specs, bool(filter_specs)
+
+ # At this point, all the parameters used by the various ListFilters
+ # have been removed from lookup_params, which now only contains other
+ # parameters passed via the query string. We now loop through the
+ # remaining parameters both to ensure that all the parameters are valid
+ # fields and to determine if at least one of them needs distinct().
+ for key, value in lookup_params.items():
+ lookup_params[key] = prepare_lookup_value(key, value)
+ use_distinct = (use_distinct or
+ lookup_needs_distinct(self.lookup_opts, key))
+
+ return filter_specs, bool(filter_specs), lookup_params, use_distinct
def get_query_string(self, new_params=None, remove=None):
if new_params is None: new_params = {}
@@ -247,78 +274,34 @@ def get_ordering_field_columns(self):
ordering_fields[idx] = 'desc' if pfx == '-' else 'asc'
return ordering_fields
- def get_lookup_params(self, use_distinct=False):
- lookup_params = self.params.copy() # a dictionary of the query string
-
- for ignored in IGNORED_PARAMS:
- if ignored in lookup_params:
- del lookup_params[ignored]
-
- for key, value in lookup_params.items():
- if not isinstance(key, str):
- # 'key' will be used as a keyword argument later, so Python
- # requires it to be a string.
- del lookup_params[key]
- lookup_params[smart_str(key)] = value
-
- field = None
- if not use_distinct:
- # Check if it's a relationship that might return more than one
- # instance
- field_name = key.split('__', 1)[0]
- try:
- field = self.lookup_opts.get_field_by_name(field_name)[0]
- use_distinct = field_needs_distinct(field)
- except models.FieldDoesNotExist:
- # It might be a custom NonFieldFilter
- pass
-
- # if key ends with __in, split parameter into separate values
- if key.endswith('__in'):
- value = value.split(',')
- lookup_params[key] = value
-
- # if key ends with __isnull, special case '' and false
- if key.endswith('__isnull'):
- if value.lower() in ('', 'false'):
- value = False
- else:
- value = True
- lookup_params[key] = value
-
- if field and not self.model_admin.lookup_allowed(key, value):
- raise SuspiciousOperation("Filtering by %s not allowed" % key)
-
- return lookup_params, use_distinct
-
def get_query_set(self, request):
- lookup_params, use_distinct = self.get_lookup_params(use_distinct=False)
- self.filter_specs, self.has_filters = self.get_filters(request, use_distinct)
-
try:
- # First, let every list filter modify the qs and params to its
- # liking.
+ # First, we collect all the declared list filters.
+ (self.filter_specs, self.has_filters, remaining_lookup_params,
+ use_distinct) = self.get_filters(request)
+
+ # Then, we let every list filter modify the qs to its liking.
qs = self.root_query_set
for filter_spec in self.filter_specs:
new_qs = filter_spec.queryset(request, qs)
if new_qs is not None:
qs = new_qs
- for param in filter_spec.used_params():
- try:
- del lookup_params[param]
- except KeyError:
- pass
-
- # Then, apply the remaining lookup parameters from the query string
- # (i.e. those that haven't already been processed by the filters).
- qs = qs.filter(**lookup_params)
+
+ # Finally, we apply the remaining lookup parameters from the query
+ # string (i.e. those that haven't already been processed by the
+ # filters).
+ qs = qs.filter(**remaining_lookup_params)
+ except (SuspiciousOperation, ImproperlyConfigured):
+ # Allow certain types of errors to be re-raised as-is so that the
+ # caller can treat them in a special way.
+ raise
except Exception, e:
- # Naked except! Because we don't have any other way of validating
- # "lookup_params". They might be invalid if the keyword arguments
- # are incorrect, or if the values are not in the correct type, so
- # we might get FieldError, ValueError, ValicationError, or ? from a
- # custom field that raises yet something else when handed
- # impossible data.
+ # Every other error is caught with a naked except, because we don't
+ # have any other way of validating lookup parameters. They might be
+ # invalid if the keyword arguments are incorrect, or if the values
+ # are not in the correct type, so we might get FieldError,
+ # ValueError, ValidationError, or ? from a custom field that raises
+ # yet something else when handed impossible data.
raise IncorrectLookupParameters(e)
# Use select_related() if one of the list_display options is a field
@@ -362,9 +345,7 @@ def construct_search(field_name):
qs = qs.filter(reduce(operator.or_, or_queries))
if not use_distinct:
for search_spec in orm_lookups:
- field_name = search_spec.split('__', 1)[0]
- f = self.lookup_opts.get_field_by_name(field_name)[0]
- if field_needs_distinct(f):
+ if lookup_needs_distinct(self.lookup_opts, search_spec):
use_distinct = True
break
View
55 tests/regressiontests/admin_filters/tests.py
@@ -68,6 +68,14 @@ def lookups(self, request, model_admin):
if qs.filter(year__gte=2000, year__lte=2009).exists():
yield ('the 00s', "the 2000's")
+class DecadeListFilterParameterEndsWith__In(DecadeListFilter):
+ title = 'publication decade'
+ parameter_name = 'decade__in' # Ends with '__in"
+
+class DecadeListFilterParameterEndsWith__Isnull(DecadeListFilter):
+ title = 'publication decade'
+ parameter_name = 'decade__isnull' # Ends with '__isnull"
+
class CustomUserAdmin(UserAdmin):
list_filter = ('books_authored', 'books_contributed')
@@ -97,6 +105,12 @@ class DecadeFilterBookAdminWithFailingQueryset(ModelAdmin):
class DecadeFilterBookAdminWithQuerysetBasedLookups(ModelAdmin):
list_filter = (DecadeListFilterWithQuerysetBasedLookups,)
+class DecadeFilterBookAdminParameterEndsWith__In(ModelAdmin):
+ list_filter = (DecadeListFilterParameterEndsWith__In,)
+
+class DecadeFilterBookAdminParameterEndsWith__Isnull(ModelAdmin):
+ list_filter = (DecadeListFilterParameterEndsWith__Isnull,)
+
class ListFiltersTests(TestCase):
def setUp(self):
@@ -570,3 +584,44 @@ def test_two_characters_long_field(self):
choices = list(filterspec.choices(changelist))
self.assertEqual(choices[2]['selected'], True)
self.assertEqual(choices[2]['query_string'], '?no=207')
+
+ def test_parameter_ends_with__in__or__isnull(self):
+ """
+ Ensure that a SimpleListFilter's parameter name is not mistaken for a
+ model field if it ends with '__isnull' or '__in'.
+ Refs #17091.
+ """
+
+ # When it ends with '__in' -----------------------------------------
+ modeladmin = DecadeFilterBookAdminParameterEndsWith__In(Book, site)
+ request = self.request_factory.get('/', {'decade__in': 'the 90s'})
+ changelist = self.get_changelist(request, Book, modeladmin)
+
+ # Make sure the correct queryset is returned
+ queryset = changelist.get_query_set(request)
+ self.assertEqual(list(queryset), [self.bio_book])
+
+ # Make sure the correct choice is selected
+ filterspec = changelist.get_filters(request)[0][0]
+ self.assertEqual(force_unicode(filterspec.title), u'publication decade')
+ choices = list(filterspec.choices(changelist))
+ self.assertEqual(choices[2]['display'], u'the 1990\'s')
+ self.assertEqual(choices[2]['selected'], True)
+ self.assertEqual(choices[2]['query_string'], '?decade__in=the+90s')
+
+ # When it ends with '__isnull' ---------------------------------------
+ modeladmin = DecadeFilterBookAdminParameterEndsWith__Isnull(Book, site)
+ request = self.request_factory.get('/', {'decade__isnull': 'the 90s'})
+ changelist = self.get_changelist(request, Book, modeladmin)
+
+ # Make sure the correct queryset is returned
+ queryset = changelist.get_query_set(request)
+ self.assertEqual(list(queryset), [self.bio_book])
+
+ # Make sure the correct choice is selected
+ filterspec = changelist.get_filters(request)[0][0]
+ self.assertEqual(force_unicode(filterspec.title), u'publication decade')
+ choices = list(filterspec.choices(changelist))
+ self.assertEqual(choices[2]['display'], u'the 1990\'s')
+ self.assertEqual(choices[2]['selected'], True)
+ self.assertEqual(choices[2]['query_string'], '?decade__isnull=the+90s')

0 comments on commit a89b156

Please sign in to comment.
Something went wrong with that request. Please try again.