Code

Opened 8 years ago

Closed 7 years ago

#2210 closed defect (duplicate)

[patch] order_by on related table with db_column different to name fails

Reported by: Russell Cloran <russell@…> Owned by: adrian
Component: Database layer (models, ORM) Version: master
Severity: normal Keywords: order_by related
Cc: bs1984@…, russell@…, gary.wilson@…, real.human@…, arthur.case@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Hi,

When ordering by a related table which has a db_column different to the field name, and ordering by the field name, incorrect SQL is generated.

This bug is on line 483 of django/db/models/query.py (r3189) -- it incorrectly uses the opts of the main table, not the related table in the call to orderfield2column(). This shows up in the admin interface, when sorting by a related field.

I do not know the correct fix for this bug.

Russell

Attachments (4)

order_by_related.diff (2.1 KB) - added by serbaut@… 8 years ago.
Patch for order_by related fields
ticket-2210.diff (4.3 KB) - added by mir@… 7 years ago.
revised and updated patch
models1.py (1.9 KB) - added by ramiro <rm0 _at_ gmx.net> 7 years ago.
models2.py (1.9 KB) - added by ramiro <rm0 _at_ gmx.net> 7 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 8 years ago by Russell Cloran <russell@…>

  • Cc russell@… added

comment:2 Changed 8 years ago by ramiro

A similar report was made in #2076, also #2121 may be related too?

comment:3 Changed 8 years ago by Russell Cloran <russell@…>

This is a different bug to #2076, fwict. The joins are being correctly generated in my case, but the wrong column name is being used.

comment:4 Changed 8 years ago by Russell Cloran <russell@…>

  • Cc russell@… added; russell@… removed

Changed 8 years ago by serbaut@…

Patch for order_by related fields

comment:5 Changed 8 years ago by serbaut@…

  • Summary changed from order_by on related table with db_column different to name fails to [patch] order_by on related table with db_column different to name fails

I added a patch that allows you to do order by related fields.

Foo.objects.all().order_by('name')
Bar.objects.order_by('-foo__name')
Baz.objects.order_by('bar__foo__name')

The Admin interface needs to be updated to make use of this.

comment:6 Changed 8 years ago by Gary Wilson <gary.wilson@…>

  • Cc gary.wilson@… added

comment:7 Changed 8 years ago by mrmachine <real dot human at mrmachine dot net>

  • Cc real.human@… added

comment:8 Changed 8 years ago by anonymous

  • Cc arthur.case@… added

Changed 7 years ago by mir@…

revised and updated patch

comment:9 Changed 7 years ago by mir@…

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

The orignal patch failed to support order_by('?'), though it tried to handle it twice. I've revised it, and now it passes all tests. It still needs additional tests to demonstrate that order_by('xxx_yyy') works now. Thanks, serbaut, for the original patch, I didn't really have to do a lot ;-)

comment:10 Changed 7 years ago by mir@…

marked #2076, #3248, #1616 marked as duplicates.

Regarding the tests, please write tests for both directions (asc/desc)

comment:11 Changed 7 years ago by bram.Dejong+djangotrac@…

  • Keywords order_by related added

This bug is confirmed. Here's a complete example to reproduce the problem:

models.py

from django.db import models

class Book(models.Model):
	title = models.TextField()

class BookStat(models.Model):
	book = models.OneToOneField(Book, related_name='stats')
	times_read = models.PositiveIntegerField()

After syncing the DB, in the command line:

 >>> from orderby.example.models import *
 >>> book = Book.objects.create(title="BFG")
 >>> book.save()
 >>> bs = BookStat.objects.create(book=book, times_read=15)
 >>> bs.save()
 >>> Book.objects.all().order_by("stats__times_read")
Traceback (most recent call last):
   File "<console>", line 1, in <module>
   File "C:\Python25\lib\site-packages\django\db\models\query.py", line 101, in __repr__
     return repr(self._get_data())
   File "C:\Python25\lib\site-packages\django\db\models\query.py", line 444, in _get_data
     self._result_cache = list(self.iterator())
   File "C:\Python25\lib\site-packages\django\db\models\query.py", line 181, in iterator
     cursor.execute("SELECT " + (self._distinct and "DISTINCT " or "") + ",".join(select) + sql, params)
   File "C:\Python25\lib\site-packages\django\db\backends\util.py", line 12, in execute
     return self.cursor.execute(sql, params)
   File "C:\Python25\lib\site-packages\django\db\backends\postgresql\base.py", line 43, in execute
     return self.cursor.execute(sql, [smart_basestring(p, self.charset) for p in params])
