Opened 17 years ago

Closed 16 years ago

#2874 closed defect (fixed)

select_related (and hence the admin) sometimes generate incorrect ordering

Reported by: bangcok_dangerus [ a t ) hotmail.com Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: normal Keywords: qs-rf-fixed
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Description: Under certain circumstances, Model.objects.select_related().order_by('table.field') will generate strangely ordered output. As a result, the admin interface displays incorrectly sorted items when the heading of a column specified in list_display is clicked.

How to reproduce: This only occurs when the model being queried contains multiple foreign keys which point to the same table, one directly and another indirectly via other ForeignKey relationships. Example:

from django.db import models

class Program(models.Model):
	name = models.CharField(maxlength=30)
	class Admin:
		pass
	def __str__(self):
		return self.name

class Teacher(models.Model):
	first_name = models.CharField(maxlength=20)
	last_name = models.CharField(maxlength=20)
	def __str__(self):
		return self.last_name +", "+ self.first_name
	class Admin:
		pass
	class Meta:
		ordering = ["last_name"]

class Department(models.Model):
	name = models.CharField(maxlength=20)
	head = models.ForeignKey(Teacher)
	def __str__(self):
		return self.name
	class Admin:
		pass

class Subject(models.Model):
	name = models.CharField(maxlength=40)
	department = models.ForeignKey(Department)
	def __str__(self):
		return self.name
	class Admin:
		pass

class Course(models.Model):
	program = models.ForeignKey(Program,default=1)
	subject = models.ForeignKey(Subject)
	teacher = models.ForeignKey(Teacher)
	def __str__(self):
		return str(self.id)
	class Admin:
		list_display = ('__str__','program','subject','teacher')

The 'model in question' here is Course. Following the relationships recursively in order:

Course.program -> Progam
Course.subject -> Subject -> Department -> Teacher
Course.teacher -> Teacher

i.e. the Teacher model is reached more than once when recursively following ForeignKey relationships. Also necessary to reproduce the bug is the fact that the the ForeignKey pointing directly to Teacher is reached AFTER having followed the indirect link to Teacher.

Using this model and browsing to the admin page for Courses, clicking on the Teacher column will not sort the teacher's names correctly. In fact, they are sorted by the name of the course's head of department, which is not (and cannot be) shown on the admin page.

Cause: This is a subtle bug in django/db/models/query.py: when recursively following ForeignKey relationships in the fill_table_cache function, a table that appears multiple times in a join is given an alias for the second (and subsequent) times it appears in the SQL statement. In the situation described above, the alias is assigned to the join for the 'more important' relationship, i.e. the direct course -> teacher. However, the ORDER BY statement is left unchanged as 'ORDER BY appname_teacher.last_name', which actually contains the course's head of department.

Temporary Solution: Changing the order in which the model's fields are ordered so that the 'direct' ForeignKey is before the 'indirect' one. For example, changing the Course model above to:

class Course(models.Model):
	teacher = models.ForeignKey(Teacher)
	program = models.ForeignKey(Program,default=1)
	subject = models.ForeignKey(Subject)
	def __str__(self):
		return str(self.id)
	class Admin:
		list_display = ('__str__','program','subject','teacher')

will now show the correct sorting in the admin.

I'm trying to work on a patch, but I'm close to giving up. I'm not a very experienced programmer, and IMHO this is a sticky one :)

-Basil

Attachments (3)

select_related.patch (6.4 KB ) - added by bangcok_dangerus ( at ] hotmail.com 17 years ago.
fixes this issue, needs testing
select_related.2.patch (6.3 KB ) - added by bangcok_dangerus ( at ] hotmail.com 17 years ago.
same as above, but this one leaves out a couple of unnecessary lines
select_related.3.patch (6.7 KB ) - added by bangcok_dangerus ( at ] hotmail.com 17 years ago.
more improvements (see post)

Download all attachments as: .zip

Change History (10)

by bangcok_dangerus ( at ] hotmail.com, 17 years ago

Attachment: select_related.patch added

fixes this issue, needs testing

comment:1 by bangcok_dangerus ( at ] hotmail.com, 17 years ago

Summary: select_related (and hence the admin) sometimes generate incorrect ordering[PATCH] select_related (and hence the admin) sometimes generate incorrect ordering

This patch works for me, but I need to test it more. If it works right, it should also close #2549.

I've been thinking about how it would be possible to allow a user to explicitly specify ordering for select_related on a complex model. Probably something like:

Course.objects.select_related().order_by('Course.Teacher', last_name)
Course.objects.select_related().order_by('Course.Subject.Department.Teacher', first_name)

where each '.' represents a ForeignKey.

I wouldn't mind trying to implement this or something similar if there's interest.

-Basil

by bangcok_dangerus ( at ] hotmail.com, 17 years ago

Attachment: select_related.2.patch added

same as above, but this one leaves out a couple of unnecessary lines

by bangcok_dangerus ( at ] hotmail.com, 17 years ago

Attachment: select_related.3.patch added

more improvements (see post)

comment:2 by bangcok_dangerus ( at ] hotmail.com, 17 years ago

Summary: [PATCH] select_related (and hence the admin) sometimes generate incorrect ordering[patch] select_related (and hence the admin) sometimes generate incorrect ordering

I discovered during testing that the patch fails for

TestModel.objects.select_related().order_by('some_table.some_field')

if some_table is met more than once by following TestModel's ForeignKeys, but is not directly related to TestModel. The updated patch (select_related.3.patch) fixes this.

-Basil

comment:3 by Gary Wilson <gary.wilson@…>, 17 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Malcolm Tredinnick, 17 years ago

Keywords: qs-rf added
Summary: [patch] select_related (and hence the admin) sometimes generate incorrect orderingselect_related (and hence the admin) sometimes generate incorrect ordering

comment:5 by Malcolm Tredinnick, 16 years ago

(In [6510]) 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).

comment:6 by Malcolm Tredinnick, 16 years ago

Keywords: qs-rf-fixed added; qs-rf removed

comment:7 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

(In [7477]) Merged the queryset-refactor branch into trunk.

This is a big internal change, but mostly backwards compatible with existing
code. Also adds a couple of new features.

Fixed #245, #1050, #1656, #1801, #2076, #2091, #2150, #2253, #2306, #2400, #2430, #2482, #2496, #2676, #2737, #2874, #2902, #2939, #3037, #3141, #3288, #3440, #3592, #3739, #4088, #4260, #4289, #4306, #4358, #4464, #4510, #4858, #5012, #5020, #5261, #5295, #5321, #5324, #5325, #5555, #5707, #5796, #5817, #5987, #6018, #6074, #6088, #6154, #6177, #6180, #6203, #6658

Note: See TracTickets for help on using tickets.
Back to Top