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

@@ -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. <module 'django.contrib.admin.models'
+        # from 'django/contrib/admin/models.pyc'>.
         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 '<AppConfig: %s>' % self.label

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:
+            try:
-                app_list = [self.app_models.get(app_label, OrderedDict())]
+                app_config = self.app_configs[app_label]
-            else:
+            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:
+        if only_installed:
-            return None
+            app_config = self.app_configs.get(app_label)
-        if (self.available_apps is not None and only_installed
+            if app_config is not None and not app_config.installed:
-                and app_label not in self.available_apps):
+                return None
-            raise UnavailableApp("App with label %s isn't available." % app_label)
+            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
+            # Add the model to the app_config's models dictionary.
-            # in the app_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

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
+        # The models need to be removed after the test in order to prevent bad
-        # need to be removed in order to prevent bad interactions
+        # interactions with the flush operation in other tests.
-        # with the flush operation in other tests.
+        self._old_models = app_cache.app_configs['app_loading'].models.copy()
-        self.old_app_models = copy.deepcopy(app_cache.app_models)
 
     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"""

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
+        # The models need to be removed after the test in order to prevent bad
-        # need to be removed in order to prevent bad interactions
+        # interactions with the flush operation in other tests.
-        # with the flush operation in other tests.
+        self._old_models = app_cache.app_configs['invalid_models'].models.copy()
-        self.old_app_models = copy.deepcopy(app_cache.app_models)
 
     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
 

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:
+        # The models need to be removed after the test in order to prevent bad
-            # This test adds dummy models to the app cache. These
+        # interactions with the flush operation in other tests.
-            # need to be removed in order to prevent bad interactions
+        _old_models = app_cache.app_configs['managers_regress'].models.copy()
-            # with the flush operation in other tests.
-            old_app_models = copy.deepcopy(app_cache.app_models)
 
+        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:
+        # The models need to be removed after the test in order to prevent bad
-            # This test adds dummy models to the app cache. These
+        # interactions with the flush operation in other tests.
-            # need to be removed in order to prevent bad interactions
+        _old_models = app_cache.app_configs['managers_regress'].models.copy()
-            # with the flush operation in other tests.
-            old_app_models = copy.deepcopy(app_cache.app_models)
 
+        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:
+        # The models need to be removed after the test in order to prevent bad
-            # This test adds dummy models to the app cache. These
+        # interactions with the flush operation in other tests.
-            # need to be removed in order to prevent bad interactions
+        _old_models = app_cache.app_configs['managers_regress'].models.copy()
-            # with the flush operation in other tests.
-            old_app_models = copy.deepcopy(app_cache.app_models)
 
+        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()

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)

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

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:

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:
+        # The models need to be removed after the test in order to prevent bad
-            # This test adds dummy applications to the app cache. These
+        # interactions with the flush operation in other tests.
-            # need to be removed in order to prevent bad interactions
+        _old_models = app_cache.app_configs['proxy_models'].models.copy()
-            # with the flush operation in other tests.
-            old_app_models = copy.deepcopy(app_cache.app_models)
 
+        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")

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):