[Django] #17755: Use database adapters for converting time zone aware datetimes in raw queries
Django
queries
----------------------------------------------+------------------------
Reporter: akaariai | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer (models, ORM) | Version: 1.4-beta-1
Severity: Release blocker | Keywords: akaariai
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17755>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
queries
Reporter: akaariai | Owner: aaugustin
Type: Uncategorized | Status: new
Component: Database layer | Version:
(models, ORM) | 1.4-beta-1
Severity: Release blocker | Resolution:
Keywords: akaariai | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by aaugustin):
* owner: nobody => aaugustin
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17755#comment:1>
Django
queries
Reporter: akaariai | Owner: aaugustin
(models, ORM) | 1.4-beta-1
Severity: Release blocker | Resolution:
Keywords: akaariai | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Comment (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...
--
Ticket URL: <https://code.djangoproject.com/ticket/17755#comment:2>
Django
queries
Reporter: akaariai | Owner: aaugustin
(models, ORM) | 1.4-beta-1
Has patch: 0 | Needs documentation: 0
Changes (by aaugustin):
* status: new => closed
* resolution: => fixed
Comment:
In [17596]:
{{{
#!CommitTicketReference repository="" revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17755#comment:3>
Django
queries
Reporter: akaariai | Owner: aaugustin
Cleanup/optimization | Version:
Component: Database layer | 1.4-beta-1
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Accepted
Keywords: akaariai | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by anonymous):
* type: Uncategorized => Cleanup/optimization
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17755#comment:4>
Django
queries
Reporter: akaariai | Owner: aaugustin
Type: | Status: closed
Cleanup/optimization | Version:
Component: Database layer | 1.4-beta-1
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Accepted
Keywords: akaariai | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17755#comment:5>
Django
queries
Reporter: akaariai | Owner: aaugustin
Type: | Status: closed
Cleanup/optimization | Version:
Component: Database layer | 1.4-beta-1
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Accepted
Keywords: akaariai | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
In [17602]:
{{{
#!CommitTicketReference repository="" revision="17602"
Avoided importing _mysql directly. Refs #17755.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17755#comment:6>
Django
queries
Reporter: akaariai | Owner: aaugustin
Type: | Status: closed
Cleanup/optimization | Version:
Component: Database layer | 1.4-beta-1
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Accepted
Keywords: akaariai | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
Replying to [comment:5 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
dev_appserver.py.
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17755#comment:7>
Django
queries
Reporter: akaariai | Owner: aaugustin
Type: | Status: closed
Cleanup/optimization |
(models, ORM) | 1.4-beta-1
Severity: Release blocker | Resolution: fixed
Keywords: akaariai | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"d9521f66b1851b0eacd55bc78f801dc64123e333" d9521f6]:
{{{
#!CommitTicketReference repository=""
revision="d9521f66b1851b0eacd55bc78f801dc64123e333"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17755#comment:8>