Skip to content

Commit

Permalink
Fixed #27858 -- Prevented read-only management commands from creating…
Browse files Browse the repository at this point in the history
… the django_migrations table.

MigrationRecorder now assumes that if the django_migrations table
doesn't exist, then no migrations are applied.

Reverted documentation change from refs #23808.
  • Loading branch information
intgr authored and timgraham committed Jun 19, 2017
1 parent 4f1eb64 commit fda55c7
Show file tree
Hide file tree
Showing 7 changed files with 30 additions and 32 deletions.
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,7 @@ answer newbie questions, and generally made Django that much better:
Markus Amalthea Magnuson <markus.magnuson@gmail.com>
Markus Holtermann <https://markusholtermann.eu>
Marten Kenbeek <marten.knbk+django@gmail.com>
Marti Raudsepp <marti@juffo.org>
martin.glueck@gmail.com
Martin Green
Martin Kosír <martin@martinkosir.net>
Expand Down
6 changes: 0 additions & 6 deletions django/core/management/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
from django.core.exceptions import ImproperlyConfigured
from django.core.management.color import color_style, no_style
from django.db import DEFAULT_DB_ALIAS, connections
from django.db.migrations.exceptions import MigrationSchemaMissing


class CommandError(Exception):
Expand Down Expand Up @@ -429,11 +428,6 @@ def check_migrations(self):
except ImproperlyConfigured:
# No databases are configured (or the dummy one)
return
except MigrationSchemaMissing:
self.stdout.write(self.style.NOTICE(
"\nNot checking migrations as it is not possible to access/create the django_migrations table."
))
return

plan = executor.migration_plan(executor.loader.graph.leaf_nodes())
if plan:
Expand Down
4 changes: 4 additions & 0 deletions django/db/migrations/executor.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ def migrate(self, targets, plan=None, state=None, fake=False, fake_initial=False
Django first needs to create all project states before a migration is
(un)applied and in a second step run all the database operations.
"""
# The django_migrations table must be present to record applied
# migrations.
self.recorder.ensure_schema()

if plan is None:
plan = self.migration_plan(targets)
# Create the forwards plan Django would follow on an empty database
Expand Down
14 changes: 11 additions & 3 deletions django/db/migrations/recorder.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,15 @@ def __init__(self, connection):
def migration_qs(self):
return self.Migration.objects.using(self.connection.alias)

def has_table(self):
"""Return True if the django_migrations table exists."""
return self.Migration._meta.db_table in self.connection.introspection.table_names(self.connection.cursor())

def ensure_schema(self):
"""Ensure the table exists and has the correct schema."""
# If the table's there, that's fine - we've never changed its schema
# in the codebase.
if self.Migration._meta.db_table in self.connection.introspection.table_names(self.connection.cursor()):
if self.has_table():
return
# Make the table
try:
Expand All @@ -54,8 +58,12 @@ def ensure_schema(self):

def applied_migrations(self):
"""Return a set of (app, name) of applied migrations."""
self.ensure_schema()
return {tuple(x) for x in self.migration_qs.values_list("app", "name")}
if self.has_table():
return {tuple(x) for x in self.migration_qs.values_list('app', 'name')}
else:
# If the django_migrations table doesn't eixst, then no migrations
# are applied.
return set()

def record_applied(self, app, name):
"""Record that a migration was applied."""
Expand Down
3 changes: 0 additions & 3 deletions docs/ref/django-admin.txt
Original file line number Diff line number Diff line change
Expand Up @@ -876,9 +876,6 @@ If the :doc:`staticfiles</ref/contrib/staticfiles>` contrib app is enabled
(default in new projects) the :djadmin:`runserver` command will be overridden
with its own :ref:`runserver<staticfiles-runserver>` command.

If :djadmin:`migrate` was not previously executed, the table that stores the
history of migrations is created at first run of ``runserver``.

Logging of each request and response of the server is sent to the
:ref:`django-server-logger` logger.

Expand Down
12 changes: 4 additions & 8 deletions tests/admin_scripts/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
BaseCommand, CommandError, call_command, color,
)
from django.db import ConnectionHandler
from django.db.migrations.exceptions import MigrationSchemaMissing
from django.db.migrations.recorder import MigrationRecorder
from django.test import (
LiveServerTestCase, SimpleTestCase, TestCase, override_settings,
Expand Down Expand Up @@ -1339,15 +1338,12 @@ def test_no_database(self):

def test_readonly_database(self):
"""
Ensure runserver.check_migrations doesn't choke when a database is read-only
(with possibly no django_migrations table).
runserver.check_migrations() doesn't choke when a database is read-only.
"""
with mock.patch.object(
MigrationRecorder, 'ensure_schema',
side_effect=MigrationSchemaMissing()):
with mock.patch.object(MigrationRecorder, 'has_table', return_value=False):
self.cmd.check_migrations()
# Check a warning is emitted
self.assertIn("Not checking migrations", self.output.getvalue())
# You have # ...
self.assertIn('unapplied migration(s)', self.output.getvalue())


class ManageRunserverMigrationWarning(TestCase):
Expand Down
22 changes: 10 additions & 12 deletions tests/migrations/test_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,7 @@
ConnectionHandler, DatabaseError, connection, connections, models,
)
from django.db.backends.base.schema import BaseDatabaseSchemaEditor
from django.db.migrations.exceptions import (
InconsistentMigrationHistory, MigrationSchemaMissing,
)
from django.db.migrations.exceptions import InconsistentMigrationHistory
from django.db.migrations.recorder import MigrationRecorder
from django.test import override_settings

Expand Down Expand Up @@ -697,35 +695,35 @@ def test_makemigrations_consistency_checks_respect_routers(self):
The history consistency checks in makemigrations respect
settings.DATABASE_ROUTERS.
"""
def patched_ensure_schema(migration_recorder):
def patched_has_table(migration_recorder):
if migration_recorder.connection is connections['other']:
raise MigrationSchemaMissing('Patched')
raise Exception('Other connection')
else:
return mock.DEFAULT

self.assertTableNotExists('migrations_unicodemodel')
apps.register_model('migrations', UnicodeModel)
with mock.patch.object(
MigrationRecorder, 'ensure_schema',
autospec=True, side_effect=patched_ensure_schema) as ensure_schema:
MigrationRecorder, 'has_table',
autospec=True, side_effect=patched_has_table) as has_table:
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
self.assertEqual(has_table.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
self.assertEqual(has_table.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'):
with self.assertRaisesMessage(Exception, 'Other connection'):
call_command('makemigrations', 'migrations', verbosity=0)
self.assertEqual(ensure_schema.call_count, 4) # 'default' and 'other'
self.assertEqual(has_table.call_count, 4) # 'default' and 'other'

# With a router that doesn't allow migrating on any database,
# no consistency checks are made.
Expand All @@ -741,7 +739,7 @@ def patched_ensure_schema(migration_recorder):
self.assertIn(connection_alias, ['default', 'other'])
# Raises an error if invalid app_name/model_name occurs.
apps.get_app_config(app_name).get_model(call_kwargs['model_name'])
self.assertEqual(ensure_schema.call_count, 4)
self.assertEqual(has_table.call_count, 4)

def test_failing_migration(self):
# If a migration fails to serialize, it shouldn't generate an empty file. #21280
Expand Down

0 comments on commit fda55c7

Please sign in to comment.