Changeset 6510

Timestamp:

10/14/07 17:38:54 (9 months ago)

Author:

mtredinnick

Message:

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: foobarbaz, 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).

Files:

• django/branches/queryset-refactor/django/core/management/validation.py (modified) (1 diff)
• django/branches/queryset-refactor/django/db/models/sql/query.py (modified) (19 diffs)
• django/branches/queryset-refactor/tests/modeltests/reserved_names/models.py (modified) (1 diff)
• django/branches/queryset-refactor/tests/regressiontests/queries/models.py (modified) (8 diffs)

django/branches/queryset-refactor/django/core/management/validation.py

@@ -191 +191 @@
-                if opts.order_with_respect_to and field_name == '_order':
-                    continue
-
+
+
+
+
-                try:
-                    opts.get_field(field_name, many_to_many=False)

django/branches/queryset-refactor/django/db/models/sql/query.py

@@ -9 +9 @@
-
-import copy
+import re
-
-from django.utils import tree
@@ -36 +37 @@
-LOOKUP_SEP = '__'
-
-er
+
-# Join lists
-TABLE_NAME = 0
@@ -44 +45 @@
-LHS_JOIN_COL = 4
-RHS_JOIN_COL = 5
-s
+
-ALIAS_TABLE = 0
-ALIAS_REFCOUNT = 1
@@ -54 +55 @@
-SINGLE = 'single'
-NONE = None
+
+ORDER_PATTERN = re.compile(r'\?|[-+]?\w+$')
-
-class Query(object):
@@ -92 +95 @@
-        self.extra_where = []
-        self.extra_params = []
+        self.extra_order_by = []
-
-    def __str__(self):
@@ -141 +145 @@
-        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
@@ -190 +195 @@
-        """
-        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)
@@ -211 +219 @@
-                result.append('AND')
-            result.append(' AND'.join(self.extra_where))
+        params.extend(w_params)
-
-        if self.group_by:
@@ -216 +225 @@
-            result.append('GROUP BY %s' % ', '.join(grouping))
-
-        ordering = self.get_ordering()
-        if ordering:
-            result.append('ORDER BY %s' % ', '.join(ordering))
@@ -313 +321 @@
-        # 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):
-        """
-
-
+
+
+
-        """
-        if not self.tables:
@@ -357 +368 @@
-        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]:
@@ -371 +387 @@
-                            qn(lhs_col), qn(alias), qn(col)))
-            else:
-
+
+
+
-        extra_tables = []
-        for t in self.extra_tables:
-            alias, created = self.table_alias(t)
-            if created:
-
+
+
+
-        return result, []
-
@@ -397 +417 @@
-        """
-        Returns a tuple representing the SQL elements in the "order by" clause.
-
-
+
+
+
+
+
+
+
-            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:
@@ -417 +444 @@
-                result.append('%s %s' % (field, order))
-                continue
-
-
-
+
+
+
+
+
+
+
+
+
+
+
+
+
+
-            else:
-
-
-
-
-
-
-
-
-
-
-
-
+
+
-        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):
@@ -558 +623 @@
-                    f.rel.get_related_field().column), exclusions=used)
-            used.append(alias)
-table
+alias
-                    for f2 in f.rel.to._meta.fields])
-            self.fill_related_selections(f.rel.to._meta, alias, cur_depth + 1,
@@ -803 +868 @@
-        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)
-
@@ -814 +885 @@
-        else:
-            self.order_by = []
+        self.extra_order_by = []
-
-    def add_count_column(self):
@@ -1044 +1116 @@
-        return ['(%s) AS A1' % result], params
-
+    def get_ordering(self):
+        return ()
+
-def find_field(name, field_list, related_query):
-    """
@@ -1078 +1153 @@
-            + field_choices(opts.fields, False))
-
-
-
-
-
-
-
-
-
-
-
-
+
+
+
+
+
+
+
+
+
+
+
+
+

django/branches/queryset-refactor/tests/modeltests/reserved_names/models.py

@@ -46 +46 @@
->>> print v.where
-2005-01-01
->>> Thing.objects.order_by('select.when')
-[<Thing: a>, <Thing: h>]
-
->>> Thing.objects.dates('where', 'year')

django/branches/queryset-refactor/tests/regressiontests/queries/models.py

@@ -1 +1 @@
-"""
-bination
+plex
-"""
-
@@ -13 +13 @@
-        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):
@@ -24 +44 @@
-    tags = models.ManyToManyField(Tag, blank=True, null=True)
-    creator = models.ForeignKey(Author)
+    note = models.ForeignKey(Note)
+
+    class Meta:
+        ordering = ['-note', 'name']
-
-    def __unicode__(self):
@@ -35 +59 @@
-        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':"""
@@ -48 +92 @@
->>> t5.save()
-
-
-
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
->>> a1.save()
-
+, extra=e1
->>> a2.save()
-
+, extra=e2
->>> a3.save()
-
+, extra=e2
->>> a4.save()
-
-
+, note=n3
->>> i1.save()
->>> i1.tags = [t1, t2]
-
+, note=n2
->>> i2.save()
->>> i2.tags = [t1, t3]
-
+, note=n3
->>> i3.save()
-
+, note=n3
->>> i4.save()
->>> i4.tags = [t4]
@@ -74 +131 @@
->>> 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
@@ -120 +191 @@
-# Create something with a duplicate 'name' so that we can test multi-column
-# cases (which require some tricky SQL transformations under the covers).
-
+, note=n1
->>> xx.save()
->>> Item.objects.exclude(name='two').values('creator', 'name').distinct().count()
@@ -207 +278 @@
->>> 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>)
-"""}
-