From 82a35e24d4de58e637661591b14d9b4658bcab11 Mon Sep 17 00:00:00 2001 From: Aymeric Augustin Date: Tue, 24 Dec 2013 10:08:04 +0100 Subject: [PATCH] Replaced ad-hoc caching of get_models with lru_cache. Invalidate properly the cache whenever all_models or app_configs change. This fixes some isolation issues in the test suite. --- django/apps/cache.py | 17 +++++++---------- tests/app_loading/tests.py | 2 +- tests/managers_regress/tests.py | 6 +++--- tests/migrations/test_commands.py | 2 +- tests/proxy_models/tests.py | 2 +- tests/runtests.py | 1 + tests/swappable_models/tests.py | 10 ---------- tests/tablespaces/tests.py | 2 +- 8 files changed, 15 insertions(+), 27 deletions(-) diff --git a/django/apps/cache.py b/django/apps/cache.py index 840b9fbf68..a67b3cf999 100644 --- a/django/apps/cache.py +++ b/django/apps/cache.py @@ -7,6 +7,7 @@ import warnings from django.conf import settings from django.core.exceptions import ImproperlyConfigured +from django.utils import lru_cache from django.utils.module_loading import import_lock from django.utils._os import upath @@ -49,9 +50,6 @@ class AppCache(object): # Pending lookups for lazy relations. self._pending_lookups = {} - # Cache for get_models. - self._get_models_cache = {} - def populate_apps(self, installed_apps=None): """ Populate app-related information. @@ -83,6 +81,7 @@ class AppCache(object): app_config = AppConfig.create(app_name) self.app_configs[app_config.label] = app_config + self.get_models.cache_clear() self._apps_loaded = True def populate_models(self): @@ -131,6 +130,7 @@ class AppCache(object): del self._postponed + self.get_models.cache_clear() self._models_loaded = True def app_cache_ready(self): @@ -180,6 +180,8 @@ class AppCache(object): raise LookupError("App with label %r doesn't have a models module." % app_label) return app_config + # This method is performance-critical at least for Django's test suite. + @lru_cache.lru_cache(maxsize=None) def get_models(self, app_mod=None, include_auto_created=False, include_deferred=False, only_installed=True, include_swapped=False): @@ -206,12 +208,7 @@ class AppCache(object): """ if not self.master: only_installed = False - cache_key = (app_mod, include_auto_created, include_deferred, only_installed, include_swapped) model_list = None - try: - return self._get_models_cache[cache_key] - except KeyError: - pass self.populate_models() if app_mod: app_label = app_mod.__name__.split('.')[-2] @@ -235,7 +232,6 @@ class AppCache(object): (not model._meta.auto_created or include_auto_created) and (not model._meta.swapped or include_swapped)) ) - self._get_models_cache[cache_key] = model_list return model_list def get_model(self, app_label, model_name, only_installed=True): @@ -272,7 +268,7 @@ class AppCache(object): if os.path.splitext(fname1)[0] == os.path.splitext(fname2)[0]: return models[model_name] = model - self._get_models_cache.clear() + self.get_models.cache_clear() def has_app(self, app_name): """ @@ -368,6 +364,7 @@ class AppCache(object): app_config = AppConfig.create(app_name) app_config.import_models(self.all_models[app_config.label]) self.app_configs[app_config.label] = app_config + self.get_models.cache_clear() return app_config.models_module def get_app(self, app_label): diff --git a/tests/app_loading/tests.py b/tests/app_loading/tests.py index a9dc70f4b1..291877108f 100644 --- a/tests/app_loading/tests.py +++ b/tests/app_loading/tests.py @@ -21,7 +21,7 @@ class EggLoadingTest(TestCase): def tearDown(self): app_cache.all_models['app_loading'] = self._old_models - app_cache._get_models_cache = {} + app_cache.get_models.cache_clear() sys.path = self.old_path diff --git a/tests/managers_regress/tests.py b/tests/managers_regress/tests.py index 2b8e183e82..bf8e5b17d4 100644 --- a/tests/managers_regress/tests.py +++ b/tests/managers_regress/tests.py @@ -128,7 +128,7 @@ class ManagersRegressionTests(TestCase): finally: app_cache.app_configs['managers_regress'].models = _old_models app_cache.all_models['managers_regress'] = _old_models - app_cache._get_models_cache = {} + app_cache.get_models.cache_clear() @override_settings(TEST_SWAPPABLE_MODEL='managers_regress.Parent') def test_custom_swappable_manager(self): @@ -156,7 +156,7 @@ class ManagersRegressionTests(TestCase): finally: app_cache.app_configs['managers_regress'].models = _old_models app_cache.all_models['managers_regress'] = _old_models - app_cache._get_models_cache = {} + app_cache.get_models.cache_clear() @override_settings(TEST_SWAPPABLE_MODEL='managers_regress.Parent') def test_explicit_swappable_manager(self): @@ -184,7 +184,7 @@ class ManagersRegressionTests(TestCase): finally: app_cache.app_configs['managers_regress'].models = _old_models app_cache.all_models['managers_regress'] = _old_models - app_cache._get_models_cache = {} + app_cache.get_models.cache_clear() def test_regress_3871(self): related = RelatedModel.objects.create() diff --git a/tests/migrations/test_commands.py b/tests/migrations/test_commands.py index 9f740f72a2..9fa9be6cbf 100644 --- a/tests/migrations/test_commands.py +++ b/tests/migrations/test_commands.py @@ -136,7 +136,7 @@ class MakeMigrationsTests(MigrationTestBase): def tearDown(self): app_cache.app_configs['migrations'].models = self._old_models app_cache.all_models['migrations'] = self._old_models - app_cache._get_models_cache = {} + app_cache.get_models.cache_clear() os.chdir(self.test_dir) try: diff --git a/tests/proxy_models/tests.py b/tests/proxy_models/tests.py index 346924e320..4a3d6e5e5a 100644 --- a/tests/proxy_models/tests.py +++ b/tests/proxy_models/tests.py @@ -175,7 +175,7 @@ class ProxyModelTests(TestCase): finally: app_cache.app_configs['proxy_models'].models = _old_models app_cache.all_models['proxy_models'] = _old_models - app_cache._get_models_cache = {} + app_cache.get_models.cache_clear() def test_myperson_manager(self): Person.objects.create(name="fred") diff --git a/tests/runtests.py b/tests/runtests.py index c75d8a3ee9..ab6f778a81 100755 --- a/tests/runtests.py +++ b/tests/runtests.py @@ -170,6 +170,7 @@ def setup(verbosity, test_labels): app_config = AppConfig.create(module_label) app_config.import_models(app_cache.all_models[app_config.label]) app_cache.app_configs[app_config.label] = app_config + app_cache.get_models.cache_clear() return state diff --git a/tests/swappable_models/tests.py b/tests/swappable_models/tests.py index cdebbef374..c703a99fcb 100644 --- a/tests/swappable_models/tests.py +++ b/tests/swappable_models/tests.py @@ -19,16 +19,6 @@ class SwappableModelTests(TestCase): 'django.contrib.contenttypes', ] - def setUp(self): - # This test modifies the installed apps, so we need to make sure - # we're not dealing with a cached app list. - app_cache._get_models_cache.clear() - - def tearDown(self): - # By fiddling with swappable models, we alter the installed models - # cache, so flush it to make sure there are no side effects. - app_cache._get_models_cache.clear() - @override_settings(TEST_ARTICLE_MODEL='swappable_models.AlternateArticle') def test_generated_data(self): "Permissions and content types are not created for a swapped model" diff --git a/tests/tablespaces/tests.py b/tests/tablespaces/tests.py index d761a93ab9..fdee877a9f 100644 --- a/tests/tablespaces/tests.py +++ b/tests/tablespaces/tests.py @@ -37,7 +37,7 @@ class TablespacesTests(TestCase): app_cache.app_configs['tablespaces'].models = self._old_models app_cache.all_models['tablespaces'] = self._old_models - app_cache._get_models_cache = {} + app_cache.get_models.cache_clear() def assertNumContains(self, haystack, needle, count): real_count = haystack.count(needle)