From e1427cc609fa6ab247501b101cfb3c0092aba55b Mon Sep 17 00:00:00 2001 From: Markus Holtermann Date: Sat, 22 Aug 2015 23:40:34 +1000 Subject: [PATCH] Fixed #24590 -- Cached calls to swappable_setting. Moved the lookup in Field.swappable_setting to Apps, and added an lru_cache to cache the results. Refs #24743 Thanks Marten Kenbeek for the initial work on the patch. Thanks Aymeric Augustin and Tim Graham for the review. --- django/apps/registry.py | 22 ++++++++++++++++++ django/db/models/fields/related.py | 13 ++--------- django/db/models/options.py | 2 +- django/test/utils.py | 12 +++++++++- tests/field_deconstruction/tests.py | 36 ++++++++++++++++------------- 5 files changed, 56 insertions(+), 29 deletions(-) diff --git a/django/apps/registry.py b/django/apps/registry.py index 269787545d..46c10ef4fb 100644 --- a/django/apps/registry.py +++ b/django/apps/registry.py @@ -260,6 +260,28 @@ class Apps(object): "Model '%s.%s' not registered." % (app_label, model_name)) return model + @lru_cache.lru_cache(maxsize=None) + def get_swappable_settings_name(self, to_string): + """ + For a given model string (e.g. "auth.User"), return the name of the + corresponding settings name if it refers to a swappable model. If the + referred model is not swappable, return None. + + This method is decorated with lru_cache because it's performance + critical when it comes to migrations. Since the swappable settings don't + change after Django has loaded the settings, there is no reason to get + the respective settings attribute over and over again. + """ + for model in self.get_models(include_swapped=True): + swapped = model._meta.swapped + # Is this model swapped out for the model given by to_string? + if swapped and swapped == to_string: + return model._meta.swappable + # Is this model swappable and the one given by to_string? + if model._meta.swappable and model._meta.label == to_string: + return model._meta.swappable + return None + def set_available_apps(self, available): """ Restricts the set of installed apps used by get_app_config[s]. diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index f61e557aac..c433ce239d 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -314,17 +314,8 @@ class RelatedField(Field): if isinstance(self.remote_field.model, six.string_types): to_string = self.remote_field.model else: - to_string = "%s.%s" % ( - self.remote_field.model._meta.app_label, - self.remote_field.model._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 + to_string = self.remote_field.model._meta.label + return apps.get_swappable_settings_name(to_string) return None def set_attributes_from_rel(self): diff --git a/django/db/models/options.py b/django/db/models/options.py index 1406ab9441..bc5a95e750 100644 --- a/django/db/models/options.py +++ b/django/db/models/options.py @@ -396,7 +396,7 @@ class Options(object): # or as part of validation. return swapped_for - if '%s.%s' % (swapped_label, swapped_object.lower()) not in (None, self.label_lower): + if '%s.%s' % (swapped_label, swapped_object.lower()) != self.label_lower: return swapped_for return None diff --git a/django/test/utils.py b/django/test/utils.py index da77d8ef4e..415f17237b 100644 --- a/django/test/utils.py +++ b/django/test/utils.py @@ -29,7 +29,7 @@ except ImportError: __all__ = ( - 'Approximate', 'ContextList', 'get_runner', + 'Approximate', 'ContextList', 'isolate_lru_cache', 'get_runner', 'modify_settings', 'override_settings', 'requires_tz_support', 'setup_test_environment', 'teardown_test_environment', @@ -503,6 +503,16 @@ def extend_sys_path(*paths): sys.path = _orig_sys_path +@contextmanager +def isolate_lru_cache(lru_cache_object): + """Clear the cache of an LRU cache object on entering and exiting.""" + lru_cache_object.cache_clear() + try: + yield + finally: + lru_cache_object.cache_clear() + + @contextmanager def captured_output(stream_name): """Return a context manager used by captured_stdout/stdin/stderr diff --git a/tests/field_deconstruction/tests.py b/tests/field_deconstruction/tests.py index c1cc083fc5..5e04552ff4 100644 --- a/tests/field_deconstruction/tests.py +++ b/tests/field_deconstruction/tests.py @@ -1,7 +1,9 @@ from __future__ import unicode_literals +from django.apps import apps from django.db import models from django.test import SimpleTestCase, override_settings +from django.test.utils import isolate_lru_cache from django.utils import six @@ -219,14 +221,15 @@ class FieldDeconstructionTests(SimpleTestCase): @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", models.CASCADE) - name, path, args, kwargs = field.deconstruct() - self.assertEqual(path, "django.db.models.ForeignKey") - self.assertEqual(args, []) - self.assertEqual(kwargs, {"to": "auth.Permission", "on_delete": models.CASCADE}) - self.assertEqual(kwargs['to'].setting_name, "AUTH_USER_MODEL") + with isolate_lru_cache(apps.get_swappable_settings_name): + # 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", models.CASCADE) + name, path, args, kwargs = field.deconstruct() + self.assertEqual(path, "django.db.models.ForeignKey") + self.assertEqual(args, []) + self.assertEqual(kwargs, {"to": "auth.Permission", "on_delete": models.CASCADE}) + 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") @@ -297,14 +300,15 @@ class FieldDeconstructionTests(SimpleTestCase): @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") + with isolate_lru_cache(apps.get_swappable_settings_name): + # 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()