Code

Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#1576 closed defect (fixed)

[newforms-admin] Incorrect SQL generated for ordering by related models.

Reported by: k.kosciuszkiewicz at gmail dot com Owned by: adrian
Component: contrib.admin Version: master
Severity: normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Test case:

class One(meta.Model):
    field_1 = meta.IntegerField()

    class META:
        ordering = ['-field_1']

class Two(meta.Model):
    field_1 = meta.IntegerField()
    fkey    = meta.ForeignKey(One)

    class META:
        admin = meta.Admin()
        ordering = ['fkey']

Displaying list of "Twos" in admin interface results in exception because of invalid SQL is generated with ORDER BY "test_ones"."-field_1" ASC.

Attachments (1)

admin_fk_ordering.patch (1.5 KB) - added by SmileyChris 7 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 8 years ago by mtredinnick

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

This has been fixed at some point in the interim.

comment:2 Changed 7 years ago by Wesley Fok <portland@…>

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version set to 0.96

I've discovered the same problem with 0.96:

## structure/models.py
class Issue(models.Model):
    (...)
    pub_date = models.DateField('Publication date', unique=True)
    (...)

    class Meta:
        ordering            = ('-pub_date',)


## stories/models.py
class Story(models.Model):
    (...)
    issue = models.ForeignKey(Issue)
    (...)

    class Admin:
        ordering        = ('issue',)

Attempting to view a list of stories in the Admin interface returns this:

Traceback (most recent call last):
File "/usr/lib/python2.4/site-packages/django/core/handlers/base.py" in get_response
  77. response = callback(request, *callback_args, **callback_kwargs)
File "/usr/lib/python2.4/site-packages/django/contrib/admin/views/decorators.py" in _checklogin
  55. return view_func(request, *args, **kwargs)
File "/usr/lib/python2.4/site-packages/django/views/decorators/cache.py" in _wrapped_view_func
  39. response = view_func(request, *args, **kwargs)
File "/usr/lib/python2.4/site-packages/django/contrib/admin/views/main.py" in change_list
  760. cl = ChangeList(request, model)
File "/usr/lib/python2.4/site-packages/django/contrib/admin/views/main.py" in __init__
  572. self.get_results(request)
File "/usr/lib/python2.4/site-packages/django/contrib/admin/views/main.py" in get_results
  630. result_list = list(self.query_set)
File "/usr/lib/python2.4/site-packages/django/db/models/query.py" in __iter__
  108. return iter(self._get_data())
File "/usr/lib/python2.4/site-packages/django/db/models/query.py" in _get_data
  470. self._result_cache = list(self.iterator())
File "/usr/lib/python2.4/site-packages/django/db/models/query.py" in iterator
  183. cursor.execute("SELECT " + (self._distinct and "DISTINCT " or "") + ",".join(select) + sql, params)
File "/usr/lib/python2.4/site-packages/django/db/backends/util.py" in execute
  12. return self.cursor.execute(sql, params)
File "/usr/lib/python2.4/site-packages/django/db/backends/mysql_old/base.py" in execute
  42. return self.cursor.execute(sql, params)
File "/usr/lib/python2.4/site-packages/MySQLdb/cursors.py" in execute
  137. self.errorhandler(self, exc, value)
File "/usr/lib/python2.4/site-packages/MySQLdb/connections.py" in defaulterrorhandler
  33. raise errorclass, errorvalue

  OperationalError at /admin/stories/story/
  (1054, "Unknown column 'structure_issue.-pub_date' in 'order clause'")

The SQL query:

