Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

Disentangled some parts of the admin ChangeList and ListFilter's inte…

…rnals. 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
  • Loading branch information...
commit a89b15628429da79dd47c04bd191de566ef7eee6 1 parent e71f336
Julien Phalip authored November 22, 2011
139  django/contrib/admin/filters.py
@@ -13,13 +13,15 @@
13 13
 from django.utils.translation import ugettext_lazy as _
14 14
 
15 15
 from django.contrib.admin.util import (get_model_from_relation,
16  
-    reverse_field_path, get_limit_choices_to_from_path)
  16
+    reverse_field_path, get_limit_choices_to_from_path, prepare_lookup_value)
17 17
 
18 18
 class ListFilter(object):
19 19
     title = None  # Human-readable title to appear in the right sidebar.
20 20
 
21 21
     def __init__(self, request, params, model, model_admin):
22  
-        self.params = params
  22
+        # This dictionary will eventually contain the request's query string
  23
+        # parameters actually used by this filter.
  24
+        self.used_parameters = {}
23 25
         if self.title is None:
24 26
             raise ImproperlyConfigured(
25 27
                 "The list filter '%s' does not specify "
@@ -27,7 +29,7 @@ def __init__(self, request, params, model, model_admin):
27 29
 
28 30
     def has_output(self):
29 31
         """
30  
-        Returns True if some choices would be output for the filter.
  32
+        Returns True if some choices would be output for this filter.
31 33
         """
32 34
         raise NotImplementedError
33 35
 
@@ -43,15 +45,14 @@ def queryset(self, request, queryset):
43 45
         """
44 46
         raise NotImplementedError
45 47
 
46  
-    def used_params(self):
  48
+    def expected_parameters(self):
47 49
         """
48  
-        Return a list of parameters to consume from the change list
49  
-        querystring.
  50
+        Returns the list of parameter names that are expected from the
  51
+        request's query string and that will be used by this filter.
50 52
         """
51 53
         raise NotImplementedError
52 54
 
53 55
 
54  
-
55 56
 class SimpleListFilter(ListFilter):
56 57
     # The parameter that should be used in the query string for that filter.
57 58
     parameter_name = None
@@ -67,16 +68,20 @@ def __init__(self, request, params, model, model_admin):
67 68
         if lookup_choices is None:
68 69
             lookup_choices = ()
69 70
         self.lookup_choices = list(lookup_choices)
  71
+        if self.parameter_name in params:
  72
+            value = params.pop(self.parameter_name)
  73
+            self.used_parameters[self.parameter_name] = value
70 74
 
71 75
     def has_output(self):
72 76
         return len(self.lookup_choices) > 0
73 77
 
74 78
     def value(self):
75 79
         """
76  
-        Returns the value given in the query string for this filter,
77  
-        if any. Returns None otherwise.
  80
+        Returns the value (in string format) provided in the request's
  81
+        query string for this filter, if any. If the value wasn't provided then
  82
+        returns None.
78 83
         """
79  
-        return self.params.get(self.parameter_name, None)
  84
+        return self.used_parameters.get(self.parameter_name, None)
80 85
 
81 86
     def lookups(self, request, model_admin):
82 87
         """
@@ -84,7 +89,7 @@ def lookups(self, request, model_admin):
84 89
         """
85 90
         raise NotImplementedError
86 91
 
87  
-    def used_params(self):
  92
+    def expected_parameters(self):
88 93
         return [self.parameter_name]
89 94
 
90 95
     def choices(self, cl):
@@ -111,15 +116,18 @@ def __init__(self, field, request, params, model, model_admin, field_path):
111 116
         self.field = field
112 117
         self.field_path = field_path
113 118
         self.title = getattr(field, 'verbose_name', field_path)
114  
-        super(FieldListFilter, self).__init__(request, params, model, model_admin)
  119
+        super(FieldListFilter, self).__init__(
  120
+            request, params, model, model_admin)
  121
+        for p in self.expected_parameters():
  122
+            if p in params:
  123
+                value = params.pop(p)
  124
+                self.used_parameters[p] = prepare_lookup_value(p, value)
115 125
 
116 126
     def has_output(self):
117 127
         return True
118 128
 
119 129
     def queryset(self, request, queryset):
120  
-        for p in self.used_params():
121  
-            if p in self.params:
122  
-                return queryset.filter(**{p: self.params[p]})
  130
+        return queryset.filter(**self.used_parameters)
123 131
 
124 132
     @classmethod
125 133
     def register(cls, test, list_filter_class, take_priority=False):
@@ -144,20 +152,20 @@ def create(cls, field, request, params, model, model_admin, field_path):
144 152
 
145 153
 class RelatedFieldListFilter(FieldListFilter):
146 154
     def __init__(self, field, request, params, model, model_admin, field_path):
147  
-        super(RelatedFieldListFilter, self).__init__(
148  
-            field, request, params, model, model_admin, field_path)
149 155
         other_model = get_model_from_relation(field)
150  
-        if hasattr(field, 'verbose_name'):
151  
-            self.lookup_title = field.verbose_name
152  
-        else:
153  
-            self.lookup_title = other_model._meta.verbose_name
154 156
         rel_name = other_model._meta.pk.name
155  
-        self.lookup_kwarg = '%s__%s__exact' % (self.field_path, rel_name)
156  
-        self.lookup_kwarg_isnull = '%s__isnull' % (self.field_path)
  157
+        self.lookup_kwarg = '%s__%s__exact' % (field_path, rel_name)
  158
+        self.lookup_kwarg_isnull = '%s__isnull' % field_path
157 159
         self.lookup_val = request.GET.get(self.lookup_kwarg, None)
158 160
         self.lookup_val_isnull = request.GET.get(
159 161
                                       self.lookup_kwarg_isnull, None)
160 162
         self.lookup_choices = field.get_choices(include_blank=False)
  163
+        super(RelatedFieldListFilter, self).__init__(
  164
+            field, request, params, model, model_admin, field_path)
  165
+        if hasattr(field, 'verbose_name'):
  166
+            self.lookup_title = field.verbose_name
  167
+        else:
  168
+            self.lookup_title = other_model._meta.verbose_name
161 169
         self.title = self.lookup_title
162 170
 
163 171
     def has_output(self):
@@ -169,7 +177,7 @@ def has_output(self):
169 177
             extra = 0
170 178
         return len(self.lookup_choices) + extra > 1
171 179
 
172  
-    def used_params(self):
  180
+    def expected_parameters(self):
173 181
         return [self.lookup_kwarg, self.lookup_kwarg_isnull]
174 182
 
175 183
     def choices(self, cl):
@@ -206,14 +214,14 @@ def choices(self, cl):
206 214
 
207 215
 class BooleanFieldListFilter(FieldListFilter):
208 216
     def __init__(self, field, request, params, model, model_admin, field_path):
209  
-        super(BooleanFieldListFilter, self).__init__(field,
210  
-            request, params, model, model_admin, field_path)
211  
-        self.lookup_kwarg = '%s__exact' % self.field_path
212  
-        self.lookup_kwarg2 = '%s__isnull' % self.field_path
  217
+        self.lookup_kwarg = '%s__exact' % field_path
  218
+        self.lookup_kwarg2 = '%s__isnull' % field_path
213 219
         self.lookup_val = request.GET.get(self.lookup_kwarg, None)
214 220
         self.lookup_val2 = request.GET.get(self.lookup_kwarg2, None)
  221
+        super(BooleanFieldListFilter, self).__init__(field,
  222
+            request, params, model, model_admin, field_path)
215 223
 
216  
-    def used_params(self):
  224
+    def expected_parameters(self):
217 225
         return [self.lookup_kwarg, self.lookup_kwarg2]
218 226
 
219 227
     def choices(self, cl):
@@ -243,12 +251,12 @@ def choices(self, cl):
243 251
 
244 252
 class ChoicesFieldListFilter(FieldListFilter):
245 253
     def __init__(self, field, request, params, model, model_admin, field_path):
  254
+        self.lookup_kwarg = '%s__exact' % field_path
  255
+        self.lookup_val = request.GET.get(self.lookup_kwarg)
246 256
         super(ChoicesFieldListFilter, self).__init__(
247 257
             field, request, params, model, model_admin, field_path)
248  
-        self.lookup_kwarg = '%s__exact' % self.field_path
249  
-        self.lookup_val = request.GET.get(self.lookup_kwarg)
250 258
 
251  
-    def used_params(self):
  259
+    def expected_parameters(self):
252 260
         return [self.lookup_kwarg]
253 261
 
254 262
     def choices(self, cl):
@@ -260,7 +268,8 @@ def choices(self, cl):
260 268
         for lookup, title in self.field.flatchoices:
261 269
             yield {
262 270
                 'selected': smart_unicode(lookup) == self.lookup_val,
263  
-                'query_string': cl.get_query_string({self.lookup_kwarg: lookup}),
  271
+                'query_string': cl.get_query_string({
  272
+                                    self.lookup_kwarg: lookup}),
264 273
                 'display': title,
265 274
             }
266 275
 
@@ -269,25 +278,19 @@ def choices(self, cl):
269 278
 
270 279
 class DateFieldListFilter(FieldListFilter):
271 280
     def __init__(self, field, request, params, model, model_admin, field_path):
272  
-        super(DateFieldListFilter, self).__init__(
273  
-            field, request, params, model, model_admin, field_path)
274  
-
275  
-        self.field_generic = '%s__' % self.field_path
  281
+        self.field_generic = '%s__' % field_path
276 282
         self.date_params = dict([(k, v) for k, v in params.items()
277 283
                                  if k.startswith(self.field_generic)])
278  
-
279 284
         today = datetime.date.today()
280 285
         one_week_ago = today - datetime.timedelta(days=7)
281 286
         today_str = str(today)
282  
-        if isinstance(self.field, models.DateTimeField):
  287
+        if isinstance(field, models.DateTimeField):
283 288
             today_str += ' 23:59:59'
284  
-
285  
-        self.lookup_kwarg_year = '%s__year' % self.field_path
286  
-        self.lookup_kwarg_month = '%s__month' % self.field_path
287  
-        self.lookup_kwarg_day = '%s__day' % self.field_path
288  
-        self.lookup_kwarg_past_7_days_gte = '%s__gte' % self.field_path
289  
-        self.lookup_kwarg_past_7_days_lte = '%s__lte' % self.field_path
290  
-
  289
+        self.lookup_kwarg_year = '%s__year' % field_path
  290
+        self.lookup_kwarg_month = '%s__month' % field_path
  291
+        self.lookup_kwarg_day = '%s__day' % field_path
  292
+        self.lookup_kwarg_past_7_days_gte = '%s__gte' % field_path
  293
+        self.lookup_kwarg_past_7_days_lte = '%s__lte' % field_path
291 294
         self.links = (
292 295
             (_('Any date'), {}),
293 296
             (_('Today'), {
@@ -307,31 +310,22 @@ def __init__(self, field, request, params, model, model_admin, field_path):
307 310
                 self.lookup_kwarg_year: str(today.year),
308 311
             }),
309 312
         )
  313
+        super(DateFieldListFilter, self).__init__(
  314
+            field, request, params, model, model_admin, field_path)
310 315
 
311  
-    def used_params(self):
  316
+    def expected_parameters(self):
312 317
         return [
313  
-            self.lookup_kwarg_year, self.lookup_kwarg_month, self.lookup_kwarg_day,
314  
-            self.lookup_kwarg_past_7_days_gte, self.lookup_kwarg_past_7_days_lte
  318
+            self.lookup_kwarg_year, self.lookup_kwarg_month,
  319
+            self.lookup_kwarg_day, self.lookup_kwarg_past_7_days_gte,
  320
+            self.lookup_kwarg_past_7_days_lte
315 321
         ]
316 322
 
317  
-    def queryset(self, request, queryset):
318  
-        """
319  
-        Override the default behaviour since there can be multiple query
320  
-        string parameters used for the same date filter (e.g. year + month).
321  
-        """
322  
-        query_dict = {}
323  
-        for p in self.used_params():
324  
-            if p in self.params:
325  
-                query_dict[p] = self.params[p]
326  
-        if len(query_dict):
327  
-            return queryset.filter(**query_dict)
328  
-
329 323
     def choices(self, cl):
330 324
         for title, param_dict in self.links:
331 325
             yield {
332 326
                 'selected': self.date_params == param_dict,
333 327
                 'query_string': cl.get_query_string(
334  
-                    param_dict, [self.field_generic]),
  328
+                                    param_dict, [self.field_generic]),
335 329
                 'display': title,
336 330
             }
337 331
 
@@ -344,13 +338,12 @@ def choices(self, cl):
344 338
 # more appropriate, and the AllValuesFieldListFilter won't get used for it.
345 339
 class AllValuesFieldListFilter(FieldListFilter):
346 340
     def __init__(self, field, request, params, model, model_admin, field_path):
347  
-        super(AllValuesFieldListFilter, self).__init__(
348  
-            field, request, params, model, model_admin, field_path)
349  
-        self.lookup_kwarg = self.field_path
350  
-        self.lookup_kwarg_isnull = '%s__isnull' % self.field_path
  341
+        self.lookup_kwarg = field_path
  342
+        self.lookup_kwarg_isnull = '%s__isnull' % field_path
351 343
         self.lookup_val = request.GET.get(self.lookup_kwarg, None)
352  
-        self.lookup_val_isnull = request.GET.get(self.lookup_kwarg_isnull, None)
353  
-        parent_model, reverse_path = reverse_field_path(model, self.field_path)
  344
+        self.lookup_val_isnull = request.GET.get(self.lookup_kwarg_isnull,
  345
+                                                 None)
  346
+        parent_model, reverse_path = reverse_field_path(model, field_path)
354 347
         queryset = parent_model._default_manager.all()
355 348
         # optional feature: limit choices base on existing relationships
356 349
         # queryset = queryset.complex_filter(
@@ -358,10 +351,14 @@ def __init__(self, field, request, params, model, model_admin, field_path):
358 351
         limit_choices_to = get_limit_choices_to_from_path(model, field_path)
359 352
         queryset = queryset.filter(limit_choices_to)
360 353
 
361  
-        self.lookup_choices = queryset.distinct(
362  
-            ).order_by(field.name).values_list(field.name, flat=True)
  354
+        self.lookup_choices = (queryset
  355
+                               .distinct()
  356
+                               .order_by(field.name)
  357
+                               .values_list(field.name, flat=True))
  358
+        super(AllValuesFieldListFilter, self).__init__(
  359
+            field, request, params, model, model_admin, field_path)
363 360
 
364  
-    def used_params(self):
  361
+    def expected_parameters(self):
365 362
         return [self.lookup_kwarg, self.lookup_kwarg_isnull]
366 363
 
367 364
     def choices(self, cl):
27  django/contrib/admin/util.py
@@ -12,6 +12,33 @@
12 12
 from django.utils.translation import ungettext
13 13
 from django.core.urlresolvers import reverse
14 14
 
  15
+def lookup_needs_distinct(opts, lookup_path):
  16
+    """
  17
+    Returns True if 'distinct()' should be used to query the given lookup path.
  18
+    """
  19
+    field_name = lookup_path.split('__', 1)[0]
  20
+    field = opts.get_field_by_name(field_name)[0]
  21
+    if ((hasattr(field, 'rel') and
  22
+         isinstance(field.rel, models.ManyToManyRel)) or
  23
+        (isinstance(field, models.related.RelatedObject) and
  24
+         not field.field.unique)):
  25
+         return True
  26
+    return False
  27
+
  28
+def prepare_lookup_value(key, value):
  29
+    """
  30
+    Returns a lookup value prepared to be used in queryset filtering.
  31
+    """
  32
+    # if key ends with __in, split parameter into separate values
  33
+    if key.endswith('__in'):
  34
+        value = value.split(',')
  35
+    # if key ends with __isnull, special case '' and false
  36
+    if key.endswith('__isnull'):
  37
+        if value.lower() in ('', 'false'):
  38
+            value = False
  39
+        else:
  40
+            value = True
  41
+    return value
15 42
 
16 43
 def quote(s):
17 44
     """
145  django/contrib/admin/views/main.py
... ...
@@ -1,6 +1,6 @@
1 1
 import operator
2 2
 
3  
-from django.core.exceptions import SuspiciousOperation
  3
+from django.core.exceptions import SuspiciousOperation, ImproperlyConfigured
4 4
 from django.core.paginator import InvalidPage
5 5
 from django.db import models
6 6
 from django.utils.datastructures import SortedDict
@@ -10,7 +10,8 @@
10 10
 
11 11
 from django.contrib.admin import FieldListFilter
12 12
 from django.contrib.admin.options import IncorrectLookupParameters
13  
-from django.contrib.admin.util import quote, get_fields_from_path
  13
+from django.contrib.admin.util import (quote, get_fields_from_path,
  14
+    lookup_needs_distinct, prepare_lookup_value)
14 15
 
15 16
 # Changelist settings
16 17
 ALL_VAR = 'all'
@@ -28,14 +29,6 @@
28 29
 # Text to display within change-list table cells if the value is blank.
29 30
 EMPTY_CHANGELIST_VALUE = ugettext_lazy('(None)')
30 31
 
31  
-def field_needs_distinct(field):
32  
-    if ((hasattr(field, 'rel') and
33  
-         isinstance(field.rel, models.ManyToManyRel)) or
34  
-        (isinstance(field, models.related.RelatedObject) and
35  
-         not field.field.unique)):
36  
-         return True
37  
-    return False
38  
-
39 32
 
40 33
 class ChangeList(object):
41 34
     def __init__(self, request, model, list_display, list_display_links,
@@ -84,14 +77,33 @@ def __init__(self, request, model, list_display, list_display_links,
84 77
         self.title = title % force_unicode(self.opts.verbose_name)
85 78
         self.pk_attname = self.lookup_opts.pk.attname
86 79
 
87  
-    def get_filters(self, request, use_distinct=False):
  80
+    def get_filters(self, request):
  81
+        lookup_params = self.params.copy() # a dictionary of the query string
  82
+        use_distinct = False
  83
+
  84
+        # Remove all the parameters that are globally and systematically
  85
+        # ignored.
  86
+        for ignored in IGNORED_PARAMS:
  87
+            if ignored in lookup_params:
  88
+                del lookup_params[ignored]
  89
+
  90
+        # Normalize the types of keys
  91
+        for key, value in lookup_params.items():
  92
+            if not isinstance(key, str):
  93
+                # 'key' will be used as a keyword argument later, so Python
  94
+                # requires it to be a string.
  95
+                del lookup_params[key]
  96
+                lookup_params[smart_str(key)] = value
  97
+
  98
+            if not self.model_admin.lookup_allowed(key, value):
  99
+                raise SuspiciousOperation("Filtering by %s not allowed" % key)
  100
+
88 101
         filter_specs = []
89  
-        cleaned_params, use_distinct = self.get_lookup_params(use_distinct)
90 102
         if self.list_filter:
91 103
             for list_filter in self.list_filter:
92 104
                 if callable(list_filter):
93 105
                     # This is simply a custom list filter class.
94  
-                    spec = list_filter(request, cleaned_params,
  106
+                    spec = list_filter(request, lookup_params,
95 107
                         self.model, self.model_admin)
96 108
                 else:
97 109
                     field_path = None
@@ -106,11 +118,26 @@ def get_filters(self, request, use_distinct=False):
106 118
                     if not isinstance(field, models.Field):
107 119
                         field_path = field
108 120
                         field = get_fields_from_path(self.model, field_path)[-1]
109  
-                    spec = field_list_filter_class(field, request, cleaned_params,
  121
+                    spec = field_list_filter_class(field, request, lookup_params,
110 122
                         self.model, self.model_admin, field_path=field_path)
  123
+                    # Check if we need to use distinct()
  124
+                    use_distinct = (use_distinct or
  125
+                                    lookup_needs_distinct(self.lookup_opts,
  126
+                                                          field_path))
111 127
                 if spec and spec.has_output():
112 128
                     filter_specs.append(spec)
113  
-        return filter_specs, bool(filter_specs)
  129
+
  130
+        # At this point, all the parameters used by the various ListFilters
  131
+        # have been removed from lookup_params, which now only contains other
  132
+        # parameters passed via the query string. We now loop through the
  133
+        # remaining parameters both to ensure that all the parameters are valid
  134
+        # fields and to determine if at least one of them needs distinct().
  135
+        for key, value in lookup_params.items():
  136
+            lookup_params[key] = prepare_lookup_value(key, value)
  137
+            use_distinct = (use_distinct or
  138
+                            lookup_needs_distinct(self.lookup_opts, key))
  139
+
  140
+        return filter_specs, bool(filter_specs), lookup_params, use_distinct
114 141
 
115 142
     def get_query_string(self, new_params=None, remove=None):
116 143
         if new_params is None: new_params = {}
@@ -247,78 +274,34 @@ def get_ordering_field_columns(self):
247 274
                 ordering_fields[idx] = 'desc' if pfx == '-' else 'asc'
248 275
         return ordering_fields
249 276
 
250  
-    def get_lookup_params(self, use_distinct=False):
251  
-        lookup_params = self.params.copy() # a dictionary of the query string
252  
-
253  
-        for ignored in IGNORED_PARAMS:
254  
-            if ignored in lookup_params:
255  
-                del lookup_params[ignored]
256  
-
257  
-        for key, value in lookup_params.items():
258  
-            if not isinstance(key, str):
259  
-                # 'key' will be used as a keyword argument later, so Python
260  
-                # requires it to be a string.
261  
-                del lookup_params[key]
262  
-                lookup_params[smart_str(key)] = value
263  
-
264  
-            field = None
265  
-            if not use_distinct:
266  
-                # Check if it's a relationship that might return more than one
267  
-                # instance
268  
-                field_name = key.split('__', 1)[0]
269  
-                try:
270  
-                    field = self.lookup_opts.get_field_by_name(field_name)[0]
271  
-                    use_distinct = field_needs_distinct(field)
272  
-                except models.FieldDoesNotExist:
273  
-                    # It might be a custom NonFieldFilter
274  
-                    pass
275  
-
276  
-            # if key ends with __in, split parameter into separate values
277  
-            if key.endswith('__in'):
278  
-                value = value.split(',')
279  
-                lookup_params[key] = value
280  
-
281  
-            # if key ends with __isnull, special case '' and false
282  
-            if key.endswith('__isnull'):
283  
-                if value.lower() in ('', 'false'):
284  
-                    value = False
285  
-                else:
286  
-                    value = True
287  
-                lookup_params[key] = value
288  
-
289  
-            if field and not self.model_admin.lookup_allowed(key, value):
290  
-                raise SuspiciousOperation("Filtering by %s not allowed" % key)
291  
-
292  
-        return lookup_params, use_distinct
293  
-
294 277
     def get_query_set(self, request):
295  
-        lookup_params, use_distinct = self.get_lookup_params(use_distinct=False)
296  
-        self.filter_specs, self.has_filters = self.get_filters(request, use_distinct)
297  
-
298 278
         try:
299  
-            # First, let every list filter modify the qs and params to its
300  
-            # liking.
  279
+            # First, we collect all the declared list filters.
  280
+            (self.filter_specs, self.has_filters, remaining_lookup_params,
  281
+             use_distinct) = self.get_filters(request)
  282
+
  283
+            # Then, we let every list filter modify the qs to its liking.
301 284
             qs = self.root_query_set
302 285
             for filter_spec in self.filter_specs:
303 286
                 new_qs = filter_spec.queryset(request, qs)
304 287
                 if new_qs is not None:
305 288
                     qs = new_qs
306  
-                    for param in filter_spec.used_params():
307  
-                        try:
308  
-                            del lookup_params[param]
309  
-                        except KeyError:
310  
-                            pass
311  
-
312  
-            # Then, apply the remaining lookup parameters from the query string
313  
-            # (i.e. those that haven't already been processed by the filters).
314  
-            qs = qs.filter(**lookup_params)
  289
+
  290
+            # Finally, we apply the remaining lookup parameters from the query
  291
+            # string (i.e. those that haven't already been processed by the
  292
+            # filters).
  293
+            qs = qs.filter(**remaining_lookup_params)
  294
+        except (SuspiciousOperation, ImproperlyConfigured):
  295
+            # Allow certain types of errors to be re-raised as-is so that the
  296
+            # caller can treat them in a special way.
  297
+            raise
315 298
         except Exception, e:
316  
-            # Naked except! Because we don't have any other way of validating
317  
-            # "lookup_params". They might be invalid if the keyword arguments
318  
-            # are incorrect, or if the values are not in the correct type, so
319  
-            # we might get FieldError, ValueError, ValicationError, or ? from a
320  
-            # custom field that raises yet something else when handed
321  
-            # impossible data.
  299
+            # Every other error is caught with a naked except, because we don't
  300
+            # have any other way of validating lookup parameters. They might be
  301
+            # invalid if the keyword arguments are incorrect, or if the values
  302
+            # are not in the correct type, so we might get FieldError,
  303
+            # ValueError, ValidationError, or ? from a custom field that raises
  304
+            # yet something else when handed impossible data.
322 305
             raise IncorrectLookupParameters(e)
323 306
 
324 307
         # Use select_related() if one of the list_display options is a field
@@ -362,9 +345,7 @@ def construct_search(field_name):
362 345
                 qs = qs.filter(reduce(operator.or_, or_queries))
363 346
             if not use_distinct:
364 347
                 for search_spec in orm_lookups:
365  
-                    field_name = search_spec.split('__', 1)[0]
366  
-                    f = self.lookup_opts.get_field_by_name(field_name)[0]
367  
-                    if field_needs_distinct(f):
  348
+                    if lookup_needs_distinct(self.lookup_opts, search_spec):
368 349
                         use_distinct = True
369 350
                         break
370 351
 
55  tests/regressiontests/admin_filters/tests.py
@@ -68,6 +68,14 @@ def lookups(self, request, model_admin):
68 68
         if qs.filter(year__gte=2000, year__lte=2009).exists():
69 69
             yield ('the 00s', "the 2000's")
70 70
 
  71
+class DecadeListFilterParameterEndsWith__In(DecadeListFilter):
  72
+    title = 'publication decade'
  73
+    parameter_name = 'decade__in' # Ends with '__in"
  74
+
  75
+class DecadeListFilterParameterEndsWith__Isnull(DecadeListFilter):
  76
+    title = 'publication decade'
  77
+    parameter_name = 'decade__isnull' # Ends with '__isnull"
  78
+
71 79
 class CustomUserAdmin(UserAdmin):
72 80
     list_filter = ('books_authored', 'books_contributed')
73 81
 
@@ -97,6 +105,12 @@ class DecadeFilterBookAdminWithFailingQueryset(ModelAdmin):
97 105
 class DecadeFilterBookAdminWithQuerysetBasedLookups(ModelAdmin):
98 106
     list_filter = (DecadeListFilterWithQuerysetBasedLookups,)
99 107
 
  108
+class DecadeFilterBookAdminParameterEndsWith__In(ModelAdmin):
  109
+    list_filter = (DecadeListFilterParameterEndsWith__In,)
  110
+
  111
+class DecadeFilterBookAdminParameterEndsWith__Isnull(ModelAdmin):
  112
+    list_filter = (DecadeListFilterParameterEndsWith__Isnull,)
  113
+
100 114
 class ListFiltersTests(TestCase):
101 115
 
102 116
     def setUp(self):
@@ -570,3 +584,44 @@ def test_two_characters_long_field(self):
570 584
         choices = list(filterspec.choices(changelist))
571 585
         self.assertEqual(choices[2]['selected'], True)
572 586
         self.assertEqual(choices[2]['query_string'], '?no=207')
  587
+
  588
+    def test_parameter_ends_with__in__or__isnull(self):
  589
+        """
  590
+        Ensure that a SimpleListFilter's parameter name is not mistaken for a
  591
+        model field if it ends with '__isnull' or '__in'.
  592
+        Refs #17091.
  593
+        """
  594
+
  595
+        # When it ends with '__in' -----------------------------------------
  596
+        modeladmin = DecadeFilterBookAdminParameterEndsWith__In(Book, site)
  597
+        request = self.request_factory.get('/', {'decade__in': 'the 90s'})
  598
+        changelist = self.get_changelist(request, Book, modeladmin)
  599
+
  600
+        # Make sure the correct queryset is returned
  601
+        queryset = changelist.get_query_set(request)
  602
+        self.assertEqual(list(queryset), [self.bio_book])
  603
+
  604
+        # Make sure the correct choice is selected
  605
+        filterspec = changelist.get_filters(request)[0][0]
  606
+        self.assertEqual(force_unicode(filterspec.title), u'publication decade')
  607
+        choices = list(filterspec.choices(changelist))
  608
+        self.assertEqual(choices[2]['display'], u'the 1990\'s')
  609
+        self.assertEqual(choices[2]['selected'], True)
  610
+        self.assertEqual(choices[2]['query_string'], '?decade__in=the+90s')
  611
+
  612
+        # When it ends with '__isnull' ---------------------------------------
  613
+        modeladmin = DecadeFilterBookAdminParameterEndsWith__Isnull(Book, site)
  614
+        request = self.request_factory.get('/', {'decade__isnull': 'the 90s'})
  615
+        changelist = self.get_changelist(request, Book, modeladmin)
  616
+
  617
+        # Make sure the correct queryset is returned
  618
+        queryset = changelist.get_query_set(request)
  619
+        self.assertEqual(list(queryset), [self.bio_book])
  620
+
  621
+        # Make sure the correct choice is selected
  622
+        filterspec = changelist.get_filters(request)[0][0]
  623
+        self.assertEqual(force_unicode(filterspec.title), u'publication decade')
  624
+        choices = list(filterspec.choices(changelist))
  625
+        self.assertEqual(choices[2]['display'], u'the 1990\'s')
  626
+        self.assertEqual(choices[2]['selected'], True)
  627
+        self.assertEqual(choices[2]['query_string'], '?decade__isnull=the+90s')

0 notes on commit a89b156

Please sign in to comment.
Something went wrong with that request. Please try again.