From 1ff9be1144e80802ea810e6e70ef2b54b6e42047 Mon Sep 17 00:00:00 2001 From: Julien Phalip Date: Sat, 17 Mar 2012 21:45:36 +0000 Subject: [PATCH] Fixed #17828 -- Ensured that when a list filter's `queryset()` method fails, it does so loudly instead of getting swallowed by a `IncorrectLookupParameters` exception. This also properly fixes #16705, which hadn't been addressed correctly in [16705]. git-svn-id: http://code.djangoproject.com/svn/django/trunk@17763 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/contrib/admin/options.py | 20 ++++----- django/contrib/admin/views/main.py | 43 +++++++++++--------- tests/regressiontests/admin_filters/tests.py | 14 ++++--- 3 files changed, 40 insertions(+), 37 deletions(-) diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index f5f6256f93..73c2958103 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -247,7 +247,12 @@ class BaseModelAdmin(object): # the pk attribute name. pk_attr_name = None for part in parts[:-1]: - field, _, _, _ = model._meta.get_field_by_name(part) + try: + field, _, _, _ = model._meta.get_field_by_name(part) + except FieldDoesNotExist: + # Lookups on non-existants fields are ok, since they're ignored + # later. + return True if hasattr(field, 'rel'): model = field.rel.to pk_attr_name = model._meta.pk.name @@ -259,17 +264,10 @@ class BaseModelAdmin(object): if pk_attr_name and len(parts) > 1 and parts[-1] == pk_attr_name: parts.pop() - try: - self.model._meta.get_field_by_name(parts[0]) - except FieldDoesNotExist: - # Lookups on non-existants fields are ok, since they're ignored - # later. + if len(parts) == 1: return True - else: - if len(parts) == 1: - return True - clean_lookup = LOOKUP_SEP.join(parts) - return clean_lookup in self.list_filter or clean_lookup == self.date_hierarchy + clean_lookup = LOOKUP_SEP.join(parts) + return clean_lookup in self.list_filter or clean_lookup == self.date_hierarchy def has_add_permission(self, request): """ diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index 56f13f8099..b11f9d566b 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -3,6 +3,7 @@ import operator from django.core.exceptions import SuspiciousOperation, ImproperlyConfigured from django.core.paginator import InvalidPage from django.db import models +from django.db.models.fields import FieldDoesNotExist from django.utils.datastructures import SortedDict from django.utils.encoding import force_unicode, smart_str from django.utils.translation import ugettext, ugettext_lazy @@ -130,13 +131,16 @@ class ChangeList(object): # 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 + # fields and to determine if at least one of them needs distinct(). If + # the lookup parameters aren't real fields, then bail out. + try: + 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 + except FieldDoesNotExist, e: + raise IncorrectLookupParameters(e) def get_query_string(self, new_params=None, remove=None): if new_params is None: new_params = {} @@ -292,18 +296,18 @@ class ChangeList(object): return ordering_fields def get_query_set(self, request): + # 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 queryset 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 + try: - # 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 - # Finally, we apply the remaining lookup parameters from the query # string (i.e. those that haven't already been processed by the # filters). @@ -317,8 +321,7 @@ class ChangeList(object): # 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. + # ValueError, ValidationError, or ?. raise IncorrectLookupParameters(e) # Use select_related() if one of the list_display options is a field diff --git a/tests/regressiontests/admin_filters/tests.py b/tests/regressiontests/admin_filters/tests.py index 539bb0cc7a..b42dfd700d 100644 --- a/tests/regressiontests/admin_filters/tests.py +++ b/tests/regressiontests/admin_filters/tests.py @@ -56,7 +56,7 @@ class DecadeListFilterWithNoneReturningLookups(DecadeListFilterWithTitleAndParam class DecadeListFilterWithFailingQueryset(DecadeListFilterWithTitleAndParameter): def queryset(self, request, queryset): - raise Exception + raise 1/0 class DecadeListFilterWithQuerysetBasedLookups(DecadeListFilterWithTitleAndParameter): @@ -77,6 +77,7 @@ class DecadeListFilterParameterEndsWith__Isnull(DecadeListFilter): title = 'publication decade' parameter_name = 'decade__isnull' # Ends with '__isnull" + class CustomUserAdmin(UserAdmin): list_filter = ('books_authored', 'books_contributed') @@ -112,6 +113,8 @@ class DecadeFilterBookAdminParameterEndsWith__In(ModelAdmin): class DecadeFilterBookAdminParameterEndsWith__Isnull(ModelAdmin): list_filter = (DecadeListFilterParameterEndsWith__Isnull,) + + class ListFiltersTests(TestCase): def setUp(self): @@ -542,14 +545,13 @@ class ListFiltersTests(TestCase): def test_filter_with_failing_queryset(self): """ - Ensure that a filter's failing queryset is interpreted as if incorrect - lookup parameters were passed (therefore causing a 302 redirection to - the changelist). - Refs #16716, #16714. + Ensure that when a filter's queryset method fails, it fails loudly and + the corresponding exception doesn't get swallowed. + Refs #17828. """ modeladmin = DecadeFilterBookAdminWithFailingQueryset(Book, site) request = self.request_factory.get('/', {}) - self.assertRaises(IncorrectLookupParameters, self.get_changelist, request, Book, modeladmin) + self.assertRaises(ZeroDivisionError, self.get_changelist, request, Book, modeladmin) def test_simplelistfilter_with_queryset_based_lookups(self): modeladmin = DecadeFilterBookAdminWithQuerysetBasedLookups(Book, site)