Django

Code

Ticket #7302 (closed: fixed)

Opened 6 months ago

Last modified 3 months ago

Missed quoting in SQL ordering fragment

Reported by: ygpark2@gmail.com Assigned to: nobody
Milestone: 1.0 Component: Database layer (models, ORM)
Version: SVN Keywords: sqlite, db, wrapper 1.0-blocker
Cc: Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 1

Description (Last modified by ramiro)

Lately, I got this error

Environment:

Request Method: GET
Request URL: http://127.0.0.1:8000/pages/
Django Version: 0.97-pre-SVN-7548
Python Version: 2.5.2
Installed Applications:
['django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.admin',
 'pages',
 'hierarchical']
Installed Middleware:
('django.contrib.sessions.middleware.SessionMiddleware',
 'django.middleware.locale.LocaleMiddleware',
 'django.middleware.common.CommonMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.middleware.doc.XViewMiddleware')


Traceback:
File "/Users/ygpark/lib/django_trunk/django/core/handlers/base.py" in get_response
  82.                 response = callback(request, *callback_args, **callback_kwargs)
File "/Users/ygpark/lib/django-page-cms/utils.py" in _dec
  9.         response = func(request, *args, **kwargs)
File "/Users/ygpark/lib/django-page-cms/pages/views.py" in details
  12.     pages = HierarchicalNode.get_first_level_objects(Page)
File "/Users/ygpark/lib/django-page-cms/hierarchical/models.py" in get_first_level_objects
  98.         return [obj.object for obj in objs]
File "/Users/ygpark/lib/django_trunk/django/db/models/query.py" in _result_iter
  78.                 self._fill_cache()
File "/Users/ygpark/lib/django_trunk/django/db/models/query.py" in _fill_cache
  490.                     self._result_cache.append(self._iter.next())
File "/Users/ygpark/lib/django_trunk/django/db/models/query.py" in iterator
  162.         for row in self.query.results_iter():
File "/Users/ygpark/lib/django_trunk/django/db/models/sql/query.py" in results_iter
  200.         for rows in self.execute_sql(MULTI):
File "/Users/ygpark/lib/django_trunk/django/db/models/sql/query.py" in execute_sql
  1466.         cursor.execute(sql, params)
File "/Users/ygpark/lib/django_trunk/django/db/backends/util.py" in execute
  18.             return self.cursor.execute(sql, params)
File "/Users/ygpark/lib/django_trunk/django/db/backends/sqlite3/base.py" in execute
  136.         return Database.Cursor.execute(self, query, params)

Exception Type: OperationalError at /pages/
Exception Value: near "order": syntax error

I think this kind of sql syntax error is caused in the django framework.

So can you check whether this is bug or not?

Attachments

7302.diff (0.6 kB) - added by toke-djangp@toke.de on 07/07/08 17:19:50.
Simple fix without tests
7302.1.diff (1.2 kB) - added by isagalaev on 08/30/08 07:17:46.
Possible fix with tests

Change History

05/24/08 04:48:31 changed by ygpark2@gmail.com

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

I omit something.

while I look up the debug page, I can find the final statement is supposed to execute.

SELECT "hierarchical_hierarchicalobject"."id", "hierarchical_hierarchicalobject"."node_id", "hierarchical_hierarchicalobject"."content_type_id", "hierarchical_hierarchicalobject"."object_id", "hierarchical_hierarchicalnode"."id", "hierarchical_hierarchicalnode"."name", "hierarchical_hierarchicalnode"."parent_id", "hierarchical_hierarchicalnode"."order", "django_content_type"."id", "django_content_type"."name", "django_content_type"."app_label", "django_content_type"."model" FROM "hierarchical_hierarchicalobject" INNER JOIN "django_content_type" ON ("hierarchical_hierarchicalobject"."content_type_id" = "django_content_type"."id") INNER JOIN "hierarchical_hierarchicalnode" ON ("hierarchical_hierarchicalobject"."node_id" = "hierarchical_hierarchicalnode"."id") WHERE "hierarchical_hierarchicalobject"."content_type_id" = ? AND "hierarchical_hierarchicalobject"."node_id" IN (?, ?, ?) ORDER BY "hierarchical_hierarchicalnode".order ASC

In this statement,

== ORDER BY "hierarchical_hierarchicalnode".order ASC == should be

ORDER BY "hierarchical_hierarchicalnode"."order" ASC

, I think

05/25/08 06:44:47 changed by russellm

  • status changed from new to closed.
  • resolution set to invalid.

Please use the mailing lists to answer 'what is wrong?' questions. The ticket database is for tracking known problems.

07/07/08 16:49:27 changed by anonymous

To reproduce use: MyModel?.objects.select_related().order_by('related_table.order')

This is not affected: MyModel?.objects.select_related().order_by('related_table.notorderfield')

It seems to produce wrong SQL when the field "order" is used in "table.order" syntax.

Only sqlite is affected. The same code runs in PostgreSQL fine.

07/07/08 17:19:50 changed by toke-djangp@toke.de

  • attachment 7302.diff added.

Simple fix without tests

