Fixed #23894 -- Made deconstruct methods favor kwargs over args

django/contrib/postgres/fields/array.py

@@ -83,8 +83,10 @@ class ArrayField(Field):
     def deconstruct(self):
         name, path, args, kwargs = super(ArrayField, self).deconstruct()
         path = 'django.contrib.postgres.fields.ArrayField'
-        args.insert(0, self.base_field)
+        kwargs.update({
-        kwargs['size'] = self.size
+            'base_field': self.base_field,
+            'size': self.size,
+        })
         return name, path, args, kwargs
 
     def to_python(self, value):

django/db/migrations/operations/fields.py

@@ -17,12 +17,16 @@ class AddField(Operation):
         self.preserve_default = preserve_default
 
     def deconstruct(self):
-        kwargs = {}
+        kwargs = {
+            'model_name': self.model_name,
+            'name': self.name,
+            'field': self.field,
+        }
         if self.preserve_default is not True:
             kwargs['preserve_default'] = self.preserve_default
         return (
             self.__class__.__name__,
-            [self.model_name, self.name, self.field],
+            [],
             kwargs
         )
 
@@ -74,10 +78,14 @@ class RemoveField(Operation):
         self.name = name
 
     def deconstruct(self):
+        kwargs = {
+            'model_name': self.model_name,
+            'name': self.name,
+        }
         return (
             self.__class__.__name__,
-            [self.model_name, self.name],
+            [],
-            {}
+            kwargs
         )
 
     def state_forwards(self, app_label, state):
@@ -120,12 +128,16 @@ class AlterField(Operation):
         self.preserve_default = preserve_default
 
     def deconstruct(self):
-        kwargs = {}
+        kwargs = {
+            'model_name': self.model_name,
+            'name': self.name,
+            'field': self.field,
+        }
         if self.preserve_default is not True:
             kwargs['preserve_default'] = self.preserve_default
         return (
             self.__class__.__name__,
-            [self.model_name, self.name, self.field],
+            [],
             kwargs
         )
 
@@ -183,10 +195,15 @@ class RenameField(Operation):
         self.new_name = new_name
 
     def deconstruct(self):
+        kwargs = {
+            'model_name': self.model_name,
+            'old_name': self.old_name,
+            'new_name': self.new_name,
+        }
         return (
             self.__class__.__name__,
-            [self.model_name, self.old_name, self.new_name],
+            [],
-            {}
+            kwargs
         )
 
     def state_forwards(self, app_label, state):

django/db/migrations/operations/models.py

@@ -21,14 +21,17 @@ class CreateModel(Operation):
         self.bases = bases or (models.Model,)
 
     def deconstruct(self):
-        kwargs = {}
+        kwargs = {
+            'name': self.name,
+            'fields': self.fields,
+        }
         if self.options:
             kwargs['options'] = self.options
         if self.bases and self.bases != (models.Model,):
             kwargs['bases'] = self.bases
         return (
             self.__class__.__name__,
-            [self.name, self.fields],
+            [],
             kwargs
         )
 
@@ -83,10 +86,13 @@ class DeleteModel(Operation):
         self.name = name
 
     def deconstruct(self):
+        kwargs = {
+            'name': self.name,
+        }
         return (
             self.__class__.__name__,
-            [self.name],
+            [],
-            {}
+            kwargs
         )
 
     def state_forwards(self, app_label, state):
@@ -121,10 +127,14 @@ class RenameModel(Operation):
         self.new_name = new_name
 
     def deconstruct(self):
+        kwargs = {
+            'old_name': self.old_name,
+            'new_name': self.new_name,
+        }
         return (
             self.__class__.__name__,
-            [self.old_name, self.new_name],
+            [],
-            {}
+            kwargs
         )
 
     def state_forwards(self, app_label, state):
@@ -214,10 +224,14 @@ class AlterModelTable(Operation):
         self.table = table
 
     def deconstruct(self):
+        kwargs = {
+            'name': self.name,
+            'table': self.table,
+        }
         return (
             self.__class__.__name__,
-            [self.name, self.table],
+            [],
-            {}
+            kwargs
         )
 
     def state_forwards(self, app_label, state):
@@ -266,10 +280,14 @@ class AlterUniqueTogether(Operation):
         self.unique_together = set(tuple(cons) for cons in unique_together)
 
     def deconstruct(self):
