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.

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

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:

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`).
 

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):
         """

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

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)