Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19720 closed Bug (fixed)

Invalid SQL statement generated when deleting rows in Meta ordered tables (found using Oracle)

Reported by: dylan.klomparens@… Owned by: nobody
Component: Database layer (models, ORM) Version: 1.5-beta-1
Severity: Release blocker Keywords: ORM SQL ordering Meta
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Given the following example model, invalid SQL deletes are composed in Django's ORM. This breaks Django's interaction with Oracle, producing error 'ORA-00907'.

from django.db import models
class OrderedModel(models.Model):
	name = models.CharField(max_length=100)
	class Meta:
		ordering = ['name']
	def __unicode__(self):
		return unicode(self.name)

Django's source code comments state that a QuerySet delete is broken into two SQL queries: one to find the items to delete, and the second to perform the deletion. The general form of this is:

DELETE FROM "TABLE" WHERE "ID" IN (SEARCH_STATEMENT)

Thus, there is no reason to order SEARCH_STATEMENT. However, Django provides an ORDER BY clause in the SEARCH_STATEMENT when Meta ordering is specified. Using the example model shown above, deleting an object from the admin interface produces this SQL SEARCH_STATEMENT:

SELECT "Test_orderedmodel"."id", "Test_orderedmodel"."name" FROM "Test_orderedmodel" WHERE "Test_orderedmodel"."id" IN (5, 6) ORDER BY "Test_orderedmodel"."name" ASC

This bug can be remedied by patching function delete() in class QuerySet in file $DJANGO_HOME/db/models/query.py, line 525.

The buggy line is: del_query.query.clear_ordering()
And it should be: del_query.query.clear_ordering(force_empty=True)

After the code is patched the example SQL statement becomes:

SELECT "Test_orderedmodel"."id", "Test_orderedmodel"."name" FROM "Test_orderedmodel" WHERE "Test_orderedmodel"."id" IN (5, 6)


Further testing reveals that this is an isolated bug.

I looked for other uses of the clear_ordering() function in Django's source code that might cause similar bugs. I found two other usage of the function in $DJANGO_HOME/db/models/query.py: one in the latest() function and the other in the order_by() function. After extensive testing I concluded that Meta ordering does not adversely affect these functions. That said, it may be prudent to make the force_empty parameter of the clear_ordering() function an explicit parameter instead of providing a default of False. All other uses of the clear_ordering() function that I found specify force_empty=True.

I am 99% confident this bug does not adversely affect any other portions of Django's code.

Change History (4)

comment:1 Changed 3 years ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

Good catch. If the search condition is a "complex condition", that is, targeting more than the base table, then a subquery select will be performed, and that subquery currently has Meta.ordering causing "missing right parentheses" error on Oracle. This seems to be a regression caused by the fast-delete code. So, fixing for 1.5.x and master in a couple of minutes.

The clear_ordering must have a flag idea is good, but I will do that only to master as a separate commit.

comment:2 Changed 3 years ago by Anssi Kääriäinen <akaariai@…>

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

In 8ef3235034a1a7616714a5d61486dc68536f74ee:

Fixed #19720 -- Oracle ordering related delete regression

When a query had a complex where condition (a condition targeting more
than the base table) a subquery was used for deletion. However, the
query had default ordering from the model's meta and Oracle doesn't
work with ordered subqueries.

The regression was caused by fast-path deletion code introduced in
1cd6e04cd4f768bcd4385b75de433d497d938f82 for fixing #18676.

Thanks to Dylan Klomparens for the report.

comment:3 Changed 3 years ago by Anssi Kääriäinen <akaariai@…>

In b18ad807e0b7f34df4e4b319af128883a54c03bd:

[1.5.x] Fixed #19720 -- Oracle ordering related delete regression

When a query had a complex where condition (a condition targeting more
than the base table) a subquery was used for deletion. However, the
query had default ordering from the model's meta and Oracle doesn't
work with ordered subqueries.

The regression was caused by fast-path deletion code introduced in
1cd6e04cd4f768bcd4385b75de433d497d938f82 for fixing #18676.

Thanks to Dylan Klomparens for the report.

Backpatch of 8ef3235034a1a7616714a5d61486dc68536f74ee

comment:4 Changed 3 years ago by Anssi Kääriäinen <akaariai@…>

In 5cc0f5f8c1f35218ee4bce2d8c943a86f1888b96:

Made Query.clear_ordering force_empty arg mandatory

Previously it was possible to call clear_ordering without the
force_empty argument. The result was that the query was still ordered
by model's meta ordering if that was defined. By making the arg
mandatory it will be easier to spot possible errors caused by assuming
clear_ordering will remove all ordering.

Thanks to Dylan Klomparens for the suggestion. Refs #19720.

Note: See TracTickets for help on using tickets.
Back to Top