Code

Opened 8 years ago

Closed 6 years ago

#2884 closed defect (fixed)

[newforms-admin] Incorrect sql generated when ordering a queryset where the related model's Meta.ordering has an FK

Reported by: bangcok_dangerus ( at ] hotmail.com Owned by: nobody
Component: contrib.admin Version: master
Severity: normal Keywords:
Cc: real.human@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

Description: In the admin, clicking to sort on a field specified in list_display causes a ProgrammingError due to a non-existent column in the SQL ORDER BY clause.

How to replicate: This happens when the field specified in list_display is a ForeignKey to another object, and this related object contains a Meta ordering option which is also a ForeignKey. Example minimal models.py:

from django.db import models

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

class Course(models.Model):
	name = models.CharField(maxlength=40)
	subject = models.ForeignKey(Subject)
	def __str__(self):
		return self.name
	class Admin:
		list_display = ('__str__', 'subject')
	class Meta:
		ordering = ["subject"]

class Class(models.Model):
	course = models.ForeignKey(Course)
	name = models.CharField(maxlength = 40)
	def __str__(self):
		return self.name
	class Admin:
		list_display = ('__str__','course')
	class Meta:
		verbose_name_plural = "Classes"

Navigating to the Classes page and clicking on the 'course' column to order will generate the error.

Cause: This is a bug in django/contrib/admin/views/main.py . The code in ChangeList.get_query_set wrongly assumes that the ordering option in the related object is a field in the related object's table. For the above model, the incorrect ORDER BY clause currently generated is 'test_course.subject'.

Fix: This patch recursively follows any ForeignKeys in the ordering options to find the correct ORDER BY clause, in this case 'test_subject.name'. In the case of circular ForeignKeys with circular ordering, it will order by the related object's raw ForeignKey field (e.g. course.subject_id)

-Basil

Attachments (2)

get_query_set.patch (2.1 KB) - added by bangcok_dangerus ( at ] hotmail.com 8 years ago.
fixes the issue, could do with more testing
t2884.py (652 bytes) - added by Simon G. <dev@…> 7 years ago.

Download all attachments as: .zip

Change History (8)

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

fixes the issue, could do with more testing

comment:1 Changed 7 years ago by mrmachine

  • Component changed from Admin interface to Database wrapper

this is not only an admin issue. it happens when ordering any queryset (in the admin or any 3rd party code) by a foreign key field, where the related model's default Meta.ordering also includes a foreign key.

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

  • Cc real.human@… added

comment:3 Changed 7 years ago by Simon G. <dev@…>

  • Component changed from Database wrapper to Admin interface
  • Patch needs improvement set
  • Summary changed from [patch] incorrect sql generated for sort in admin to incorrect sql generated when ordering a queryset where the related model's Meta.ordering has an FK
  • Triage Stage changed from Unreviewed to Ready for checkin

I can replicate this in the admin framework, but not outside the admin. I've attached the test I was using in case anyone can improve them.

Changed 7 years ago by Simon G. <dev@…>

comment:4 Changed 7 years ago by mtredinnick

  • Summary changed from incorrect sql generated when ordering a queryset where the related model's Meta.ordering has an FK to [newforms-admin] Incorrect sql generated when ordering a queryset where the related model's Meta.ordering has an FK

The patch is more or less correct for newforms-admin, although I doubt it will apply automatically because the line numbers have changed. Not totally in love with the idea of another nested function, but that can be fixed when committing.

comment:5 Changed 7 years ago by jacob

  • Triage Stage changed from Ready for checkin to Accepted

This should be deferred until after queryset-refactor.

comment:6 Changed 6 years ago by mtredinnick

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

Since ordering by fields that are relations Just Works now, this shouldn't be a problem any longer.

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.