From b55282b98bf7883490fcc586d501e1b36c7a1c9c Mon Sep 17 00:00:00 2001 From: Aymeric Augustin Date: Thu, 12 Dec 2013 21:34:39 +0100 Subject: [PATCH] Moved list of models inside AppConfig instances. This commit is a refactoring with no change of functionality, according to the following invariants: - An app_label that was in app_configs and app_models stays in app_config and has its 'installed' attribute set to True. - An app_label that was in app_models but not in app_configs is added to app_configs and has its 'installed' attribute set to True. As a consequence, all the code that iterated on app_configs is modified to check for the 'installed' attribute. Code that iterated on app_models is rewritten in terms of app_configs. Many tests that stored and restored the state of the app cache were updated. In the long term, we should reconsider the usefulness of allowing importing models from non-installed applications. This doesn't sound particularly useful, can be a trap in some circumstances, and causes significant complexity in sensitive areas of Django. --- django/apps/base.py | 16 ++++++- django/apps/cache.py | 58 +++++++++++++++----------- tests/app_loading/tests.py | 12 +++--- tests/invalid_models/tests.py | 10 ++--- tests/managers_regress/tests.py | 37 ++++++++-------- tests/migrations/test_commands.py | 5 +-- tests/migrations/test_operations.py | 1 - tests/proxy_model_inheritance/tests.py | 2 - tests/proxy_models/tests.py | 13 +++--- tests/tablespaces/tests.py | 6 +-- 10 files changed, 87 insertions(+), 73 deletions(-) diff --git a/django/apps/base.py b/django/apps/base.py index 37c16fb22f..588709be9f 100644 --- a/django/apps/base.py +++ b/django/apps/base.py @@ -1,11 +1,25 @@ +from collections import OrderedDict + + class AppConfig(object): """ Class representing a Django application and its configuration. """ - def __init__(self, label, models_module): + def __init__(self, label, models_module=None, installed=True): + # Last component of the Python path to the application eg. 'admin'. self.label = label + + # Module containing models eg. . self.models_module = models_module + # Mapping of lower case model names to model classes. + self.models = OrderedDict() + + # Whether the app is in INSTALLED_APPS or was automatically created + # when one of its models was imported. + self.installed = installed + def __repr__(self): return '' % self.label diff --git a/django/apps/cache.py b/django/apps/cache.py index 34909deeac..b3991e5e34 100644 --- a/django/apps/cache.py +++ b/django/apps/cache.py @@ -31,10 +31,6 @@ def _initialize(): # Mapping of labels to AppConfig instances for installed apps. app_configs=OrderedDict(), - # Mapping of app_labels to a dictionary of model names to model code. - # May contain apps that are not installed. - app_models=OrderedDict(), - # Pending lookups for lazy relations pending_lookups={}, @@ -138,9 +134,15 @@ class BaseAppCache(object): self.nesting_level -= 1 label = self._label_for(models_module) - if label not in self.app_configs: + try: + app_config = self.app_configs[label] + except KeyError: self.app_configs[label] = AppConfig( label=label, models_module=models_module) + else: + if not app_config.installed: + app_config.models_module = models_module + app_config.installed = True return models_module def app_cache_ready(self): @@ -161,7 +163,7 @@ class BaseAppCache(object): # app_configs is an OrderedDict, which ensures that the returned list # is always in the same order (with new apps added at the end). This # avoids unstable ordering on the admin app list page, for example. - apps = self.app_configs.items() + apps = [app for app in self.app_configs.items() if app[1].installed] if self.available_apps is not None: apps = [app for app in apps if app[0] in self.available_apps] return [app[1].models_module for app in apps] @@ -258,20 +260,20 @@ class BaseAppCache(object): self._populate() if app_mod: app_label = self._label_for(app_mod) - if app_label in self.app_configs: - app_list = [self.app_models.get(app_label, OrderedDict())] - else: + try: + app_config = self.app_configs[app_label] + except KeyError: app_list = [] - else: - if only_installed: - app_list = [self.app_models.get(app_label, OrderedDict()) - for app_label in self.app_configs] else: - app_list = six.itervalues(self.app_models) + app_list = [app_config] if app_config.installed else [] + else: + app_list = six.itervalues(self.app_configs) + if only_installed: + app_list = (app for app in app_list if app.installed) model_list = [] for app in app_list: model_list.extend( - model for model in app.values() + model for model in app.models.values() if ((not model._deferred or include_deferred) and (not model._meta.auto_created or include_auto_created) and (not model._meta.swapped or include_swapped)) @@ -296,13 +298,15 @@ class BaseAppCache(object): only_installed = False if seed_cache: self._populate() - if only_installed and app_label not in self.app_configs: - return None - if (self.available_apps is not None and only_installed - and app_label not in self.available_apps): - raise UnavailableApp("App with label %s isn't available." % app_label) + if only_installed: + app_config = self.app_configs.get(app_label) + if app_config is not None and not app_config.installed: + return None + if (self.available_apps is not None + and app_label not in self.available_apps): + raise UnavailableApp("App with label %s isn't available." % app_label) try: - return self.app_models[app_label][model_name.lower()] + return self.app_configs[app_label].models[model_name.lower()] except KeyError: return None @@ -310,11 +314,17 @@ class BaseAppCache(object): """ Register a set of models as belonging to an app. """ + if models: + try: + app_config = self.app_configs[app_label] + except KeyError: + app_config = AppConfig( + label=app_label, installed=False) + self.app_configs[app_label] = app_config for model in models: - # Store as 'name: model' pair in a dictionary - # in the app_models dictionary + # Add the model to the app_config's models dictionary. model_name = model._meta.model_name - model_dict = self.app_models.setdefault(app_label, OrderedDict()) + model_dict = app_config.models if model_name in model_dict: # The same model may be imported via different paths (e.g. # appname.models and project.appname.models). We use the source diff --git a/tests/app_loading/tests.py b/tests/app_loading/tests.py index b24522fab6..6909564040 100644 --- a/tests/app_loading/tests.py +++ b/tests/app_loading/tests.py @@ -1,6 +1,5 @@ from __future__ import unicode_literals -import copy import os import sys from unittest import TestCase @@ -17,14 +16,15 @@ class EggLoadingTest(TestCase): self.old_path = sys.path[:] self.egg_dir = '%s/eggs' % os.path.dirname(upath(__file__)) - # This test adds dummy applications to the app cache. These - # need to be removed in order to prevent bad interactions - # with the flush operation in other tests. - self.old_app_models = copy.deepcopy(app_cache.app_models) + # The models need to be removed after the test in order to prevent bad + # interactions with the flush operation in other tests. + self._old_models = app_cache.app_configs['app_loading'].models.copy() def tearDown(self): + app_cache.app_configs['app_loading'].models = self._old_models + app_cache._get_models_cache = {} + sys.path = self.old_path - app_cache.app_models = self.old_app_models def test_egg1(self): """Models module can be loaded from an app in an egg""" diff --git a/tests/invalid_models/tests.py b/tests/invalid_models/tests.py index abb7d9b33d..860d5e23a6 100644 --- a/tests/invalid_models/tests.py +++ b/tests/invalid_models/tests.py @@ -1,4 +1,3 @@ -import copy import sys import unittest @@ -19,13 +18,12 @@ class InvalidModelTestCase(unittest.TestCase): self.stdout = StringIO() sys.stdout = self.stdout - # This test adds dummy applications to the app cache. These - # need to be removed in order to prevent bad interactions - # with the flush operation in other tests. - self.old_app_models = copy.deepcopy(app_cache.app_models) + # The models need to be removed after the test in order to prevent bad + # interactions with the flush operation in other tests. + self._old_models = app_cache.app_configs['invalid_models'].models.copy() def tearDown(self): - app_cache.app_models = self.old_app_models + app_cache.app_configs['invalid_models'].models = self._old_models app_cache._get_models_cache = {} sys.stdout = self.old_stdout diff --git a/tests/managers_regress/tests.py b/tests/managers_regress/tests.py index de9f72c333..0c939bfd2e 100644 --- a/tests/managers_regress/tests.py +++ b/tests/managers_regress/tests.py @@ -1,5 +1,4 @@ from __future__ import unicode_literals -import copy from django.apps import app_cache from django.db import models @@ -110,12 +109,11 @@ class ManagersRegressionTests(TestCase): @override_settings(TEST_SWAPPABLE_MODEL='managers_regress.Parent') def test_swappable_manager(self): - try: - # This test adds dummy models to the app cache. These - # need to be removed in order to prevent bad interactions - # with the flush operation in other tests. - old_app_models = copy.deepcopy(app_cache.app_models) + # The models need to be removed after the test in order to prevent bad + # interactions with the flush operation in other tests. + _old_models = app_cache.app_configs['managers_regress'].models.copy() + try: class SwappableModel(models.Model): class Meta: swappable = 'TEST_SWAPPABLE_MODEL' @@ -129,16 +127,16 @@ class ManagersRegressionTests(TestCase): self.assertEqual(str(e), "Manager isn't available; SwappableModel has been swapped for 'managers_regress.Parent'") finally: - app_cache.app_models = old_app_models + app_cache.app_configs['managers_regress'].models = _old_models + app_cache._get_models_cache = {} @override_settings(TEST_SWAPPABLE_MODEL='managers_regress.Parent') def test_custom_swappable_manager(self): - try: - # This test adds dummy models to the app cache. These - # need to be removed in order to prevent bad interactions - # with the flush operation in other tests. - old_app_models = copy.deepcopy(app_cache.app_models) + # The models need to be removed after the test in order to prevent bad + # interactions with the flush operation in other tests. + _old_models = app_cache.app_configs['managers_regress'].models.copy() + try: class SwappableModel(models.Model): stuff = models.Manager() @@ -156,16 +154,16 @@ class ManagersRegressionTests(TestCase): self.assertEqual(str(e), "Manager isn't available; SwappableModel has been swapped for 'managers_regress.Parent'") finally: - app_cache.app_models = old_app_models + app_cache.app_configs['managers_regress'].models = _old_models + app_cache._get_models_cache = {} @override_settings(TEST_SWAPPABLE_MODEL='managers_regress.Parent') def test_explicit_swappable_manager(self): - try: - # This test adds dummy models to the app cache. These - # need to be removed in order to prevent bad interactions - # with the flush operation in other tests. - old_app_models = copy.deepcopy(app_cache.app_models) + # The models need to be removed after the test in order to prevent bad + # interactions with the flush operation in other tests. + _old_models = app_cache.app_configs['managers_regress'].models.copy() + try: class SwappableModel(models.Model): objects = models.Manager() @@ -183,7 +181,8 @@ class ManagersRegressionTests(TestCase): self.assertEqual(str(e), "Manager isn't available; SwappableModel has been swapped for 'managers_regress.Parent'") finally: - app_cache.app_models = old_app_models + app_cache.app_configs['managers_regress'].models = _old_models + app_cache._get_models_cache = {} def test_regress_3871(self): related = RelatedModel.objects.create() diff --git a/tests/migrations/test_commands.py b/tests/migrations/test_commands.py index 08b7371301..bad1b307db 100644 --- a/tests/migrations/test_commands.py +++ b/tests/migrations/test_commands.py @@ -2,7 +2,6 @@ from __future__ import unicode_literals import codecs -import copy import os import shutil @@ -132,10 +131,10 @@ class MakeMigrationsTests(MigrationTestBase): self.test_dir = os.path.abspath(os.path.dirname(upath(__file__))) self.migration_dir = os.path.join(self.test_dir, 'migrations_%d' % self.creation_counter) self.migration_pkg = "migrations.migrations_%d" % self.creation_counter - self._old_app_models = copy.deepcopy(app_cache.app_models) + self._old_models = app_cache.app_configs['migrations'].models.copy() def tearDown(self): - app_cache.app_models = self._old_app_models + app_cache.app_configs['migrations'].models = self._old_models app_cache._get_models_cache = {} os.chdir(self.test_dir) diff --git a/tests/migrations/test_operations.py b/tests/migrations/test_operations.py index d844d36c39..0ce030a66c 100644 --- a/tests/migrations/test_operations.py +++ b/tests/migrations/test_operations.py @@ -1,5 +1,4 @@ import unittest -from django.apps import app_cache from django.db import connection, models, migrations, router from django.db.models.fields import NOT_PROVIDED from django.db.transaction import atomic diff --git a/tests/proxy_model_inheritance/tests.py b/tests/proxy_model_inheritance/tests.py index f3cdba20a9..4c6e8c433e 100644 --- a/tests/proxy_model_inheritance/tests.py +++ b/tests/proxy_model_inheritance/tests.py @@ -34,8 +34,6 @@ class ProxyModelInheritanceTests(TransactionTestCase): sys.path = self.old_sys_path del app_cache.app_configs['app1'] del app_cache.app_configs['app2'] - del app_cache.app_models['app1'] - del app_cache.app_models['app2'] def test_table_exists(self): try: diff --git a/tests/proxy_models/tests.py b/tests/proxy_models/tests.py index cdd979a399..0c643991a7 100644 --- a/tests/proxy_models/tests.py +++ b/tests/proxy_models/tests.py @@ -1,5 +1,4 @@ from __future__ import unicode_literals -import copy from django.apps import app_cache from django.contrib import admin @@ -155,12 +154,11 @@ class ProxyModelTests(TestCase): @override_settings(TEST_SWAPPABLE_MODEL='proxy_models.AlternateModel') def test_swappable(self): - try: - # This test adds dummy applications to the app cache. These - # need to be removed in order to prevent bad interactions - # with the flush operation in other tests. - old_app_models = copy.deepcopy(app_cache.app_models) + # The models need to be removed after the test in order to prevent bad + # interactions with the flush operation in other tests. + _old_models = app_cache.app_configs['proxy_models'].models.copy() + try: class SwappableModel(models.Model): class Meta: @@ -176,7 +174,8 @@ class ProxyModelTests(TestCase): class Meta: proxy = True finally: - app_cache.app_models = old_app_models + app_cache.app_configs['proxy_models'].models = _old_models + app_cache._get_models_cache = {} def test_myperson_manager(self): Person.objects.create(name="fred") diff --git a/tests/tablespaces/tests.py b/tests/tablespaces/tests.py index 9bcfed05ed..a1e2673f38 100644 --- a/tests/tablespaces/tests.py +++ b/tests/tablespaces/tests.py @@ -1,7 +1,5 @@ from __future__ import unicode_literals -import copy - from django.apps import app_cache from django.conf import settings from django.db import connection @@ -28,7 +26,7 @@ class TablespacesTests(TestCase): def setUp(self): # The unmanaged models need to be removed after the test in order to # prevent bad interactions with the flush operation in other tests. - self.old_app_models = copy.deepcopy(app_cache.app_models) + self._old_models = app_cache.app_configs['tablespaces'].models.copy() for model in Article, Authors, Reviewers, Scientist: model._meta.managed = True @@ -37,7 +35,7 @@ class TablespacesTests(TestCase): for model in Article, Authors, Reviewers, Scientist: model._meta.managed = False - app_cache.app_models = self.old_app_models + app_cache.app_configs['tablespaces'].models = self._old_models app_cache._get_models_cache = {} def assertNumContains(self, haystack, needle, count):