From 8f6dff372b174e772920de6d82bd085f1a74eaf2 Mon Sep 17 00:00:00 2001 From: Andrew Godwin Date: Wed, 30 Apr 2014 12:25:12 -0700 Subject: [PATCH] Fixed #22485: Include all unmigrated apps in project state by default. --- .../management/commands/makemigrations.py | 4 +- django/core/management/commands/migrate.py | 2 +- django/db/migrations/autodetector.py | 7 +-- django/db/migrations/executor.py | 8 +-- django/db/migrations/graph.py | 4 +- django/db/migrations/loader.py | 40 ++++++--------- django/db/migrations/state.py | 50 +++++++++++++------ tests/migrations/test_loader.py | 8 +-- tests/migrations/test_state.py | 25 ++++++++++ 9 files changed, 93 insertions(+), 55 deletions(-) diff --git a/django/core/management/commands/makemigrations.py b/django/core/management/commands/makemigrations.py index 1a4f96cf9c..d9d7a05b6c 100644 --- a/django/core/management/commands/makemigrations.py +++ b/django/core/management/commands/makemigrations.py @@ -54,7 +54,7 @@ class Command(BaseCommand): # (makemigrations doesn't look at the database state). # Also make sure the graph is built without unmigrated apps shoehorned in. loader = MigrationLoader(connections[DEFAULT_DB_ALIAS], load=False) - loader.build_graph(ignore_unmigrated=True) + loader.build_graph() # Before anything else, see if there's conflicting apps and drop out # hard if there are any and they don't want to merge @@ -78,7 +78,7 @@ class Command(BaseCommand): # Set up autodetector autodetector = MigrationAutodetector( - loader.graph.project_state(), + loader.project_state(), ProjectState.from_apps(apps), InteractiveMigrationQuestioner(specified_apps=app_labels), ) diff --git a/django/core/management/commands/migrate.py b/django/core/management/commands/migrate.py index 923211f09a..a48e10bed0 100644 --- a/django/core/management/commands/migrate.py +++ b/django/core/management/commands/migrate.py @@ -135,7 +135,7 @@ class Command(BaseCommand): self.stdout.write(" No migrations needed.") # If there's changes that aren't in migrations yet, tell them how to fix it. autodetector = MigrationAutodetector( - executor.loader.graph.project_state(), + executor.loader.project_state(), ProjectState.from_apps(apps), ) changes = autodetector.changes(graph=executor.loader.graph) diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py index 134dcfed95..f8b631270f 100644 --- a/django/db/migrations/autodetector.py +++ b/django/db/migrations/autodetector.py @@ -50,17 +50,18 @@ class MigrationAutodetector(object): old_apps = self.from_state.render() new_apps = self.to_state.render() # Prepare lists of old/new model keys that we care about - # (i.e. ignoring proxy ones) + # (i.e. ignoring proxy ones and unmigrated ones) + old_model_keys = [] for al, mn in self.from_state.models.keys(): model = old_apps.get_model(al, mn) - if not model._meta.proxy and model._meta.managed: + if not model._meta.proxy and model._meta.managed and al not in self.from_state.real_apps: old_model_keys.append((al, mn)) new_model_keys = [] for al, mn in self.to_state.models.keys(): model = new_apps.get_model(al, mn) - if not model._meta.proxy and model._meta.managed: + if not model._meta.proxy and model._meta.managed and al not in self.to_state.real_apps: new_model_keys.append((al, mn)) def _rel_agnostic_fields_def(fields): diff --git a/django/db/migrations/executor.py b/django/db/migrations/executor.py index 1a7832b76f..147aa550ae 100644 --- a/django/db/migrations/executor.py +++ b/django/db/migrations/executor.py @@ -69,7 +69,7 @@ class MigrationExecutor(object): statements = [] for migration, backwards in plan: with self.connection.schema_editor(collect_sql=True) as schema_editor: - project_state = self.loader.graph.project_state((migration.app_label, migration.name), at_end=False) + project_state = self.loader.project_state((migration.app_label, migration.name), at_end=False) if not backwards: migration.apply(project_state, schema_editor, collect_sql=True) else: @@ -90,7 +90,7 @@ class MigrationExecutor(object): else: # Alright, do it normally with self.connection.schema_editor() as schema_editor: - project_state = self.loader.graph.project_state((migration.app_label, migration.name), at_end=False) + project_state = self.loader.project_state((migration.app_label, migration.name), at_end=False) migration.apply(project_state, schema_editor) # For replacement migrations, record individual statuses if migration.replaces: @@ -110,7 +110,7 @@ class MigrationExecutor(object): self.progress_callback("unapply_start", migration, fake) if not fake: with self.connection.schema_editor() as schema_editor: - project_state = self.loader.graph.project_state((migration.app_label, migration.name), at_end=False) + project_state = self.loader.project_state((migration.app_label, migration.name), at_end=False) migration.unapply(project_state, schema_editor) # For replacement migrations, record individual statuses if migration.replaces: @@ -128,7 +128,7 @@ class MigrationExecutor(object): tables it would create exist. This is intended only for use on initial migrations (as it only looks for CreateModel). """ - project_state = self.loader.graph.project_state((migration.app_label, migration.name), at_end=True) + project_state = self.loader.project_state((migration.app_label, migration.name), at_end=True) apps = project_state.render() for operation in migration.operations: if isinstance(operation, migrations.CreateModel): diff --git a/django/db/migrations/graph.py b/django/db/migrations/graph.py index eddabbd434..af6a31bda8 100644 --- a/django/db/migrations/graph.py +++ b/django/db/migrations/graph.py @@ -121,7 +121,7 @@ class MigrationGraph(object): def __str__(self): return "Graph: %s nodes, %s edges" % (len(self.nodes), sum(len(x) for x in self.dependencies.values())) - def project_state(self, nodes=None, at_end=True): + def make_state(self, nodes=None, at_end=True, real_apps=None): """ Given a migration node or nodes, returns a complete ProjectState for it. If at_end is False, returns the state before the migration has run. @@ -140,7 +140,7 @@ class MigrationGraph(object): if not at_end and migration in nodes: continue plan.append(migration) - project_state = ProjectState() + project_state = ProjectState(real_apps=real_apps) for node in plan: project_state = self.nodes[node].mutate_state(project_state) return project_state diff --git a/django/db/migrations/loader.py b/django/db/migrations/loader.py index ee4f4c9597..c0ac4b7f44 100644 --- a/django/db/migrations/loader.py +++ b/django/db/migrations/loader.py @@ -135,7 +135,7 @@ class MigrationLoader(object): else: return self.disk_migrations[results[0]] - def build_graph(self, ignore_unmigrated=False): + def build_graph(self): """ Builds a migration dependency graph using both the disk and database. You'll need to rebuild the graph if you apply migrations. This isn't @@ -200,31 +200,10 @@ class MigrationLoader(object): # even have migrations. if parent[1] == "__first__" and parent not in self.graph: if parent[0] in self.unmigrated_apps: - if ignore_unmigrated: - parent = None - else: - # This app isn't migrated, but something depends on it. - # We'll add a fake initial migration for it into the - # graph. - app_config = apps.get_app_config(parent[0]) - ops = [] - for model in app_config.get_models(): - model_state = ModelState.from_model(model) - ops.append( - operations.CreateModel( - name=model_state.name, - fields=model_state.fields, - options=model_state.options, - bases=model_state.bases, - ) - ) - new_migration = type( - "FakeInitialMigration", - (Migration, ), - {"operations": ops}, - )(parent[1], parent[0]) - self.graph.add_node(parent, new_migration) - self.applied_migrations.add(parent) + # This app isn't migrated, but something depends on it. + # The models will get auto-added into the state, though + # so we're fine. + continue elif parent[0] in self.migrated_apps: parent = list(self.graph.root_nodes(parent[0]))[0] else: @@ -246,6 +225,15 @@ class MigrationLoader(object): seen_apps.setdefault(app_label, set()).add(migration_name) return dict((app_label, seen_apps[app_label]) for app_label in conflicting_apps) + def project_state(self, nodes=None, at_end=True): + """ + Returns a ProjectState object representing the most recent state + that the migrations we loaded represent. + + See graph.make_state for the meaning of "nodes" and "at_end" + """ + return self.graph.make_state(nodes=nodes, at_end=at_end, real_apps=list(self.unmigrated_apps)) + class BadMigrationError(Exception): """ diff --git a/django/db/migrations/state.py b/django/db/migrations/state.py index 86693f4398..a2e44d26d3 100644 --- a/django/db/migrations/state.py +++ b/django/db/migrations/state.py @@ -1,7 +1,8 @@ from django.apps import AppConfig -from django.apps.registry import Apps +from django.apps.registry import Apps, apps as global_apps from django.db import models from django.db.models.options import DEFAULT_NAMES, normalize_together +from django.db.models.fields.related import do_pending_lookups from django.utils import six from django.utils.module_loading import import_string @@ -17,9 +18,11 @@ class ProjectState(object): app level so that cross-app FKs/etc. resolve properly. """ - def __init__(self, models=None): + def __init__(self, models=None, real_apps=None): self.models = models or {} self.apps = None + # Apps to include from main registry, usually unmigrated ones + self.real_apps = real_apps or [] def add_model_state(self, model_state): self.models[(model_state.app_label, model_state.name.lower())] = model_state @@ -27,20 +30,29 @@ class ProjectState(object): def clone(self): "Returns an exact copy of this ProjectState" return ProjectState( - models=dict((k, v.clone()) for k, v in self.models.items()) + models=dict((k, v.clone()) for k, v in self.models.items()), + real_apps=self.real_apps, ) - def render(self): + def render(self, include_real=None): "Turns the project state into actual models in a new Apps" if self.apps is None: + # Any apps in self.real_apps should have all their models included + # in the render. We don't use the original model instances as there + # are some variables that refer to the Apps object. + real_models = [] + for app_label in self.real_apps: + app = global_apps.get_app_config(app_label) + for model in app.get_models(): + real_models.append(ModelState.from_model(model)) # Populate the app registry with a stub for each application. app_labels = set(model_state.app_label for model_state in self.models.values()) - self.apps = Apps([AppConfigStub(label) for label in sorted(app_labels)]) + self.apps = Apps([AppConfigStub(label) for label in sorted(self.real_apps + list(app_labels))]) # We keep trying to render the models in a loop, ignoring invalid # base errors, until the size of the unrendered models doesn't # decrease by at least one, meaning there's a base dependency loop/ # missing base. - unrendered_models = list(self.models.values()) + unrendered_models = list(self.models.values()) + real_models while unrendered_models: new_unrendered_models = [] for model in unrendered_models: @@ -53,14 +65,22 @@ class ProjectState(object): unrendered_models = new_unrendered_models # make sure apps has no dangling references if self.apps._pending_lookups: - # Raise an error with a best-effort helpful message - # (only for the first issue). Error message should look like: - # "ValueError: Lookup failed for model referenced by - # field migrations.Book.author: migrations.Author" - dangling_lookup = list(self.apps._pending_lookups.items())[0] - raise ValueError("Lookup failed for model referenced by field {field}: {model[0]}.{model[1]}".format( - field=dangling_lookup[1][0][1], - model=dangling_lookup[0])) + # There's some lookups left. See if we can first resolve them + # ourselves - sometimes fields are added after class_prepared is sent + for lookup_model, operations in self.apps._pending_lookups.items(): + try: + model = self.apps.get_model(lookup_model[0], lookup_model[1]) + except LookupError: + # Raise an error with a best-effort helpful message + # (only for the first issue). Error message should look like: + # "ValueError: Lookup failed for model referenced by + # field migrations.Book.author: migrations.Author" + raise ValueError("Lookup failed for model referenced by field {field}: {model[0]}.{model[1]}".format( + field=operations[0][1], + model=lookup_model + )) + else: + do_pending_lookups(model) return self.apps @classmethod @@ -75,6 +95,8 @@ class ProjectState(object): def __eq__(self, other): if set(self.models.keys()) != set(other.models.keys()): return False + if set(self.real_apps) != set(other.real_apps): + return False return all(model == other.models[key] for key, model in self.models.items()) def __ne__(self, other): diff --git a/tests/migrations/test_loader.py b/tests/migrations/test_loader.py index 0e9b9c23d7..34bc340495 100644 --- a/tests/migrations/test_loader.py +++ b/tests/migrations/test_loader.py @@ -61,7 +61,7 @@ class LoaderTests(TestCase): ], ) # Now render it out! - project_state = migration_loader.graph.project_state(("migrations", "0002_second")) + project_state = migration_loader.project_state(("migrations", "0002_second")) self.assertEqual(len(project_state.models), 2) author_state = project_state.models["migrations", "author"] @@ -76,6 +76,9 @@ class LoaderTests(TestCase): ["id", "author"] ) + # Ensure we've included unmigrated apps in there too + self.assertIn("contenttypes", project_state.real_apps) + @override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations_unmigdep"}) def test_load_unmigrated_dependency(self): """ @@ -86,12 +89,11 @@ class LoaderTests(TestCase): self.assertEqual( migration_loader.graph.forwards_plan(("migrations", "0001_initial")), [ - ("auth", "__first__"), ("migrations", "0001_initial"), ], ) # Now render it out! - project_state = migration_loader.graph.project_state(("migrations", "0001_initial")) + project_state = migration_loader.project_state(("migrations", "0001_initial")) self.assertEqual(len([m for a, m in project_state.models if a == "migrations"]), 1) book_state = project_state.models["migrations", "book"] diff --git a/tests/migrations/test_state.py b/tests/migrations/test_state.py index 9a97613485..e5cee5f6cd 100644 --- a/tests/migrations/test_state.py +++ b/tests/migrations/test_state.py @@ -338,6 +338,31 @@ class StateTests(TestCase): with self.assertRaises(ValueError): rendered_state = project_state.render() + def test_real_apps(self): + """ + Tests that including real apps can resolve dangling FK errors. + This test relies on the fact that contenttypes is always loaded. + """ + new_apps = Apps() + + class TestModel(models.Model): + ct = models.ForeignKey("contenttypes.ContentType") + class Meta: + app_label = "migrations" + apps = new_apps + + # If we just stick it into an empty state it should fail + project_state = ProjectState() + project_state.add_model_state(ModelState.from_model(TestModel)) + with self.assertRaises(ValueError): + rendered_state = project_state.render() + + # If we include the real app it should succeed + project_state = ProjectState(real_apps=["contenttypes"]) + project_state.add_model_state(ModelState.from_model(TestModel)) + rendered_state = project_state.render() + self.assertEqual(len(rendered_state.get_models()), 2) + class ModelStateTests(TestCase): def test_custom_model_base(self):