Opened 4 years ago

Closed 4 years ago

Last modified 7 months ago

#17755 closed Cleanup/optimization (fixed)

Use database adapters for converting time zone aware datetimes in raw queries

Reported by: akaariai Owned by: aaugustin
Component: Database layer (models, ORM) Version: 1.4-beta-1
Severity: Release blocker Keywords: akaariai
Cc: akaariai Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


The issue is that if you use timezone aware datetimes in raw SQL queries the datetimes aren't converted to UTC. This will give wrong results, as times are in UTC in the database, but the query parameters aren't. This issue is present at least on SQLite3 and MySQL. Oracle might suffer from this issue, too, but I can't verify that currently. The issues is present only when USE_TZ=True.

There has been some discussion about this both on django-developers and in ticket #17728. The conclusion seems to be that while the ORM is safe from these issues, the current behavior isn't acceptable for raw SQL users.

The fix for this would be using the adapter interfaces different databases offer. MySQL has a "converters" dictionary, and SQLite has register_adapter. These databases should be somewhat straightforward to support. Oracle seems to have a class based implementation for adapters, but I haven't studied it enough to say anything more about it.

I think the adapter should raise a warning when non-aware datetime is given as a parameter, and convert aware datetimes to UTC. Basically do what value_to_db_datetime does.

I am marking this as a release blocker, as some resolution (even if that would be wontfix) should be done before 1.4 release.

Change History (8)

comment:1 Changed 4 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to aaugustin
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

I'll see what I can do before 1.4 final.

You may have noticed that there's quite a bit legacy code in the adapters / converters definitions. Some functions appear to be workarounds for bugs of very old versions of the database adapters. A cleanup would be useful, but I'd prefer to postpone it to the beginning of the 1.5 release cycle.

comment:2 Changed 4 years ago by akaariai

There seems to be enough work to do before 1.4 release :) Fixing this by a quick note in documentation and doing proper fix later is probably fine as long as the current behavior will not be seen as backwards compatibility issue in 1.5.

If you wish I could take a deeper look on this issue. I already have done some investigation on this issue, so I already have some idea on how to fix this issue. However, I am pretty busy currently, so I can't guarantee any schedule...

comment:3 Changed 4 years ago by aaugustin

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

In [17596]:

Fixed #17755 -- Ensured datetime objects that bypass the model layer (for instance, in raw SQL queries) are converted to UTC before sending them to the database when time zone support is enabled. Thanks Anssi for the report.

comment:4 Changed 4 years ago by anonymous

  • Type changed from Uncategorized to Cleanup/optimization

I am wondering if there is any reason using "from _mysql import string_literal" instead of "from MySQLdb.converters import string_literal".
I have run into problem because C-extension is disallowed when running a Django project.

comment:5 follow-up: Changed 4 years ago by aaugustin

You're right, there's a way to avoid importing _mysql in Django, and I'm going to do that.

However, I don't believe it will make much of a difference, since MySQLdb.converters imports _mysql anyway.

comment:6 Changed 4 years ago by aaugustin

In [17602]:

Avoided importing _mysql directly. Refs #17755.

comment:7 in reply to: ↑ 5 Changed 4 years ago by anonymous

Replying to aaugustin:

You're right, there's a way to avoid importing _mysql in Django, and I'm going to do that.

However, I don't believe it will make much of a difference, since MySQLdb.converters imports _mysql anyway.

In fact, the problem is happened when I am running Google App Engine

A direct import of _mysql was causing C-extension disallowed while importing from MySQLdb has no problem. I guess the package has monkey-patched or white-listed the MySQLdb module.

Anyway, thanks for the quick fix.

comment:8 Changed 7 months ago by Aymeric Augustin <aymeric.augustin@…>

In d9521f6:

Removed global timezone-aware datetime adapters.

Refs #23820.

Fixed #19738.

Refs #17755. In order not to introduce a regression for raw queries,
parameters are passed through the connection.ops.value_to_db_* methods,
depending on their type.

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