Code

Opened 3 years ago

Last modified 2 years ago

#16745 new Bug

Different times on fields with auto_now and auto_now_add

Reported by: kantntreiber@… Owned by: michaelmior
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords: auto_now, auto_now_add, DateTimeField, DateField, field
Cc: kantntreiber@…, michaelmior 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 michaelmior 3 years ago.
patch
auto-use-transaction-time.diff (4.4 KB) - added by michaelmior 3 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 3 years ago by akaariai

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

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...

Changed 3 years ago by michaelmior

patch

comment:2 Changed 3 years ago by michaelmior

  • Cc michaelmior added
  • Has patch set
  • Needs documentation set
  • Owner changed from nobody to michaelmior
  • 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 :)

Changed 3 years ago by michaelmior

comment:3 Changed 3 years ago by aaugustin

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 Changed 3 years ago by akaariai

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 Changed 2 years ago by michaelmior

@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 2 years ago by michaelmior (previous) (diff)

comment:6 Changed 2 years ago by akaariai

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 Changed 2 years ago by michaelmior

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from michaelmior to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.