From c9de1b4a55713ad1557f8c40621226253038f665 Mon Sep 17 00:00:00 2001 From: Andrew Godwin Date: Wed, 15 Jan 2014 14:20:47 +0000 Subject: [PATCH] Implement swappable model support for migrations --- django/db/migrations/__init__.py | 2 +- django/db/migrations/autodetector.py | 21 ++++++++-- django/db/migrations/loader.py | 2 +- django/db/migrations/migration.py | 7 ++++ django/db/migrations/writer.py | 27 +++++++++++- django/db/models/fields/related.py | 60 ++++++++++++++++++++++++++- docs/ref/models/fields.txt | 34 +++++++++++++++ tests/field_deconstruction/tests.py | 44 +++++++++++++++++++- tests/migrations/test_autodetector.py | 16 ++++++- tests/migrations/test_writer.py | 16 +++++-- 10 files changed, 216 insertions(+), 13 deletions(-) diff --git a/django/db/migrations/__init__.py b/django/db/migrations/__init__.py index e006bcbabc..e7f3d2d768 100644 --- a/django/db/migrations/__init__.py +++ b/django/db/migrations/__init__.py @@ -1,2 +1,2 @@ -from .migration import Migration # NOQA +from .migration import Migration, swappable_dependency # NOQA from .operations import * # NOQA diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py index c7ac756b2e..df49088a62 100644 --- a/django/db/migrations/autodetector.py +++ b/django/db/migrations/autodetector.py @@ -105,8 +105,12 @@ class MigrationAutodetector(object): ) ) for field_name, other_app_label, other_model_name in related_fields: - if app_label != other_app_label: - self.add_dependency(app_label, other_app_label) + # If it depends on a swappable something, add a dynamic depend'cy + swappable_setting = new_apps.get_model(app_label, model_name)._meta.get_field_by_name(field_name)[0].swappable_setting + if swappable_setting is not None: + self.add_swappable_dependency(app_label, swappable_setting) + elif app_label != other_app_label: + self.add_dependency(app_label, other_app_label) del pending_add[app_label, model_name] # Ah well, we'll need to split one. Pick deterministically. else: @@ -140,7 +144,11 @@ class MigrationAutodetector(object): ), new=True, ) - if app_label != other_app_label: + # If it depends on a swappable something, add a dynamic depend'cy + swappable_setting = new_apps.get_model(app_label, model_name)._meta.get_field_by_name(field_name)[0].swappable_setting + if swappable_setting is not None: + self.add_swappable_dependency(app_label, swappable_setting) + elif app_label != other_app_label: self.add_dependency(app_label, other_app_label) # Removing models removed_models = set(old_model_keys) - set(new_model_keys) @@ -276,6 +284,13 @@ class MigrationAutodetector(object): dependency = (other_app_label, "__first__") self.migrations[app_label][-1].dependencies.append(dependency) + def add_swappable_dependency(self, app_label, setting_name): + """ + Adds a dependency to the value of a swappable model setting. + """ + dependency = ("__setting__", setting_name) + self.migrations[app_label][-1].dependencies.append(dependency) + def _arrange_for_graph(self, changes, graph): """ Takes in a result from changes() and a MigrationGraph, diff --git a/django/db/migrations/loader.py b/django/db/migrations/loader.py index 73f9d2a71e..5bc6318c8c 100644 --- a/django/db/migrations/loader.py +++ b/django/db/migrations/loader.py @@ -223,7 +223,7 @@ class MigrationLoader(object): self.graph.add_node(parent, new_migration) self.applied_migrations.add(parent) elif parent[0] in self.migrated_apps: - parent = (parent[0], list(self.graph.root_nodes(parent[0]))[0]) + parent = list(self.graph.root_nodes(parent[0]))[0] else: raise ValueError("Dependency on unknown app %s" % parent[0]) self.graph.add_dependency(key, parent) diff --git a/django/db/migrations/migration.py b/django/db/migrations/migration.py index c0ed1a9564..6bbfce2dfb 100644 --- a/django/db/migrations/migration.py +++ b/django/db/migrations/migration.py @@ -127,3 +127,10 @@ class Migration(object): to_run.reverse() for operation, to_state, from_state in to_run: operation.database_backwards(self.app_label, schema_editor, from_state, to_state) + + +def swappable_dependency(value): + """ + Turns a setting value into a dependency. + """ + return (value.split(".", 1)[0], "__first__") diff --git a/django/db/migrations/writer.py b/django/db/migrations/writer.py index 549c72ed60..e7d82e488b 100644 --- a/django/db/migrations/writer.py +++ b/django/db/migrations/writer.py @@ -13,6 +13,20 @@ from django.utils.functional import Promise from django.utils import six +class SettingsReference(str): + """ + Special subclass of string which actually references a current settings + value. It's treated as the value in memory, but serializes out to a + settings.NAME attribute reference. + """ + + def __new__(self, value, setting_name): + return str.__new__(self, value) + + def __init__(self, value, setting_name): + self.setting_name = setting_name + + class MigrationWriter(object): """ Takes a Migration instance and is able to produce the contents @@ -27,7 +41,6 @@ class MigrationWriter(object): Returns a string of the file contents. """ items = { - "dependencies": repr(self.migration.dependencies), "replaces_str": "", } imports = set() @@ -46,6 +59,15 @@ class MigrationWriter(object): arg_strings.append("%s = %s" % (kw, arg_string)) operation_strings.append("migrations.%s(%s\n )" % (name, "".join("\n %s," % arg for arg in arg_strings))) items["operations"] = "[%s\n ]" % "".join("\n %s," % s for s in operation_strings) + # Format dependencies and write out swappable dependencies right + items["dependencies"] = "[" + for dependency in self.migration.dependencies: + if dependency[0] == "__setting__": + items["dependencies"] += "\n migrations.swappable_dependency(settings.%s)," % dependency[1] + imports.add("from django.conf import settings") + else: + items["dependencies"] += "\n %s," % repr(dependency) + items["dependencies"] += "\n ]" # Format imports nicely imports.discard("from django.db import models") if not imports: @@ -136,6 +158,9 @@ class MigrationWriter(object): # Datetimes elif isinstance(value, (datetime.datetime, datetime.date)): return repr(value), set(["import datetime"]) + # Settings references + elif isinstance(value, SettingsReference): + return "settings.%s" % value.setting_name, set(["from django.conf import settings"]) # Simple types elif isinstance(value, six.integer_types + (float, six.binary_type, six.text_type, bool, type(None))): return repr(value), set() diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 69fb3f8492..56a74da67e 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -16,6 +16,7 @@ from django.utils.deprecation import RenameMethodsBase from django.utils.translation import ugettext_lazy as _ from django.utils.functional import curry, cached_property from django.core import exceptions +from django.apps import apps from django import forms RECURSIVE_RELATIONSHIP_CONSTANT = 'self' @@ -121,6 +122,30 @@ class RelatedField(Field): else: self.do_related_class(other, cls) + @property + def swappable_setting(self): + """ + Gets the setting that this is powered from for swapping, or None + if it's not swapped in / marked with swappable=False. + """ + if self.swappable: + # Work out string form of "to" + if isinstance(self.rel.to, six.string_types): + to_string = self.rel.to + else: + to_string = "%s.%s" % ( + self.rel.to._meta.app_label, + self.rel.to._meta.object_name, + ) + # See if anything swapped/swappable matches + for model in apps.get_models(include_swapped=True): + if model._meta.swapped: + if model._meta.swapped == to_string: + return model._meta.swappable + if ("%s.%s" % (model._meta.app_label, model._meta.object_name)) == to_string and model._meta.swappable: + return model._meta.swappable + return None + def set_attributes_from_rel(self): self.name = self.name or (self.rel.to._meta.model_name + '_' + self.rel.to._meta.pk.name) if self.verbose_name is None: @@ -1061,9 +1086,10 @@ class ForeignObject(RelatedField): generate_reverse_relation = True related_accessor_class = ForeignRelatedObjectsDescriptor - def __init__(self, to, from_fields, to_fields, **kwargs): + def __init__(self, to, from_fields, to_fields, swappable=True, **kwargs): self.from_fields = from_fields self.to_fields = to_fields + self.swappable = swappable if 'rel' not in kwargs: kwargs['rel'] = ForeignObjectRel( @@ -1082,10 +1108,25 @@ class ForeignObject(RelatedField): name, path, args, kwargs = super(ForeignObject, self).deconstruct() kwargs['from_fields'] = self.from_fields kwargs['to_fields'] = self.to_fields + # Work out string form of "to" if isinstance(self.rel.to, six.string_types): kwargs['to'] = self.rel.to else: kwargs['to'] = "%s.%s" % (self.rel.to._meta.app_label, self.rel.to._meta.object_name) + # If swappable is True, then see if we're actually pointing to the target + # of a swap. + swappable_setting = self.swappable_setting + if swappable_setting is not None: + # If it's already a settings reference, error + if hasattr(kwargs['to'], "setting_name"): + if kwargs['to'].setting_name != swappable_setting: + raise ValueError("Cannot deconstruct a ForeignKey pointing to a model that is swapped in place of more than one model (%s and %s)" % (kwargs['to'].setting_name, swappable_setting)) + # Set it + from django.db.migrations.writer import SettingsReference + kwargs['to'] = SettingsReference( + kwargs['to'], + swappable_setting, + ) return name, path, args, kwargs def resolve_related_fields(self): @@ -1516,7 +1557,7 @@ def create_many_to_many_intermediary_model(field, klass): class ManyToManyField(RelatedField): description = _("Many-to-many relationship") - def __init__(self, to, db_constraint=True, **kwargs): + def __init__(self, to, db_constraint=True, swappable=True, **kwargs): try: assert not to._meta.abstract, "%s cannot define a relation with abstract class %s" % (self.__class__.__name__, to._meta.object_name) except AttributeError: # to._meta doesn't exist, so it must be RECURSIVE_RELATIONSHIP_CONSTANT @@ -1534,6 +1575,7 @@ class ManyToManyField(RelatedField): db_constraint=db_constraint, ) + self.swappable = swappable self.db_table = kwargs.pop('db_table', None) if kwargs['rel'].through is not None: assert self.db_table is None, "Cannot specify a db_table if an intermediary model is used." @@ -1552,6 +1594,20 @@ class ManyToManyField(RelatedField): kwargs['to'] = self.rel.to else: kwargs['to'] = "%s.%s" % (self.rel.to._meta.app_label, self.rel.to._meta.object_name) + # If swappable is True, then see if we're actually pointing to the target + # of a swap. + swappable_setting = self.swappable_setting + if swappable_setting is not None: + # If it's already a settings reference, error + if hasattr(kwargs['to'], "setting_name"): + if kwargs['to'].setting_name != swappable_setting: + raise ValueError("Cannot deconstruct a ManyToManyField pointing to a model that is swapped in place of more than one model (%s and %s)" % (kwargs['to'].setting_name, swappable_setting)) + # Set it + from django.db.migrations.writer import SettingsReference + kwargs['to'] = SettingsReference( + kwargs['to'], + swappable_setting, + ) return name, path, args, kwargs def _get_path_info(self, direct=False): diff --git a/docs/ref/models/fields.txt b/docs/ref/models/fields.txt index 2f62a6448b..22fadd26c3 100644 --- a/docs/ref/models/fields.txt +++ b/docs/ref/models/fields.txt @@ -1201,6 +1201,23 @@ The possible values for :attr:`~ForeignKey.on_delete` are found in you manually add an SQL ``ON DELETE`` constraint to the database field (perhaps using :ref:`initial sql`). +.. attribute:: ForeignKey.swappable + + .. versionadded:: 1.7 + + Controls the migration framework's reaction if this :class:`ForeignKey` + is pointing at a swappable model. If it is ``True`` - the default - + then if the :class:`ForeignKey` is pointing at a model which matches + the current value of ``settings.AUTH_USER_MODEL`` (or another swappable + model setting) the relationship will be stored in the migration using + a reference to the setting, not to the model directly. + + You only want to override this to be ``False`` if you are sure your + model should always point towards the swapped-in model - for example, + if it is a profile model designed specifically for your custom user model. + + If in doubt, leave it to its default of ``True``. + .. _ref-manytomany: ``ManyToManyField`` @@ -1309,6 +1326,23 @@ that control how the relationship functions. It is an error to pass both ``db_constraint`` and ``through``. +.. attribute:: ManyToManyField.swappable + + .. versionadded:: 1.7 + + Controls the migration framework's reaction if this :class:`ManyToManyField` + is pointing at a swappable model. If it is ``True`` - the default - + then if the :class:`ManyToManyField` is pointing at a model which matches + the current value of ``settings.AUTH_USER_MODEL`` (or another swappable + model setting) the relationship will be stored in the migration using + a reference to the setting, not to the model directly. + + You only want to override this to be ``False`` if you are sure your + model should always point towards the swapped-in model - for example, + if it is a profile model designed specifically for your custom user model. + + If in doubt, leave it to its default of ``True``. + .. _ref-onetoone: diff --git a/tests/field_deconstruction/tests.py b/tests/field_deconstruction/tests.py index cb78003318..3fea7ae853 100644 --- a/tests/field_deconstruction/tests.py +++ b/tests/field_deconstruction/tests.py @@ -1,5 +1,5 @@ import warnings -from django.test import TestCase +from django.test import TestCase, override_settings from django.db import models @@ -149,22 +149,44 @@ class FieldDeconstructionTests(TestCase): self.assertEqual(kwargs, {}) def test_foreign_key(self): + # Test basic pointing + field = models.ForeignKey("auth.Permission") + name, path, args, kwargs = field.deconstruct() + self.assertEqual(path, "django.db.models.ForeignKey") + self.assertEqual(args, []) + self.assertEqual(kwargs, {"to": "auth.Permission"}) + self.assertFalse(hasattr(kwargs['to'], "setting_name")) + # Test swap detection for swappable model field = models.ForeignKey("auth.User") name, path, args, kwargs = field.deconstruct() self.assertEqual(path, "django.db.models.ForeignKey") self.assertEqual(args, []) self.assertEqual(kwargs, {"to": "auth.User"}) + self.assertEqual(kwargs['to'].setting_name, "AUTH_USER_MODEL") + # Test nonexistent (for now) model field = models.ForeignKey("something.Else") name, path, args, kwargs = field.deconstruct() self.assertEqual(path, "django.db.models.ForeignKey") self.assertEqual(args, []) self.assertEqual(kwargs, {"to": "something.Else"}) + # Test on_delete field = models.ForeignKey("auth.User", on_delete=models.SET_NULL) name, path, args, kwargs = field.deconstruct() self.assertEqual(path, "django.db.models.ForeignKey") self.assertEqual(args, []) self.assertEqual(kwargs, {"to": "auth.User", "on_delete": models.SET_NULL}) + @override_settings(AUTH_USER_MODEL="auth.Permission") + def test_foreign_key_swapped(self): + # It doesn't matter that we swapped out user for permission; + # there's no validation. We just want to check the setting stuff works. + field = models.ForeignKey("auth.Permission") + name, path, args, kwargs = field.deconstruct() + self.assertEqual(path, "django.db.models.ForeignKey") + self.assertEqual(args, []) + self.assertEqual(kwargs, {"to": "auth.Permission"}) + self.assertEqual(kwargs['to'].setting_name, "AUTH_USER_MODEL") + def test_image_field(self): field = models.ImageField(upload_to="foo/barness", width_field="width", height_field="height") name, path, args, kwargs = field.deconstruct() @@ -201,11 +223,31 @@ class FieldDeconstructionTests(TestCase): self.assertEqual(kwargs, {"protocol": "IPv6"}) def test_many_to_many_field(self): + # Test normal + field = models.ManyToManyField("auth.Permission") + name, path, args, kwargs = field.deconstruct() + self.assertEqual(path, "django.db.models.ManyToManyField") + self.assertEqual(args, []) + self.assertEqual(kwargs, {"to": "auth.Permission"}) + self.assertFalse(hasattr(kwargs['to'], "setting_name")) + # Test swappable field = models.ManyToManyField("auth.User") name, path, args, kwargs = field.deconstruct() self.assertEqual(path, "django.db.models.ManyToManyField") self.assertEqual(args, []) self.assertEqual(kwargs, {"to": "auth.User"}) + self.assertEqual(kwargs['to'].setting_name, "AUTH_USER_MODEL") + + @override_settings(AUTH_USER_MODEL="auth.Permission") + def test_many_to_many_field_swapped(self): + # It doesn't matter that we swapped out user for permission; + # there's no validation. We just want to check the setting stuff works. + field = models.ManyToManyField("auth.Permission") + name, path, args, kwargs = field.deconstruct() + self.assertEqual(path, "django.db.models.ManyToManyField") + self.assertEqual(args, []) + self.assertEqual(kwargs, {"to": "auth.Permission"}) + self.assertEqual(kwargs['to'].setting_name, "AUTH_USER_MODEL") def test_null_boolean_field(self): field = models.NullBooleanField() diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index 42e18c69cc..013a0d2287 100644 --- a/tests/migrations/test_autodetector.py +++ b/tests/migrations/test_autodetector.py @@ -1,5 +1,5 @@ # encoding: utf8 -from django.test import TestCase +from django.test import TestCase, override_settings from django.db.migrations.autodetector import MigrationAutodetector from django.db.migrations.questioner import MigrationQuestioner from django.db.migrations.state import ProjectState, ModelState @@ -18,6 +18,7 @@ class AutodetectorTests(TestCase): author_name_renamed = ModelState("testapp", "Author", [("id", models.AutoField(primary_key=True)), ("names", models.CharField(max_length=200))]) author_with_book = ModelState("testapp", "Author", [("id", models.AutoField(primary_key=True)), ("name", models.CharField(max_length=200)), ("book", models.ForeignKey("otherapp.Book"))]) author_with_publisher = ModelState("testapp", "Author", [("id", models.AutoField(primary_key=True)), ("name", models.CharField(max_length=200)), ("publisher", models.ForeignKey("testapp.Publisher"))]) + author_with_custom_user = ModelState("testapp", "Author", [("id", models.AutoField(primary_key=True)), ("name", models.CharField(max_length=200)), ("user", models.ForeignKey("thirdapp.CustomUser"))]) author_proxy = ModelState("testapp", "AuthorProxy", [], {"proxy": True}, ("testapp.author", )) author_proxy_notproxy = ModelState("testapp", "AuthorProxy", [], {}, ("testapp.author", )) publisher = ModelState("testapp", "Publisher", [("id", models.AutoField(primary_key=True)), ("name", models.CharField(max_length=100))]) @@ -29,6 +30,7 @@ class AutodetectorTests(TestCase): book_unique = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))], {"unique_together": [("author", "title")]}) book_unique_2 = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))], {"unique_together": [("title", "author")]}) edition = ModelState("thirdapp", "Edition", [("id", models.AutoField(primary_key=True)), ("book", models.ForeignKey("otherapp.Book"))]) + custom_user = ModelState("thirdapp", "CustomUser", [("id", models.AutoField(primary_key=True)), ("username", models.CharField(max_length=255))]) def make_project_state(self, model_states): "Shortcut to make ProjectStates from lists of predefined models" @@ -355,3 +357,15 @@ class AutodetectorTests(TestCase): action = migration.operations[0] self.assertEqual(action.__class__.__name__, "CreateModel") self.assertEqual(action.name, "AuthorProxy") + + @override_settings(AUTH_USER_MODEL="thirdapp.CustomUser") + def test_swappable(self): + before = self.make_project_state([self.custom_user]) + after = self.make_project_state([self.custom_user, self.author_with_custom_user]) + autodetector = MigrationAutodetector(before, after) + changes = autodetector._detect_changes() + # Right number of migrations? + self.assertEqual(len(changes), 1) + # Check the dependency is correct + migration = changes['testapp'][0] + self.assertEqual(migration.dependencies, [("__setting__", "AUTH_USER_MODEL")]) diff --git a/tests/migrations/test_writer.py b/tests/migrations/test_writer.py index 42e6c4356d..425f1bd48a 100644 --- a/tests/migrations/test_writer.py +++ b/tests/migrations/test_writer.py @@ -7,8 +7,9 @@ import os from django.core.validators import RegexValidator, EmailValidator from django.db import models, migrations -from django.db.migrations.writer import MigrationWriter +from django.db.migrations.writer import MigrationWriter, SettingsReference from django.test import TestCase +from django.conf import settings from django.utils import six from django.utils.deconstruct import deconstructible from django.utils.translation import ugettext_lazy as _ @@ -37,8 +38,8 @@ class WriterTests(TestCase): def assertSerializedEqual(self, value): self.assertEqual(self.serialize_round_trip(value), value) - def assertSerializedIs(self, value): - self.assertIs(self.serialize_round_trip(value), value) + def assertSerializedResultEqual(self, value, target): + self.assertEqual(MigrationWriter.serialize(value), target) def assertSerializedFieldEqual(self, value): new_value = self.serialize_round_trip(value) @@ -92,6 +93,15 @@ class WriterTests(TestCase): # Django fields self.assertSerializedFieldEqual(models.CharField(max_length=255)) self.assertSerializedFieldEqual(models.TextField(null=True, blank=True)) + # Setting references + self.assertSerializedEqual(SettingsReference(settings.AUTH_USER_MODEL, "AUTH_USER_MODEL")) + self.assertSerializedResultEqual( + SettingsReference("someapp.model", "AUTH_USER_MODEL"), + ( + "settings.AUTH_USER_MODEL", + set(["from django.conf import settings"]), + ) + ) def test_simple_migration(self): """