From 098c07a03286b5ed133102733e67a83061647ea0 Mon Sep 17 00:00:00 2001 From: Tim Graham Date: Thu, 1 Sep 2016 16:19:29 -0400 Subject: [PATCH] Fixed #27142, #27110 -- Made makemigrations consistency checks respect database routers. Partially reverted refs #27054 except for one of the tests as this solution supersedes that one. Thanks Shai Berger for the review. --- .../management/commands/makemigrations.py | 14 +++++--- django/db/migrations/loader.py | 8 +---- docs/releases/1.10.1.txt | 4 ++- tests/migrations/routers.py | 4 +++ tests/migrations/test_commands.py | 33 ++++++++++++++++--- tests/migrations/test_loader.py | 14 ++------ 6 files changed, 48 insertions(+), 29 deletions(-) diff --git a/django/core/management/commands/makemigrations.py b/django/core/management/commands/makemigrations.py index f9f50cc2ad..2fb2d713ca 100644 --- a/django/core/management/commands/makemigrations.py +++ b/django/core/management/commands/makemigrations.py @@ -4,8 +4,9 @@ import warnings from itertools import takewhile from django.apps import apps +from django.conf import settings from django.core.management.base import BaseCommand, CommandError -from django.db import connections +from django.db import DEFAULT_DB_ALIAS, connections, router from django.db.migrations import Migration from django.db.migrations.autodetector import MigrationAutodetector from django.db.migrations.loader import MigrationLoader @@ -94,9 +95,14 @@ class Command(BaseCommand): loader = MigrationLoader(None, ignore_no_migrations=True) # Raise an error if any migrations are applied before their dependencies. - for db in connections: - connection = connections[db] - if connection.settings_dict['ENGINE'] != 'django.db.backends.dummy': + consistency_check_labels = set(config.label for config in apps.get_app_configs()) + # Non-default databases are only checked if database routers used. + aliases_to_check = connections if settings.DATABASE_ROUTERS else [DEFAULT_DB_ALIAS] + for alias in sorted(aliases_to_check): + connection = connections[alias] + if (connection.settings_dict['ENGINE'] != 'django.db.backends.dummy' and + # At least one app must be migrated to the database. + any(router.allow_migrate(connection.alias, label) for label in consistency_check_labels)): loader.check_consistent_history(connection) # Before anything else, see if there's conflicting apps and drop out diff --git a/django/db/migrations/loader.py b/django/db/migrations/loader.py index c78aaed573..ce6fd9e3c0 100644 --- a/django/db/migrations/loader.py +++ b/django/db/migrations/loader.py @@ -6,7 +6,6 @@ from importlib import import_module from django.apps import apps from django.conf import settings -from django.db.migrations.exceptions import MigrationSchemaMissing from django.db.migrations.graph import MigrationGraph from django.db.migrations.recorder import MigrationRecorder from django.utils import six @@ -280,12 +279,7 @@ class MigrationLoader(object): unapplied dependencies. """ recorder = MigrationRecorder(connection) - try: - applied = recorder.applied_migrations() - except MigrationSchemaMissing: - # Skip check if the django_migrations table is missing and can't be - # created. - return + applied = recorder.applied_migrations() for migration in applied: # If the migration is unknown, skip it. if migration not in self.graph.nodes: diff --git a/docs/releases/1.10.1.txt b/docs/releases/1.10.1.txt index 6dbf40de02..80313a92b5 100644 --- a/docs/releases/1.10.1.txt +++ b/docs/releases/1.10.1.txt @@ -55,7 +55,9 @@ Bugfixes * Reallowed the ``{% for %}`` tag to unpack any iterable (:ticket:`27058`). -* Fixed ``makemigrations`` crash if a database is read-only (:ticket:`27054`). +* Made ``makemigrations`` skip inconsistent history checks on non-default + databases if database routers aren't in use or if no apps can be migrated + to the database (:ticket:`27054`, :ticket:`27110`, :ticket:`27142`). * Removed duplicated managers in ``Model._meta.managers`` (:ticket:`27073`). diff --git a/tests/migrations/routers.py b/tests/migrations/routers.py index 8970d9d681..9857363937 100644 --- a/tests/migrations/routers.py +++ b/tests/migrations/routers.py @@ -1,3 +1,7 @@ +class EmptyRouter(object): + pass + + class TestRouter(object): def allow_migrate(self, db, app_label, model_name=None, **hints): """ diff --git a/tests/migrations/test_commands.py b/tests/migrations/test_commands.py index dca1a46780..7ff09de84e 100644 --- a/tests/migrations/test_commands.py +++ b/tests/migrations/test_commands.py @@ -22,6 +22,7 @@ from django.utils.deprecation import RemovedInDjango20Warning from django.utils.encoding import force_text from .models import UnicodeModel, UnserializableModel +from .routers import TestRouter from .test_base import MigrationTestBase @@ -597,14 +598,14 @@ class MakeMigrationsTests(MigrationTestBase): init_file = os.path.join(migration_dir, '__init__.py') self.assertTrue(os.path.exists(init_file)) - def test_makemigrations_other_datbase_is_readonly(self): + def test_makemigrations_consistency_checks_respect_routers(self): """ - makemigrations ignores the non-default database if it's read-only. + The history consistency checks in makemigrations respect + settings.DATABASE_ROUTERS. """ def patched_ensure_schema(migration_recorder): - from django.db import connections if migration_recorder.connection is connections['other']: - raise MigrationSchemaMissing() + raise MigrationSchemaMissing('Patched') else: return mock.DEFAULT @@ -612,11 +613,33 @@ class MakeMigrationsTests(MigrationTestBase): apps.register_model('migrations', UnicodeModel) with mock.patch.object( MigrationRecorder, 'ensure_schema', - autospec=True, side_effect=patched_ensure_schema): + autospec=True, side_effect=patched_ensure_schema) as ensure_schema: with self.temporary_migration_module() as migration_dir: call_command("makemigrations", "migrations", verbosity=0) initial_file = os.path.join(migration_dir, "0001_initial.py") self.assertTrue(os.path.exists(initial_file)) + self.assertEqual(ensure_schema.call_count, 1) # 'default' is checked + + # Router says not to migrate 'other' so consistency shouldn't + # be checked. + with self.settings(DATABASE_ROUTERS=['migrations.routers.TestRouter']): + call_command('makemigrations', 'migrations', verbosity=0) + self.assertEqual(ensure_schema.call_count, 2) # 'default' again + + # With a router that doesn't prohibit migrating 'other', + # consistency is checked. + with self.settings(DATABASE_ROUTERS=['migrations.routers.EmptyRouter']): + with self.assertRaisesMessage(MigrationSchemaMissing, 'Patched'): + call_command('makemigrations', 'migrations', verbosity=0) + self.assertEqual(ensure_schema.call_count, 4) # 'default' and 'other' + + # With a router that doesn't allow migrating on any database, + # no consistency checks are made. + with self.settings(DATABASE_ROUTERS=['migrations.routers.TestRouter']): + with mock.patch.object(TestRouter, 'allow_migrate', return_value=False) as allow_migrate: + call_command('makemigrations', 'migrations', verbosity=0) + allow_migrate.assert_called_with('other', 'migrations') + self.assertEqual(ensure_schema.call_count, 4) def test_failing_migration(self): # If a migration fails to serialize, it shouldn't generate an empty file. #21280 diff --git a/tests/migrations/test_loader.py b/tests/migrations/test_loader.py index 5b31fcbd2c..05edd7f1e0 100644 --- a/tests/migrations/test_loader.py +++ b/tests/migrations/test_loader.py @@ -4,12 +4,11 @@ from unittest import skipIf from django.db import connection, connections from django.db.migrations.exceptions import ( - AmbiguityError, InconsistentMigrationHistory, MigrationSchemaMissing, - NodeNotFoundError, + AmbiguityError, InconsistentMigrationHistory, NodeNotFoundError, ) from django.db.migrations.loader import MigrationLoader from django.db.migrations.recorder import MigrationRecorder -from django.test import TestCase, mock, modify_settings, override_settings +from django.test import TestCase, modify_settings, override_settings from django.utils import six @@ -484,12 +483,3 @@ class LoaderTests(TestCase): ('app1', '4_auto'), } self.assertEqual(plan, expected_plan) - - def test_readonly_database(self): - """ - check_consistent_history() ignores read-only databases, possibly - without a django_migrations table. - """ - with mock.patch.object(MigrationRecorder, 'ensure_schema', side_effect=MigrationSchemaMissing()): - loader = MigrationLoader(connection=None) - loader.check_consistent_history(connection)