diff options
| -rw-r--r-- | django/contrib/admin/filters.py | 139 | ||||
| -rw-r--r-- | django/contrib/admin/util.py | 27 | ||||
| -rw-r--r-- | django/contrib/admin/views/main.py | 143 | ||||
| -rw-r--r-- | 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 |