From 394053ce605a30c742d6270a80986b255adb3a30 Mon Sep 17 00:00:00 2001 From: Andrew Godwin Date: Mon, 28 Jul 2014 10:47:28 -0700 Subject: [PATCH] [1.7.x] Fixed #23121: AlterModelOptions operation not changing state right --- django/db/migrations/autodetector.py | 16 +++----------- django/db/migrations/operations/models.py | 14 ++++++++++++ tests/migrations/test_autodetector.py | 7 ++++++ tests/migrations/test_operations.py | 26 ++++++++++++++++++----- 4 files changed, 45 insertions(+), 18 deletions(-) diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py index dfbfe869fdc..e27d8ace74c 100644 --- a/django/db/migrations/autodetector.py +++ b/django/db/migrations/autodetector.py @@ -10,6 +10,7 @@ from django.db.migrations import operations from django.db.migrations.migration import Migration from django.db.migrations.questioner import MigrationQuestioner from django.db.migrations.optimizer import MigrationOptimizer +from django.db.migrations.operations.models import AlterModelOptions class MigrationAutodetector(object): @@ -25,17 +26,6 @@ class MigrationAutodetector(object): if it wishes, with the caveat that it may not always be possible. """ - # Model options we want to compare and preserve in an AlterModelOptions op - ALTER_OPTION_KEYS = [ - "get_latest_by", - "ordering", - "permissions", - "default_permissions", - "select_on_save", - "verbose_name", - "verbose_name_plural", - ] - def __init__(self, from_state, to_state, questioner=None): self.from_state = from_state self.to_state = to_state @@ -864,11 +854,11 @@ class MigrationAutodetector(object): new_model_state = self.to_state.models[app_label, model_name] old_options = dict( option for option in old_model_state.options.items() - if option[0] in self.ALTER_OPTION_KEYS + if option[0] in AlterModelOptions.ALTER_OPTION_KEYS ) new_options = dict( option for option in new_model_state.options.items() - if option[0] in self.ALTER_OPTION_KEYS + if option[0] in AlterModelOptions.ALTER_OPTION_KEYS ) if old_options != new_options: self.add_operation( diff --git a/django/db/migrations/operations/models.py b/django/db/migrations/operations/models.py index 1ded7c6db41..4e8100a5bbb 100644 --- a/django/db/migrations/operations/models.py +++ b/django/db/migrations/operations/models.py @@ -337,6 +337,17 @@ class AlterModelOptions(Operation): may still need them. """ + # Model options we want to compare and preserve in an AlterModelOptions op + ALTER_OPTION_KEYS = [ + "get_latest_by", + "ordering", + "permissions", + "default_permissions", + "select_on_save", + "verbose_name", + "verbose_name_plural", + ] + def __init__(self, name, options): self.name = name self.options = options @@ -345,6 +356,9 @@ class AlterModelOptions(Operation): model_state = state.models[app_label, self.name.lower()] model_state.options = dict(model_state.options) model_state.options.update(self.options) + for key in self.ALTER_OPTION_KEYS: + if key not in self.options and key in model_state.options: + del model_state.options[key] def database_forwards(self, app_label, schema_editor, from_state, to_state): pass diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index 60811e85779..3e0195d0d2e 100644 --- a/tests/migrations/test_autodetector.py +++ b/tests/migrations/test_autodetector.py @@ -885,6 +885,13 @@ class AutodetectorTests(TestCase): self.assertNumberMigrations(changes, "testapp", 1) # Right actions in right order? self.assertOperationTypes(changes, "testapp", 0, ["AlterModelOptions"]) + # Changing them back to empty should also make a change + before = self.make_project_state([self.author_with_options]) + after = self.make_project_state([self.author_empty]) + autodetector = MigrationAutodetector(before, after) + changes = autodetector._detect_changes() + self.assertNumberMigrations(changes, "testapp", 1) + self.assertOperationTypes(changes, "testapp", 0, ["AlterModelOptions"]) def test_alter_model_options_proxy(self): """ diff --git a/tests/migrations/test_operations.py b/tests/migrations/test_operations.py index dffa3a4ebea..12c0502f913 100644 --- a/tests/migrations/test_operations.py +++ b/tests/migrations/test_operations.py @@ -46,7 +46,7 @@ class OperationTestBase(MigrationTestBase): operation.state_forwards(app_label, new_state) return project_state, new_state - def set_up_test_model(self, app_label, second_model=False, third_model=False, related_model=False, mti_model=False, proxy_model=False, unique_together=False): + def set_up_test_model(self, app_label, second_model=False, third_model=False, related_model=False, mti_model=False, proxy_model=False, unique_together=False, options=False): """ Creates a test model state and database table. """ @@ -76,6 +76,12 @@ class OperationTestBase(MigrationTestBase): except DatabaseError: pass # Make the "current" state + model_options = { + "swappable": "TEST_SWAP_MODEL", + "unique_together": [["pink", "weight"]] if unique_together else [], + } + if options: + model_options["permissions"] = [("can_groom", "Can groom")] operations = [migrations.CreateModel( "Pony", [ @@ -83,10 +89,7 @@ class OperationTestBase(MigrationTestBase): ("pink", models.IntegerField(default=3)), ("weight", models.FloatField()), ], - options={ - "swappable": "TEST_SWAP_MODEL", - "unique_together": [["pink", "weight"]] if unique_together else [], - }, + options=model_options, )] if second_model: operations.append(migrations.CreateModel( @@ -975,6 +978,19 @@ class OperationTests(OperationTestBase): 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") + def test_alter_model_options_emptying(self): + """ + Tests that the AlterModelOptions operation removes keys from the dict (#23121) + """ + project_state = self.set_up_test_model("test_almoop", options=True) + # Test the state alteration (no DB alteration to test) + operation = migrations.AlterModelOptions("Pony", {}) + self.assertEqual(operation.describe(), "Change Meta options on Pony") + new_state = project_state.clone() + 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) + def test_alter_order_with_respect_to(self): """ Tests the AlterOrderWithRespectTo operation.