innov
/
django1
1
0
Fork 0
Code Issues Pull Requests 1 Projects Releases Wiki Activity

Disentangled some parts of the admin ChangeList and ListFilter's internals. 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
This commit is contained in:
Julien Phalip 2011-11-22 12:26:17 +00:00
parent e71f33625c
commit a89b156284
4 changed files with 212 additions and 152 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

139
django/contrib/admin/filters.py
View File

@ -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):

27
django/contrib/admin/util.py
View File

@ -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):
"""

143
django/contrib/admin/views/main.py
View File

@ -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

55
tests/regressiontests/admin_filters/tests.py
View File

@ -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')