8000 Fixed #23609 -- Fixed IntegrityError that prevented altering a NULL c… · alex-python/django@f633ba7 · GitHub
[go: up one dir, main page]
Skip to content

Commit f633ba7

Browse files
MarkusHloic
MarkusH
authored and
loic
committed
Fixed #23609 -- Fixed IntegrityError that prevented altering a NULL column into a NOT NULL one due to existing rows
Thanks to Simon Charette, Loic Bistuer and Tim Graham for the review.
1 parent 15d350f commit f633ba7

File tree

9 files changed

+256
-36
lines changed

9 files changed

+256
-36
lines changed

‎django/db/backends/schema.py

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ class BaseDatabaseSchemaEditor(object):
4444
sql_alter_column_no_default = "ALTER COLUMN %(column)s DROP DEFAULT"
4545
sql_delete_column = "ALTER TABLE %(table)s DROP COLUMN %(column)s CASCADE"
4646
sql_rename_column = "ALTER TABLE %(table)s RENAME COLUMN %(old_column)s TO %(new_column)s"
47+
sql_update_with_default = "UPDATE %(table)s SET %(column)s = %(default)s WHERE %(column)s IS NULL"
4748

4849
sql_create_check = "ALTER TABLE %(table)s ADD CONSTRAINT %(name)s CHECK (%(check)s)"
4950
sql_delete_check = "ALTER TABLE %(table)s DROP CONSTRAINT %(name)s"
@@ -533,12 +534,19 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type,
533534
})
534535
# Next, start accumulating actions to do
535536
actions = []
537+
null_actions = []
536538
post_actions = []
537539
# Type change?
538540
if old_type != new_type:
539541
fragment, other_actions = self._alter_column_type_sql(model._meta.db_table, new_field.column, new_type)
540542
actions.append(fragment)
541543
post_actions.extend(other_actions)
544+
# When changing a column NULL constraint to NOT NULL with a given
545+
# default value, we need to perform 4 steps:
546+
# 1. Add a default for new incoming writes
547+
# 2. Update existing NULL rows with new default
548+
# 3. Replace NULL constraint with NOT NULL
549+
# 4. Drop the default again.
542550
# Default change?
543551
old_default = self.effective_default(old_field)
544552
new_default = self.effective_default(new_field)
@@ -573,22 +581,31 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type,
573581
# Nullability change?
574582
if old_field.null != new_field.null:
575583
if new_field.null:
576-
actions.append((
584+
null_actions.append((
577585
self.sql_alter_column_null % {
578586
"column": self.quote_name(new_field.column),
579587
"type": new_type,
580588
},
581589
[],
582590
))
583591
else:
584-
actions.append((
592+
null_actions.append((
585593
self.sql_alter_column_not_null % {
586594
"column": self.quote_name(new_field.column),
587595
"type": new_type,
588596
},
589597
[],
590598
))
591-
if actions:
599+
# Only if we have a default and there is a change from NULL to NOT NULL
600+
four_way_default_alteration = (
601+
new_field.has_default() and
602+
(old_field.null and not new_field.null)
603+
)
604+
if actions or null_actions:
605+
if not four_way_default_alteration:
606+
# If we don't have to do a 4-way default alteration we can
607+
# directly run a (NOT) NULL alteration
608+
actions = actions + null_actions
592609
# Combine actions together if we can (e.g. postgres)
593610
if self.connection.features.supports_combined_alters:
594611
sql, params = tuple(zip(*actions))
@@ -602,6 +619,26 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type,
602619
},
603620
params,
604621
)
622+
if four_way_default_alteration:
623+
# Update existing rows with default value
624+
self.execute(
625+
self.sql_update_with_default % {
626+
"table": self.quote_name(model._meta.db_table),
627+
"column": self.quote_name(new_field.column),
628+
"default": "%s",
629+
},
630+
[new_default],
631+
)
632+
# Since we didn't run a NOT NULL change before we need to do it
633+
# now
634+
for sql, params in null_actions:
635+
self.execute(
636+
self.sql_alter_column % {
637+
"table": self.quote_name(model._meta.db_table),
638+
"changes": sql,
639+
},
640+
params,
641+
)
605642
if post_actions:
606643
for sql, params in post_actions:
607644
self.execute(sql, params)

‎django/db/backends/sqlite3/schema.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,14 @@ def _remake_table(self, model, create_fields=[], delete_fields=[], alter_fields=
7878
del body[old_field.name]
7979
del mapping[old_field.column]
8080
body[new_field.name] = new_field
81-
mapping[new_field.column] = self.quote_name(old_field.column)
81+
if old_field.null and not new_field.null:
82+
case_sql = "coalesce(%(col)s, %(default)s)" % {
83+
'col': self.quote_name(old_field.column),
84+
'default': self.quote_value(self.effective_default(new_field))
85+
}
86+
mapping[new_field.column] = case_sql
87+
else:
88+
mapping[new_field.column] = self.quote_name(old_field.column)
8289
rename_mapping[old_field.name] = new_field.name
8390
# Remove any deleted fields
8491
for field in delete_fields:

‎django/db/migrations/autodetector.py

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,8 @@
66
from itertools import chain
77

88
from django.utils import six
9-
from django.db import models
109
from django.conf import settings
10+
from django.db import models
1111
from django.db.migrations import operations
1212
from django.db.migrations.migration import Migration
1313
from django.db.migrations.questioner import MigrationQuestioner
@@ -838,7 +838,6 @@ def generate_altered_fields(self):
838838
for app_label, model_name, field_name in sorted(self.old_field_keys.intersection(self.new_field_keys)):
839839
# Did the field change?
840840
old_model_name = self.renamed_models.get((app_label, model_name), model_name)
841-
new_model_state = self.to_state.models[app_label, model_name]
842841
old_field_name = self.renamed_fields.get((app_label, model_name, field_name), field_name)
843842
old_field = self.old_apps.get_model(app_label, old_model_name)._meta.get_field_by_name(old_field_name)[0]
844843
new_field = self.new_apps.get_model(app_label, model_name)._meta.get_field_by_name(field_name)[0]
@@ -854,12 +853,23 @@ def generate_altered_fields(self):
854853
old_field_dec = self.deep_deconstruct(old_field)
855854
new_field_dec = self.deep_deconstruct(new_field)
856855
if old_field_dec != new_field_dec:
856+
preserve_default = True
857+
if (old_field.null and not new_field.null and not new_field.has_default() and
858+
not isinstance(new_field, models.ManyToManyField)):
859+
field = new_field.clone()
860+
new_default = self.questioner.ask_not_null_alteration(field_name, model_name)
861+
if new_default is not models.NOT_PROVIDED:
862+
field.default = new_default
863+
preserve_default = False
864+
else:
865+
field = new_field
857866
self.add_operation(
858867
app_label,
859868
operations.AlterField(
860869
model_name=model_name,
861870
name=field_name,
862-
field=new_model_state.get_field_by_name(field_name),
871+
field=field,
872+
preserve_default=preserve_default,
863873
)
864874
)
865875

‎django/db/migrations/operations/fields.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,14 +104,20 @@ class AlterField(Operation):
104104
Alters a field's database column (e.g. null, max_length) to the provided new field
105105
"""
106106

107-
def __init__(self, model_name, name, field):
107+
def __init__(self, model_name, name, field, preserve_default=True):
108108
self.model_name = model_name
109109
self.name = name
110110
self.field = field
111+
self.preserve_default = preserve_default
111112

112113
def state_forwards(self, app_label, state):
114+
if not self.preserve_default:
115+
field = self.field.clone()
116+
field.default = NOT_PROVIDED
117+
else:
118+
field = self.field
113119
state.models[app_label, self.model_name.lower()].fields = [
114-
(n, self.field if n == self.name else f) for n, f in state.models[app_label, self.model_name.lower()].fields
120+
(n, field if n == self.name else f) for n, f in state.models[app_label, self.model_name.lower()].fields
115121
]
116122

117123
def database_forwards(self, app_label, schema_editor, from_state, to_state):
@@ -128,7 +134,11 @@ def database_forwards(self, app_label, schema_editor, from_state, to_state):
128134
from_field.rel.to = to_field.rel.to
129135
elif to_field.rel and isinstance(to_field.rel.to, six.string_types):
130136
to_field.rel.to = from_field.rel.to
137+
if not self.preserve_default:
138+
to_field.default = self.field.default
131139
schema_editor.alter_field(from_model, from_field, to_field)
140+
if not self.preserve_default:
141+
to_field.default = NOT_PROVIDED
132142

133143
def database_backwards(self, app_label, schema_editor, from_state, to_state):
134144
self.database_forwards(app_label, schema_editor, from_state, to_state)

‎django/db/migrations/questioner.py

Lines changed: 56 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
1-
from __future__ import unicode_literals
1+
from __future__ import print_function, unicode_literals
22

33
import importlib
44
import os
55
import sys
66

77
from django.apps import apps
8+
from django.db.models.fields import NOT_PROVIDED
89
from django.utils import datetime_safe, six, timezone
910
from django.utils.six.moves import input
1011

@@ -55,6 +56,11 @@ def ask_not_null_addition(self, field_name, model_name):
5556
# None means quit
5657
return None
5758

59+
def ask_not_null_alteration(self, field_name, model_name):
60+
"Changing a NULL field to NOT NULL"
61+
# None means quit
62+
return None
63+
5864
def ask_rename(self, model_name, old_name, new_name, field_instance):
5965
"Was this field really renamed?"
6066
return self.defaults.get("ask_rename", False)
@@ -92,41 +98,67 @@ def _choice_input(self, question, choices):
9298
pass
9399
result = input("Please select a valid option: ")
94100

101+
def _ask_default(self):
102+
print("Please enter the default value now, as valid Python")
103+
print("The datetime and django.utils.timezone modules are available, so you can do e.g. timezone.now()")
104+
while True:
105+
if six.PY3:
106+
# Six does not correctly abstract over the fact that
107+
# py3 input returns a unicode string, while py2 raw_input
108+
# returns a bytestring.
109+
code = input(">>> ")
110+
else:
111+
code = input(">>> ").decode(sys.stdin.encoding)
112+
if not code:
113+
print("Please enter some code, or 'exit' (with no quotes) to exit.")
114+
elif code == "exit":
115+
sys.exit(1)
116+
else:
117+
try:
118+
return eval(code, {}, {"datetime": datetime_safe, "timezone": timezone})
119+
except (SyntaxError, NameError) as e:
120+
print("Invalid input: %s" % e)
121+
95122
def ask_not_null_addition(self, field_name, model_name):
96123
"Adding a NOT NULL field to a model"
97124
if not self.dry_run:
98125
choice = self._choice_input(
99-
"You are trying to add a non-nullable field '%s' to %s without a default;\n" % (field_name, model_name) +
100-
"we can't do that (the database needs something to populate existing rows).\n" +
101-
"Please select a fix:",
126+
"You are trying to add a non-nullable field '%s' to %s without a default; "
127+
"we can't do that (the database needs something to populate existing rows).\n"
128+
"Please select a fix:" % (field_name, model_name),
129+
[
130+
"Provide a one-off default now (will be set on all existing rows)",
131+
"Quit, and let me add a default in models.py",
132+
]
133+
)
134+
if choice == 2:
135+
sys.exit(3)
136+
else:
137+
return self._ask_default()
138+
return None
139+
140+
def ask_not_null_alteration(self, field_name, model_name):
141+
"Changing a NULL field to NOT NULL"
142+
if not self.dry_run:
143+
choice = self._choice_input(
144+
"You are trying to change the nullable field '%s' on %s to non-nullable "
145+
"without a default; we can't do that (the database needs something to "
146+
"populate existing rows).\n"
147+
"Please select a fix:" % (field_name, model_name),
102148
[
103149
"Provide a one-off default now (will be set on all existing rows)",
150+
("Ignore for now, and let me handle existing rows with NULL myself "
151+
"(e.g. adding a RunPython or RunSQL operation in the new migration "
152+
"file before the AlterField operation)"),
104153
"Quit, and let me add a default in models.py",
105154
]
106155
)
107156
if choice == 2:
157+
return NOT_PROVIDED
158+
elif choice == 3:
108159
sys.exit(3)
109160
else:
110-
print("Please enter the default value now, as valid Python")
111-
print("The datetime and django.utils.timezone modules are "
112-
"available, so you can do e.g. timezone.now()")
113-
while True:
114-
if six.PY3:
115-
# Six does not correctly abstract over the fact that
116-
# py3 input returns a unicode string, while py2 raw_input
117-
# returns a bytestring.
118-
code = input(">>> ")
119-
else:
120-
code = input(">>> ").decode(sys.stdin.encoding)
121-
if not code:
122-
print("Please enter some code, or 'exit' (with no quotes) to exit.")
123-
elif code == "exit":
124-
sys.exit(1)
125-
else:
126-
try:
127-
return eval(code, {}, {"datetime": datetime_safe, "timezone": timezone})
128-
except (SyntaxError, NameError) as e:
129-
print("Invalid input: %s" % e)
161+
return self._ask_default()
130162
return None
131163

132164
def ask_rename(self, model_name, old_name, new_name, field_instance):

‎docs/ref/migration-operations.txt

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ or if it is temporary and just for this migration (``False``) - usually
137137
because the migration is adding a non-nullable field to a table and needs
138138
a default value to put into existing rows. It does not effect the behavior
139139
of setting defaults in the database directly - Django never sets database
140-
defaults, and always applies them in the Django ORM code.
140+
defaults and always applies them in the Django ORM code.
141141

142142
RemoveField
143143
-----------
@@ -153,16 +153,28 @@ from any data loss, which of course is irreversible).
153153
AlterField
154154
----------
155155

156-
.. class:: AlterField(model_name, name, field)
156+
.. class:: AlterField(model_name, name, field, preserve_default=True)
157157

158158
Alters a field's definition, including changes to its type,
159159
:attr:`~django.db.models.Field.null`, :attr:`~django.db.models.Field.unique`,
160160
:attr:`~django.db.models.Field.db_column` and other field attributes.
161161

162+
The ``preserve_default`` argument indicates whether the field's default
163+
value is permanent and should be baked into the project state (``True``),
164+
or if it is temporary and just for this migration (``False``) - usually
165+
because the migration is altering a nullable field to a non-nullable one and
166+
needs a default value to put into existing rows. It does not effect the
167+
behavior of setting defaults in the database directly - Django never sets
168+
database defaults and always applies them in the Django ORM code.
169+
162170
Note that not all changes are possible on all databases - for example, you
163171
cannot change a text-type field like ``models.TextField()`` into a number-type
164172
field like ``models.IntegerField()`` on most databases.
165173

174+
.. versionchanged:: 1.7.1
175+
176+
The ``preserve_default`` argument was added.
177+
166178
RenameField
167179
-----------
168180

‎docs/releases/1.7.1.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,3 +106,7 @@ Bugfixes
106106

107107
* Made :func:`~django.utils.http.urlsafe_base64_decode` return the proper
108108
type (byte string) on Python 3 (:ticket:`23333`).
109+
110+
* Added a prompt to the migrations questioner when removing the null constraint
111+
from a field to prevent an IntegrityError on existing NULL rows
112+
(:ticket:`23609`).

0 commit comments

Comments
 (0)
0