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

[k.kosciuszkiewicz at gmail dot com]

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.

[Malcolm Tredinnick]

This has been fixed at some point in the interim.

[Wesley Fok <portland@…>]

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.

[Chris Beaven]

Just tested, and I can confirm the above error.

[Chris Beaven]

(it's for SVN too)

[Chris Beaven]

Fixed!

[Malcolm Tredinnick]

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

[Malcolm Tredinnick]

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

[Malcolm Tredinnick]

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

[Gary Wilson]

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.

[Gary Wilson]

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.

[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)

[Malcolm Tredinnick]

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.