Opened 13 years ago

Closed 8 years ago

#16745 closed Bug (wontfix)

Different times on fields with auto_now and auto_now_add

Reported by: kantntreiber@… Owned by: Michael Mior
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords: auto_now, auto_now_add, DateTimeField, DateField, field
Cc: kantntreiber@…, Michael Mior Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

There is a little issue with using "auto_now" and "auto_now_add" together in one model. Because the time is set by calling datetime.now() in the pre_save method of the field, using together more than one such field would lead to different values for every field. This is bad, as one expects that the time is the time of the creation or change of the object and therefore should be identical across all of its fields.

An example:

class FooModel(models.Model):
    time_changed = models.DateTimeField(auto_now=True)
    time_created = models.DateTimeField(auto_now_add=True)


In [3]: from models import FooModel

In [4]: o=FooModel()

In [5]: o.time_changed

In [6]: o.save()

In [7]: o.time_changed
Out[7]: datetime.datetime(2011, 9, 2, 11, 21, 37, 582956)

In [8]: o.time_created
Out[8]: datetime.datetime(2011, 9, 2, 11, 21, 37, 582857)

Attachments (2)

x.patch (2.6 KB ) - added by Michael Mior 12 years ago.
patch
auto-use-transaction-time.diff (4.4 KB ) - added by Michael Mior 12 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 by Anssi Kääriäinen, 13 years ago

Triage Stage: UnreviewedAccepted

Accepted because, yes, it would be nice if the times matched. However I believe this will not get fixed soon, as there isn't a trivial (as in one-liner) fix, and for most developers this doesn't have a high priority. Unless of course somebody interested in fixing this creates a clean patch with tests.

To get me interested in this ticket, an approach which would go along the lines of:

  • Record transaction start time in Django (something like PostgreSQL now())
  • Use that time in auto_now and auto_now_add

In my opinion the approach is a good one because transaction start time has other uses than solving this ticket's problem, and it would be hopefully easy to solve this ticket's problem as a trivial one-liner as a side effect of having the transaction start time available. In addition, the transaction start time would make all objects saved in one transaction have the same time.

Then again, I don't know if core developers would be interested in this approach...

by Michael Mior, 12 years ago

Attachment: x.patch added

patch

comment:2 by Michael Mior, 12 years ago

Cc: Michael Mior added
Has patch: set
Needs documentation: set
Owner: changed from nobody to Michael Mior
Patch needs improvement: set

Apologies for the poorly named initial patch attachment. (Can't seem to delete.) This is my first time attempting a contribution to Django. I attached a patch which is working for me. However, I'm not quite sure how this interacts with the newly-introduced timezone-aware times.

I noticed for DateField and TimeField, the current date/time is derived from datetime (i.e. datetime.date.today() or datetime.datetime.now().time()), but for DateTimeField, the time comes from timezone.now(). Is this a bug also, or is there a reason for this I'm not aware of.

Anyway, the patch likely needs some work since I'm not too familiar with the Django core, but I'm happy to help :)

by Michael Mior, 12 years ago

comment:3 by Aymeric Augustin, 12 years ago

DateFields and TimeFiles are naive – they don't store timezone information – while DateTimeField is aware.

You should convert timezone.now() to the default time zone before extracting the date or time part.

comment:4 by Anssi Kääriäinen, 12 years ago

I will try to get some time reviewing this patch. Recording the transaction start time reliably could be surprisingly hard, as generally Django has pretty bad knowledge of when in transaction and when not. But it seems the proposed patch will work.

I would like it even more if there were some way to get the actual database transaction start time ("SELECT now()" in PostgreSQL). But even as is, I think this patch has merit.

comment:5 by Michael Mior, 12 years ago

@aaugustin Would I be correct in understanding that django.utils.timezone.localtime performs the conversion to the default time zone?

@akaariai Agreed that the transaction start time may be unreliable, but it should be roughly accurate. As far as I can tell, Django has no guarantees on the delta of these times from their actual values anyway. (Although perhaps calling the time transaction_start is a bit of a misnomer.) My concern with using NOW() from the database end is that it requires knowledge of the server's configured time zone, which I don't believe Django currently has. Correct me if I'm wrong.

Last edited 12 years ago by Michael Mior (previous) (diff)

comment:6 by Anssi Kääriäinen, 12 years ago

In the case of PostgreSQL, the server configured time zone doesn't matter, the time zone used in the connection is what matters. And Django knows that.

But I might be pushing this a bit too far, maybe using database.now() should be a separate field option, and done in separate ticket. I am not sure if transaction start time is easily available when using other databases.

It seems I will be really busy at least until weekend. Have to see if I can find time to review this.

comment:7 by Michael Mior, 12 years ago

I suppose it could be helpful for debugging purposes and correlating logs. And maybe especially so in the case where one needs to integrate with legacy systems. The ability to use database time could be a plus. Although it seems like other parts for the core would need to be changed to have a consistent view of time. Actually. perhaps it would be easiest to allow TIME_ZONE to be configured such that it will use the DB server's TZ. In any case, this feels outside the scope of this issue, but I'm open to continuing work on something along these lines if it proves useful.

comment:8 by Tim Graham, 8 years ago

Maybe it's worth closing this ticket now that we can use database functions, e.g. default=NOW. I think auto_now and auto_now_add are somewhat discouraged these days (#22995).

comment:9 by Claude Paroz, 8 years ago

Resolution: wontfix
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top