+        kwargs = {
+            'name': self.name,
+            'unique_together': self.unique_together,
+        }
         return (
             self.__class__.__name__,
-            [self.name, self.unique_together],
+            [],
-            {}
+            kwargs
         )
 
     def state_forwards(self, app_label, state):
@@ -311,10 +329,14 @@ class AlterIndexTogether(Operation):
         self.index_together = set(tuple(cons) for cons in index_together)
 
     def deconstruct(self):
+        kwargs = {
+            'name': self.name,
+            'index_together': self.index_together,
+        }
         return (
             self.__class__.__name__,
-            [self.name, self.index_together],
+            [],
-            {}
+            kwargs
         )
 
     def state_forwards(self, app_label, state):
@@ -353,10 +375,14 @@ class AlterOrderWithRespectTo(Operation):
         self.order_with_respect_to = order_with_respect_to
 
     def deconstruct(self):
+        kwargs = {
+            'name': self.name,
+            'order_with_respect_to': self.order_with_respect_to,
+        }
         return (
             self.__class__.__name__,
-            [self.name, self.order_with_respect_to],
+            [],
-            {}
+            kwargs
         )
 
     def state_forwards(self, app_label, state):
@@ -412,10 +438,14 @@ class AlterModelOptions(Operation):
         self.options = options
 
     def deconstruct(self):
+        kwargs = {
+            'name': self.name,
+            'options': self.options,
+        }
         return (
             self.__class__.__name__,
-            [self.name, self.options],
+            [],
-            {}
+            kwargs
         )
 
     def state_forwards(self, app_label, state):

django/db/migrations/operations/special.py

@@ -68,14 +68,16 @@ class RunSQL(Operation):
         self.state_operations = state_operations or []
 
     def deconstruct(self):
-        kwargs = {}
+        kwargs = {
+            'sql': self.sql,
+        }
         if self.reverse_sql is not None:
             kwargs['reverse_sql'] = self.reverse_sql
         if self.state_operations:
             kwargs['state_operations'] = self.state_operations
         return (
             self.__class__.__name__,
-            [self.sql],
+            [],
             kwargs
         )
 
@@ -137,14 +139,16 @@ class RunPython(Operation):
             self.reverse_code = reverse_code
 
     def deconstruct(self):
-        kwargs = {}
+        kwargs = {
-        if self.reverse_code:
+            'code': self.code,
+        }
+        if self.reverse_code is not None:
             kwargs['reverse_code'] = self.reverse_code
         if self.atomic is not True:
             kwargs['atomic'] = self.atomic
         return (
             self.__class__.__name__,
-            [self.code],
+            [],
             kwargs
         )
 

tests/migrations/test_operations.py

@@ -184,9 +184,8 @@ class OperationTests(OperationTestBase):
         # And deconstruction
         definition = operation.deconstruct()
         self.assertEqual(definition[0], "CreateModel")
-        self.assertEqual(len(definition[1]), 2)
+        self.assertEqual(definition[1], [])
-        self.assertEqual(len(definition[2]), 0)
+        self.assertEqual(sorted(definition[2].keys()), ["fields", "name"])
-        self.assertEqual(definition[1][0], "Pony")
 
     def test_create_model_with_unique_after(self):
         """
@@ -331,6 +330,11 @@ class OperationTests(OperationTestBase):
             operation.database_backwards("test_crprmo", editor, new_state, project_state)
         self.assertTableNotExists("test_crprmo_proxypony")
         self.assertTableExists("test_crprmo_pony")
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "CreateModel")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(sorted(definition[2].keys()), ["bases", "fields", "name", "options"])
 
     def test_create_unmanaged_model(self):
         """
@@ -381,6 +385,11 @@ class OperationTests(OperationTestBase):
         with connection.schema_editor() as editor:
             operation.database_backwards("test_dlmo", editor, new_state, project_state)
         self.assertTableExists("test_dlmo_pony")
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "DeleteModel")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(list(definition[2]), ["name"])
 
     def test_delete_proxy_model(self):
         """
@@ -440,6 +449,11 @@ class OperationTests(OperationTestBase):
         if connection.features.supports_foreign_keys:
             self.assertFKExists("test_rnmo_rider", ["pony_id"], ("test_rnmo_pony", "id"))
             self.assertFKNotExists("test_rnmo_rider", ["pony_id"], ("test_rnmo_horse", "id"))
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "RenameModel")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(definition[2], {'old_name': "Pony", 'new_name': "Horse"})
 
     def test_rename_model_with_self_referential_fk(self):
         """
