Opened 8 years ago

Closed 7 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: master
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: UI/UX:

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 8 years ago.
fixes this issue, needs testing
select_related.2.patch (6.3 KB) - added by bangcok_dangerus ( at ] hotmail.com 8 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 8 years ago.
more improvements (see post)

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by bangcok_dangerus ( at ] hotmail.com

fixes this issue, needs testing

comment:1 Changed 8 years ago by bangcok_dangerus ( at ] hotmail.com

  • Summary changed from select_related (and hence the admin) sometimes generate incorrect ordering to [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

Changed 8 years ago by bangcok_dangerus ( at ] hotmail.com

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

Changed 8 years ago by bangcok_dangerus ( at ] hotmail.com

more improvements (see post)

comment:2 Changed 8 years ago by bangcok_dangerus ( at ] hotmail.com

  • Summary changed from [PATCH] select_related (and hence the admin) sometimes generate incorrect ordering to [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 Changed 8 years ago by Gary Wilson <gary.wilson@…>

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 8 years ago by mtredinnick

  • Keywords qs-rf added
  • Summary changed from [patch] select_related (and hence the admin) sometimes generate incorrect ordering to select_related (and hence the admin) sometimes generate incorrect ordering

comment:5 Changed 7 years ago by mtredinnick

(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 Changed 7 years ago by mtredinnick

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

comment:7 Changed 7 years ago by mtredinnick

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

(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