Code

Opened 3 years ago

Closed 18 months ago

#17158 closed Bug (fixed)

django.db.backends.BaseDatabaseOperations.last_executed_query can raise an exception

Reported by: a.gerchenov@… Owned by: aaugustin
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords:
Cc: rmanoch@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Method "last_executed_query" should not always return "smart_unicode(sql) % u_params". If params variable is empty it should return just "smart_unicode(sql)". The problem is when we try to execute query with MySQL DATA_FORMAT function it says error.

Example:

from django.db import connection
c = connection.cursor()
c.execute("SELECT * FROM VAR_current GROUP BY DATE_FORMAT(closing_month, '%Y-%m');")
print c.fetchall()

Error:

django/db/backends/__init__.py", line 216, in last_executed_query
    return smart_unicode(sql) % u_params
TypeError: not enough arguments for format string

This issue is closely related to #9055.

Attachments (2)

17158-testcase.patch (944 bytes) - added by aaugustin 3 years ago.
17158.patch (3.1 KB) - added by aaugustin 18 months ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from django.db.backends.BaseDatabaseOperations.last_executed_query issue to django.db.backends.BaseDatabaseOperations.last_executed_query can raise an exception
  • Triage Stage changed from Unreviewed to Accepted

Since r16081, there is a better implementation of last_executed_query for MySQL (and Oracle), so the problem is solved as far as MySQL is concerned.

It still exists for SQLite, and although we came to the conclusion that we couldn't build a reliable last_executed_query for SQLite in #14091, it shouldn't raise an exception.

I see three possible solutions:

  • 1) give up on interpolation entirely, and return a string like "SQL = ..., PARAMS = ...".
  • 2) catch exceptions in the interpolation and return smart_unicode(sql) if an exception occurs
  • 3) mangle the SQL to replace some % by %% before interpolation.

I'm in favor of 1 because it will avoid returning subtly invalid SQL when the database backend in SQLite. It's backwards incompatible, but last_executed_query never returned something usable under SQLite. Returning obviously incomplete SQL is less likely to confuse newcomers.

I'm against 3 because it's overengineering, and I'm not convinced it's possible to get it right for all cases anyway.

Last edited 3 years ago by aaugustin (previous) (diff)

Changed 3 years ago by aaugustin

comment:2 Changed 3 years ago by aaugustin

I've just uploaded a test case demonstrating the issue under SQLite. The assertion in this test case may have to be changed depending on the solution that we actually choose.

comment:3 Changed 21 months ago by aaugustin

  • Owner changed from nobody to aaugustin

Changed 18 months ago by aaugustin

comment:4 Changed 18 months ago by aaugustin

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

I'll commit the patch I just attached if no one objects.

comment:5 Changed 18 months ago by akaariai

looks good.

comment:6 Changed 18 months ago by Aymeric Augustin <aymeric.augustin@…>

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

In 6605ac331a9e03fa41c301d122c5727c0d98b970:

Fixed #17158 -- Used a non-ambiguous representation of SQL queries

when a correct representation cannot be obtained.

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.