makemigrations tries to create django_migrations in external database

[Jim Nicholls]

I have 2 databases set up. A database for Django to use and a read-only database connection to an external system that is outside of Django's control. I am using a database router to select between the two databases. My models for the external database all have managed = False.

In Django 1.9, I was successfully running makemigrations. In Django 1.10, makemigrations raises an error.

django.db.migrations.exceptions.MigrationSchemaMissing: Unable to create the django_migrations table (permission denied for schema external_db)

[Jim Nicholls]

Traceback of MigrationSchemaMissing

[Tim Graham]

I guess the regression might be due to 02ae5fd31a56ffb42feadb49c1f3870ba0a24869. Have you thought about how to test your patch?

[Jim Nicholls]

Replying to timgraham:

I guess the regression might be due to 02ae5fd31a56ffb42feadb49c1f3870ba0a24869. Have you thought about how to test your patch?

This is first time I've touched the code of Django itself. I'm not familiar with Django's tests. I'm happy to write tests.

Any guidance? Is testing the new behaviour of applied_migrations enough, or do I have to look bigger at the whole of makemigrations?

[Tim Graham]

I'm not sure if the fix is at the appropriate level. Catching the exception in recorder.py might hide it too much. It might be more appropriate to catch the exception in makemigrations.py. Is the exception coming from loader.check_consistent_history(connection) there?

[Jim Nicholls]

For each connection: loader.check_consistent_history calls recorder.MigrationRecorder.applied_migrations to get the set of migrations that have already been applied to the database on the other end of the connection. MigrationRecorder.applied_migrations calls MigrationRecorder.ensure_schema so that it can query the table underlying MigrationRecorder.Migration. It is MigrationRecorder.ensure_schema that catches DatabaseError and raises MigrationSchemaMissing in its place.

Conceptually MigrationRecorder.applied_migrations is a read-only operation with respect to the database, but nevertheless it does attempt write operations as an implementation side-effect.

I'm inexperienced in Django's code, so I'll lift catch the exception higher up the call stack if you think that's best.

My thoughts at catching the MigrationSchemaMissing exception within applied_migrations was along the following lines.

The existing usages of applied_migrations are:

• django.db.migrations.executor.MigrationsExecutor.check_replacements(self): doesn't handle MigrationSchemaMissing, copes with applied_migrations returning an empty set
• django.db.migrations.loader.MigrationLoader.build_graph(self): this code explicitly treats no connection as being equivalent to applied_migrations returning an empty set
• django.db.migrations.loader.MigrationLoader.check_consistent_history(self, connection): uses the result of applied_migrations in a for loop, so copes with applied_migrations returning an empty set

MigrationSchemaMissing is only raised in MigrationRecorder.ensure_schema.

MigrationSchemaMissing is only handled in django.core.management.core.management.base.BaseCommand.check_migrations, where it logs a message to stdout and then skips checking the migrations. However check_migrations doesn't directly or indirectly call applied_migrations in the try suite.

MigrationSchemaMissing is a subclass of DatabaseError, which has too many usages to consider.

Considering the existing usages above, and possible future usages, my thinking for catching MigrationSchemaMissing inside applied_migrations is:

• not being able to create django_migrations is not exceptional. I think my use-case and BaseCommand.check_migrations proves that
• MigrationSchemaMissing is indirectly raised in applied_migrations because of the implementation side-effect, not because of the semantics of applied_migrations. I think it is leaking an exception in a non-exceptional circumstance.
• I don't think a callee would usefully want to distinguish between a database with no migrations, where empty set returned, from a sitatuation of the django_migrations table failed to be created, where MigrationSchemaMissing is raised.
• requiring a callee to have to consider MigrationSchemaMissing being raise is onerous, when returning an empty set would most likely have the same effect in the callee's code.

[Jim Nicholls]

I think I've missed the obvious.

applied_migrations is calling ensure_schema just to avoid a DatabaseError when it calls Migration.objects.values_list. For my the call to ensure_schema has unwanted side-effect and causes a problem.

