From c1276785f90b89c8bced606b9cbf7b1a612506e5 Mon Sep 17 00:00:00 2001 From: Andrew Godwin Date: Sun, 15 Jun 2014 16:01:49 -0700 Subject: [PATCH] Fixed #22568: Better proxy model support in migrations --- django/db/migrations/autodetector.py | 122 ++++++++++++++++++---- django/db/migrations/operations/models.py | 10 +- django/db/migrations/optimizer.py | 3 +- tests/migrations/test_autodetector.py | 63 ++++++++--- tests/migrations/test_operations.py | 61 ++++++++++- 5 files changed, 220 insertions(+), 39 deletions(-) diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py index e937e10b80b..735b77441a2 100644 --- a/django/db/migrations/autodetector.py +++ b/django/db/migrations/autodetector.py @@ -3,6 +3,7 @@ from __future__ import unicode_literals import re import datetime +from django.utils import six from django.db import models from django.db.migrations import operations from django.db.migrations.migration import Migration @@ -104,23 +105,32 @@ class MigrationAutodetector(object): # into migrations to resolve dependencies caused by M2Ms and FKs. self.generated_operations = {} - # Prepare some old/new state and model lists, ignoring - # proxy models and unmigrated apps. + # Prepare some old/new state and model lists, separating + # proxy models and ignoring unmigrated apps. self.old_apps = self.from_state.render(ignore_swappable=True) self.new_apps = self.to_state.render() self.old_model_keys = [] + self.old_proxy_keys = [] + self.new_model_keys = [] + self.new_proxy_keys = [] for al, mn in sorted(self.from_state.models.keys()): model = self.old_apps.get_model(al, mn) - if not model._meta.proxy and model._meta.managed and al not in self.from_state.real_apps: - self.old_model_keys.append((al, mn)) - self.new_model_keys = [] + if model._meta.managed and al not in self.from_state.real_apps: + if model._meta.proxy: + self.old_proxy_keys.append((al, mn)) + else: + self.old_model_keys.append((al, mn)) + for al, mn in sorted(self.to_state.models.keys()): model = self.new_apps.get_model(al, mn) - if not model._meta.proxy and model._meta.managed and ( + if model._meta.managed and ( al not in self.from_state.real_apps or (convert_apps and al in convert_apps) ): - self.new_model_keys.append((al, mn)) + if model._meta.proxy: + self.new_proxy_keys.append((al, mn)) + else: + self.new_model_keys.append((al, mn)) # Renames have to come first self.generate_renamed_models() @@ -155,6 +165,8 @@ class MigrationAutodetector(object): # Generate non-rename model operations self.generate_created_models() self.generate_deleted_models() + self.generate_created_proxies() + self.generate_deleted_proxies() self.generate_altered_options() # Generate field operations @@ -232,7 +244,12 @@ class MigrationAutodetector(object): if self.migrations.get(dep[0], None): operation_dependencies.add((dep[0], self.migrations[dep[0]][-1].name)) else: - operation_dependencies.add((dep[0], "__latest__")) + # If we can't find the other app, we add a __latest__ dependency, + # but only if we've already been through once and checked everything + if chop_mode: + operation_dependencies.add((dep[0], "__latest__")) + else: + deps_satisfied = False if deps_satisfied: chopped.append(operation) dependencies.update(operation_dependencies) @@ -306,6 +323,12 @@ class MigrationAutodetector(object): operation.model_name.lower() == dependency[1].lower() and operation.name.lower() == dependency[2].lower() ) + # Removed model + elif dependency[2] is None and dependency[3] is False: + return ( + isinstance(operation, operations.DeleteModel) and + operation.name.lower() == dependency[1].lower() + ) # order_with_respect_to being unset for a field elif dependency[2] is not None and dependency[3] == "order_wrt_unset": return ( @@ -384,7 +407,16 @@ class MigrationAutodetector(object): unique_together = model_state.options.pop('unique_together', None) index_together = model_state.options.pop('index_together', None) order_with_respect_to = model_state.options.pop('order_with_respect_to', None) - # Generate creation operatoin + # Depend on the deletion of any possible proxy version of us + dependencies = [ + (app_label, model_name, None, False), + ] + # Depend on all bases + for base in model_state.bases: + if isinstance(base, six.string_types) and "." in base: + base_app_label, base_name = base.split(".", 1) + dependencies.append((base_app_label, base_name, None, False)) + # Generate creation operation self.add_operation( app_label, operations.CreateModel( @@ -392,7 +424,8 @@ class MigrationAutodetector(object): fields=[d for d in model_state.fields if d[0] not in related_fields], options=model_state.options, bases=model_state.bases, - ) + ), + dependencies = dependencies, ) # Generate operations for each related field for name, field in sorted(related_fields.items()): @@ -412,6 +445,8 @@ class MigrationAutodetector(object): None, True )) + # Depend on our own model being created + dependencies.append((app_label, model_name, None, True)) # Make operation self.add_operation( app_label, @@ -423,6 +458,11 @@ class MigrationAutodetector(object): dependencies=list(set(dependencies)), ) # Generate other opns + related_dependencies = [ + (app_label, model_name, name, True) + for name, field in sorted(related_fields.items()) + ] + related_dependencies.append((app_label, model_name, None, True)) if unique_together: self.add_operation( app_label, @@ -430,10 +470,7 @@ class MigrationAutodetector(object): name=model_name, unique_together=unique_together, ), - dependencies=[ - (app_label, model_name, name, True) - for name, field in sorted(related_fields.items()) - ] + dependencies=related_dependencies ) if index_together: self.add_operation( @@ -442,10 +479,7 @@ class MigrationAutodetector(object): name=model_name, index_together=index_together, ), - dependencies=[ - (app_label, model_name, name, True) - for name, field in sorted(related_fields.items()) - ] + dependencies=related_dependencies ) if order_with_respect_to: self.add_operation( @@ -456,9 +490,43 @@ class MigrationAutodetector(object): ), dependencies=[ (app_label, model_name, order_with_respect_to, True), + (app_label, model_name, None, True), ] ) + def generate_created_proxies(self): + """ + Makes CreateModel statements for proxy models. + We use the same statements as that way there's less code duplication, + but of course for proxy models we can skip all that pointless field + stuff and just chuck out an operation. + """ + added_proxies = set(self.new_proxy_keys) - set(self.old_proxy_keys) + for app_label, model_name in sorted(added_proxies): + model_state = self.to_state.models[app_label, model_name] + assert model_state.options.get("proxy", False) + # Depend on the deletion of any possible non-proxy version of us + dependencies = [ + (app_label, model_name, None, False), + ] + # Depend on all bases + for base in model_state.bases: + if isinstance(base, six.string_types) and "." in base: + base_app_label, base_name = base.split(".", 1) + dependencies.append((base_app_label, base_name, None, False)) + # Generate creation operation + self.add_operation( + app_label, + operations.CreateModel( + name=model_state.name, + fields=[], + options=model_state.options, + bases=model_state.bases, + ), + # Depend on the deletion of any possible non-proxy version of us + dependencies = dependencies, + ) + def generate_deleted_models(self): """ Find all deleted models and make creation operations for them, @@ -547,6 +615,21 @@ class MigrationAutodetector(object): dependencies=list(set(dependencies)), ) + def generate_deleted_proxies(self): + """ + Makes DeleteModel statements for proxy models. + """ + deleted_proxies = set(self.old_proxy_keys) - set(self.new_proxy_keys) + for app_label, model_name in sorted(deleted_proxies): + model_state = self.from_state.models[app_label, model_name] + assert model_state.options.get("proxy", False) + self.add_operation( + app_label, + operations.DeleteModel( + name=model_state.name, + ), + ) + def generate_added_fields(self): # New fields self.renamed_fields = {} @@ -687,7 +770,8 @@ class MigrationAutodetector(object): makes an operation to represent them in state changes (in case Python code in migrations needs them) """ - for app_label, model_name in sorted(self.kept_model_keys): + models_to_check = self.kept_model_keys.union(set(self.new_proxy_keys).intersection(self.old_proxy_keys)) + for app_label, model_name in sorted(models_to_check): old_model_name = self.renamed_models.get((app_label, model_name), model_name) old_model_state = self.from_state.models[app_label, old_model_name] new_model_state = self.to_state.models[app_label, model_name] diff --git a/django/db/migrations/operations/models.py b/django/db/migrations/operations/models.py index d8f98b79264..a3c0cc74b25 100644 --- a/django/db/migrations/operations/models.py +++ b/django/db/migrations/operations/models.py @@ -32,17 +32,17 @@ class CreateModel(Operation): def database_forwards(self, app_label, schema_editor, from_state, to_state): apps = to_state.render() model = apps.get_model(app_label, self.name) - if router.allow_migrate(schema_editor.connection.alias, model): + if router.allow_migrate(schema_editor.connection.alias, model) and not model._meta.proxy: schema_editor.create_model(model) def database_backwards(self, app_label, schema_editor, from_state, to_state): apps = from_state.render() model = apps.get_model(app_label, self.name) - if router.allow_migrate(schema_editor.connection.alias, model): + if router.allow_migrate(schema_editor.connection.alias, model) and not model._meta.proxy: schema_editor.delete_model(model) def describe(self): - return "Create model %s" % (self.name, ) + return "Create %smodel %s" % ("proxy " if self.options.get("proxy", False) else "", self.name) def references_model(self, name, app_label=None): strings_to_check = [self.name] @@ -85,13 +85,13 @@ class DeleteModel(Operation): def database_forwards(self, app_label, schema_editor, from_state, to_state): apps = from_state.render() model = apps.get_model(app_label, self.name) - if router.allow_migrate(schema_editor.connection.alias, model): + if router.allow_migrate(schema_editor.connection.alias, model) and not model._meta.proxy: schema_editor.delete_model(model) def database_backwards(self, app_label, schema_editor, from_state, to_state): apps = to_state.render() model = apps.get_model(app_label, self.name) - if router.allow_migrate(schema_editor.connection.alias, model): + if router.allow_migrate(schema_editor.connection.alias, model) and not model._meta.proxy: schema_editor.create_model(model) def references_model(self, name, app_label=None): diff --git a/django/db/migrations/optimizer.py b/django/db/migrations/optimizer.py index 2f6ab6be4a2..342b88e99d7 100644 --- a/django/db/migrations/optimizer.py +++ b/django/db/migrations/optimizer.py @@ -163,7 +163,8 @@ class MigrationOptimizer(object): """ Folds a CreateModel and a DeleteModel into nothing. """ - if operation.name.lower() == other.name.lower(): + if operation.name.lower() == other.name.lower() and \ + not operation.options.get("proxy", False): return [] def reduce_model_alter_delete(self, operation, other, in_between): diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index 9a8be73f27d..b1233dc41f1 100644 --- a/tests/migrations/test_autodetector.py +++ b/tests/migrations/test_autodetector.py @@ -37,7 +37,9 @@ class AutodetectorTests(TestCase): author_with_publisher = ModelState("testapp", "Author", [("id", models.AutoField(primary_key=True)), ("name", models.CharField(max_length=200)), ("publisher", models.ForeignKey("testapp.Publisher"))]) author_with_custom_user = ModelState("testapp", "Author", [("id", models.AutoField(primary_key=True)), ("name", models.CharField(max_length=200)), ("user", models.ForeignKey("thirdapp.CustomUser"))]) author_proxy = ModelState("testapp", "AuthorProxy", [], {"proxy": True}, ("testapp.author", )) + author_proxy_options = ModelState("testapp", "AuthorProxy", [], {"proxy": True, "verbose_name": "Super Author"}, ("testapp.author", )) author_proxy_notproxy = ModelState("testapp", "AuthorProxy", [], {}, ("testapp.author", )) + author_proxy_third = ModelState("thirdapp", "AuthorProxy", [], {"proxy": True}, ("testapp.author", )) author_unmanaged = ModelState("testapp", "AuthorUnmanaged", [], {"managed": False}, ("testapp.author", )) author_unmanaged_managed = ModelState("testapp", "AuthorUnmanaged", [], {}, ("testapp.author", )) author_with_m2m = ModelState("testapp", "Author", [ @@ -54,6 +56,7 @@ class AutodetectorTests(TestCase): other_stable = ModelState("otherapp", "Stable", [("id", models.AutoField(primary_key=True))]) third_thing = ModelState("thirdapp", "Thing", [("id", models.AutoField(primary_key=True))]) book = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))]) + book_proxy_fk = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("thirdapp.AuthorProxy")), ("title", models.CharField(max_length=200))]) book_with_no_author = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("title", models.CharField(max_length=200))]) book_with_author_renamed = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Writer")), ("title", models.CharField(max_length=200))]) book_with_field_and_author_renamed = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("writer", models.ForeignKey("testapp.Writer")), ("title", models.CharField(max_length=200))]) @@ -363,6 +366,29 @@ class AutodetectorTests(TestCase): self.assertEqual(migration2.dependencies, [("testapp", "auto_1")]) self.assertEqual(migration3.dependencies, [("otherapp", "auto_1")]) + def test_proxy_fk_dependency(self): + "Tests that FK dependencies still work on proxy models" + # Make state + # Note that testapp (author) has no dependencies, + # otherapp (book) depends on testapp (authorproxy) + before = self.make_project_state([]) + after = self.make_project_state([self.author_empty, self.author_proxy_third, self.book_proxy_fk]) + autodetector = MigrationAutodetector(before, after) + changes = autodetector._detect_changes() + # Right number of migrations? + self.assertNumberMigrations(changes, 'testapp', 1) + self.assertNumberMigrations(changes, 'otherapp', 1) + self.assertNumberMigrations(changes, 'thirdapp', 1) + # Right number of actions? + # Right actions? + self.assertOperationTypes(changes, 'otherapp', 0, ["CreateModel"]) + self.assertOperationTypes(changes, 'testapp', 0, ["CreateModel"]) + self.assertOperationTypes(changes, 'thirdapp', 0, ["CreateModel"]) + # Right dependencies? + self.assertEqual(changes['testapp'][0].dependencies, []) + self.assertEqual(changes['otherapp'][0].dependencies, [("thirdapp", "auto_1")]) + self.assertEqual(changes['thirdapp'][0].dependencies, [("testapp", "auto_1")]) + def test_same_app_no_fk_dependency(self): """ Tests that a migration with a FK between two models of the same app @@ -537,30 +563,29 @@ class AutodetectorTests(TestCase): self.assertEqual(action2.__class__.__name__, "AlterUniqueTogether") self.assertEqual(action2.unique_together, set([("title", "newfield")])) - def test_proxy_ignorance(self): - "Tests that the autodetector correctly ignores proxy models" + def test_proxy(self): + "Tests that the autodetector correctly deals with proxy models" # First, we test adding a proxy model before = self.make_project_state([self.author_empty]) after = self.make_project_state([self.author_empty, self.author_proxy]) autodetector = MigrationAutodetector(before, after) changes = autodetector._detect_changes() # Right number of migrations? - self.assertEqual(len(changes), 0) + self.assertNumberMigrations(changes, "testapp", 1) + self.assertOperationTypes(changes, "testapp", 0, ["CreateModel"]) + self.assertOperationAttributes(changes, "testapp", 0, 0, name="AuthorProxy", options={"proxy": True}) # Now, we test turning a proxy model into a non-proxy model + # It should delete the proxy then make the real one before = self.make_project_state([self.author_empty, self.author_proxy]) after = self.make_project_state([self.author_empty, self.author_proxy_notproxy]) autodetector = MigrationAutodetector(before, after) changes = autodetector._detect_changes() # Right number of migrations? - self.assertEqual(len(changes['testapp']), 1) - # Right number of actions? - migration = changes['testapp'][0] - self.assertEqual(len(migration.operations), 1) - # Right action? - action = migration.operations[0] - self.assertEqual(action.__class__.__name__, "CreateModel") - self.assertEqual(action.name, "AuthorProxy") + self.assertNumberMigrations(changes, "testapp", 1) + self.assertOperationTypes(changes, "testapp", 0, ["DeleteModel", "CreateModel"]) + self.assertOperationAttributes(changes, "testapp", 0, 0, name="AuthorProxy") + self.assertOperationAttributes(changes, "testapp", 0, 1, name="AuthorProxy", options={}) def test_unmanaged_ignorance(self): "Tests that the autodetector correctly ignores managed models" @@ -804,8 +829,7 @@ class AutodetectorTests(TestCase): def test_alter_model_options(self): """ - If two models with a ForeignKey from one to the other are removed at the same time, - the autodetector should remove them in the correct order. + Changing a model's options should make a change """ before = self.make_project_state([self.author_empty]) after = self.make_project_state([self.author_with_options]) @@ -816,6 +840,19 @@ class AutodetectorTests(TestCase): # Right actions in right order? self.assertOperationTypes(changes, "testapp", 0, ["AlterModelOptions"]) + def test_alter_model_options_proxy(self): + """ + Changing a proxy model's options should also make a change + """ + before = self.make_project_state([self.author_proxy, self.author_empty]) + after = self.make_project_state([self.author_proxy_options, self.author_empty]) + autodetector = MigrationAutodetector(before, after) + changes = autodetector._detect_changes() + # Right number of migrations? + self.assertNumberMigrations(changes, "testapp", 1) + # Right actions in right order? + self.assertOperationTypes(changes, "testapp", 0, ["AlterModelOptions"]) + def test_set_alter_order_with_respect_to(self): "Tests that setting order_with_respect_to adds a field" # Make state diff --git a/tests/migrations/test_operations.py b/tests/migrations/test_operations.py index 64703d9140d..9f5cad0391c 100644 --- a/tests/migrations/test_operations.py +++ b/tests/migrations/test_operations.py @@ -37,7 +37,7 @@ class OperationTests(MigrationTestBase): with connection.schema_editor() as editor: return migration.unapply(project_state, editor) - def set_up_test_model(self, app_label, second_model=False, third_model=False, related_model=False, mti_model=False): + def set_up_test_model(self, app_label, second_model=False, third_model=False, related_model=False, mti_model=False, proxy_model=False): """ Creates a test model state and database table. """ @@ -112,6 +112,13 @@ class OperationTests(MigrationTestBase): ], bases=['%s.Pony' % app_label], )) + if proxy_model: + operations.append(migrations.CreateModel( + "ProxyPony", + fields=[], + options={"proxy": True}, + bases=['%s.Pony' % app_label], + )) return self.apply_operations(app_label, ProjectState(), operations) @@ -221,6 +228,34 @@ class OperationTests(MigrationTestBase): operation.database_backwards("test_crmoih", editor, new_state, project_state) self.assertTableNotExists("test_crmoih_shetlandpony") + def test_create_proxy_model(self): + """ + Tests that CreateModel ignores proxy models. + """ + project_state = self.set_up_test_model("test_crprmo") + # Test the state alteration + operation = migrations.CreateModel( + "ProxyPony", + [], + options = {"proxy": True}, + bases = ("test_crprmo.Pony", ), + ) + new_state = project_state.clone() + operation.state_forwards("test_crprmo", new_state) + self.assertIn(("test_crprmo", "proxypony"), new_state.models) + # Test the database alteration + self.assertTableNotExists("test_crprmo_proxypony") + self.assertTableExists("test_crprmo_pony") + with connection.schema_editor() as editor: + operation.database_forwards("test_crprmo", editor, project_state, new_state) + self.assertTableNotExists("test_crprmo_proxypony") + self.assertTableExists("test_crprmo_pony") + # And test reversal + with connection.schema_editor() as editor: + operation.database_backwards("test_crprmo", editor, new_state, project_state) + self.assertTableNotExists("test_crprmo_proxypony") + self.assertTableExists("test_crprmo_pony") + def test_delete_model(self): """ Tests the DeleteModel operation. @@ -241,6 +276,30 @@ class OperationTests(MigrationTestBase): operation.database_backwards("test_dlmo", editor, new_state, project_state) self.assertTableExists("test_dlmo_pony") + def test_delete_proxy_model(self): + """ + Tests the DeleteModel operation ignores proxy models. + """ + project_state = self.set_up_test_model("test_dlprmo", proxy_model=True) + # Test the state alteration + operation = migrations.DeleteModel("ProxyPony") + new_state = project_state.clone() + operation.state_forwards("test_dlprmo", new_state) + self.assertIn(("test_dlprmo", "proxypony"), project_state.models) + self.assertNotIn(("test_dlprmo", "proxypony"), new_state.models) + # Test the database alteration + self.assertTableExists("test_dlprmo_pony") + self.assertTableNotExists("test_dlprmo_proxypony") + with connection.schema_editor() as editor: + operation.database_forwards("test_dlprmo", editor, project_state, new_state) + self.assertTableExists("test_dlprmo_pony") + self.assertTableNotExists("test_dlprmo_proxypony") + # And test reversal + with connection.schema_editor() as editor: + operation.database_backwards("test_dlprmo", editor, new_state, project_state) + self.assertTableExists("test_dlprmo_pony") + self.assertTableNotExists("test_dlprmo_proxypony") + def test_rename_model(self): """ Tests the RenameModel operation.