[1.7.x] Fixed #24595 -- Prevented loss of null info in MySQL field al…

…teration

Thanks Simon Percivall for the report, and Simon Charette and Tim
Graham for the reviews.
Backport of 02260ea from master.

django/db/backends/mysql/schema.py

@@ -48,3 +48,11 @@ def add_field(self, model, field):
                 'table': self.quote_name(model._meta.db_table),
                 'column': self.quote_name(field.column),
             }, [effective_default])
+
+    def _alter_column_type_sql(self, table, old_field, new_field, new_type):
+        # Keep null property of old field, if it has changed, it will be handled separately
+        if old_field.null:
+            new_type += " NULL"
+        else:
+            new_type += " NOT NULL"
+        return super(DatabaseSchemaEditor, self)._alter_column_type_sql(table, old_field, new_field, new_type)

django/db/backends/postgresql_psycopg2/schema.py

@@ -34,11 +34,12 @@ def _model_indexes_sql(self, model):
                         model, [field], suffix='_like', sql=self.sql_create_text_index))
         return output
 
-    def _alter_column_type_sql(self, table, column, type):
+    def _alter_column_type_sql(self, table, old_field, new_field, new_type):
         """
         Makes ALTER TYPE with SERIAL make sense.
         """
-        if type.lower() == "serial":
+        if new_type.lower() == "serial":
+            column = new_field.column
             sequence_name = "%s_%s_seq" % (table, column)
             return (
                 (
@@ -82,4 +83,6 @@ def _alter_column_type_sql(self, table, column, type):
                 ],
             )
         else:
-            return super(DatabaseSchemaEditor, self)._alter_column_type_sql(table, column, type)
+            return super(DatabaseSchemaEditor, self)._alter_column_type_sql(
+                table, old_field, new_field, new_type
+            )

django/db/backends/schema.py

@@ -13,6 +13,14 @@
 logger = getLogger('django.db.backends.schema')
 
 
+def _related_objects(old_field, new_field):
+    # Returns (old_relation, new_relation) tuples.
+    return zip(
+        old_field.model._meta.get_all_related_objects(),
+        new_field.model._meta.get_all_related_objects()
+    )
+
+
 class BaseDatabaseSchemaEditor(object):
     """
     This class (and its subclasses) are responsible for emitting schema-changing
@@ -438,12 +446,17 @@ def alter_field(self, model, old_field, new_field, strict=False):
         old_type = old_db_params['type']
         new_db_params = new_field.db_parameters(connection=self.connection)
         new_type = new_db_params['type']
-        if (old_type is None and old_field.rel is None) or (new_type is None and new_field.rel is None):
-            raise ValueError("Cannot alter field %s into %s - they do not properly define db_type (are you using PostGIS 1.5 or badly-written custom fields?)" % (
-                old_field,
-                new_field,
-            ))
-        elif old_type is None and new_type is None and (old_field.rel.through and new_field.rel.through and old_field.rel.through._meta.auto_created and new_field.rel.through._meta.auto_created):
+        if ((old_type is None and old_field.rel is None) or
+                (new_type is None and new_field.rel is None)):
+            raise ValueError(
+                "Cannot alter field %s into %s - they do not properly define "
+                "db_type (are you using PostGIS 1.5 or badly-written custom "
+                "fields?)" % (old_field, new_field)
+            )
+        elif old_type is None and new_type is None and (
+                old_field.rel.through and new_field.rel.through and
+                old_field.rel.through._meta.auto_created and
+                new_field.rel.through._meta.auto_created):
             return self._alter_many_to_many(model, old_field, new_field, strict)
         elif old_type is None and new_type is None and (old_field.rel.through and new_field.rel.through and not old_field.rel.through._meta.auto_created and not new_field.rel.through._meta.auto_created):
             # Both sides have through models; this is a no-op.
@@ -487,10 +500,12 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, old_db_p
         # Drop incoming FK constraints if we're a primary key and things are going
         # to change.
         if old_field.primary_key and new_field.primary_key and old_type != new_type:
-            for rel in new_field.model._meta.get_all_related_objects():
-                rel_fk_names = self._constraint_names(rel.model, [rel.field.column], foreign_key=True)
+            for _old_rel, new_rel in _related_objects(old_field, new_field):
+                rel_fk_names = self._constraint_names(
+                    new_rel.model, [new_rel.field.column], foreign_key=True
+                )
                 for fk_name in rel_fk_names:
-                    self.execute(self._delete_constraint_sql(self.sql_delete_fk, rel.model, fk_name))
+                    self.execute(self._delete_constraint_sql(self.sql_delete_fk, new_rel.model, fk_name))
         # Removed an index? (no strict check, as multiple indexes are possible)
         if (old_field.db_index and not new_field.db_index and
                 not old_field.unique and not
@@ -524,7 +539,9 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, old_db_p
         post_actions = []
         # Type change?
         if old_type != new_type:
-            fragment, other_actions = self._alter_column_type_sql(model._meta.db_table, new_field.column, new_type)
+            fragment, other_actions = self._alter_column_type_sql(
+                model._meta.db_table, old_field, new_field, new_type
+            )
             actions.append(fragment)
             post_actions.extend(other_actions)
         # When changing a column NULL constraint to NOT NULL with a given
@@ -637,7 +654,7 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, old_db_p
         # referring to us.
         rels_to_update = []
         if old_field.primary_key and new_field.primary_key and old_type != new_type:
-            rels_to_update.extend(new_field.model._meta.get_all_related_objects())
+            rels_to_update.extend(_related_objects(old_field, new_field))
         # Changed to become primary key?
         # Note that we don't detect unsetting of a PK, as we assume another field
         # will always come along and replace it.
@@ -660,20 +677,23 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, old_db_p
                 }
             )
             # Update all referencing columns
-            rels_to_update.extend(new_field.model._meta.get_all_related_objects())
+            rels_to_update.extend(_related_objects(old_field, new_field))
         # Handle our type alters on the other end of rels from the PK stuff above
-        for rel in rels_to_update:
-            rel_db_params = rel.field.db_parameters(connection=self.connection)
+        for old_rel, new_rel in rels_to_update:
+            rel_db_params = new_rel.field.db_parameters(connection=self.connection)
             rel_type = rel_db_params['type']
+            fragment, other_actions = self._alter_column_type_sql(
+                new_rel.model._meta.db_table, old_rel.field, new_rel.field, rel_type
+            )
             self.execute(
                 self.sql_alter_column % {
-                    "table": self.quote_name(rel.model._meta.db_table),
-                    "changes": self.sql_alter_column_type % {
-                        "column": self.quote_name(rel.field.column),
-                        "type": rel_type,
-                    }
-                }
+                    "table": self.quote_name(new_rel.model._meta.db_table),
+                    "changes": fragment[0],
+                },
+                fragment[1],
             )
+            for sql, params in other_actions:
+                self.execute(sql, params)
         # Does it have a foreign key?
         if (new_field.rel and
                 (fks_dropped or not old_field.rel or not old_field.db_constraint) and
@@ -707,7 +727,7 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, old_db_p
         if self.connection.features.connection_persists_old_columns:
             self.connection.close()
 
-    def _alter_column_type_sql(self, table, column, type):
+    def _alter_column_type_sql(self, table, old_field, new_field, new_type):
         """
         Hook to specialize column type alteration for different backends,
         for cases when a creation type is different to an alteration type