Maybe it would better to not call ensure_schema at all, but instead catch the DatabaseError that is going to happen if there is no ` table and then return an empty set.

MigrationRecorder.record_applied and MigrationRecorder.record_unapplied both call ensure_schema themselves. This would mean that the django_migrations table would be created at the point when a migration is actually to being applied. At that point, if the django_migrations table cannot be created, that is an exceptional circumstance.

[Jim Nicholls]

I'm wrong about django.core.management.core.management.base.BaseCommand.check_migrations not directly or indirectly calling applied_migrations in the try suite.

Catching MigrationSchemaMissing in applied_migrations causes admin_scripts.tests.ManageRunserver.test_readonly_database to fail. This is because check_migrations no longer outputs "Not checking migrations" and instead outputs the list of missing migrations.

Is this new behaviour of BaseCommand.check_migrations better?

[Jim Nicholls]

I've withdrawn the ​pull request. I'll submit a new pull request leaves MigrationRecorder as is and catches the exception higher up.

[Jim Nicholls]

Submitted new ​pull request.

[Tim Graham <timograham@…>]

In 76ab885:

Fixed #27054 -- Fixed makemigrations crash with a read-only database.

[Tim Graham <timograham@…>]

In 16f032e6:

[1.10.x] Fixed #27054 -- Fixed makemigrations crash with a read-only database.

Backport of 76ab8851181675a59425f9637bdbf3f2de95f6f1 from master

[GitHub <noreply@…>]

In 098c07a:

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.

[Tim Graham <timograham@…>]

In 3e913d2:

[1.10.x] 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.

Backport of 098c07a03286b5ed133102733e67a83061647ea0 from master

[MiddleFork]

#27110 breaks this fix in the case where a database is present in DATABASES but absent from DATABASE_ROUTERS.

In spite of #27110 testing for DATABASE_ROUTERS, makemigrations still using the connections in django.db.utils.ConnectionHandler (ie as populated from settings.DATABASES) rather than the routers in DATABASE_ROUTERS for the set of databases for which it attempts to prepare to migrate.

This simplified example should result in makemigrations failing:

DATABASES = {
  'default': { ... } 
  'read_only': { ... }
  }


Class MyDefaultRouter():
   class Meta:


DATABASE_ROUTERS = ['MyDefaultRouter',]

As long as we explicitly specify our database by employing the .objects.using() syntax, the database need not be present in DATABASE_ROUTERS

class MyReadOnlyModel(models.Model):
  class Meta

queryset = MyReadOnlyModel.objects.using('read_only').all()

so at line 109 of makemigrations the call to loader.check_consistent_history(connection) on our read-only database, and will again raise the applied_migrations / ensure_schema traceback

Traceback (most recent call last):
  File "manage.py", line 11, in <module>
    execute_from_command_line(sys.argv)
  File "venv/lib/python2.7/site-packages/django/core/management/__init__.py", line 367, in execute_from_command_line
    utility.execute()
  File "venv/lib/python2.7/site-packages/django/core/management/__init__.py", line 359, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "venv/lib/python2.7/site-packages/django/core/management/base.py", line 294, in run_from_argv
    self.execute(*args, **cmd_options)
  File "venv/lib/python2.7/site-packages/django/core/management/base.py", line 345, in execute
    output = self.handle(*args, **options)
  File "venv/lib/python2.7/site-packages/django/core/management/commands/makemigrations.py", line 109, in handle
    loader.check_consistent_history(connection)
  File "venv/lib/python2.7/site-packages/django/db/migrations/loader.py", line 276, in check_consistent_history
    applied = recorder.applied_migrations()
  File "venv/lib/python2.7/site-packages/django/db/migrations/recorder.py", line 65, in applied_migrations
    self.ensure_schema()
  File "venv/lib/python2.7/site-packages/django/db/migrations/recorder.py", line 59, in ensure_schema
    raise MigrationSchemaMissing("Unable to create the django_migrations table (%s)" % exc)
django.db.migrations.exceptions.MigrationSchemaMissing: Unable to create the django_migrations table ((1142, "CREATE command denied to user 'readonly_user'@'192.168.1.1' for table 'django_migrations'"))

[Tim Graham]

You need to add an allow_migrate() method to your router.

[MiddleFork]

Replying to Tim Graham:

You need to add an allow_migrate() method to your router.

OK, I can see that now.

Still, it's unfortunate that we need to identify
read-only databases in this manner, which if I understand correctly is like this:

class MyRouter():
  def allow_migrate(self, db):
    if db in ['read_only0', 'read_only1', 'another_read_only_database_with_a_very_long_name']:
      return False

Something like this might be fun:

DATABASES = {
    'default' : { 
       'HOST': 'localhost',
       'NAME': 'database0' },  
    'read_only': {
      'NAME': 'database1',
      'HOST': 'localhost',
      'OPTIONS': 
        {'read_only': True,}
     
    }
}

Then,

not DATABASES['default'].get('OPTIONS').get('read_only')

will return True,
and

not DATABASES['read_only'].get('OPTIONS').get('read_only')

will return False,

so back in makemigrations we can check:

# 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 any(
                    # At least one model must be migrated to the database.
                    router.allow_migrate(connection.alias, app_label, model_name=model._meta.object_name)
                    for app_label in consistency_check_labels
                    for model in apps.get_app_config(app_label).get_models()
                    # The database must not be read-only    
                    and not connections.settings_dict.get('OPTIONS').get('read_only') 
            )):
                loader.check_consistent_history(connection)

​https://github.com/MiddleFork/django/commit/36641fca286a9687f6b1b4ff5605f2c0bc5565a2#diff-8eb3b5673aa9386f8fcb2cd99a8ebce3

On further thought, the use of the term read_only is loaded and maybe OPTIONSmigratable would be preferable

Anyway, it might be fun.

[Tim Graham]

I haven't used routers much myself, but I think the current structure provides enough flexibility that adding on "shortcuts" like that would only further complicate things.