Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#8354 closed (fixed)

MySQL rejects datetimes with unusual unicode() representations

Reported by: jmillikin Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords: 1.0-blocker
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description

The database adapters assume that unicode(datetime.datetime) will return a string that can be passed to the database. Unfortunately, this assumption is false. Datetime objects with timezones will be formatted as a string that can't be read by MySQL:

>>> test_date
datetime.datetime(2008, 8, 15, 21, 31, 8, tzinfo=<UTC>)
>>> print unicode(test_date)
2008-08-15 21:31:08+00:00

I've patched the MySQL database adapter to use datetime.strftime() instead of unicode (patch attached). This sort of patch should probably be adapted to other database adapters, so they have more consistent datetime formatting.

Attachments (2)

mysql-datetime.diff (651 bytes) - added by jmillikin 6 years ago.
failing-test.patch (623 bytes) - added by jacob 6 years ago.

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by jmillikin

comment:1 Changed 6 years ago by mtredinnick

  • milestone set to 1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Should certainly be tested on other database backends to work out what the right backend-specific solution is in each case. However, throwing away timezone information when provided is a not nice as a general rule. For MySQL we have to do it, since they don't have a proper datetime field. For PostgreSQL (and others) it would be interesting to check if passing in a timezone-enabled datetime is accepted by the database even for non-timezone-aware datetime fields. That way when we do add proper timezone handling in that area down the track, we won't need to track down lots of problems with throwing away timezones.

I'm wondering if we should convert timezone-enabled datetimes to the equivalent time in the server's specified timezone, however. The "right" solution here needs a little thought, but the current behaviour is probably sub-optimal (crashing usually is).

comment:2 Changed 6 years ago by julianb

I think for MySQL it needs to be converted. MySQL uses the server time zone by default, but it can be changed per-connection. However, the current time zone setting does not affect values in DATE, TIME, or DATETIME columns, but we could assume that these are in the time zone set in Django settings. So we would need to convert from the specified zone to TIME_ZONE from settings, if I'm not mistaken.

comment:3 Changed 6 years ago by jacob

  • Keywords 1.0-blocker added
  • Needs tests set

This looks fine, but we need tests for this.

Changed 6 years ago by jacob

comment:4 Changed 6 years ago by jacob

So I missed Malcolm's comment before -- this is indeed a bit more complicated. It's not clear if silently discarding the time zone is acceptible, or weather we should convert it. I know that revisiting datetime handling is something we need to do in the future (a few threads on django-dev have discussed this), but we need to fix this particular issue for 1.0.

I've added a failing test; I can verify that it only fails on MySQL.

I see three choices:

  • Silently discard the timezone as in the patch. Not great, but easy.
  • Silently convert TZ-aware tickets to settings.TIME_ZONE. This opens up a big can of worms.
  • Raise a ValueError there and just refuse to handle tz-aware datetimes.

comment:5 Changed 6 years ago by julianb

Maybe raising an error would be the best thing to do for 1.0. If you work with tz-aware datetimes you could then decide for yourself what to do.

comment:6 Changed 6 years ago by mtredinnick

Following discussion on IRC and in light of the limited time and the need for a more comprehensive approach post-1.0 here, we're going to make it an error to pass in tz-aware datetimes here so that the caller has to explicitly handle it themselves without us either inserting the wrong values or changing their data (which can also be wrong sometimes).

comment:7 Changed 6 years ago by jacob

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

Fixed in [8802].

comment:8 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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.