Opened 10 years ago
Last modified 8 years ago
#22995 new Cleanup/optimization
Deprecate auto_now and auto_now_add and document alternatives
Reported by: | Jannis Leidel | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | cmawebsite@…, someuniquename@… | Triage Stage: | Someday/Maybe |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
We've previously documented some side issues of the two options (e.g. #6434) but need to better clarify the downsides of those magic parameters. The ability to override the save
method as well as the default
parameter has long made this implicit feature capable of being replaced.
This would probably also be a way to not let users fall in the trap as documented in #16745 and #22571.
In a previous *old* discussion there were multiple core developers in favor of the deprecation: https://groups.google.com/forum/#!topic/django-developers/TNYxwiXLTlI
Change History (17)
comment:1 by , 10 years ago
Description: | modified (diff) |
---|
comment:2 by , 10 years ago
comment:3 by , 10 years ago
I'm not really enthused about removing them, I agree with what Russell said here: https://code.djangoproject.com/ticket/12785#comment:1
In my experience they are (still) very commonly used. That original pre-1.0 discussion brings up that having to manually override save for every model where you have an auto-something field is going to be a pain in the neck, there was agreement on that, no specific alternative proposal got implemented and I don't see one here so we'd deprecate these w/ no replacement? That seems rude.
comment:4 by , 10 years ago
I've seen several people go through the pain of replacing auto_now(_add) with the more explicit alternatives (default=timezone.now / override save()) after hitting bugs in production.
In fact, they weren't really bugs, but cases where the actual and documented behaviour diverged from what the developer expected — which has always been the rationale for removing them.
I would be fine with deprecating them (i.e. raise a DeprecationWarning) but never actually remove them (i.e. we wouldn't advance the deprecation at each release like we usually do). That would discourage their use without giving the finger to people who find them good enough.
comment:5 by , 10 years ago
I put together a proof of concept for removing usage of auto_add
from the admin. I agree removing them would be disruptive. We have done a similar thing to what Aymeric suggested (deprecating, but not removing) with @permalink.
comment:6 by , 10 years ago
Doesn't the POC replace an auto_now flag w/ a save override that is implementing auto_add_now? Which possibly does not matter since admin log entries are generally not editable...?
What are the unexpected behaviors that can be fixed by replacing with default/override save()? Or is it just more clear what the behavior will be?
comment:7 by , 10 years ago
Yes, I'd consider the fact that it currently uses auto_now
and not auto_add_now
to be a bug. It probably doesn't matter as you said, but if for some reason admin log entries are edited in a shell or whatever, I wouldn't expect the action_time
to be updated.
The tickets linked in the description show some "gotchas" when these flags, you can also google "django auto_now evil" to find complaints. #22981 is also a related issue which stems from the fact that the expected behavior is not clearly defined (or at least it's poorly documented).
comment:8 by , 10 years ago
Right, I do understand some of the gothcas, what I'm missing is if/how they are alleviated by users implementing their own? All of them I have seen it seems are still going to be a problem with self-implementation...? Hence my question on how this improves things, is is just (hopefully) clearer to the person doing it what the side-effects will be vs. a magical flag that does less than they think it will?
follow-ups: 10 11 comment:9 by , 10 years ago
If auto_now
and auto_now_add
have bugs, then we should address them; if there is potential for confusion around when they will (or won't) apply changes, we should document that too - just as we do when we note that update
doesn't trigger pre/post save signals.
I simply don't understand the motivation of removing features that implement an obvious and well defined use case - creation timestamp and modification timestamp - without providing an equally functional alternative. In order to use default= as a replacement for auto_now_add, you need to:
- Remember to use
timezone.now
, notdatetime.now
- Remember to use
timezone.now
, nottimezone.now()
- Remember to
include django.utils import timezone
That's three easily avoidable opportunities for someone to mess up the simple use case of "creation timestamp". And it gets worse if you want auto_now, because you have to write and test a bunch of boilerplate code at the model level.
If there really is a visceral hatred of auto_now
and auto_now_add
as names, then at the very least we need to introduce a CreationDateTimeField
, ModificationDateTimeField
, CreationDateField
, and ModificationDateField
... but are those *really* that much better names than auto_now
and auto_now_add
?
comment:10 by , 10 years ago
+1 on marking it with DeprecationWarning but letting it sit there to ease the upgrading pain.
Replying to russellm:
If
auto_now
andauto_now_add
have bugs, then we should address them; if there is potential for confusion around when they will (or won't) apply changes, we should document that too - just as we do when we note thatupdate
doesn't trigger pre/post save signals.
I simply don't understand the motivation of removing features that implement an obvious and well defined use case - creation timestamp and modification timestamp - without providing an equally functional alternative. In order to use default= as a replacement for auto_now_add, you need to:
- Remember to use
timezone.now
, notdatetime.now
- Remember to use
timezone.now
, nottimezone.now()
- Remember to
include django.utils import timezone
That's three easily avoidable opportunities for someone to mess up the simple use case of "creation timestamp". And it gets worse if you want auto_now, because you have to write and test a bunch of boilerplate code at the model level.
And yet, I believe knowing those steps are essential for any non-trivial Django project. Anecdotally I've often at some point converted the auto_now to a default call as soon as a model got a little more complex to use a consistent API for default values. I also consider overriding save methods not boilerplate but a core API of model classes -- again an often used one.
In that sense I believe the auto_now and auto_now_add parameters has not only actual technical issues, but also prevents our users to learn important APIs.
If there really is a visceral hatred of
auto_now
andauto_now_add
as names, then at the very least we need to introduce aCreationDateTimeField
,ModificationDateTimeField
,CreationDateField
, andModificationDateField
... but are those *really* that much better names thanauto_now
andauto_now_add
?
Agreed, I think that would be a good compromise for users that don't want to manually handle auto_now and auto_now_add via default/save. There are CreationDateTimeField and ModificationDateTimeField fields in django-extensions in case we want to rely on a solution that's been in wide use for years.
comment:11 by , 10 years ago
Replying to russellm:
I simply don't understand the motivation of removing features that implement an obvious and well defined use case - creation timestamp and modification timestamp - without providing an equally functional alternative.
Precisely, I think we should at least deprecate them because the use case is very obvious but auto_now(_add) don't implement it — and maybe can't!
Creation and modification timestamps are expected to be set automatically, regardless of how you access the row. Rails does this well at the database level.
comment:12 by , 10 years ago
I'm not much of a fan of deprecating without removing either as I'm not a big fan of nanny warnings. If I know what I'm doing, have read the doc and understand the limitations of what I'm using, I don't really want the code nagging me that what I'm doing is "unclean". Bleh. Near as I can tell the straightforward do-it-yourself implementations are going to be equally vulnerable to gotchas...how is forcing people to try doing it themselves any better?
For the alternatives proposed: are they going to be immune from the gotcahs of the existing implementation? If not then why bother?
comment:13 by , 10 years ago
Triage Stage: | Unreviewed → Someday/Maybe |
---|---|
Version: | 1.6 → master |
Moving to someday/maybe as this needs more investigation to answer the above questions before being accepted or rejected.
comment:14 by , 10 years ago
We wouldn't ever consider adding a models.now()
shortcut, would we? It's a little magical, but a very common use case. The import is what bugs me the most. You need to import something from "utils" in many of your models files. It seems similar to models.SET_NULL
.
Or, have a magical models.DateField(default='now')
(solves the now
vs now()
) or the longer: models.DateField(default=models.DateField.now)
comment:15 by , 10 years ago
Cc: | added |
---|
comment:17 by , 8 years ago
This issue was recently brought up again on django-users and I'd like to propose a different path.
There are three issues to consider:
- The initial design thought was that these fields are immutable, from the application perspective and should be used for auditing. In real life, auto_now_add is used as a default and expectation is that it should be mutable. For auto_now this distinction only a systematic concern, but should not impact real world scenario's beyond the microsecond scope.
- The implementation has bugs that in certain scenario's they do not do their job. While documented, this is still a bug.
- Setting
editable
to False does not prevent setting values for the field. It only does so for model forms.
FIxing auto_now_add
To make transition easier, we could implement a keyword auto_now
that can be used as argument to default
. What this does is up for debate. IMO every supported database can use some form of DEFAULT CURRENT_TIMESTAMP
, so if we move implementation to the database layer, this can be wrapped as a default argument at table creation. This would also fix any issues with it not being set under certain circumstances.
It could also be a callback, that does the same work as auto_now_add does now, including the timezone imports etc. It would also fix the implementation bug and it would be consistent with any timezone configuration within Django, while setting a default at the database layer would set the default to whatever the database layer thinks is correct. Probably the best fix overall would be to enforce UTC at the database layer and do all interpretation in Django, but that's beyond the scope of this ticket.
The immutable fields
Instead of naming them ambiguous again, I think the replacements for this functionality should be prefixed with Auto
, so for example AutoLastModifiedField
. This should be ample hint to the implementer that all the work is done for her and she can't touch it. It also allows for a LastModifiedField that can be overridden by forms. A use case would be formatting changes to a wiki document - it would not constitute a document change, so one could preserve the original value.
Fixing the implementation
I'm all for moving all implementations (so the auto fields and auto_now default) to the database layer, but that requires some thought and perspective. Why was it originally chosen to do this in python?
Not editable but changeable
So even when the auto_now keywords are marked not editable, they are still not conforming to the original design thought for auditing purposes. A value can still be set and saved. Is this intentional and does the original reasoning still apply?
AFAIK most core devs still agree that they should be removed. But it was never a priority because we simply avoid them in our projets ;-)