From 3b8e46cbc7cdb03bb40b3b099997a5f659a2d402 Mon Sep 17 00:00:00 2001 From: Andrew Godwin Date: Wed, 4 Dec 2013 16:01:31 +0000 Subject: [PATCH] Migration VCS conflict detection and --merge for makemigrations --- .../management/commands/makemigrations.py | 97 +++++++++++++- django/core/management/commands/migrate.py | 10 ++ django/db/migrations/autodetector.py | 112 +--------------- django/db/migrations/loader.py | 14 ++ django/db/migrations/questioner.py | 122 ++++++++++++++++++ tests/migrations/test_autodetector.py | 5 +- tests/migrations/test_commands.py | 30 ++++- .../test_migrations_conflict/0001_initial.py | 27 ++++ .../0002_conflicting_second.py | 17 +++ .../test_migrations_conflict/0002_second.py | 24 ++++ .../test_migrations_conflict/__init__.py | 0 11 files changed, 341 insertions(+), 117 deletions(-) create mode 100644 django/db/migrations/questioner.py create mode 100644 tests/migrations/test_migrations_conflict/0001_initial.py create mode 100644 tests/migrations/test_migrations_conflict/0002_conflicting_second.py create mode 100644 tests/migrations/test_migrations_conflict/0002_second.py create mode 100644 tests/migrations/test_migrations_conflict/__init__.py diff --git a/django/core/management/commands/makemigrations.py b/django/core/management/commands/makemigrations.py index bb07160bb6..284898d8d4 100644 --- a/django/core/management/commands/makemigrations.py +++ b/django/core/management/commands/makemigrations.py @@ -1,21 +1,26 @@ import sys import os +import operator from optparse import make_option -from django.core.management.base import BaseCommand +from django.core.management.base import BaseCommand, CommandError from django.core.exceptions import ImproperlyConfigured -from django.db import connections, DEFAULT_DB_ALIAS +from django.db import connections, DEFAULT_DB_ALIAS, migrations from django.db.migrations.loader import MigrationLoader -from django.db.migrations.autodetector import MigrationAutodetector, InteractiveMigrationQuestioner +from django.db.migrations.autodetector import MigrationAutodetector +from django.db.migrations.questioner import MigrationQuestioner, InteractiveMigrationQuestioner from django.db.migrations.state import ProjectState from django.db.migrations.writer import MigrationWriter from django.db.models.loading import cache +from django.utils.six.moves import reduce class Command(BaseCommand): option_list = BaseCommand.option_list + ( make_option('--dry-run', action='store_true', dest='dry_run', default=False, help="Just show what migrations would be made; don't actually write them."), + make_option('--merge', action='store_true', dest='merge', default=False, + help="Enable fixing of migration conflicts."), ) help = "Creates new migration(s) for apps." @@ -26,6 +31,7 @@ class Command(BaseCommand): self.verbosity = int(options.get('verbosity')) self.interactive = options.get('interactive') self.dry_run = options.get('dry_run', False) + self.merge = options.get('merge', False) # Make sure the app they asked for exists app_labels = set(app_labels) @@ -44,6 +50,26 @@ class Command(BaseCommand): # (makemigrations doesn't look at the database state). loader = MigrationLoader(connections[DEFAULT_DB_ALIAS]) + # Before anything else, see if there's conflicting apps and drop out + # hard if there are any and they don't want to merge + conflicts = loader.detect_conflicts() + if conflicts and not self.merge: + name_str = "; ".join( + "%s in %s" % (", ".join(names), app) + for app, names in conflicts.items() + ) + raise CommandError("Conflicting migrations detected (%s).\nTo fix them run 'python manage.py makemigrations --merge'" % name_str) + + # If they want to merge and there's nothing to merge, then politely exit + if self.merge and not conflicts: + self.stdout.write("No conflicts detected to merge.") + return + + # If they want to merge and there is something to merge, then + # divert into the merge code + if self.merge and conflicts: + return self.handle_merge(loader, conflicts) + # Detect changes autodetector = MigrationAutodetector( loader.graph.project_state(), @@ -87,3 +113,68 @@ class Command(BaseCommand): migration_string = writer.as_string() with open(writer.path, "wb") as fh: fh.write(migration_string) + + def handle_merge(self, loader, conflicts): + """ + Handles merging together conflicted migrations interactively, + if it's safe; otherwise, advises on how to fix it. + """ + if self.interactive: + questioner = InteractiveMigrationQuestioner() + else: + questioner = MigrationQuestioner() + for app_label, migration_names in conflicts.items(): + # Grab out the migrations in question, and work out their + # common ancestor. + merge_migrations = [] + for migration_name in migration_names: + migration = loader.get_migration(app_label, migration_name) + migration.ancestry = loader.graph.forwards_plan((app_label, migration_name)) + merge_migrations.append(migration) + common_ancestor = None + for level in zip(*[m.ancestry for m in merge_migrations]): + if reduce(operator.eq, level): + common_ancestor = level[0] + else: + break + if common_ancestor is None: + raise ValueError("Could not find common ancestor of %s" % migration_names) + # Now work out the operations along each divergent branch + for migration in merge_migrations: + migration.branch = migration.ancestry[ + (migration.ancestry.index(common_ancestor) + 1): + ] + migration.merged_operations = [] + for node_app, node_name in migration.branch: + migration.merged_operations.extend( + loader.get_migration(node_app, node_name).operations + ) + # In future, this could use some of the Optimizer code + # (can_optimize_through) to automatically see if they're + # mergeable. For now, we always just prompt the user. + if self.verbosity > 0: + self.stdout.write(self.style.MIGRATE_HEADING("Merging %s" % app_label)) + for migration in merge_migrations: + self.stdout.write(self.style.MIGRATE_LABEL(" Branch %s" % migration.name)) + for operation in migration.merged_operations: + self.stdout.write(" - %s\n" % operation.describe()) + if questioner.ask_merge(app_label): + # If they still want to merge it, then write out an empty + # file depending on the migrations needing merging. + numbers = [ + MigrationAutodetector.parse_number(migration.name) + for migration in merge_migrations + ] + try: + biggest_number = max([x for x in numbers if x is not None]) + except ValueError: + biggest_number = 1 + subclass = type("Migration", (migrations.Migration, ), { + "dependencies": [(app_label, migration.name) for migration in merge_migrations], + }) + new_migration = subclass("%04i_merge" % (biggest_number + 1), app_label) + writer = MigrationWriter(new_migration) + with open(writer.path, "wb") as fh: + fh.write(writer.as_string()) + if self.verbosity > 0: + self.stdout.write("\nCreated new merge migration %s" % writer.path) diff --git a/django/core/management/commands/migrate.py b/django/core/management/commands/migrate.py index 3e796b655f..093c8a79d0 100644 --- a/django/core/management/commands/migrate.py +++ b/django/core/management/commands/migrate.py @@ -62,6 +62,16 @@ class Command(BaseCommand): # Work out which apps have migrations and which do not executor = MigrationExecutor(connection, self.migration_progress_callback) + # Before anything else, see if there's conflicting apps and drop out + # hard if there are any + conflicts = executor.loader.detect_conflicts() + if conflicts: + name_str = "; ".join( + "%s in %s" % (", ".join(names), app) + for app, names in conflicts.items() + ) + raise CommandError("Conflicting migrations detected (%s).\nTo fix them run 'python manage.py makemigrations --merge'" % name_str) + # If they supplied command line arguments, work out what they mean. run_syncdb = False target_app_labels_only = True diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py index f8ec49ab0a..86ddd3c3d2 100644 --- a/django/db/migrations/autodetector.py +++ b/django/db/migrations/autodetector.py @@ -1,13 +1,8 @@ -import importlib -import os import re -import sys from django.db.migrations import operations from django.db.migrations.migration import Migration -from django.db.models.loading import cache -from django.utils import datetime_safe -from django.utils.six.moves import input +from django.db.migrations.questioner import MigrationQuestioner class MigrationAutodetector(object): @@ -369,108 +364,3 @@ class MigrationAutodetector(object): if re.match(r"^\d+_", name): return int(name.split("_")[0]) return None - - -class MigrationQuestioner(object): - """ - Gives the autodetector responses to questions it might have. - This base class has a built-in noninteractive mode, but the - interactive subclass is what the command-line arguments will use. - """ - - def __init__(self, defaults=None): - self.defaults = defaults or {} - - def ask_initial(self, app_label): - "Should we create an initial migration for the app?" - return self.defaults.get("ask_initial", False) - - def ask_not_null_addition(self, field_name, model_name): - "Adding a NOT NULL field to a model" - # None means quit - return None - - def ask_rename(self, model_name, old_name, new_name, field_instance): - "Was this field really renamed?" - return self.defaults.get("ask_rename", False) - - -class InteractiveMigrationQuestioner(MigrationQuestioner): - - def __init__(self, specified_apps=set()): - self.specified_apps = specified_apps - - def _boolean_input(self, question, default=None): - result = input("%s " % question) - if not result and default is not None: - return default - while len(result) < 1 or result[0].lower() not in "yn": - result = input("Please answer yes or no: ") - return result[0].lower() == "y" - - def _choice_input(self, question, choices): - print(question) - for i, choice in enumerate(choices): - print(" %s) %s" % (i + 1, choice)) - result = input("Select an option: ") - while True: - try: - value = int(result) - if 0 < value <= len(choices): - return value - except ValueError: - pass - result = input("Please select a valid option: ") - - def ask_initial(self, app_label): - "Should we create an initial migration for the app?" - # If it was specified on the command line, definitely true - if app_label in self.specified_apps: - return True - # Otherwise, we look to see if it has a migrations module - # without any Python files in it, apart from __init__.py. - # Apps from the new app template will have these; the python - # file check will ensure we skip South ones. - models_module = cache.get_app(app_label) - migrations_import_path = "%s.migrations" % models_module.__package__ - try: - migrations_module = importlib.import_module(migrations_import_path) - except ImportError: - return False - else: - filenames = os.listdir(os.path.dirname(migrations_module.__file__)) - return not any(x.endswith(".py") for x in filenames if x != "__init__.py") - - def ask_not_null_addition(self, field_name, model_name): - "Adding a NOT NULL field to a model" - choice = self._choice_input( - "You are trying to add a non-nullable field '%s' to %s without a default;\n" % (field_name, model_name) + - "we can't do that (the database needs something to populate existing rows).\n" + - "Please select a fix:", - [ - "Provide a one-off default now (will be set on all existing rows)", - "Quit, and let me add a default in models.py", - ] - ) - if choice == 2: - sys.exit(3) - else: - print("Please enter the default value now, as valid Python") - print("The datetime module is available, so you can do e.g. datetime.date.today()") - while True: - code = input(">>> ") - if not code: - print("Please enter some code, or 'exit' (with no quotes) to exit.") - elif code == "exit": - sys.exit(1) - else: - try: - return eval(code, {}, {"datetime": datetime_safe}) - except (SyntaxError, NameError) as e: - print("Invalid input: %s" % e) - else: - break - - def ask_rename(self, model_name, old_name, new_name, field_instance): - "Was this field really renamed?" - return self._boolean_input("Did you rename %s.%s to %s.%s (a %s)? [y/N]" % (model_name, old_name, model_name, new_name, field_instance.__class__.__name__), False) diff --git a/django/db/migrations/loader.py b/django/db/migrations/loader.py index 598c582fa0..f2510b8368 100644 --- a/django/db/migrations/loader.py +++ b/django/db/migrations/loader.py @@ -187,6 +187,20 @@ class MigrationLoader(object): for parent in migration.dependencies: self.graph.add_dependency(key, parent) + def detect_conflicts(self): + """ + Looks through the loaded graph and detects any conflicts - apps + with more than one leaf migration. Returns a dict of the app labels + that conflict with the migration names that conflict. + """ + seen_apps = {} + conflicting_apps = set() + for app_label, migration_name in self.graph.leaf_nodes(): + if app_label in seen_apps: + conflicting_apps.add(app_label) + seen_apps.setdefault(app_label, set()).add(migration_name) + return dict((app_label, seen_apps[app_label]) for app_label in conflicting_apps) + class BadMigrationError(Exception): """ diff --git a/django/db/migrations/questioner.py b/django/db/migrations/questioner.py new file mode 100644 index 0000000000..2ae74d4ef6 --- /dev/null +++ b/django/db/migrations/questioner.py @@ -0,0 +1,122 @@ +import importlib +import os +import sys + +from django.db.models.loading import cache +from django.utils import datetime_safe +from django.utils.six.moves import input +from django.core.exceptions import ImproperlyConfigured + + +class MigrationQuestioner(object): + """ + Gives the autodetector responses to questions it might have. + This base class has a built-in noninteractive mode, but the + interactive subclass is what the command-line arguments will use. + """ + + def __init__(self, defaults=None, specified_apps=None): + self.defaults = defaults or {} + self.specified_apps = specified_apps or set() + + def ask_initial(self, app_label): + "Should we create an initial migration for the app?" + # If it was specified on the command line, definitely true + if app_label in self.specified_apps: + return True + # Otherwise, we look to see if it has a migrations module + # without any Python files in it, apart from __init__.py. + # Apps from the new app template will have these; the python + # file check will ensure we skip South ones. + try: + models_module = cache.get_app(app_label) + except ImproperlyConfigured: # It's a fake app + return self.defaults.get("ask_initial", False) + migrations_import_path = "%s.migrations" % models_module.__package__ + try: + migrations_module = importlib.import_module(migrations_import_path) + except ImportError: + return self.defaults.get("ask_initial", False) + else: + filenames = os.listdir(os.path.dirname(migrations_module.__file__)) + return not any(x.endswith(".py") for x in filenames if x != "__init__.py") + + def ask_not_null_addition(self, field_name, model_name): + "Adding a NOT NULL field to a model" + # None means quit + return None + + def ask_rename(self, model_name, old_name, new_name, field_instance): + "Was this field really renamed?" + return self.defaults.get("ask_rename", False) + + def ask_merge(self, app_label): + "Do you really want to merge these migrations?" + return self.defaults.get("ask_merge", False) + + +class InteractiveMigrationQuestioner(MigrationQuestioner): + + def _boolean_input(self, question, default=None): + result = input("%s " % question) + if not result and default is not None: + return default + while len(result) < 1 or result[0].lower() not in "yn": + result = input("Please answer yes or no: ") + return result[0].lower() == "y" + + def _choice_input(self, question, choices): + print(question) + for i, choice in enumerate(choices): + print(" %s) %s" % (i + 1, choice)) + result = input("Select an option: ") + while True: + try: + value = int(result) + if 0 < value <= len(choices): + return value + except ValueError: + pass + result = input("Please select a valid option: ") + + def ask_not_null_addition(self, field_name, model_name): + "Adding a NOT NULL field to a model" + choice = self._choice_input( + "You are trying to add a non-nullable field '%s' to %s without a default;\n" % (field_name, model_name) + + "we can't do that (the database needs something to populate existing rows).\n" + + "Please select a fix:", + [ + "Provide a one-off default now (will be set on all existing rows)", + "Quit, and let me add a default in models.py", + ] + ) + if choice == 2: + sys.exit(3) + else: + print("Please enter the default value now, as valid Python") + print("The datetime module is available, so you can do e.g. datetime.date.today()") + while True: + code = input(">>> ") + if not code: + print("Please enter some code, or 'exit' (with no quotes) to exit.") + elif code == "exit": + sys.exit(1) + else: + try: + return eval(code, {}, {"datetime": datetime_safe}) + except (SyntaxError, NameError) as e: + print("Invalid input: %s" % e) + else: + break + + def ask_rename(self, model_name, old_name, new_name, field_instance): + "Was this field really renamed?" + return self._boolean_input("Did you rename %s.%s to %s.%s (a %s)? [y/N]" % (model_name, old_name, model_name, new_name, field_instance.__class__.__name__), False) + + def ask_merge(self, app_label): + return self._boolean_input( + "\nMerging will only work if the operations printed above do not conflict\n" + + "with each other (working on different fields or models)\n" + + "Do you want to merge these migration branches? [y/N]", + False, + ) diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index a3db6e1e45..42e18c69cc 100644 --- a/tests/migrations/test_autodetector.py +++ b/tests/migrations/test_autodetector.py @@ -1,6 +1,7 @@ # encoding: utf8 from django.test import TestCase -from django.db.migrations.autodetector import MigrationAutodetector, MigrationQuestioner +from django.db.migrations.autodetector import MigrationAutodetector +from django.db.migrations.questioner import MigrationQuestioner from django.db.migrations.state import ProjectState, ModelState from django.db.migrations.graph import MigrationGraph from django.db import models @@ -63,7 +64,7 @@ class AutodetectorTests(TestCase): # Use project state to make a new migration change set before = self.make_project_state([]) after = self.make_project_state([self.author_empty, self.other_pony, self.other_stable, self.third_thing]) - autodetector = MigrationAutodetector(before, after, MigrationQuestioner({"ask_initial": True})) + autodetector = MigrationAutodetector(before, after, MigrationQuestioner(defaults={"ask_initial": True})) changes = autodetector._detect_changes() # Run through arrange_for_graph graph = MigrationGraph() diff --git a/tests/migrations/test_commands.py b/tests/migrations/test_commands.py index dbed522dd5..48fb68b03d 100644 --- a/tests/migrations/test_commands.py +++ b/tests/migrations/test_commands.py @@ -6,7 +6,7 @@ import copy import os import shutil -from django.core.management import call_command +from django.core.management import call_command, CommandError from django.db.models.loading import cache from django.test.utils import override_settings from django.utils import six @@ -72,6 +72,34 @@ class MigrateTests(MigrationTestBase): # Cleanup by unmigrating everything call_command("migrate", "migrations", "zero", verbosity=0) + @override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations_conflict"}) + def test_migrate_conflict_exit(self): + """ + Makes sure that migrate exits if it detects a conflict. + """ + with self.assertRaises(CommandError): + call_command("migrate", "migrations") + + @override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations_conflict"}) + def test_makemigrations_conflict_exit(self): + """ + Makes sure that makemigrations exits if it detects a conflict. + """ + with self.assertRaises(CommandError): + call_command("makemigrations") + + @override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations_conflict"}) + def test_makemigrations_merge_basic(self): + """ + Makes sure that makemigrations doesn't error if you ask for + merge mode with a conflict present. Doesn't test writing of the merge + file, as that requires temp directories. + """ + try: + call_command("makemigrations", merge=True, verbosity=0) + except CommandError: + self.fail("Makemigrations errored in merge mode with conflicts") + @override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations"}) def test_sqlmigrate(self): """ diff --git a/tests/migrations/test_migrations_conflict/0001_initial.py b/tests/migrations/test_migrations_conflict/0001_initial.py new file mode 100644 index 0000000000..344bebdfe3 --- /dev/null +++ b/tests/migrations/test_migrations_conflict/0001_initial.py @@ -0,0 +1,27 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + + operations = [ + + migrations.CreateModel( + "Author", + [ + ("id", models.AutoField(primary_key=True)), + ("name", models.CharField(max_length=255)), + ("slug", models.SlugField(null=True)), + ("age", models.IntegerField(default=0)), + ("silly_field", models.BooleanField(default=False)), + ], + ), + + migrations.CreateModel( + "Tribble", + [ + ("id", models.AutoField(primary_key=True)), + ("fluffy", models.BooleanField(default=True)), + ], + ) + + ] diff --git a/tests/migrations/test_migrations_conflict/0002_conflicting_second.py b/tests/migrations/test_migrations_conflict/0002_conflicting_second.py new file mode 100644 index 0000000000..15ea1f063a --- /dev/null +++ b/tests/migrations/test_migrations_conflict/0002_conflicting_second.py @@ -0,0 +1,17 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [("migrations", "0001_initial")] + + operations = [ + + migrations.CreateModel( + "Something", + [ + ("id", models.AutoField(primary_key=True)), + ], + ) + + ] diff --git a/tests/migrations/test_migrations_conflict/0002_second.py b/tests/migrations/test_migrations_conflict/0002_second.py new file mode 100644 index 0000000000..ace9a83347 --- /dev/null +++ b/tests/migrations/test_migrations_conflict/0002_second.py @@ -0,0 +1,24 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [("migrations", "0001_initial")] + + operations = [ + + migrations.DeleteModel("Tribble"), + + migrations.RemoveField("Author", "silly_field"), + + migrations.AddField("Author", "rating", models.IntegerField(default=0)), + + migrations.CreateModel( + "Book", + [ + ("id", models.AutoField(primary_key=True)), + ("author", models.ForeignKey("migrations.Author", null=True)), + ], + ) + + ] diff --git a/tests/migrations/test_migrations_conflict/__init__.py b/tests/migrations/test_migrations_conflict/__init__.py new file mode 100644 index 0000000000..e69de29bb2