From 83622b824b7014977dfc7086bbc2628ea53f4cd0 Mon Sep 17 00:00:00 2001 From: Markus Holtermann Date: Fri, 23 Jan 2026 14:45:33 +0100 Subject: [PATCH] Fixed #36878 -- Unified data type for *_together options in ModelState. Ever since the beginning of Django's migration framework, there's been a bit of an inconsistency on how index_together and unique_together values have been stored on the ModelState[^1]. It's only really obvious, when looking at the current code for `from_model()`[^2] and the `rename_field()` state alteration code[^3]. The problem in the autodetector's detection of the `*_together` options as raised in the ticket, reinforces the inconsistency[^4]: the old value is being normalized to a set of tuples, whereas the new value is taken as-is. Why this hasn't been caught before, is likely to the fact, that we never really look at a `to_state` that comes from migration operations in the autodetector. Instead, in both usages in Django[^5], [^6] the `to_state` is a `ProjectState.from_apps()`. And that state is consistently using sets of tuples and not lists of lists. [^1]: https://github.com/django/django/commit/67dcea711e92025d0e8676b869b7ef15dbc6db73#diff-5dd147e9e978e645313dd99eab3a7bab1f1cb0a53e256843adb68aeed71e61dcR85-R87 [^2]: https://github.com/django/django/blob/b1ffa9a9d78b0c2c5ad6ed5a1d84e380d5cfd010/django/db/migrations/state.py#L842 [^3]: https://github.com/django/django/blob/b1ffa9a9d78b0c2c5ad6ed5a1d84e380d5cfd010/django/db/migrations/state.py#L340-L345 [^4]: https://github.com/django/django/blob/b1ffa9a9d78b0c2c5ad6ed5a1d84e380d5cfd010/django/db/migrations/autodetector.py#L1757-L1771 [^5]: https://github.com/django/django/blob/2351c1b12cc9cf82d642f769c774bc3ea0cc4006/django/core/management/commands/makemigrations.py#L215-L219 [^6]: https://github.com/django/django/blob/2351c1b12cc9cf82d642f769c774bc3ea0cc4006/django/core/management/commands/migrate.py#L329-L332 --- django/db/migrations/state.py | 13 ++++---- tests/migrations/test_autodetector.py | 45 +++++++++++++++++++++++++++ tests/migrations/test_base.py | 7 ++--- tests/migrations/test_operations.py | 21 ++++++++----- 4 files changed, 69 insertions(+), 17 deletions(-) diff --git a/django/db/migrations/state.py b/django/db/migrations/state.py index 802aeb0b5e..9e9cc58fae 100644 --- a/django/db/migrations/state.py +++ b/django/db/migrations/state.py @@ -192,9 +192,10 @@ class ProjectState: def remove_model_options(self, app_label, model_name, option_name, value_to_remove): model_state = self.models[app_label, model_name] if objs := model_state.options.get(option_name): - model_state.options[option_name] = [ - obj for obj in objs if tuple(obj) != tuple(value_to_remove) - ] + new_value = [obj for obj in objs if tuple(obj) != tuple(value_to_remove)] + if option_name in {"index_together", "unique_together"}: + new_value = set(normalize_together(new_value)) + model_state.options[option_name] = new_value self.reload_model(app_label, model_name, delay=True) def alter_model_managers(self, app_label, model_name, managers): @@ -339,10 +340,10 @@ class ProjectState: options = model_state.options for option in ("index_together", "unique_together"): if option in options: - options[option] = [ - [new_name if n == old_name else n for n in together] + options[option] = { + tuple(new_name if n == old_name else n for n in together) for together in options[option] - ] + } # Fix to_fields to refer to the new field. delay = True references = get_references(self, model_key, (old_name, found)) diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index e333621855..7a66e500cb 100644 --- a/tests/migrations/test_autodetector.py +++ b/tests/migrations/test_autodetector.py @@ -5590,6 +5590,51 @@ class AutodetectorTests(BaseAutodetectorTests): preserve_default=True, ) + def test_does_not_crash_after_rename_on_unique_together(self): + fields = ("first", "second") + before = self.make_project_state( + [ + ModelState( + "app", + "Foo", + [ + ("id", models.AutoField(primary_key=True)), + ("first", models.IntegerField()), + ("second", models.IntegerField()), + ], + options={"unique_together": {fields}}, + ), + ] + ) + after = before.clone() + after.rename_field("app", "foo", "first", "first_renamed") + + changes = MigrationAutodetector( + before, after, MigrationQuestioner({"ask_rename": True}) + )._detect_changes() + + self.assertNumberMigrations(changes, "app", 1) + self.assertOperationTypes( + changes, "app", 0, ["RenameField", "AlterUniqueTogether"] + ) + self.assertOperationAttributes( + changes, + "app", + 0, + 0, + model_name="foo", + old_name="first", + new_name="first_renamed", + ) + self.assertOperationAttributes( + changes, + "app", + 0, + 1, + name="foo", + unique_together={("first_renamed", "second")}, + ) + class MigrationSuggestNameTests(SimpleTestCase): def test_no_operations(self): diff --git a/tests/migrations/test_base.py b/tests/migrations/test_base.py index 24ae59ca1b..31967f9ed4 100644 --- a/tests/migrations/test_base.py +++ b/tests/migrations/test_base.py @@ -290,14 +290,13 @@ class OperationTestBase(MigrationTestBase): ): """Creates a test model state and database table.""" # Make the "current" state. - model_options = { - "swappable": "TEST_SWAP_MODEL", - "unique_together": [["pink", "weight"]] if unique_together else [], - } + model_options = {"swappable": "TEST_SWAP_MODEL"} if options: model_options["permissions"] = [("can_groom", "Can groom")] if db_table: model_options["db_table"] = db_table + if unique_together: + model_options["unique_together"] = {("pink", "weight")} operations = [ migrations.CreateModel( "Pony", diff --git a/tests/migrations/test_operations.py b/tests/migrations/test_operations.py index 00c76538e0..e859b62a10 100644 --- a/tests/migrations/test_operations.py +++ b/tests/migrations/test_operations.py @@ -3336,11 +3336,11 @@ class OperationTests(OperationTestBase): # unique_together has the renamed column. self.assertIn( "blue", - new_state.models["test_rnflut", "pony"].options["unique_together"][0], + list(new_state.models["test_rnflut", "pony"].options["unique_together"])[0], ) self.assertNotIn( "pink", - new_state.models["test_rnflut", "pony"].options["unique_together"][0], + list(new_state.models["test_rnflut", "pony"].options["unique_together"])[0], ) # Rename field. self.assertColumnExists("test_rnflut_pony", "pink") @@ -3377,7 +3377,7 @@ class OperationTests(OperationTestBase): ("weight", models.FloatField()), ], options={ - "index_together": [("weight", "pink")], + "index_together": {("weight", "pink")}, }, ), ] @@ -3390,10 +3390,12 @@ class OperationTests(OperationTestBase): self.assertNotIn("pink", new_state.models["test_rnflit", "pony"].fields) # index_together has the renamed column. self.assertIn( - "blue", new_state.models["test_rnflit", "pony"].options["index_together"][0] + "blue", + list(new_state.models["test_rnflit", "pony"].options["index_together"])[0], ) self.assertNotIn( - "pink", new_state.models["test_rnflit", "pony"].options["index_together"][0] + "pink", + list(new_state.models["test_rnflit", "pony"].options["index_together"])[0], ) # Rename field. @@ -3952,7 +3954,7 @@ class OperationTests(OperationTestBase): ("weight", models.FloatField()), ], options={ - "index_together": [("weight", "pink")], + "index_together": {("weight", "pink")}, }, ), ] @@ -3972,6 +3974,11 @@ class OperationTests(OperationTestBase): ) new_state = project_state.clone() operation.state_forwards(app_label, new_state) + # Ensure the model state has the correct type for the index_together + # option. + self.assertIsInstance( + new_state.models[app_label, "pony"].options["index_together"], set + ) # Rename index. with connection.schema_editor() as editor: operation.database_forwards(app_label, editor, project_state, new_state) @@ -4079,7 +4086,7 @@ class OperationTests(OperationTestBase): ("weight", models.FloatField()), ], options={ - "index_together": [("weight", "pink")], + "index_together": {("weight", "pink")}, }, ), ]