From a89b15628429da79dd47c04bd191de566ef7eee6 Mon Sep 17 00:00:00 2001 From: Julien Phalip Date: Tue, 22 Nov 2011 12:26:17 +0000 Subject: [PATCH] Disentangled some parts of the admin ChangeList and ListFilter's internals. 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 --- django/contrib/admin/filters.py | 139 +++++++++--------- django/contrib/admin/util.py | 27 ++++ django/contrib/admin/views/main.py | 143 ++++++++----------- tests/regressiontests/admin_filters/tests.py | 55 +++++++ 4 files changed, 212 insertions(+), 152 deletions(-) diff --git a/django/contrib/admin/filters.py b/django/contrib/admin/filters.py index ed98a9ebb5..b2d74ce671 100644 --- a/django/contrib/admin/filters.py +++ b/django/contrib/admin/filters.py @@ -13,13 +13,15 @@ from django.utils.encoding import smart_unicode 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 @@ class ListFilter(object): 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 @@ class ListFilter(object): """ 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 @@ class SimpleListFilter(ListFilter): 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 @@ class SimpleListFilter(ListFilter): """ raise NotImplementedError - def used_params(self): + def expected_parameters(self): return [self.parameter_name] def choices(self, cl): @@ -111,15 +116,18 @@ class FieldListFilter(ListFilter): 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 @@ class FieldListFilter(ListFilter): 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 @@ class RelatedFieldListFilter(FieldListFilter): 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 @@ FieldListFilter.register(lambda f: ( 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 @@ FieldListFilter.register(lambda f: isinstance(f, 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 @@ class ChoicesFieldListFilter(FieldListFilter): 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 @@ FieldListFilter.register(lambda f: bool(f.choices), ChoicesFieldListFilter) 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 @@ class DateFieldListFilter(FieldListFilter): 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 @@ FieldListFilter.register( # 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 @@ class AllValuesFieldListFilter(FieldListFilter): 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): diff --git a/django/contrib/admin/util.py b/django/contrib/admin/util.py index 551146472f..61182a6d02 100644 --- a/django/contrib/admin/util.py +++ b/django/contrib/admin/util.py @@ -12,6 +12,33 @@ from django.utils.encoding import force_unicode, smart_unicode, smart_str 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): """ diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index 0294be187e..32113f56eb 100644 --- a/django/contrib/admin/views/main.py +++ b/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.utils.http import urlencode 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 @@ IGNORED_PARAMS = ( # 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 @@ class ChangeList(object): 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 @@ class ChangeList(object): 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 @@ class ChangeList(object): 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 @@ class ChangeList(object): 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 diff --git a/tests/regressiontests/admin_filters/tests.py b/tests/regressiontests/admin_filters/tests.py index 28693aef98..4988e57e61 100644 --- a/tests/regressiontests/admin_filters/tests.py +++ b/tests/regressiontests/admin_filters/tests.py @@ -68,6 +68,14 @@ class DecadeListFilterWithQuerysetBasedLookups(DecadeListFilterWithTitleAndParam 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 @@ class ListFiltersTests(TestCase): 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') \ No newline at end of file