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

@@ -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:
+        if len(parts) == 1:
-            self.model._meta.get_field_by_name(parts[0])
-        except FieldDoesNotExist:
-            # Lookups on non-existants fields are ok, since they're ignored
-            # later.
             return True
-        else:
+        clean_lookup = LOOKUP_SEP.join(parts)
-            if len(parts) == 1:
+        return clean_lookup in self.list_filter or clean_lookup == self.date_hierarchy
-                return True
-            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):
         """

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().
+        # fields and to determine if at least one of them needs distinct(). If
-        for key, value in lookup_params.items():
+        # the lookup parameters aren't real fields, then bail out.
-            lookup_params[key] = prepare_lookup_value(key, value)
+        try:
-            use_distinct = (use_distinct or
+            for key, value in lookup_params.items():
-                            lookup_needs_distinct(self.lookup_opts, key))
+                lookup_params[key] = prepare_lookup_value(key, value)
-
+                use_distinct = (use_distinct or
-        return filter_specs, bool(filter_specs), lookup_params, use_distinct
+                                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
+            # ValueError, ValidationError, or ?.
-            # yet something else when handed impossible data.
             raise IncorrectLookupParameters(e)
 
         # Use select_related() if one of the list_display options is a field

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
+        Ensure that when a filter's queryset method fails, it fails loudly and
-        lookup parameters were passed (therefore causing a 302 redirection to
+        the corresponding exception doesn't get swallowed.
-        the changelist).
+        Refs #17828.
-        Refs #16716, #16714.
         """
         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)