ProgrammingError: ERROR:  column example_book.stats__times_read does not exist at character 80
SELECT "example_book"."id","example_book"."title" FROM "example_book" ORDER BY "example_book"."stats__times_read" ASC

comment:12 Changed 7 years ago by bram.Dejong+djangotrac@…

I just found out exactly the same error happens in this case:

from django.db import models

class Book(models.Model):
	title = models.TextField()

class BookStat(models.Model):
	book = models.OneToOneField(Book)
	times_read = models.PositiveIntegerField()

Book.objects.all().order_by("bookstat__times_read")

I.e. can't sort by a one-to-one field in another model.

  • bram

comment:13 Changed 7 years ago by bram.Dejong+djangotrac@…

The patch breaks on:

{{
queryset.extra(select={'squeek': "......"}).order_by("-squeek")
}}

resulting in the error:

Cannot resolve keyword 'comment_count' into field

comment:14 Changed 7 years ago by bram.Dejong+djangotrac@…

  • Patch needs improvement set

comment:15 Changed 7 years ago by Michael Radziej <mir@…>

Can you please post your models and the full traceback? How does 'comment_count' get into it?

comment:16 Changed 7 years ago by Michael Radziej <mir@…>

May be this is related to #3355?

comment:17 Changed 7 years ago by Michael Radziej <mir@…>

Bram, can you please also send in the full sql statement that was sent to the database? queryset._get_sql_clause() can do this trick if it isn't visible elsewhere.

comment:18 Changed 7 years ago by anonymous

Sorry, comment_count should have been "squeek".

That said, I had to un-apply the patch and work with the extra( ) to get it all working without the patch as I'm on a very tight deadline. However here's what I found the last time:

Models:

from django.db import models

class Book(models.Model):
    title = models.TextField()

class BookStat(models.Model):
    book = models.OneToOneField(Book)
    times_read = models.PositiveIntegerField()

class BookRating(models.Model):
    book = models.ForeignKey(Book)
    rating = models.PositiveIntegerField()

Shell:

from example.models import *
 
bfg = Book.objects.create(title="bfg")
bfg.save()
matilda = Book.objects.create(title="matilda")
matilda.save()
 
BookRating.objects.create(book=bfg, rating=5).save()
BookRating.objects.create(book=bfg, rating=8).save()
BookRating.objects.create(book=bfg, rating=15).save()
BookRating.objects.create(book=matilda, rating=15).save()
BookRating.objects.create(book=matilda, rating=16).save()
BookRating.objects.create(book=matilda, rating=17).save()
BookStat.objects.create(book=bfg, times_read=5).save()
BookStat.objects.create(book=matilda, times_read=3).save()

# works with patch:
qs = Book.objects.all().order_by("stats__times_read")

# the equivalent, works without patch, same result
qs = qs.extra(where=["example_book.id = example_bookstat.book_id"], tables=["example_bookstat"]).order_by("-example_bookstat.times_read")

# works WITHOUT patch, breaks with patch 
qs = Book.objects.all().extra(select={ 'avg_rating': 'SELECT avg(rating) FROM example_bookrating WHERE example_bookrating.book_id = example_book.id'}).order_by("-avg_rating")

Right now I can't do the stack-trace as I would have to re-apply the patch...

For people looking into these problems, this is a workaround for the initial problem described in the ticket:

comment:19 Changed 7 years ago by anonymous

  • Cc bs1984@… added

comment:20 Changed 7 years ago by Michael Radziej <mir@…>

  • Has patch unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Unreviewed

OUCH

The first two patches and the later comments (beginning with comment 9) actually belong to #2076.

See thread in django-developers

Thank you for digging this up, Ramiro Gonzales.

comment:21 Changed 7 years ago by ramiro <rm0 _at_ gmx.net>

So, it seems the patch for #2076 also solves this ticket :-/

Attached you will find a models1.py, it has some doctest [1]testcases that reproduce the problem described by the original reporter. It should be installed as the models.py file of a ticket2210 application.

Also attached is a models2.py, it has the same [1]testcases but using the order_by notation introduced by the patch for #2076 (so that patch should be applied first) and they complete without errors. It should be installed as the app models.py file.

Apologies for the noise.

  1. Run them executing
    $ ./manage.py test ticket2210
    

Changed 7 years ago by ramiro <rm0 _at_ gmx.net>

Changed 7 years ago by ramiro <rm0 _at_ gmx.net>

comment:22 Changed 7 years ago by Gary Wilson <gary.wilson@…>

  • Triage Stage changed from Unreviewed to Accepted

comment:23 Changed 7 years ago by mtredinnick

  • Resolution set to duplicate
  • Status changed from new to closed

Closing this as a duplicate of #2076, since that ticket has a fairly good patch and test cases.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.