07/07/08 17:30:16 changed by axiak

  • status changed from closed to reopened.
  • resolution deleted.
  • stage changed from Unreviewed to Accepted.

It seems that this is a valid bug -- that the column name in an ORDER BY clause is not quoted (at least in SQLite).

Am I wrong? Also, is the patch correct? I don't know the DB API all that much, but it seems too obvious to have not been there before.

07/07/08 17:58:14 changed by ramiro

  • description changed.

Seems to be a valid bug regarding the 0.96 backwards compatible, abstraction-leaking syntax for specifying ordering by a related model's field (namely, using <name of the related model table>.<field name>. Since queryset-refactor (that the OP seems to be running as he is using r7548) was merged, it is also possible (recommended?) to use 'relatedmodel__fieldname'. See http://www.djangoproject.com/documentation/db-api/#order-by-fields

07/08/08 02:55:25 changed by anonymous

  • has_patch set to 1.

08/13/08 04:33:20 changed by anonymous

I confirm that the fix is always working with the revision 8335.

the line affected is the following :

objs = HierarchicalObject?.objects.filter(content_type=ctype, nodein=nodes).order_by('hierarchical_hierarchicalnode.order').select_related()

08/15/08 09:48:01 changed by toke

  • stage changed from Accepted to Ready for checkin.
  • milestone set to 1.0.

08/15/08 23:05:55 changed by russellm

  • status changed from reopened to closed.
  • resolution set to invalid.

First off - there's no way in hell that this is ready for checkin. It may well be a simple fix, but there is no test case, and the discussion is so horribly confused it's impossible to tell what actually causes the problem. In all the discussion we have stack traces, example code, but no actual models with corresponding queries that fail.

What what I can glean, this isn't a bug - It's a usage of a syntax that has been deprecated.

If you disagree, you need to provide:

  • A test case that demonstrates the problem - preferably one that is integrated with the regressiontests/queries suite.
  • A link to the documentation that says that the old syntax should actually work.

08/19/08 15:26:36 changed by toke

News.objects.order_by('news_news.category_id') resulting in: SELECT "news_news"."id", "news_news"."title", "news_news"."category_id" FROM "news_news" ORDER BY "news_news".category_id ASC

which leads to wrong SQL. Every developer is strongly advised not to allow users input the value for order_by() unchecked or at least filter the "." out to force the new syntax. It has some other issues too but these are out of scope of this ticket. This is in my opinion definitively a bug as long as this table.field syntax exists in that way (documented or not).

08/20/08 19:25:42 changed by toke

  • status changed from closed to reopened.
  • resolution deleted.
  • stage changed from Ready for checkin to Accepted.

For completeness to reproduce:

bug/models.py

from django.db import models

class MyModel(models.Model):
    """
    >>> MyModel(title="test",order=3).save()
    >>> MyModel.objects.order_by('order')
    [<MyModel: test>]
    >>> MyModel.objects.order_by('bug_mymodel.title')
    [<MyModel: test>]
    >>> MyModel.objects.order_by('bug_mymodel.order')
    [<MyModel: test>]
    """
    title = models.CharField(max_length=200)
    order = models.IntegerField()
    
    def __unicode__(self):
        return self.title

08/20/08 19:28:33 changed by toke

  • owner changed from nobody to toke.
  • status changed from reopened to new.

08/20/08 20:14:13 changed by toke

  • owner changed from toke to nobody.

08/20/08 23:38:16 changed by mtredinnick

  • summary changed from Bug in the query statement to Missed quoting in SQL ordering fragment.

This is a bug, but not for the reasons you claim (we aren't going to fix bugs that just exist for that old-style ordering, since it is bug-for-bug compatible with old code at the moment and the way to fix those bugs is not to use it). It's a bug because it bites users of extra_order_by.

So we need a test that exercises the bug using extra_order_by, probably by creating a model with a field called order and including that model's table in an extra() block. Then we can look at including it.

In future, however, please do not reopen tickets that have been closed by core developers. We have a process for what to do if you don't like the closure.

08/27/08 15:23:30 changed by toke

  • needs_better_patch set to 1.

The patch does not solve the whole issue. The "bad" parts still exists.

08/27/08 15:36:26 changed by mtredinnick

But you're not going to help by providing a test case or any further explanation of what you mean? How are we going to be able to fix the problem then?

08/30/08 07:17:46 changed by isagalaev

  • attachment 7302.1.diff added.

Possible fix with tests

08/30/08 07:20:55 changed by isagalaev

I've made a minimal test that fails with current trunk upon not quoting the word "order" in order_by. It's in the patch 7302.1.diff along with a possible fix. It's "possible" because I'm not sure that quoting with qn2 there is the right thing for all cases.

08/31/08 15:45:47 changed by jacob

  • keywords changed from sqlite, db, wrapper to sqlite, db, wrapper 1.0-blocker.

09/01/08 07:07:27 changed by russellm

  • status changed from new to closed.
  • resolution set to fixed.

(In [8794]) Fixed #7302: Corrected quoting of columns in extra_group_by. Thanks to Ivan Sagalaev for the patch and initial test.


Add/Change #7302 (Missed quoting in SQL ordering fragment)




Change Properties
Action