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

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

django/db/backends/mysql/schema.py

@@ -48,3 +48,11 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
                 '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 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
                         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 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
                 ],
             )
         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 @@ from django.utils import six
 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 @@ class BaseDatabaseSchemaEditor(object):
         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 @@ class BaseDatabaseSchemaEditor(object):
         # 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 @@ class BaseDatabaseSchemaEditor(object):
         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 @@ class BaseDatabaseSchemaEditor(object):
         # 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 @@ class BaseDatabaseSchemaEditor(object):
                 }
             )
             # 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 @@ class BaseDatabaseSchemaEditor(object):
         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 @@ class BaseDatabaseSchemaEditor(object):
         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 @@ class OperationTests(OperationTestBase):
 
         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 @@ class SchemaTests(TransactionTestCase):
     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 @@ class SchemaTests(TransactionTestCase):
                 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.