@@ -520,6 +534,11 @@ class OperationTests(OperationTestBase):
         with connection.schema_editor() as editor:
             operation.database_backwards("test_adfl", editor, new_state, project_state)
         self.assertColumnNotExists("test_adfl_pony", "height")
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "AddField")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(sorted(definition[2]), ["field", "model_name", "name"])
 
     def test_add_charfield(self):
         """
@@ -698,6 +717,11 @@ class OperationTests(OperationTestBase):
         with connection.schema_editor() as editor:
             operation.database_forwards("test_adflpd", editor, project_state, new_state)
         self.assertColumnExists("test_adflpd_pony", "height")
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "AddField")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(sorted(definition[2]), ["field", "model_name", "name", "preserve_default"])
 
     def test_add_field_m2m(self):
         """
@@ -818,6 +842,11 @@ class OperationTests(OperationTestBase):
         with connection.schema_editor() as editor:
             operation.database_backwards("test_rmfl", editor, new_state, project_state)
         self.assertColumnExists("test_rmfl_pony", "pink")
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "RemoveField")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(definition[2], {'model_name': "Pony", 'name': 'pink'})
 
     def test_remove_fk(self):
         """
@@ -859,6 +888,11 @@ class OperationTests(OperationTestBase):
             operation.database_backwards("test_almota", editor, new_state, project_state)
         self.assertTableExists("test_almota_pony")
         self.assertTableNotExists("test_almota_pony_2")
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "AlterModelTable")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(definition[2], {'name': "Pony", 'table': "test_almota_pony_2"})
 
     def test_alter_model_table_noop(self):
         """
@@ -932,6 +966,11 @@ class OperationTests(OperationTestBase):
         with connection.schema_editor() as editor:
             operation.database_backwards("test_alfl", editor, new_state, project_state)
         self.assertColumnNotNull("test_alfl_pony", "pink")
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "AlterField")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(sorted(definition[2]), ["field", "model_name", "name"])
 
     def test_alter_field_pk(self):
         """
@@ -1023,6 +1062,11 @@ class OperationTests(OperationTestBase):
         # Ensure the index constraint has been reset
         # TODO: Uncomment assert when #23880 is fixed
         # self.assertIndexExists("test_rnfl_pony", ["weight", "pink"])
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "RenameField")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(definition[2], {'model_name': "Pony", 'old_name': "pink", 'new_name': "blue"})
 
     def test_alter_unique_together(self):
         """
@@ -1059,6 +1103,11 @@ class OperationTests(OperationTestBase):
         operation = migrations.AlterUniqueTogether("Pony", ("pink", "weight"))
         operation.state_forwards("test_alunto", new_state)
         self.assertEqual(len(new_state.models["test_alunto", "pony"].options.get("unique_together", set())), 1)
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "AlterUniqueTogether")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(definition[2], {'name': "Pony", 'unique_together': {("pink", "weight")}})
 
     def test_alter_unique_together_remove(self):
         operation = migrations.AlterUniqueTogether("Pony", None)
@@ -1086,6 +1135,11 @@ class OperationTests(OperationTestBase):
         with connection.schema_editor() as editor:
             operation.database_backwards("test_alinto", editor, new_state, project_state)
         self.assertIndexNotExists("test_alinto_pony", ["pink", "weight"])
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "AlterIndexTogether")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(definition[2], {'name': "Pony", 'index_together': {("pink", "weight")}})
 
     def test_alter_index_together_remove(self):
         operation = migrations.AlterIndexTogether("Pony", None)
@@ -1104,6 +1158,11 @@ class OperationTests(OperationTestBase):
         self.assertEqual(len(project_state.models["test_almoop", "pony"].options.get("permissions", [])), 0)
         self.assertEqual(len(new_state.models["test_almoop", "pony"].options.get("permissions", [])), 1)
         self.assertEqual(new_state.models["test_almoop", "pony"].options["permissions"][0][0], "can_groom")
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "AlterModelOptions")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(definition[2], {'name': "Pony", 'options': {"permissions": [("can_groom", "Can groom")]}})
 
     def test_alter_model_options_emptying(self):
         """
@@ -1117,6 +1176,11 @@ class OperationTests(OperationTestBase):
         operation.state_forwards("test_almoop", new_state)
         self.assertEqual(len(project_state.models["test_almoop", "pony"].options.get("permissions", [])), 1)
         self.assertEqual(len(new_state.models["test_almoop", "pony"].options.get("permissions", [])), 0)
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "AlterModelOptions")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(definition[2], {'name': "Pony", 'options': {}})
 
     def test_alter_order_with_respect_to(self):
         """
