Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

queryset-refactor: Fixed a large bag of order_by() problems.

This also picked up a small bug in some twisted select_related() handling.

Introduces a new syntax for cross-model ordering: foo__bar__baz, using field
names, instead of a strange combination of table names and field names. This
might turn out to be backwards compatible (although the old syntax leads to
bugs and is not to be recommended).

Still to come: fixes for extra() handling, since the new syntax can't handle
that and doc updates.

Things are starting to get a bit slow here, so we might eventually have to
remove ordering by many-many and other multiple-result fields, since they don't
make a lot of sense in any case. For now, it's legal.

Refs #2076, #2874, #3002 (although the admin bit doesn't work yet).


git-svn-id: http://code.djangoproject.com/svn/django/branches/queryset-refactor@6510 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information...
commit a1d160e2ea22cb010b276cfc2a085f9cd3d81b22 1 parent 6678de1
@malcolmt malcolmt authored
View
5 django/core/management/validation.py
@@ -190,7 +190,10 @@ def get_validation_errors(outfile, app=None):
field_name = field_name[1:]
if opts.order_with_respect_to and field_name == '_order':
continue
- if '.' in field_name: continue # Skip ordering in the format 'table.field'.
+ # Skip ordering in the format field1__field2 (FIXME: checking
+ # this format would be nice, but it's a little fiddly).
+ if '_' in field_name:
+ continue
try:
opts.get_field(field_name, many_to_many=False)
except models.FieldDoesNotExist:
View
149 django/db/models/sql/query.py
@@ -8,6 +8,7 @@
"""
import copy
+import re
from django.utils import tree
from django.db.models.sql.where import WhereNode, AND, OR
@@ -35,7 +36,7 @@
# Separator used to split filter strings apart.
LOOKUP_SEP = '__'
-# Constants to make looking up tuple values clearerer.
+# Constants to make looking up tuple values clearer.
# Join lists
TABLE_NAME = 0
RHS_ALIAS = 1
@@ -43,7 +44,7 @@
LHS_ALIAS = 3
LHS_JOIN_COL = 4
RHS_JOIN_COL = 5
-# Alias maps lists
+# Alias map lists
ALIAS_TABLE = 0
ALIAS_REFCOUNT = 1
ALIAS_JOIN = 2
@@ -54,6 +55,8 @@
SINGLE = 'single'
NONE = None
+ORDER_PATTERN = re.compile(r'\?|[-+]?\w+$')
+
class Query(object):
"""
A single SQL query.
@@ -91,6 +94,7 @@ def __init__(self, model, connection):
self.extra_tables = []
self.extra_where = []
self.extra_params = []
+ self.extra_order_by = []
def __str__(self):
"""
@@ -140,6 +144,7 @@ def clone(self, klass=None, **kwargs):
obj.extra_tables = self.extra_tables[:]
obj.extra_where = self.extra_where[:]
obj.extra_params = self.extra_params[:]
+ obj.extra_order_by = self.extra_order_by[:]
obj.__dict__.update(kwargs)
return obj
@@ -189,19 +194,22 @@ def as_sql(self, with_limits=True):
in the query.
"""
self.pre_sql_setup()
+ out_cols = self.get_columns()
+ ordering = self.get_ordering()
+ # This must come after 'select' and 'ordering' -- see docstring of
+ # get_from_clause() for details.
+ from_, f_params = self.get_from_clause()
+ where, w_params = self.where.as_sql()
+
result = ['SELECT']
if self.distinct:
result.append('DISTINCT')
- out_cols = self.get_columns()
result.append(', '.join(out_cols))
result.append('FROM')
- from_, f_params = self.get_from_clause()
result.extend(from_)
params = list(f_params)
- where, w_params = self.where.as_sql()
- params.extend(w_params)
if where:
result.append('WHERE %s' % where)
if self.extra_where:
@@ -210,12 +218,12 @@ def as_sql(self, with_limits=True):
else:
result.append('AND')
result.append(' AND'.join(self.extra_where))
+ params.extend(w_params)
if self.group_by:
grouping = self.get_grouping()
result.append('GROUP BY %s' % ', '.join(grouping))
- ordering = self.get_ordering()
if ordering:
result.append('ORDER BY %s' % ', '.join(ordering))
@@ -312,11 +320,14 @@ def combine(self, rhs, connection):
# Ordering uses the 'rhs' ordering, unless it has none, in which case
# the current ordering is used.
self.order_by = rhs.order_by and rhs.order_by[:] or self.order_by
+ self.extra_order_by = (rhs.extra_order_by and rhs.extra_order_by[:] or
+ self.extra_order_by)
def pre_sql_setup(self):
"""
- Does any necessary class setup prior to producing SQL. This is for
- things that can't necessarily be done in __init__.
+ Does any necessary class setup immediately prior to producing SQL. This
+ is for things that can't necessarily be done in __init__ because we
+ might not have all the pieces in place at that time.
"""
if not self.tables:
self.join((None, self.model._meta.db_table, None, None))
@@ -356,9 +367,14 @@ def get_from_clause(self):
"FROM" part of the query, as well as any extra parameters that need to
be included. Sub-classes, can override this to create a from-clause via
a "select", for example (e.g. CountQuery).
+
+ This should only be called after any SQL construction methods that
+ might change the tables we need. This means the select columns and
+ ordering must be done first.
"""
result = []
qn = self.quote_name_unless_alias
+ first = True
for alias in self.tables:
if not self.alias_map[alias][ALIAS_REFCOUNT]:
continue
@@ -370,12 +386,16 @@ def get_from_clause(self):
% (join_type, qn(name), alias_str, qn(lhs),
qn(lhs_col), qn(alias), qn(col)))
else:
- result.append('%s%s' % (qn(name), alias_str))
+ connector = not first and ', ' or ''
+ result.append('%s%s%s' % (connector, qn(name), alias_str))
+ first = False
extra_tables = []
for t in self.extra_tables:
alias, created = self.table_alias(t)
if created:
- result.append(', %s' % alias)
+ connector = not first and ', ' or ''
+ result.append('%s%s' % (connector, alias))
+ first = False
return result, []
def get_grouping(self):
@@ -396,13 +416,20 @@ def get_grouping(self):
def get_ordering(self):
"""
Returns a tuple representing the SQL elements in the "order by" clause.
+
+ Determining the ordering SQL can change the tables we need to include,
+ so this should be run *before* get_from_clause().
"""
- if self.order_by is None:
+ if self.extra_order_by:
+ ordering = self.extra_order_by
+ elif self.order_by is None:
ordering = []
else:
+ # Note that self.order_by can be empty in two ways: [] ("use the
+ # default"), which is handled here, and None ("no ordering"), which
+ # is handled in the previous test.
ordering = self.order_by or self.model._meta.ordering
qn = self.quote_name_unless_alias
- opts = self.model._meta
result = []
for field in ordering:
if field == '?':
@@ -416,24 +443,62 @@ def get_ordering(self):
order = 'ASC'
result.append('%s %s' % (field, order))
continue
- if field[0] == '-':
- col = field[1:]
- order = 'DESC'
- else:
- col = field
- order = 'ASC'
- if '.' in col:
+ if '.' in field:
+ # This came in through an extra(ordering=...) addition. Pass it
+ # on verbatim, after mapping the table name to an alias, if
+ # necessary.
+ col, order = get_order_dir(field)
table, col = col.split('.', 1)
- table = '%s.' % qn(self.table_alias(table)[0])
- elif col not in self.extra_select:
- # Use the root model's database table as the referenced table.
- table = '%s.' % qn(self.tables[0])
+ result.append('%s.%s %s' % (qn(self.table_alias(table)[0]), col,
+ order))
+ elif get_order_dir(field)[0] not in self.extra_select:
+ # 'col' is of the form 'field' or 'field1__field2' or
+ # 'field1__field2__field', etc.
+ for table, col, order in self.find_ordering_name(field,
+ self.model._meta):
+ result.append('%s.%s %s' % (qn(table), qn(col), order))
else:
- table = ''
- result.append('%s%s %s' % (table,
- qn(orderfield_to_column(col, opts)), order))
+ col, order = get_order_dir(field)
+ result.append('%s %s' % (qn(col), order))
return result
+ def find_ordering_name(self, name, opts, alias=None, default_order='ASC'):
+ """
+ Returns the table alias (the name might not be unambiguous, the alias
+ will be) and column name for ordering by the given 'name' parameter.
+ The 'name' is of the form 'field1__field2__...__fieldN'.
+ """
+ name, order = get_order_dir(name, default_order)
+ pieces = name.split(LOOKUP_SEP)
+ if not alias:
+ alias = self.join((None, opts.db_table, None, None))
+ for elt in pieces:
+ joins, opts, unused1, field, col, unused2 = \
+ self.get_next_join(elt, opts, alias, False)
+ if joins:
+ alias = joins[-1]
+ col = col or field.column
+
+ # If we get to this point and the field is a relation to another model,
+ # append the default ordering for that model.
+ if joins and opts.ordering:
+ results = []
+ for item in opts.ordering:
+ results.extend(self.find_ordering_name(item, opts, alias,
+ order))
+ return results
+
+ if alias:
+ # We have to do the same "final join" optimisation as in
+ # add_filter, since the final column might not otherwise be part of
+ # the select set (so we can't order on it).
+ join = self.alias_map[alias][ALIAS_JOIN]
+ if col == join[RHS_JOIN_COL]:
+ self.unref_alias(alias)
+ alias = join[LHS_ALIAS]
+ col = join[LHS_JOIN_COL]
+ return [(alias, col, order)]
+
def table_alias(self, table_name, create=False):
"""
Returns a table alias for the given table_name and whether this is a
@@ -557,7 +622,7 @@ def fill_related_selections(self, opts=None, root_alias=None, cur_depth=0,
alias = self.join((root_alias, table, f.column,
f.rel.get_related_field().column), exclusions=used)
used.append(alias)
- self.select.extend([(table, f2.column)
+ self.select.extend([(alias, f2.column)
for f2 in f.rel.to._meta.fields])
self.fill_related_selections(f.rel.to._meta, alias, cur_depth + 1,
used)
@@ -802,6 +867,12 @@ def add_ordering(self, *ordering):
possibly with a direction prefix ('-' or '?') -- or ordinals,
corresponding to column positions in the 'select' list.
"""
+ errors = []
+ for item in ordering:
+ if not ORDER_PATTERN.match(item):
+ errors.append(item)
+ if errors:
+ raise TypeError('Invalid order_by arguments: %s' % errors)
self.order_by.extend(ordering)
def clear_ordering(self, force_empty=False):
@@ -813,6 +884,7 @@ def clear_ordering(self, force_empty=False):
self.order_by = None
else:
self.order_by = []
+ self.extra_order_by = []
def add_count_column(self):
"""
@@ -1043,6 +1115,9 @@ def get_from_clause(self):
result, params = self._query.as_sql()
return ['(%s) AS A1' % result], params
+ def get_ordering(self):
+ return ()
+
def find_field(name, field_list, related_query):
"""
Finds a field with a specific name in a list of field instances.
@@ -1077,14 +1152,16 @@ def get_legal_fields(opts):
+ field_choices(opts.get_all_related_objects(), True)
+ field_choices(opts.fields, False))
-def orderfield_to_column(name, opts):
+def get_order_dir(field, default='ASC'):
"""
- For a field name specified in an "order by" clause, returns the database
- column name. If 'name' is not a field in the current model, it is returned
- unchanged.
+ Returns the field name and direction for an order specification. For
+ example, '-foo' is returned as ('foo', 'DESC').
+
+ The 'default' param is used to indicate which way no prefix (or a '+'
+ prefix) should sort. The '-' prefix always sorts the opposite way.
"""
- try:
- return opts.get_field(name, False).column
- except FieldDoesNotExist:
- return name
+ dirn = {'ASC': ('ASC', 'DESC'), 'DESC': ('DESC', 'ASC')}[default]
+ if field[0] == '-':
+ return field[1:], dirn[1]
+ return field, dirn[0]
View
2  tests/modeltests/reserved_names/models.py
@@ -45,8 +45,6 @@ def __unicode__(self):
b
>>> print v.where
2005-01-01
->>> Thing.objects.order_by('select.when')
-[<Thing: a>, <Thing: h>]
>>> Thing.objects.dates('where', 'year')
[datetime.datetime(2005, 1, 1, 0, 0), datetime.datetime(2006, 1, 1, 0, 0)]
View
128 tests/regressiontests/queries/models.py
@@ -1,5 +1,5 @@
"""
-Various combination queries that have been problematic in the past.
+Various complex queries that have been problematic in the past.
"""
from django.db import models
@@ -12,9 +12,29 @@ class Tag(models.Model):
def __unicode__(self):
return self.name
+class Note(models.Model):
+ note = models.CharField(maxlength=100)
+
+ class Meta:
+ ordering = ['note']
+
+ def __unicode__(self):
+ return self.note
+
+class ExtraInfo(models.Model):
+ info = models.CharField(maxlength=100)
+ note = models.ForeignKey(Note)
+
+ class Meta:
+ ordering = ['info']
+
+ def __unicode__(self):
+ return self.info
+
class Author(models.Model):
name = models.CharField(maxlength=10)
num = models.IntegerField(unique=True)
+ extra = models.ForeignKey(ExtraInfo)
def __unicode__(self):
return self.name
@@ -23,6 +43,10 @@ class Item(models.Model):
name = models.CharField(maxlength=10)
tags = models.ManyToManyField(Tag, blank=True, null=True)
creator = models.ForeignKey(Author)
+ note = models.ForeignKey(Note)
+
+ class Meta:
+ ordering = ['-note', 'name']
def __unicode__(self):
return self.name
@@ -34,6 +58,26 @@ class Report(models.Model):
def __unicode__(self):
return self.name
+class Ranking(models.Model):
+ rank = models.IntegerField()
+ author = models.ForeignKey(Author)
+
+ class Meta:
+ # A complex ordering specification. Should stress the system a bit.
+ ordering = ('author__extra__note', 'author__name', 'rank')
+
+ def __unicode__(self):
+ return '%d: %s' % (self.rank, self.author.name)
+
+class Cover(models.Model):
+ title = models.CharField(maxlength=50)
+ item = models.ForeignKey(Item)
+
+ class Meta:
+ ordering = ['item']
+
+ def __unicode__(self):
+ return self.title
__test__ = {'API_TESTS':"""
>>> t1 = Tag(name='t1')
@@ -47,25 +91,38 @@ def __unicode__(self):
>>> t5 = Tag(name='t5', parent=t3)
>>> t5.save()
-
->>> a1 = Author(name='a1', num=1001)
+>>> n1 = Note(note='n1')
+>>> n1.save()
+>>> n2 = Note(note='n2')
+>>> n2.save()
+>>> n3 = Note(note='n3')
+>>> n3.save()
+
+Create these out of order so that sorting by 'id' will be different to sorting
+by 'info'. Helps detect some problems later.
+>>> e2 = ExtraInfo(info='e2', note=n2)
+>>> e2.save()
+>>> e1 = ExtraInfo(info='e1', note=n1)
+>>> e1.save()
+
+>>> a1 = Author(name='a1', num=1001, extra=e1)
>>> a1.save()
->>> a2 = Author(name='a2', num=2002)
+>>> a2 = Author(name='a2', num=2002, extra=e1)
>>> a2.save()
->>> a3 = Author(name='a3', num=3003)
+>>> a3 = Author(name='a3', num=3003, extra=e2)
>>> a3.save()
->>> a4 = Author(name='a4', num=4004)
+>>> a4 = Author(name='a4', num=4004, extra=e2)
>>> a4.save()
->>> i1 = Item(name='one', creator=a1)
+>>> i1 = Item(name='one', creator=a1, note=n3)
>>> i1.save()
>>> i1.tags = [t1, t2]
->>> i2 = Item(name='two', creator=a2)
+>>> i2 = Item(name='two', creator=a2, note=n2)
>>> i2.save()
>>> i2.tags = [t1, t3]
->>> i3 = Item(name='three', creator=a2)
+>>> i3 = Item(name='three', creator=a2, note=n3)
>>> i3.save()
->>> i4 = Item(name='four', creator=a4)
+>>> i4 = Item(name='four', creator=a4, note=n3)
>>> i4.save()
>>> i4.tags = [t4]
@@ -74,6 +131,20 @@ def __unicode__(self):
>>> r2 = Report(name='r2', creator=a3)
>>> r2.save()
+Ordering by 'rank' gives us rank2, rank1, rank3. Ordering by the Meta.ordering
+will be rank3, rank2, rank1.
+>>> rank1 = Ranking(rank=2, author=a2)
+>>> rank1.save()
+>>> rank2 = Ranking(rank=1, author=a3)
+>>> rank2.save()
+>>> rank3 = Ranking(rank=3, author=a1)
+>>> rank3.save()
+
+>>> c1 = Cover(title="first", item=i4)
+>>> c1.save()
+>>> c2 = Cover(title="second", item=i2)
+>>> c2.save()
+
Bug #1050
>>> Item.objects.filter(tags__isnull=True)
[<Item: three>]
@@ -119,7 +190,7 @@ def __unicode__(self):
# Create something with a duplicate 'name' so that we can test multi-column
# cases (which require some tricky SQL transformations under the covers).
->>> xx = Item(name='four', creator=a2)
+>>> xx = Item(name='four', creator=a2, note=n1)
>>> xx.save()
>>> Item.objects.exclude(name='two').values('creator', 'name').distinct().count()
4
@@ -206,5 +277,40 @@ def __unicode__(self):
Bug #2496
>>> Item.objects.extra(tables=['queries_author']).select_related().order_by('name')[:1]
[<Item: four>]
+
+Bug #2076
+# Ordering on related tables should be possible, even if the table is not
+# otherwise involved.
+>>> Item.objects.order_by('note__note', 'name')
+[<Item: two>, <Item: four>, <Item: one>, <Item: three>]
+
+# Ordering on a related field should use the remote model's default ordering as
+# a final step.
+>>> Author.objects.order_by('extra', '-name')
+[<Author: a2>, <Author: a1>, <Author: a4>, <Author: a3>]
+
+# If the remote model does not have a default ordering, we order by its 'id'
+# field.
+>>> Item.objects.order_by('creator', 'name')
+[<Item: one>, <Item: three>, <Item: two>, <Item: four>]
+
+# Cross model ordering is possible in Meta, too.
+>>> Ranking.objects.all()
+[<Ranking: 3: a1>, <Ranking: 2: a2>, <Ranking: 1: a3>]
+>>> Ranking.objects.all().order_by('rank')
+[<Ranking: 1: a3>, <Ranking: 2: a2>, <Ranking: 3: a1>]
+
+>>> Cover.objects.all()
+[<Cover: first>, <Cover: second>]
+
+Bugs #2874, #3002
+>>> qs = Item.objects.select_related().order_by('note__note', 'name')
+>>> list(qs)
+[<Item: two>, <Item: four>, <Item: one>, <Item: three>]
+
+# This is also a good select_related() test because there are multiple Note
+# entries in the SQL. The two Note items should be different.
+>>> qs[0].note, qs[0].creator.extra.note
+(<Note: n2>, <Note: n1>)
"""}
Please sign in to comment.
Something went wrong with that request. Please try again.