From a6ecd5dbb34249f756a337c359eef1e8d78dc01e Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Sat, 26 Apr 2014 03:34:20 -0400 Subject: [PATCH] [1.7.x] Fixed #19195 -- Allow explicit ordering by a relation `_id` field. Thanks to chrisedgemon for the report and shaib, akaariai and timgraham for the review. Backport of 24ec9538b7 from master --- django/db/models/sql/compiler.py | 5 +- docs/ref/models/querysets.txt | 27 ++++++++ docs/releases/1.7.txt | 3 + tests/ordering/models.py | 16 ++--- tests/ordering/tests.py | 112 +++++++++++++++++++++---------- 5 files changed, 113 insertions(+), 50 deletions(-) diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index 09b1f7d4d8f48..6792824ecd3c1 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -464,8 +464,9 @@ def find_ordering_name(self, name, opts, alias=None, default_order='ASC', field, targets, alias, joins, path, opts = self._setup_joins(pieces, opts, alias) # If we get to this point and the field is a relation to another model, - # append the default ordering for that model. - if field.rel and path and opts.ordering: + # append the default ordering for that model unless the attribute name + # of the field is specified. + if field.rel and path and opts.ordering and name != field.attname: # Firstly, avoid infinite loops. if not already_seen: already_seen = set() diff --git a/docs/ref/models/querysets.txt b/docs/ref/models/querysets.txt index 84577d40fadf7..55d697ffcff92 100644 --- a/docs/ref/models/querysets.txt +++ b/docs/ref/models/querysets.txt @@ -294,6 +294,18 @@ primary key if there is no :attr:`Meta.ordering ...since the ``Blog`` model has no default ordering specified. +.. versionadded:: 1.7 + + Note that it is also possible to order a queryset by a related field, + without incurring the cost of a JOIN, by referring to the ``_id`` of the + related field:: + + # No Join + Entry.objects.order_by('blog_id') + + # Join + Entry.objects.order_by('blog__id') + Be cautious when ordering by fields in related models if you are also using :meth:`distinct()`. See the note in :meth:`distinct` for an explanation of how related model ordering can change the expected results. @@ -435,6 +447,21 @@ Examples (those after the first will only work on PostgreSQL):: >>> Entry.objects.order_by('author', 'pub_date').distinct('author') [...] +.. note:: + Keep in mind that :meth:`order_by` uses any default related model ordering + that has been defined. You might have to explicitly order by the relation + ``_id`` or referenced field to make sure the ``DISTINCT ON`` expressions + match those at the beginning of the ``ORDER BY`` clause. For example, if + the ``Blog`` model defined an :attr:`~django.db.models.Options.ordering` by + ``name``:: + + Entry.objects.order_by('blog').distinct('blog') + + ...wouldn't work because the query would be ordered by ``blog__name`` thus + mismatching the ``DISTINCT ON`` expression. You'd have to explicitly order + by the relation `_id` field (``blog_id`` in this case) or the referenced + one (``blog__pk``) to make sure both expressions match. + values ~~~~~~ diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index 98a6d7cca019a..86ad76a1dbd6b 100644 --- a/docs/releases/1.7.txt +++ b/docs/releases/1.7.txt @@ -702,6 +702,9 @@ Models Previously model field validation didn't prevent values out of their associated column data type range from being saved resulting in an integrity error. +* It is now possible to explicitly :meth:`~django.db.models.query.QuerySet.order_by` + a relation ``_id`` field by using its attribute name. + Signals ^^^^^^^ diff --git a/tests/ordering/models.py b/tests/ordering/models.py index 415b28bb419be..2ab16f3c5fd59 100644 --- a/tests/ordering/models.py +++ b/tests/ordering/models.py @@ -17,25 +17,19 @@ from django.utils.encoding import python_2_unicode_compatible -@python_2_unicode_compatible -class Article(models.Model): - headline = models.CharField(max_length=100) - pub_date = models.DateTimeField() - +class Author(models.Model): class Meta: - ordering = ('-pub_date', 'headline') - - def __str__(self): - return self.headline + ordering = ('-pk',) @python_2_unicode_compatible -class ArticlePKOrdering(models.Model): +class Article(models.Model): + author = models.ForeignKey(Author, null=True) headline = models.CharField(max_length=100) pub_date = models.DateTimeField() class Meta: - ordering = ('-pk',) + ordering = ('-pub_date', 'headline') def __str__(self): return self.headline diff --git a/tests/ordering/tests.py b/tests/ordering/tests.py index 95631af5a7641..5d323e006b152 100644 --- a/tests/ordering/tests.py +++ b/tests/ordering/tests.py @@ -5,26 +5,29 @@ from django.test import TestCase -from .models import Article, ArticlePKOrdering +from .models import Article, Author class OrderingTests(TestCase): - def test_basic(self): - Article.objects.create( + def setUp(self): + self.a1 = Article.objects.create( headline="Article 1", pub_date=datetime(2005, 7, 26) ) - Article.objects.create( + self.a2 = Article.objects.create( headline="Article 2", pub_date=datetime(2005, 7, 27) ) - Article.objects.create( + self.a3 = Article.objects.create( headline="Article 3", pub_date=datetime(2005, 7, 27) ) - a4 = Article.objects.create( + self.a4 = Article.objects.create( headline="Article 4", pub_date=datetime(2005, 7, 28) ) - # By default, Article.objects.all() orders by pub_date descending, then - # headline ascending. + def test_default_ordering(self): + """ + By default, Article.objects.all() orders by pub_date descending, then + headline ascending. + """ self.assertQuerysetEqual( Article.objects.all(), [ "Article 4", @@ -35,8 +38,14 @@ def test_basic(self): attrgetter("headline") ) - # Override ordering with order_by, which is in the same format as the - # ordering attribute in models. + # Getting a single item should work too: + self.assertEqual(Article.objects.all()[0], self.a4) + + def test_default_ordering_override(self): + """ + Override ordering with order_by, which is in the same format as the + ordering attribute in models. + """ self.assertQuerysetEqual( Article.objects.order_by("headline"), [ "Article 1", @@ -56,8 +65,11 @@ def test_basic(self): attrgetter("headline") ) - # Only the last order_by has any effect (since they each override any - # previous ordering). + def test_order_by_override(self): + """ + Only the last order_by has any effect (since they each override any + previous ordering). + """ self.assertQuerysetEqual( Article.objects.order_by("id"), [ "Article 1", @@ -77,7 +89,10 @@ def test_basic(self): attrgetter("headline") ) - # Use the 'stop' part of slicing notation to limit the results. + def test_stop_slicing(self): + """ + Use the 'stop' part of slicing notation to limit the results. + """ self.assertQuerysetEqual( Article.objects.order_by("headline")[:2], [ "Article 1", @@ -86,8 +101,11 @@ def test_basic(self): attrgetter("headline") ) - # Use the 'stop' and 'start' parts of slicing notation to offset the - # result list. + def test_stop_start_slicing(self): + """ + Use the 'stop' and 'start' parts of slicing notation to offset the + result list. + """ self.assertQuerysetEqual( Article.objects.order_by("headline")[1:3], [ "Article 2", @@ -96,17 +114,20 @@ def test_basic(self): attrgetter("headline") ) - # Getting a single item should work too: - self.assertEqual(Article.objects.all()[0], a4) - - # Use '?' to order randomly. + def test_random_ordering(self): + """ + Use '?' to order randomly. + """ self.assertEqual( len(list(Article.objects.order_by("?"))), 4 ) - # Ordering can be reversed using the reverse() method on a queryset. - # This allows you to extract things like "the last two items" (reverse - # and then take the first two). + def test_reversed_ordering(self): + """ + Ordering can be reversed using the reverse() method on a queryset. + This allows you to extract things like "the last two items" (reverse + and then take the first two). + """ self.assertQuerysetEqual( Article.objects.all().reverse()[:2], [ "Article 1", @@ -115,7 +136,10 @@ def test_basic(self): attrgetter("headline") ) - # Ordering can be based on fields included from an 'extra' clause + def test_extra_ordering(self): + """ + Ordering can be based on fields included from an 'extra' clause + """ self.assertQuerysetEqual( Article.objects.extra(select={"foo": "pub_date"}, order_by=["foo", "headline"]), [ "Article 1", @@ -126,8 +150,11 @@ def test_basic(self): attrgetter("headline") ) - # If the extra clause uses an SQL keyword for a name, it will be - # protected by quoting. + def test_extra_ordering_quoting(self): + """ + If the extra clause uses an SQL keyword for a name, it will be + protected by quoting. + """ self.assertQuerysetEqual( Article.objects.extra(select={"order": "pub_date"}, order_by=["order", "headline"]), [ "Article 1", @@ -143,21 +170,32 @@ def test_order_by_pk(self): Ensure that 'pk' works as an ordering option in Meta. Refs #8291. """ - ArticlePKOrdering.objects.create( - pk=1, headline="Article 1", pub_date=datetime(2005, 7, 26) - ) - ArticlePKOrdering.objects.create( - pk=2, headline="Article 2", pub_date=datetime(2005, 7, 27) - ) - ArticlePKOrdering.objects.create( - pk=3, headline="Article 3", pub_date=datetime(2005, 7, 27) - ) - ArticlePKOrdering.objects.create( - pk=4, headline="Article 4", pub_date=datetime(2005, 7, 28) + Author.objects.create(pk=1) + Author.objects.create(pk=2) + Author.objects.create(pk=3) + Author.objects.create(pk=4) + + self.assertQuerysetEqual( + Author.objects.all(), [ + 4, 3, 2, 1 + ], + attrgetter("pk") ) + def test_order_by_fk_attname(self): + """ + Ensure that ordering by a foreign key by its attribute name prevents + the query from inheriting it's related model ordering option. + Refs #19195. + """ + for i in range(1, 5): + author = Author.objects.create(pk=i) + article = getattr(self, "a%d" % (5 - i)) + article.author = author + article.save(update_fields={'author'}) + self.assertQuerysetEqual( - ArticlePKOrdering.objects.all(), [ + Article.objects.order_by('author_id'), [ "Article 4", "Article 3", "Article 2",