@@ -1140,6 +1204,11 @@ class OperationTests(OperationTestBase):
         with connection.schema_editor() as editor:
             operation.database_backwards("test_alorwrtto", editor, new_state, project_state)
         self.assertColumnNotExists("test_alorwrtto_rider", "_order")
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "AlterOrderWithRespectTo")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(definition[2], {'name': "Rider", 'order_with_respect_to': "pony"})
 
     def test_alter_fk(self):
         """
@@ -1239,6 +1308,11 @@ class OperationTests(OperationTestBase):
         with connection.schema_editor() as editor:
             operation.database_backwards("test_runsql", editor, new_state, project_state)
         self.assertTableNotExists("i_love_ponies")
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "RunSQL")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(sorted(definition[2]), ["reverse_sql", "sql", "state_operations"])
 
     def test_run_sql_params(self):
         """
@@ -1356,7 +1430,12 @@ class OperationTests(OperationTestBase):
         self.assertEqual(project_state.render().get_model("test_runpython", "Pony").objects.count(), 0)
         # Now test we can't use a string
         with self.assertRaises(ValueError):
-            operation = migrations.RunPython("print 'ahahaha'")
+            migrations.RunPython("print 'ahahaha'")
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "RunPython")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(sorted(definition[2]), ["code", "reverse_code"])
 
         # Also test reversal fails, with an operation identical to above but without reverse_code set
         no_reverse_operation = migrations.RunPython(inner_method)
@@ -1379,6 +1458,11 @@ class OperationTests(OperationTestBase):
         with connection.schema_editor() as editor:
             operation.database_forwards("test_runpython", editor, project_state, new_state)
         self.assertEqual(project_state.render().get_model("test_runpython", "Pony").objects.count(), 4)
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "RunPython")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(sorted(definition[2]), ["code"])
 
         def create_shetlandponies(models, schema_editor):
             ShetlandPony = models.get_model("test_runpython", "ShetlandPony")
@@ -1431,6 +1515,11 @@ class OperationTests(OperationTestBase):
                 with connection.schema_editor() as editor:
                     non_atomic_migration.apply(project_state, editor)
             self.assertEqual(project_state.render().get_model("test_runpythonatomic", "Pony").objects.count(), 1)
+        # And deconstruction
+        definition = non_atomic_migration.operations[0].deconstruct()
+        self.assertEqual(definition[0], "RunPython")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(sorted(definition[2]), ["atomic", "code"])
 
     @unittest.skipIf(sqlparse is None and connection.features.requires_sqlparse_for_splitting, "Missing sqlparse")
     def test_separate_database_and_state(self):
@@ -1464,6 +1553,11 @@ class OperationTests(OperationTestBase):
         with connection.schema_editor() as editor:
             operation.database_backwards("test_separatedatabaseandstate", editor, new_state, project_state)
         self.assertTableNotExists("i_love_ponies")
+        # And deconstruction
+        definition = operation.deconstruct()
+        self.assertEqual(definition[0], "SeparateDatabaseAndState")
+        self.assertEqual(definition[1], [])
+        self.assertEqual(sorted(definition[2]), ["database_operations", "state_operations"])
 
 
 class MigrateNothingRouter(object):

tests/postgres_tests/test_array.py

@@ -6,7 +6,6 @@ from django.contrib.postgres.forms import SimpleArrayField, SplitArrayField
 from django.core import exceptions, serializers
 from django.core.management import call_command
 from django.db import models, IntegrityError, connection
-from django.db.migrations.writer import MigrationWriter
 from django import forms
 from django.test import TestCase, override_settings
 from django.utils import timezone
@@ -229,11 +228,6 @@ class TestMigrations(TestCase):
         new = ArrayField(*args, **kwargs)
         self.assertEqual(new.base_field.max_length, field.base_field.max_length)
 
-    def test_makemigrations(self):
-        field = ArrayField(models.CharField(max_length=20))
-        statement, imports = MigrationWriter.serialize(field)
-        self.assertEqual(statement, 'django.contrib.postgres.fields.ArrayField(models.CharField(max_length=20), size=None)')
-
     @override_settings(MIGRATION_MODULES={
         "postgres_tests": "postgres_tests.array_default_migrations",
     })