@@ -720,8 +740,8 @@ def _alter_column_type_sql(self, table, column, type):
         return (
             (
                 self.sql_alter_column_type % {
-                    "column": self.quote_name(column),
-                    "type": type,
+                    "column": self.quote_name(new_field.column),
+                    "type": new_type,
                 },
                 [],
             ),

docs/releases/1.7.8.txt

@@ -10,3 +10,6 @@ Django 1.7.8 fixes:
   (:ticket:`24637`).
 
 * A database table name quoting regression in 1.7.2 (:ticket:`24605`).
+
+* Prevented the loss of ``null``/``not null`` column properties during field
+  alteration of MySQL databases (:ticket:`24595`).

tests/migrations/test_operations.py

@@ -1032,9 +1032,18 @@ def test_alter_field_pk_fk(self):
 
         def assertIdTypeEqualsFkType():
             with connection.cursor() as cursor:
-                id_type = [c.type_code for c in connection.introspection.get_table_description(cursor, "test_alflpkfk_pony") if c.name == "id"][0]
-                fk_type = [c.type_code for c in connection.introspection.get_table_description(cursor, "test_alflpkfk_rider") if c.name == "pony_id"][0]
+                id_type, id_null = [
+                    (c.type_code, c.null_ok)
+                    for c in connection.introspection.get_table_description(cursor, "test_alflpkfk_pony")
+                    if c.name == "id"
+                ][0]
+                fk_type, fk_null = [
+                    (c.type_code, c.null_ok)
+                    for c in connection.introspection.get_table_description(cursor, "test_alflpkfk_rider")
+                    if c.name == "pony_id"
+                ][0]
             self.assertEqual(id_type, fk_type)
+            self.assertEqual(id_null, fk_null)
 
         assertIdTypeEqualsFkType()
         # Test the database alteration

tests/schema/tests.py

@@ -29,7 +29,7 @@ class SchemaTests(TransactionTestCase):
         Author, AuthorWithM2M, Book, BookWithLongName, BookWithSlug,
         BookWithM2M, Tag, TagIndexed, TagM2MTest, TagUniqueRename, UniqueTest,
         Thing, TagThrough, BookWithM2MThrough, AuthorWithEvenLongerName,
-        BookWeak, BookWithO2O, BookWithoutFK,
+        BookWeak, BookWithO2O, BookWithoutFK, Note,
     ]
 
     # Utility functions
@@ -427,6 +427,8 @@ def test_alter(self):
     def test_alter_text_field(self):
         # Regression for "BLOB/TEXT column 'info' can't have a default value")
         # on MySQL.
+        with connection.schema_editor() as editor:
+            editor.create_model(Note)
         new_field = TextField(blank=True)
         new_field.set_attributes_from_name("info")
         with connection.schema_editor() as editor:
@@ -437,6 +439,22 @@ def test_alter_text_field(self):
                 strict=True,
             )
 
+    def test_alter_field_keep_null_status(self):
+        """
+        Changing a field type shouldn't affect the not null status.
+        """
+        with connection.schema_editor() as editor:
+            editor.create_model(Note)
+        with self.assertRaises(IntegrityError):
+            Note.objects.create(info=None)
+        old_field = Note._meta.get_field("info")
+        new_field = CharField(max_length=50)
+        new_field.set_attributes_from_name("info")
+        with connection.schema_editor() as editor:
+            editor.alter_field(Note, old_field, new_field, strict=True)
+        with self.assertRaises(IntegrityError):
+            Note.objects.create(info=None)
+
     def test_alter_null_to_not_null(self):
         """
         #23609 - Tests handling of default values when altering from NULL to NOT NULL.