Skip to content

Commit

Permalink
Replaced ad-hoc caching of get_models with lru_cache.
Browse files Browse the repository at this point in the history
Invalidate properly the cache whenever all_models or app_configs change.
This fixes some isolation issues in the test suite.
  • Loading branch information
aaugustin committed Dec 24, 2013
1 parent 2ec8e34 commit 82a35e2
Show file tree
Hide file tree
Showing 8 changed files with 15 additions and 27 deletions.
17 changes: 7 additions & 10 deletions django/apps/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

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

Expand Down Expand Up @@ -49,9 +50,6 @@ def __init__(self, master=False):
# 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.
Expand Down Expand Up @@ -83,6 +81,7 @@ def populate_apps(self, installed_apps=None):
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):
Expand Down Expand Up @@ -131,6 +130,7 @@ def populate_models(self):

del self._postponed

self.get_models.cache_clear()
self._models_loaded = True

def app_cache_ready(self):
Expand Down Expand Up @@ -180,6 +180,8 @@ def get_app_config(self, app_label, only_with_models_module=False):
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):
Expand All @@ -206,12 +208,7 @@ def get_models(self, app_mod=None,
"""
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]
Expand All @@ -235,7 +232,6 @@ def get_models(self, app_mod=None,
(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):
Expand Down Expand Up @@ -272,7 +268,7 @@ def register_model(self, app_label, model):
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):
"""
Expand Down Expand Up @@ -368,6 +364,7 @@ def load_app(self, app_name):
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):
Expand Down
2 changes: 1 addition & 1 deletion tests/app_loading/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def setUp(self):

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

Expand Down
6 changes: 3 additions & 3 deletions tests/managers_regress/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ class Meta:
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):
Expand Down Expand Up @@ -156,7 +156,7 @@ class Meta:
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):
Expand Down Expand Up @@ -184,7 +184,7 @@ class Meta:
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()
Expand Down
2 changes: 1 addition & 1 deletion tests/migrations/test_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ def setUp(self):
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:
Expand Down
2 changes: 1 addition & 1 deletion tests/proxy_models/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ class Meta:
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")
Expand Down
1 change: 1 addition & 0 deletions tests/runtests.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ def no_available_apps(self):
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

Expand Down
10 changes: 0 additions & 10 deletions tests/swappable_models/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion tests/tablespaces/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def tearDown(self):

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)
Expand Down

0 comments on commit 82a35e2

Please sign in to comment.