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

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:

django/db/models/sql/query.py

@@ -8,6 +8,7 @@ all about the internals of models in order to get the information it needs.
 """
 
 import copy
+import re
 
 from django.utils import tree
 from django.db.models.sql.where import WhereNode, AND, OR
@@ -35,7 +36,7 @@ GET_ITERATOR_CHUNK_SIZE = 100
 # 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 @@ JOIN_TYPE = 2
 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 @@ MULTI = 'multi'
 SINGLE = 'single'
 NONE = None
 
+ORDER_PATTERN = re.compile(r'\?|[-+]?\w+$')
+
 class Query(object):
     """
     A single SQL query.
@@ -91,6 +94,7 @@ class Query(object):
         self.extra_tables = []
         self.extra_where = []
         self.extra_params = []
+        self.extra_order_by = []
 
     def __str__(self):
         """
@@ -140,6 +144,7 @@ class Query(object):
         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 @@ class Query(object):
         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 @@ class Query(object):
             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 @@ class Query(object):
         # 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
+        Does any necessary class setup immediately prior to producing SQL. This
-        things that can't necessarily be done in __init__.
+        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 @@ class Query(object):
         "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 @@ class Query(object):
                         % (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 @@ class Query(object):
     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 @@ class Query(object):
                     order = 'ASC'
                 result.append('%s %s' % (field, order))
                 continue
-            if field[0] == '-':
+            if '.' in field:
-                col = field[1:]
+                # This came in through an extra(ordering=...) addition. Pass it
-                order = 'DESC'
+                # on verbatim, after mapping the table name to an alias, if
-            else:
+                # necessary.
-                col = field
+                col, order = get_order_dir(field)
-                order = 'ASC'
-            if '.' in col:
                 table, col = col.split('.', 1)
-                table = '%s.' % qn(self.table_alias(table)[0])
+                result.append('%s.%s %s' % (qn(self.table_alias(table)[0]), col,
-            elif col not in self.extra_select:
+                        order))
-                # Use the root model's database table as the referenced table.
+            elif get_order_dir(field)[0] not in self.extra_select:
-                table = '%s.' % qn(self.tables[0])
+                # '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 = ''
+                col, order = get_order_dir(field)
-            result.append('%s%s %s' % (table,
+                result.append('%s %s' % (qn(col), order))
-                    qn(orderfield_to_column(col, opts)), 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 @@ class Query(object):
             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 @@ class Query(object):
         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 @@ class Query(object):
             self.order_by = None
         else:
             self.order_by = []
+        self.extra_order_by = []
 
     def add_count_column(self):
         """
@@ -1043,6 +1115,9 @@ class CountQuery(Query):
         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
+    Returns the field name and direction for an order specification. For
-    column name. If 'name' is not a field in the current model, it is returned
+    example, '-foo' is returned as ('foo', 'DESC').
-    unchanged.
+
-    """
+    The 'default' param is used to indicate which way no prefix (or a '+'
-    try:
+    prefix) should sort. The '-' prefix always sorts the opposite way.
-        return opts.get_field(name, False).column
+    """
-    except FieldDoesNotExist:
+    dirn = {'ASC': ('ASC', 'DESC'), 'DESC': ('DESC', 'ASC')}[default]
-        return name
+    if field[0] == '-':
+        return field[1:], dirn[1]
+    return field, dirn[0]
 

tests/modeltests/reserved_names/models.py

@@ -45,8 +45,6 @@ h
 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)]

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 @@ __test__ = {'API_TESTS':"""
 >>> t5 = Tag(name='t5', parent=t3)
 >>> t5.save()
 
+>>> n1 = Note(note='n1')
+>>> n1.save()
+>>> n2 = Note(note='n2')
+>>> n2.save()
+>>> n3 = Note(note='n3')
+>>> n3.save()
 
->>> a1 = Author(name='a1', num=1001)
+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 @@ __test__ = {'API_TESTS':"""
 >>> 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 @@ Bug #1878, #2939
 
 # 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 @@ Bug #2400
 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>)
 """}