From fddc5957c53bd654312c4a238a8cdcfe5f4ef4cc Mon Sep 17 00:00:00 2001 From: Andrew Godwin Date: Tue, 30 Jul 2013 12:34:31 +0100 Subject: [PATCH] Implement allow_migrate for migration operations --- django/db/migrations/operations/fields.py | 46 ++++++++++------- django/db/migrations/operations/models.py | 61 +++++++++++++++-------- docs/releases/1.7.txt | 12 +++++ docs/topics/db/multi-db.txt | 10 +++- docs/topics/migrations.txt | 23 +++++++++ tests/migrations/test_operations.py | 48 +++++++++++++++++- 6 files changed, 156 insertions(+), 44 deletions(-) diff --git a/django/db/migrations/operations/fields.py b/django/db/migrations/operations/fields.py index 37e0c063e1..7c619d49ce 100644 --- a/django/db/migrations/operations/fields.py +++ b/django/db/migrations/operations/fields.py @@ -1,3 +1,4 @@ +from django.db import router from .base import Operation @@ -17,11 +18,13 @@ class AddField(Operation): def database_forwards(self, app_label, schema_editor, from_state, to_state): from_model = from_state.render().get_model(app_label, self.model_name) to_model = to_state.render().get_model(app_label, self.model_name) - schema_editor.add_field(from_model, to_model._meta.get_field_by_name(self.name)[0]) + if router.allow_migrate(schema_editor.connection.alias, to_model): + schema_editor.add_field(from_model, to_model._meta.get_field_by_name(self.name)[0]) def database_backwards(self, app_label, schema_editor, from_state, to_state): from_model = from_state.render().get_model(app_label, self.model_name) - schema_editor.remove_field(from_model, from_model._meta.get_field_by_name(self.name)[0]) + if router.allow_migrate(schema_editor.connection.alias, from_model): + schema_editor.remove_field(from_model, from_model._meta.get_field_by_name(self.name)[0]) def describe(self): return "Add field %s to %s" % (self.name, self.model_name) @@ -45,12 +48,14 @@ class RemoveField(Operation): def database_forwards(self, app_label, schema_editor, from_state, to_state): from_model = from_state.render().get_model(app_label, self.model_name) - schema_editor.remove_field(from_model, from_model._meta.get_field_by_name(self.name)[0]) + if router.allow_migrate(schema_editor.connection.alias, from_model): + schema_editor.remove_field(from_model, from_model._meta.get_field_by_name(self.name)[0]) def database_backwards(self, app_label, schema_editor, from_state, to_state): from_model = from_state.render().get_model(app_label, self.model_name) to_model = to_state.render().get_model(app_label, self.model_name) - schema_editor.add_field(from_model, to_model._meta.get_field_by_name(self.name)[0]) + if router.allow_migrate(schema_editor.connection.alias, to_model): + schema_editor.add_field(from_model, to_model._meta.get_field_by_name(self.name)[0]) def describe(self): return "Remove field %s from %s" % (self.name, self.model_name) @@ -74,11 +79,12 @@ class AlterField(Operation): def database_forwards(self, app_label, schema_editor, from_state, to_state): from_model = from_state.render().get_model(app_label, self.model_name) to_model = to_state.render().get_model(app_label, self.model_name) - schema_editor.alter_field( - from_model, - from_model._meta.get_field_by_name(self.name)[0], - to_model._meta.get_field_by_name(self.name)[0], - ) + if router.allow_migrate(schema_editor.connection.alias, to_model): + schema_editor.alter_field( + from_model, + from_model._meta.get_field_by_name(self.name)[0], + to_model._meta.get_field_by_name(self.name)[0], + ) def database_backwards(self, app_label, schema_editor, from_state, to_state): self.database_forwards(app_label, schema_editor, from_state, to_state) @@ -105,20 +111,22 @@ class RenameField(Operation): def database_forwards(self, app_label, schema_editor, from_state, to_state): from_model = from_state.render().get_model(app_label, self.model_name) to_model = to_state.render().get_model(app_label, self.model_name) - schema_editor.alter_field( - from_model, - from_model._meta.get_field_by_name(self.old_name)[0], - to_model._meta.get_field_by_name(self.new_name)[0], - ) + if router.allow_migrate(schema_editor.connection.alias, to_model): + schema_editor.alter_field( + from_model, + from_model._meta.get_field_by_name(self.old_name)[0], + to_model._meta.get_field_by_name(self.new_name)[0], + ) def database_backwards(self, app_label, schema_editor, from_state, to_state): from_model = from_state.render().get_model(app_label, self.model_name) to_model = to_state.render().get_model(app_label, self.model_name) - schema_editor.alter_field( - from_model, - from_model._meta.get_field_by_name(self.new_name)[0], - to_model._meta.get_field_by_name(self.old_name)[0], - ) + if router.allow_migrate(schema_editor.connection.alias, to_model): + schema_editor.alter_field( + from_model, + from_model._meta.get_field_by_name(self.new_name)[0], + to_model._meta.get_field_by_name(self.old_name)[0], + ) def describe(self): return "Rename field %s on %s to %s" % (self.old_name, self.model_name, self.new_name) diff --git a/django/db/migrations/operations/models.py b/django/db/migrations/operations/models.py index bf15201194..406efa6ef1 100644 --- a/django/db/migrations/operations/models.py +++ b/django/db/migrations/operations/models.py @@ -1,5 +1,5 @@ from .base import Operation -from django.db import models +from django.db import models, router from django.db.migrations.state import ModelState @@ -17,13 +17,17 @@ class CreateModel(Operation): def state_forwards(self, app_label, state): state.models[app_label, self.name.lower()] = ModelState(app_label, self.name, self.fields, self.options, self.bases) - def database_forwards(self, app, schema_editor, from_state, to_state): + def database_forwards(self, app_label, schema_editor, from_state, to_state): app_cache = to_state.render() - schema_editor.create_model(app_cache.get_model(app, self.name)) + model = app_cache.get_model(app_label, self.name) + if router.allow_migrate(schema_editor.connection.alias, model): + schema_editor.create_model(model) - def database_backwards(self, app, schema_editor, from_state, to_state): + def database_backwards(self, app_label, schema_editor, from_state, to_state): app_cache = from_state.render() - schema_editor.delete_model(app_cache.get_model(app, self.name)) + model = app_cache.get_model(app_label, self.name) + if router.allow_migrate(schema_editor.connection.alias, model): + schema_editor.delete_model(model) def describe(self): return "Create model %s" % (self.name, ) @@ -42,11 +46,15 @@ class DeleteModel(Operation): def database_forwards(self, app_label, schema_editor, from_state, to_state): app_cache = from_state.render() - schema_editor.delete_model(app_cache.get_model(app_label, self.name)) + model = app_cache.get_model(app_label, self.name) + if router.allow_migrate(schema_editor.connection.alias, model): + schema_editor.delete_model(model) def database_backwards(self, app_label, schema_editor, from_state, to_state): app_cache = to_state.render() - schema_editor.create_model(app_cache.get_model(app_label, self.name)) + model = app_cache.get_model(app_label, self.name) + if router.allow_migrate(schema_editor.connection.alias, model): + schema_editor.create_model(model) def describe(self): return "Delete model %s" % (self.name, ) @@ -67,11 +75,14 @@ class AlterModelTable(Operation): def database_forwards(self, app_label, schema_editor, from_state, to_state): old_app_cache = from_state.render() new_app_cache = to_state.render() - schema_editor.alter_db_table( - new_app_cache.get_model(app_label, self.name), - old_app_cache.get_model(app_label, self.name)._meta.db_table, - new_app_cache.get_model(app_label, self.name)._meta.db_table, - ) + old_model = old_app_cache.get_model(app_label, self.name) + new_model = new_app_cache.get_model(app_label, self.name) + if router.allow_migrate(schema_editor.connection.alias, new_model): + schema_editor.alter_db_table( + new_model, + old_model._meta.db_table, + new_model._meta.db_table, + ) def database_backwards(self, app_label, schema_editor, from_state, to_state): return self.database_forwards(app_label, schema_editor, from_state, to_state) @@ -97,11 +108,14 @@ class AlterUniqueTogether(Operation): def database_forwards(self, app_label, schema_editor, from_state, to_state): old_app_cache = from_state.render() new_app_cache = to_state.render() - schema_editor.alter_unique_together( - new_app_cache.get_model(app_label, self.name), - getattr(old_app_cache.get_model(app_label, self.name)._meta, "unique_together", set()), - getattr(new_app_cache.get_model(app_label, self.name)._meta, "unique_together", set()), - ) + old_model = old_app_cache.get_model(app_label, self.name) + new_model = new_app_cache.get_model(app_label, self.name) + if router.allow_migrate(schema_editor.connection.alias, new_model): + schema_editor.alter_unique_together( + new_model, + getattr(old_model._meta, "unique_together", set()), + getattr(new_model._meta, "unique_together", set()), + ) def database_backwards(self, app_label, schema_editor, from_state, to_state): return self.database_forwards(app_label, schema_editor, from_state, to_state) @@ -127,11 +141,14 @@ class AlterIndexTogether(Operation): def database_forwards(self, app_label, schema_editor, from_state, to_state): old_app_cache = from_state.render() new_app_cache = to_state.render() - schema_editor.alter_index_together( - new_app_cache.get_model(app_label, self.name), - getattr(old_app_cache.get_model(app_label, self.name)._meta, "index_together", set()), - getattr(new_app_cache.get_model(app_label, self.name)._meta, "index_together", set()), - ) + old_model = old_app_cache.get_model(app_label, self.name) + new_model = new_app_cache.get_model(app_label, self.name) + if router.allow_migrate(schema_editor.connection.alias, new_model): + schema_editor.alter_index_together( + new_model, + getattr(old_model._meta, "index_together", set()), + getattr(new_model._meta, "index_together", set()), + ) def database_backwards(self, app_label, schema_editor, from_state, to_state): return self.database_forwards(app_label, schema_editor, from_state, to_state) diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index df2b10d18c..a617c90b34 100644 --- a/docs/releases/1.7.txt +++ b/docs/releases/1.7.txt @@ -110,6 +110,18 @@ Backwards incompatible changes in 1.7 deprecation timeline for a given feature, its removal may appear as a backwards incompatible change. +allow_syncdb/allow_migrate +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +While Django will still look at ``allow_syncdb`` methods even though they +should be renamed to ``allow_migrate``, there is a subtle difference in which +models get passed to these methods. + +For apps with migrations, ``allow_migrate`` will now get passed +:ref:`historical models `, which are special versioned models +without custom attributes, methods or managers. Make sure your ``allow_migrate`` +methods are only referring to fields or other items in ``model._meta``. + Miscellaneous ~~~~~~~~~~~~~ diff --git a/docs/topics/db/multi-db.txt b/docs/topics/db/multi-db.txt index 6c74fb944d..6e19844b5c 100644 --- a/docs/topics/db/multi-db.txt +++ b/docs/topics/db/multi-db.txt @@ -163,8 +163,14 @@ A database Router is a class that provides up to four methods: the router has no opinion. This method can be used to determine the availability of a model on a given database. - Note that if this returns ``True`` for an app with migrations but - ``False`` for an app those migrations depend on, Django will error. + Note that migrations will just silently not perform any operations + on a model for which this returns ``False``. This may result in broken + ForeignKeys, extra tables or missing tables if you change it once you + have applied some migrations. + + The value passed for ``model`` may be a + :ref:`historical model `, and thus not have any + custom attributes, methods or managers. You should only rely on ``_meta``. A router doesn't have to provide *all* these methods -- it may omit one or more of them. If one of the methods is omitted, Django will skip diff --git a/docs/topics/migrations.txt b/docs/topics/migrations.txt index f80cbf81fd..5f7def7107 100644 --- a/docs/topics/migrations.txt +++ b/docs/topics/migrations.txt @@ -272,3 +272,26 @@ Note that this only works given two things: * You have not manually edited your database - Django won't be able to detect that your database doesn't match your models, you'll just get errors when migrations try and modify those tables. + + +.. historical-models: + +Historical models +----------------- + +When you run migrations, Django is working from historical versions of +your models stored in the migration files. If you write Python code +using the ``django.db.migrations.RunPython`` operation, or if you have +``allow_migrate`` methods on your database routers, you will be exposed +to these versions of your models. + +Because it's impossible to serialize arbitrary Python code, these historical +models will not have any custom methods or managers that you have defined. +They will, however, have the same fields, relationships and ``Meta`` options +(also versioned, so they may be different from your current ones). + +In addition, the base classes of the model are just stored as pointers, +so you must always keep base classes around for as long as there is a migration +that contains a reference to them. On the plus side, methods and managers +from these base classes inherit normally, so if you absolutely need access +to these you can opt to move them into a superclass. diff --git a/tests/migrations/test_operations.py b/tests/migrations/test_operations.py index ad909f7fdd..33b870a335 100644 --- a/tests/migrations/test_operations.py +++ b/tests/migrations/test_operations.py @@ -1,4 +1,4 @@ -from django.db import connection, models, migrations +from django.db import connection, models, migrations, router from django.db.transaction import atomic from django.db.utils import IntegrityError from django.db.migrations.state import ProjectState @@ -271,3 +271,49 @@ class OperationTests(MigrationTestBase): with connection.schema_editor() as editor: operation.database_backwards("test_alinto", editor, new_state, project_state) self.assertIndexNotExists("test_alinto_pony", ["pink", "weight"]) + + +class MigrateNothingRouter(object): + """ + A router that sends all writes to the other database. + """ + def allow_migrate(self, db, model): + return False + + +class MultiDBOperationTests(MigrationTestBase): + multi_db = True + + def setUp(self): + # Make the 'other' database appear to be a slave of the 'default' + self.old_routers = router.routers + router.routers = [MigrateNothingRouter()] + + def tearDown(self): + # Restore the 'other' database as an independent database + router.routers = self.old_routers + + def test_create_model(self): + """ + Tests that CreateModel honours multi-db settings. + """ + operation = migrations.CreateModel( + "Pony", + [ + ("id", models.AutoField(primary_key=True)), + ("pink", models.IntegerField(default=1)), + ], + ) + # Test the state alteration + project_state = ProjectState() + new_state = project_state.clone() + operation.state_forwards("test_crmo", new_state) + # Test the database alteration + self.assertTableNotExists("test_crmo_pony") + with connection.schema_editor() as editor: + operation.database_forwards("test_crmo", editor, project_state, new_state) + self.assertTableNotExists("test_crmo_pony") + # And test reversal + with connection.schema_editor() as editor: + operation.database_backwards("test_crmo", editor, new_state, project_state) + self.assertTableNotExists("test_crmo_pony")