Ticket #17828: 17828.listfilter-proper-error-handling.2.diff

django/contrib/admin/options.py

@@ -247 +247 @@
         # 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 +264 @@
         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):
         """

django/contrib/admin/views/main.py

@@ -130 +130 @@
         # 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
+        # an error occurs then it means the lookup parameters are incorrect, so
+        # we raise the proper exception.
+        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 Exception, e:
+            # 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 ?
+            raise IncorrectLookupParameters(e)
 
     def get_query_string(self, new_params=None, remove=None):
         if new_params is None: new_params = {}
@@ -292 +301 @@
         return ordering_fields
 
     def get_query_set(self, request):
-        try:
-            # First, we collect all the declared list filters.
-            (self.filter_specs, self.has_filters, remaining_lookup_params,
-             use_distinct) = self.get_filters(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 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
+        # 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:
             # Finally, we apply the remaining lookup parameters from the query
             # string (i.e. those that haven't already been processed by the
             # filters).
@@ -317 +326 @@
             # 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

tests/regressiontests/admin_filters/tests.py

@@ -56 +56 @@
 class DecadeListFilterWithFailingQueryset(DecadeListFilterWithTitleAndParameter):
 
     def queryset(self, request, queryset):
-        raise Exception
+        raise 1/0
 
 class DecadeListFilterWithQuerysetBasedLookups(DecadeListFilterWithTitleAndParameter):
 
@@ -77 +77 @@
     title = 'publication decade'
     parameter_name = 'decade__isnull' # Ends with '__isnull"
 
+
 class CustomUserAdmin(UserAdmin):
     list_filter = ('books_authored', 'books_contributed')
 
@@ -112 +113 @@
 class DecadeFilterBookAdminParameterEndsWith__Isnull(ModelAdmin):
     list_filter = (DecadeListFilterParameterEndsWith__Isnull,)
 
+
+
 class ListFiltersTests(TestCase):
 
     def setUp(self):
@@ -542 +545 @@
 
     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)