SELECT `stories_story`.`id`,`stories_story`.`head`,`stories_story`.`deck`,`stories_story`.`slug`,`stories_story`.`section_id`,`stories_story`.`issue_id`,`stories_story`.`content`,`stories_story`.`summary`,`stories_story`.`section_order`,`structure_section`.`id`,`structure_section`.`name`,`structure_section`.`short_name`,`structure_section`.`slug`,`structure_issue`.`id`,`structure_issue`.`volume_id`,`structure_issue`.`issue`,`structure_issue`.`extra`,`structure_issue`.`pub_date`,`structure_issue`.`sections_id`,`structure_issue`.`is_published`,`structure_volume`.`id`,`structure_volume`.`volume`,`structure_flatplanconfig`.`id`,`structure_flatplanconfig`.`name` FROM `stories_story` , `structure_section`, `structure_issue`, `structure_volume`, `structure_flatplanconfig` WHERE `stories_story`.`section_id` = `structure_section`.`id` AND `stories_story`.`issue_id` = `structure_issue`.`id` AND `structure_issue`.`volume_id` = `structure_volume`.`id` AND `structure_issue`.`sections_id` = `structure_flatplanconfig`.`id` ORDER BY `structure_issue`.`-pub_date` ASC

Moving the "ordering = ('issue',)" field to the Meta class in Story doesn't help. What does help is if I simply swap the minuses; giving Issues an ordering of 'pub_date' and Stories an ordering of '-issue' works just fine.

comment:3 Changed 7 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Accepted

Just tested, and I can confirm the above error.

comment:4 Changed 7 years ago by SmileyChris

  • Version changed from 0.96 to SVN

(it's for SVN too)

Changed 7 years ago by SmileyChris

comment:5 Changed 7 years ago by SmileyChris

  • Has patch set
  • Triage Stage changed from Accepted to Ready for checkin

Fixed!

comment:6 Changed 7 years ago by mtredinnick

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

(In [5092]) Fixed #1576 -- Fixed incorrect SQL generated when using descending ordering
from related models. Patch from SmileyChris.

comment:7 Changed 7 years ago by mtredinnick

(In [5097]) Revert [5092], since this should only have been applied to newforms-admin.
Refs #1576.

comment:8 Changed 7 years ago by mtredinnick

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Summary changed from Incorrect SQL generated for ordering by related models. to [newforms-admin] Incorrect SQL generated for ordering by related models.

comment:9 Changed 7 years ago by mtredinnick

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

(In [5122]) newforms-admin: Fixed #1576 -- Fixed SQL generation when using descending
ordering from related models. Based on a patch from SmileyChris.

comment:10 follow-ups: Changed 7 years ago by gwilson

Malcolm, what was the reason this was applied to newforms-admin instead of trunk? I ask as there are a couple open tickets about this same issue in trunk (#3825 and #5057) that could be marked duplicates of this ticket.

comment:11 in reply to: ↑ 10 Changed 7 years ago by gwilson

Replying to gwilson:

I ask as there are a couple open tickets about this same issue in trunk (#3825 and #5057) that could be marked duplicates of this ticket.

#2895 could be marked a duplicate of this too if we want to re-open this one. In any case, I have marked #3825 and #5057 as duplicates of #2895.

comment:12 Changed 7 years ago by Ramiro Morales

Just wanted to point to the comment/patch I posted here on #3002

So, after #2076 is solved, or even better after Malcolm's ongoing queryset refactoring efforts lands in trunk and solves these recursive order_by+FK+Meta.ordering problems (I'm hoping it's one of the itemos in his TODO list) maybe this problem can be solved by applying DRY principle: Letting the ORM handle this stuff and removing similar code from the admin app (be it classical or newforms-admin)

comment:13 in reply to: ↑ 10 Changed 7 years ago by mtredinnick

Replying to gwilson:

Malcolm, what was the reason this was applied to newforms-admin instead of trunk? I ask as there are a couple open tickets about this same issue in trunk (#3825 and #5057) that could be marked duplicates of this ticket.

As per a discussion on django-dev a while back, we're trying not to apply anything we don't absolutely have to against admin in trunk so that merging with newforms-admin is easier and we can focus on getting newforms-admin finished. Sometimes we forget and apply stuff to trunk admin, but as much as possible patches should be rewritten against newforms-admin.

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.