Opened 8 years ago
Last modified 20 months ago
#28715 new Cleanup/optimization
Prevent a migration changing DateTimeField(auto_now_add=True) to default=timezone.now from generating SQL
| Reported by: | Дилян Палаузов | Owned by: | nobody |
|---|---|---|---|
| Component: | Migrations | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Ülgen Sarıkavak | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | yes |
| Easy pickings: | no | UI/UX: | no |
Description (last modified by )
A switch from DateTimeField(auto_now_add=True) to DateTimeField(default=django.utils.timezone.new) creates the statements
ALTER TABLE SET DEFAULT '2017-10-16T09:35:52.710695'::timestamp;
ALTER TABLE DROP DEFAULT
which have no effects, apart from locking the whole table twice.
A proposal to recognize, when the effective default-callable doesn't change and skip changing the DEFAULT twice in this case, as well as not generating a migration when this is the only change on a field:
diff --git a/django/db/backends/base/schema.py b/django/db/backends/base/schema.py
--- a/django/db/backends/base/schema.py
+++ b/django/db/backends/base/schema.py
@@ -199,28 +199,33 @@ class BaseDatabaseSchemaEditor(object):
'requires_literal_defaults must provide a prepare_default() method'
)
- def effective_default(self, field):
+ @staticmethod
+ def effective_default_before_callable(field):
"""
- Returns a field's effective database default value
+ Returns a field's effective database default callable or value
"""
if field.has_default():
- default = field.get_default()
+ return field._get_default
elif not field.null and field.blank and field.empty_strings_allowed:
if field.get_internal_type() == "BinaryField":
- default = six.binary_type()
+ return six.binary_type()
else:
- default = six.text_type()
+ return six.text_type()
elif getattr(field, 'auto_now', False) or getattr(field, 'auto_now_add', False):
default = datetime.now()
internal_type = field.get_internal_type()
if internal_type == 'DateField':
- default = default.date
+ return default.date
elif internal_type == 'TimeField':
- default = default.time
+ return default.time
elif internal_type == 'DateTimeField':
- default = timezone.now
- else:
- default = None
+ return timezone.now
+
+ def effective_default(self, field):
+ """
+ Returns a field's effective database default value
+ """
+ default = BaseDatabaseSchemaEditor.effective_default_before_callable(field)
# If it's a callable, call it
if callable(default):
default = default()
@@ -615,6 +620,7 @@ class BaseDatabaseSchemaEditor(object):
old_default != new_default and
new_default is not None and
not self.skip_default(new_field)
+ and BaseDatabaseSchemaEditor.effective_default_before_callable(old_field) != BaseDatabaseSchemaEdit
)
if needs_database_default:
if self.connection.features.requires_literal_defaults:
diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py
index 8d40c77..2f5d5c2 100644
--- a/django/db/models/fields/__init__.py
+++ b/django/db/models/fields/__init__.py
@@ -1232,7 +1232,7 @@ class DateField(DateTimeCheckMixin, Field):
if self.auto_now:
kwargs['auto_now'] = True
if self.auto_now_add:
- kwargs['auto_now_add'] = True
+ kwargs['default'] = datetime.date.today
if self.auto_now or self.auto_now_add:
del kwargs['editable']
del kwargs['blank']
@@ -1372,6 +1372,12 @@ class DateTimeField(DateField):
return []
+ def deconstruct(self):
+ name, path, args, kwargs = super(DateTimeField, self).deconstruct()
+ if self.auto_now_add:
+ kwargs['default'] = timezone.now
+ return name, path, args, kwargs
+
def get_internal_type(self):
return "DateTimeField"
Attachments (2)
Change History (11)
comment:1 by , 8 years ago
| Summary: | Migration DateTimeField(auto_now_add=True -> default=django.utils.timezone.new) → Prevent a migration changing DateTimeField(auto_now_add=True) to default=timezone.now from generating SQL |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
| Type: | Uncategorized → Cleanup/optimization |
comment:2 by , 8 years ago
| Description: | modified (diff) |
|---|
comment:3 by , 8 years ago
| Description: | modified (diff) |
|---|
comment:4 by , 8 years ago
comment:5 by , 8 years ago
The patch must apply cleanly to Django's master branch and it must have a test. Ideally, you could provide the patch as a pull request and then check "Has patch" on the ticket so that it appears in the review queue.
comment:6 by , 8 years ago
| Has patch: | set |
|---|---|
| Version: | 1.11 → master |
by , 8 years ago
| Attachment: | auto_now_add_skip_sql_calls.patch added |
|---|
Probably somebody with more experience will tweak the test.migrations.test_autodetector.test_add_date_fields_with_auto_now_add_(not_asking_for_null_addition, add_asking_for_default) tests, so that the line "kwargsauto_now_add = True" in django/db/models/fields/init.py can be removed. But otherwise this shall be ready to go.
comment:7 by , 8 years ago
| Patch needs improvement: | set |
|---|
Sorry, I should have looked at the initial patch before I suggested bringing it up to date. I don't think the approach is correct. I think that a migration should be generated if auto_now_add=True changes to default=timezone.now as they aren't entirely equivalent. The fix will have to be at the SchemaEditor level. Given that auto_now_add and auto_now might be deprecated (#22995), it might be a better use of time to focus on that issue (although the patch forward isn't entirely clear).
by , 8 years ago
| Attachment: | auto_now_add_skip_sql_calls.2.patch added |
|---|
comment:8 by , 8 years ago
The purpose of the change is to tweak the migrations auto_now_add -> default for DateField and DateTimeField not to call
ALTER TABLE SET DEFAULT '2017-10-16T09:35:52.710695'::timestamp;
ALTER TABLE DROP DEFAULT
towards the database. The attached auto_now_add_skip_sql_calls.2.patch touches BaseDatabaseSchemaEditor, and does generate a migration, which however skips modifying the DEFAULT.
auto_now_add being possibly deprecated means that this type of migration will be created more often in the future and this patch accelerates the execution of the migration (as it does not lock the table).
comment:9 by , 20 months ago
| Cc: | added |
|---|
My plan is to work one more month with Django, so if there are concerns on this patch, please raise them by then.