From 00110904ac67050c639f93cfd7b5e19218bcef5b Mon Sep 17 00:00:00 2001 From: Aymeric Augustin Date: Fri, 27 Dec 2013 20:55:58 +0100 Subject: [PATCH] Refactored the migration signals to use app configs. De-aliased pre/post_syncdb to pre/post_migrate to increase backwards-compatibility. --- django/contrib/auth/management/__init__.py | 27 +++-- django/contrib/auth/tests/test_management.py | 18 ++- django/contrib/contenttypes/management.py | 16 ++- django/contrib/sites/management.py | 26 +++-- django/core/management/sql.py | 14 +++ django/db/models/signals.py | 9 +- docs/internals/deprecation.txt | 5 +- docs/ref/signals.txt | 117 ++++++++++++++++--- docs/releases/1.7.txt | 9 +- tests/migrate_signals/models.py | 4 +- tests/migrate_signals/tests.py | 18 ++- 11 files changed, 187 insertions(+), 76 deletions(-) diff --git a/django/contrib/auth/management/__init__.py b/django/contrib/auth/management/__init__.py index 306b051284..f933b61c86 100644 --- a/django/contrib/auth/management/__init__.py +++ b/django/contrib/auth/management/__init__.py @@ -60,18 +60,21 @@ def _check_permission_clashing(custom, builtin, ctype): pool.add(codename) -def create_permissions(app, created_models, verbosity, db=DEFAULT_DB_ALIAS, **kwargs): +def create_permissions(app_config, verbosity=22, interactive=True, db=DEFAULT_DB_ALIAS, **kwargs): + if not app_config.models_module: + return + try: - apps.get_model('auth', 'Permission') + Permission = apps.get_model('auth', 'Permission') except LookupError: return - if not router.allow_migrate(db, auth_app.Permission): + if not router.allow_migrate(db, Permission): return from django.contrib.contenttypes.models import ContentType - app_models = apps.get_models(app) + app_models = apps.get_models(app_config.models_module) # This will hold the permissions we're looking for as # (content_type, (codename, name)) @@ -89,20 +92,20 @@ def create_permissions(app, created_models, verbosity, db=DEFAULT_DB_ALIAS, **kw # Find all the Permissions that have a content_type for a model we're # looking for. We don't need to check for codenames since we already have # a list of the ones we're going to create. - all_perms = set(auth_app.Permission.objects.using(db).filter( + all_perms = set(Permission.objects.using(db).filter( content_type__in=ctypes, ).values_list( "content_type", "codename" )) perms = [ - auth_app.Permission(codename=codename, name=name, content_type=ctype) + Permission(codename=codename, name=name, content_type=ctype) for ctype, (codename, name) in searched_perms if (ctype.pk, codename) not in all_perms ] # Validate the permissions before bulk_creation to avoid cryptic # database error when the verbose_name is longer than 50 characters - permission_name_max_length = auth_app.Permission._meta.get_field('name').max_length + permission_name_max_length = Permission._meta.get_field('name').max_length verbose_name_max_length = permission_name_max_length - 11 # len('Can change ') prefix for perm in perms: if len(perm.name) > permission_name_max_length: @@ -112,13 +115,13 @@ def create_permissions(app, created_models, verbosity, db=DEFAULT_DB_ALIAS, **kw verbose_name_max_length, ) ) - auth_app.Permission.objects.using(db).bulk_create(perms) + Permission.objects.using(db).bulk_create(perms) if verbosity >= 2: for perm in perms: print("Adding permission '%s'" % perm) -def create_superuser(app, created_models, verbosity, db, **kwargs): +def create_superuser(app_config, verbosity=22, interactive=True, db=DEFAULT_DB_ALIAS, **kwargs): try: apps.get_model('auth', 'Permission') except LookupError: @@ -128,7 +131,7 @@ def create_superuser(app, created_models, verbosity, db, **kwargs): from django.core.management import call_command - if UserModel in created_models and kwargs.get('interactive', True): + if not UserModel.objects.exists() and interactive: msg = ("\nYou just installed Django's auth system, which means you " "don't have any superusers defined.\nWould you like to create one " "now? (yes/no): ") @@ -203,7 +206,9 @@ def get_default_username(check_db=True): return '' return default_username + signals.post_migrate.connect(create_permissions, dispatch_uid="django.contrib.auth.management.create_permissions") signals.post_migrate.connect(create_superuser, - sender=auth_app, dispatch_uid="django.contrib.auth.management.create_superuser") + sender=apps.get_app_config('auth'), + dispatch_uid="django.contrib.auth.management.create_superuser") diff --git a/django/contrib/auth/tests/test_management.py b/django/contrib/auth/tests/test_management.py index db4b698d8b..c60346c2ac 100644 --- a/django/contrib/auth/tests/test_management.py +++ b/django/contrib/auth/tests/test_management.py @@ -232,13 +232,15 @@ class PermissionTestCase(TestCase): Test that we show proper error message if we are trying to create duplicate permissions. """ + auth_app_config = apps.get_app_config('auth') + # check duplicated default permission models.Permission._meta.permissions = [ ('change_permission', 'Can edit permission (duplicate)')] six.assertRaisesRegex(self, CommandError, "The permission codename 'change_permission' clashes with a " "builtin permission for model 'auth.Permission'.", - create_permissions, models, [], verbosity=0) + create_permissions, auth_app_config, verbosity=0) # check duplicated custom permissions models.Permission._meta.permissions = [ @@ -249,21 +251,23 @@ class PermissionTestCase(TestCase): six.assertRaisesRegex(self, CommandError, "The permission codename 'my_custom_permission' is duplicated for model " "'auth.Permission'.", - create_permissions, models, [], verbosity=0) + create_permissions, auth_app_config, verbosity=0) # should not raise anything models.Permission._meta.permissions = [ ('my_custom_permission', 'Some permission'), ('other_one', 'Some other permission'), ] - create_permissions(models, [], verbosity=0) + create_permissions(auth_app_config, verbosity=0) def test_default_permissions(self): + auth_app_config = apps.get_app_config('auth') + permission_content_type = ContentType.objects.get_by_natural_key('auth', 'permission') models.Permission._meta.permissions = [ ('my_custom_permission', 'Some permission'), ] - create_permissions(models, [], verbosity=0) + create_permissions(auth_app_config, verbosity=0) # add/change/delete permission by default + custom permission self.assertEqual(models.Permission.objects.filter( @@ -272,7 +276,7 @@ class PermissionTestCase(TestCase): models.Permission.objects.filter(content_type=permission_content_type).delete() models.Permission._meta.default_permissions = [] - create_permissions(models, [], verbosity=0) + create_permissions(auth_app_config, verbosity=0) # custom permission only since default permissions is empty self.assertEqual(models.Permission.objects.filter( @@ -280,10 +284,12 @@ class PermissionTestCase(TestCase): ).count(), 1) def test_verbose_name_length(self): + auth_app_config = apps.get_app_config('auth') + permission_content_type = ContentType.objects.get_by_natural_key('auth', 'permission') models.Permission.objects.filter(content_type=permission_content_type).delete() models.Permission._meta.verbose_name = "some ridiculously long verbose name that is out of control" six.assertRaisesRegex(self, exceptions.ValidationError, "The verbose_name of permission is longer than 39 characters", - create_permissions, models, [], verbosity=0) + create_permissions, auth_app_config, verbosity=0) diff --git a/django/contrib/contenttypes/management.py b/django/contrib/contenttypes/management.py index 6b2d2d22f1..a7b47456a6 100644 --- a/django/contrib/contenttypes/management.py +++ b/django/contrib/contenttypes/management.py @@ -1,5 +1,4 @@ from django.apps import apps -from django.contrib.contenttypes.models import ContentType from django.db import DEFAULT_DB_ALIAS, router from django.db.models import signals from django.utils.encoding import smart_text @@ -7,13 +6,16 @@ from django.utils import six from django.utils.six.moves import input -def update_contenttypes(app, created_models, verbosity=2, db=DEFAULT_DB_ALIAS, **kwargs): +def update_contenttypes(app_config, verbosity=2, interactive=True, db=DEFAULT_DB_ALIAS, **kwargs): """ Creates content types for models in the given app, removing any model entries that no longer have a matching model class. """ + if not app_config.models_module: + return + try: - apps.get_model('contenttypes', 'ContentType') + ContentType = apps.get_model('contenttypes', 'ContentType') except LookupError: return @@ -21,7 +23,7 @@ def update_contenttypes(app, created_models, verbosity=2, db=DEFAULT_DB_ALIAS, * return ContentType.objects.clear_cache() - app_models = apps.get_models(app) + app_models = apps.get_models(app_config.models_module) if not app_models: return # They all have the same app_label, get the first one. @@ -85,11 +87,13 @@ If you're unsure, answer 'no'. print("Stale content types remain.") -def update_all_contenttypes(verbosity=2, **kwargs): +def update_all_contenttypes(**kwargs): for app_config in apps.get_app_configs(only_with_models_module=True): - update_contenttypes(app_config.models_module, None, verbosity, **kwargs) + update_contenttypes(app_config, **kwargs) + signals.post_migrate.connect(update_contenttypes) + if __name__ == "__main__": update_all_contenttypes() diff --git a/django/contrib/sites/management.py b/django/contrib/sites/management.py index 7dec568fa4..ab9ff69c35 100644 --- a/django/contrib/sites/management.py +++ b/django/contrib/sites/management.py @@ -2,17 +2,22 @@ Creates the default Site object. """ -from django.db.models import signals -from django.db import connections -from django.db import router -from django.contrib.sites.models import Site -from django.contrib.sites import models as site_app +from django.apps import apps from django.core.management.color import no_style +from django.db import DEFAULT_DB_ALIAS, connections, router +from django.db.models import signals -def create_default_site(app, created_models, verbosity, db, **kwargs): - # Only create the default sites in databases where Django created the table - if Site in created_models and router.allow_migrate(db, Site): +def create_default_site(app_config, verbosity=22, interactive=True, db=DEFAULT_DB_ALIAS, **kwargs): + try: + Site = apps.get_model('sites', 'Site') + except LookupError: + return + + if not router.allow_migrate(db, Site): + return + + if not Site.objects.exists(): # The default settings set SITE_ID = 1, and some tests in Django's test # suite rely on this value. However, if database sequences are reused # (e.g. in the test suite after flush/syncdb), it isn't guaranteed that @@ -32,6 +37,7 @@ def create_default_site(app, created_models, verbosity, db, **kwargs): for command in sequence_sql: cursor.execute(command) - Site.objects.clear_cache() + Site.objects.clear_cache() -signals.post_migrate.connect(create_default_site, sender=site_app) + +signals.post_migrate.connect(create_default_site, sender=apps.get_app_config('sites')) diff --git a/django/core/management/sql.py b/django/core/management/sql.py index 35ce693e3b..40becd34d7 100644 --- a/django/core/management/sql.py +++ b/django/core/management/sql.py @@ -211,6 +211,13 @@ def emit_pre_migrate_signal(create_models, verbosity, interactive, db): if verbosity >= 2: print("Running pre-migrate handlers for application %s" % app_config.label) models.signals.pre_migrate.send( + sender=app_config, + app_config=app_config, + verbosity=verbosity, + interactive=interactive, + db=db) + # For backwards-compatibility -- remove in Django 1.9. + models.signals.pre_syncdb.send( sender=app_config.models_module, app=app_config.models_module, create_models=create_models, @@ -225,6 +232,13 @@ def emit_post_migrate_signal(created_models, verbosity, interactive, db): if verbosity >= 2: print("Running post-migrate handlers for application %s" % app_config.label) models.signals.post_migrate.send( + sender=app_config, + app_config=app_config, + verbosity=verbosity, + interactive=interactive, + db=db) + # For backwards-compatibility -- remove in Django 1.9. + models.signals.post_syncdb.send( sender=app_config.models_module, app=app_config.models_module, created_models=created_models, diff --git a/django/db/models/signals.py b/django/db/models/signals.py index f5e1ac2cf4..627358ac63 100644 --- a/django/db/models/signals.py +++ b/django/db/models/signals.py @@ -62,7 +62,8 @@ post_delete = ModelSignal(providing_args=["instance", "using"], use_caching=True m2m_changed = ModelSignal(providing_args=["action", "instance", "reverse", "model", "pk_set", "using"], use_caching=True) -pre_migrate = Signal(providing_args=["app", "create_models", "verbosity", "interactive", "db"]) -pre_syncdb = pre_migrate -post_migrate = Signal(providing_args=["class", "app", "created_models", "verbosity", "interactive", "db"]) -post_syncdb = post_migrate +pre_migrate = Signal(providing_args=["app_config", "verbosity", "interactive", "db"]) +post_migrate = Signal(providing_args=["app_config", "verbosity", "interactive", "db"]) + +pre_syncdb = Signal(providing_args=["app", "create_models", "verbosity", "interactive", "db"]) +post_syncdb = Signal(providing_args=["class", "app", "created_models", "verbosity", "interactive", "db"]) diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index 29f7df53ae..1b8b45a520 100644 --- a/docs/internals/deprecation.txt +++ b/docs/internals/deprecation.txt @@ -165,10 +165,7 @@ these changes. * The ``syncdb`` command will be removed. * ``django.db.models.signals.pre_syncdb`` and - ``django.db.models.signals.post_syncdb`` will be removed, and - ``django.db.models.signals.pre_migrate`` and - ``django.db.models.signals.post_migrate`` will lose their - ``create_models`` and ``created_models`` arguments. + ``django.db.models.signals.post_syncdb`` will be removed. * ``allow_syncdb`` on database routers will no longer automatically become ``allow_migrate``. diff --git a/docs/ref/signals.txt b/docs/ref/signals.txt index 1abfed8806..ece6990aa7 100644 --- a/docs/ref/signals.txt +++ b/docs/ref/signals.txt @@ -381,12 +381,10 @@ handlers are registered anywhere else they may not be loaded by Arguments sent with this signal: ``sender`` - The ``models`` module of the app about to be migrated/synced. - For example, if :djadmin:`migrate` is about to install - an app called ``"foo.bar.myapp"``, ``sender`` will be the - ``foo.bar.myapp.models`` module. + An :class:`~django.apps.AppConfig` instance for the application about to + be migrated/synced. -``app`` +``app_config`` Same as ``sender``. ``verbosity`` @@ -415,14 +413,48 @@ pre_syncdb .. deprecated:: 1.7 - This signal has been renamed to :data:`~django.db.models.signals.pre_migrate`. + This signal has been replaced by :data:`~django.db.models.signals.pre_migrate`. -Alias of :data:`django.db.models.signals.pre_migrate`. As long as this alias -is present, for backwards-compatibility this signal has an extra argument it sends: +Sent by the :djadmin:`syncdb` command before it starts to install an +application. + +Any handlers that listen to this signal need to be written in a particular +place: a ``management`` module in one of your :setting:`INSTALLED_APPS`. If +handlers are registered anywhere else they may not be loaded by +:djadmin:`syncdb`. + +Arguments sent with this signal: + +``sender`` + The ``models`` module that was just installed. That is, if + :djadmin:`syncdb` just installed an app called ``"foo.bar.myapp"``, + ``sender`` will be the ``foo.bar.myapp.models`` module. + +``app`` + Same as ``sender``. ``create_models`` - A list of the model classes from any app which :djadmin:`migrate` is - going to create, **only if the app has no migrations**. + A list of the model classes from any app which :djadmin:`syncdb` plans to + create. + + +``verbosity`` + Indicates how much information manage.py is printing on screen. See + the :djadminopt:`--verbosity` flag for details. + + Functions which listen for :data:`pre_syncdb` should adjust what they + output to the screen based on the value of this argument. + +``interactive`` + If ``interactive`` is ``True``, it's safe to prompt the user to input + things on the command line. If ``interactive`` is ``False``, functions + which listen for this signal should not try to prompt for anything. + + For example, the :mod:`django.contrib.auth` app only prompts to create a + superuser when ``interactive`` is ``True``. + +``db`` + The alias of database on which a command will operate. post_migrate ------------ @@ -444,11 +476,10 @@ idempotent changes (e.g. no database alterations) as this may cause the Arguments sent with this signal: ``sender`` - The ``models`` module that was just installed. That is, if - :djadmin:`migrate` just installed an app called ``"foo.bar.myapp"``, - ``sender`` will be the ``foo.bar.myapp.models`` module. + An :class:`~django.apps.AppConfig` instance for the application that was + just installed. -``app`` +``app_config`` Same as ``sender``. ``verbosity`` @@ -489,14 +520,62 @@ post_syncdb .. deprecated:: 1.7 - This signal has been renamed to :data:`~django.db.models.signals.post_migrate`. + This signal has been replaced by :data:`~django.db.models.signals.post_migrate`. -Alias of :data:`django.db.models.signals.post_migrate`. As long as this alias -is present, for backwards-compatibility this signal has an extra argument it sends: +Sent by the :djadmin:`syncdb` command after it installs an application, and the +:djadmin:`flush` command. + +Any handlers that listen to this signal need to be written in a particular +place: a ``management`` module in one of your :setting:`INSTALLED_APPS`. If +handlers are registered anywhere else they may not be loaded by +:djadmin:`syncdb`. It is important that handlers of this signal perform +idempotent changes (e.g. no database alterations) as this may cause the +:djadmin:`flush` management command to fail if it also ran during the +:djadmin:`syncdb` command. + +Arguments sent with this signal: + +``sender`` + The ``models`` module that was just installed. That is, if + :djadmin:`syncdb` just installed an app called ``"foo.bar.myapp"``, + ``sender`` will be the ``foo.bar.myapp.models`` module. + +``app`` + Same as ``sender``. ``created_models`` - A list of the model classes from any app which :djadmin:`migrate` has - created, **only if the app has no migrations**. + A list of the model classes from any app which :djadmin:`syncdb` has + created so far. + +``verbosity`` + Indicates how much information manage.py is printing on screen. See + the :djadminopt:`--verbosity` flag for details. + + Functions which listen for :data:`post_syncdb` should adjust what they + output to the screen based on the value of this argument. + +``interactive`` + If ``interactive`` is ``True``, it's safe to prompt the user to input + things on the command line. If ``interactive`` is ``False``, functions + which listen for this signal should not try to prompt for anything. + + For example, the :mod:`django.contrib.auth` app only prompts to create a + superuser when ``interactive`` is ``True``. + +``db`` + The database alias used for synchronization. Defaults to the ``default`` + database. + +For example, ``yourapp/management/__init__.py`` could be written like:: + + from django.db.models.signals import post_syncdb + import yourapp.models + + def my_callback(sender, **kwargs): + # Your specific logic here + pass + + post_syncdb.connect(my_callback, sender=yourapp.models) Request/response signals ======================== diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index a7e7da6d15..5416d9dc0b 100644 --- a/docs/releases/1.7.txt +++ b/docs/releases/1.7.txt @@ -47,11 +47,12 @@ but a few of the key features are: * A new ``makemigrations`` command provides an easy way to autodetect changes to your models and make migrations for them. -* :data:`~django.db.models.signals.pre_syncdb` and - :data:`~django.db.models.signals.post_syncdb` have been renamed to + :data:`~django.db.models.signals.pre_syncdb` and + :data:`~django.db.models.signals.post_syncdb` have been replaced by :data:`~django.db.models.signals.pre_migrate` and - :data:`~django.db.models.signals.post_migrate` respectively. The - ``create_models``/``created_models`` argument has also been deprecated. + :data:`~django.db.models.signals.post_migrate` respectively. These new + signals have slightly different arguments. Check the documentation for + details. * The ``allow_syncdb`` method on database routers is now called ``allow_migrate``, but still performs the same function. Routers with ``allow_syncdb`` methods diff --git a/tests/migrate_signals/models.py b/tests/migrate_signals/models.py index 55a8657a3c..2cd71391fb 100644 --- a/tests/migrate_signals/models.py +++ b/tests/migrate_signals/models.py @@ -1,2 +1,2 @@ -# Remove this module when pre/post_migrate are refactored to use something -# other than a models module for their "sender" argument. +# This module has to exist, otherwise pre/post_migrate aren't sent for the +# migrate_signals application. diff --git a/tests/migrate_signals/tests.py b/tests/migrate_signals/tests.py index 9daa7ae376..7e1d5be103 100644 --- a/tests/migrate_signals/tests.py +++ b/tests/migrate_signals/tests.py @@ -1,12 +1,12 @@ +from django.apps import apps from django.db.models import signals from django.test import TestCase from django.core import management from django.utils import six -from . import models - -PRE_MIGRATE_ARGS = ['app', 'create_models', 'verbosity', 'interactive', 'db'] +APP_CONFIG = apps.get_app_config('migrate_signals') +PRE_MIGRATE_ARGS = ['app_config', 'verbosity', 'interactive', 'db'] MIGRATE_DATABASE = 'default' MIGRATE_VERBOSITY = 1 MIGRATE_INTERACTIVE = False @@ -39,7 +39,7 @@ class OneTimeReceiver(object): self.call_counter = self.call_counter + 1 self.call_args = kwargs # we need to test only one call of migrate - signals.pre_migrate.disconnect(pre_migrate_receiver, sender=models) + signals.pre_migrate.disconnect(pre_migrate_receiver, sender=APP_CONFIG) # We connect receiver here and not in unit test code because we need to @@ -51,21 +51,19 @@ class OneTimeReceiver(object): # 3. Test runner calls migrate for create default database. # 4. Test runner execute our unit test code. pre_migrate_receiver = OneTimeReceiver() -signals.pre_migrate.connect(pre_migrate_receiver, sender=models) +signals.pre_migrate.connect(pre_migrate_receiver, sender=APP_CONFIG) class MigrateSignalTests(TestCase): - available_apps = [ - 'migrate_signals', - ] + available_apps = ['migrate_signals'] def test_pre_migrate_call_time(self): self.assertEqual(pre_migrate_receiver.call_counter, 1) def test_pre_migrate_args(self): r = PreMigrateReceiver() - signals.pre_migrate.connect(r, sender=models) + signals.pre_migrate.connect(r, sender=APP_CONFIG) management.call_command('migrate', database=MIGRATE_DATABASE, verbosity=MIGRATE_VERBOSITY, interactive=MIGRATE_INTERACTIVE, load_initial_data=False, stdout=six.StringIO()) @@ -73,7 +71,7 @@ class MigrateSignalTests(TestCase): args = r.call_args self.assertEqual(r.call_counter, 1) self.assertEqual(set(args), set(PRE_MIGRATE_ARGS)) - self.assertEqual(args['app'], models) + self.assertEqual(args['app_config'], APP_CONFIG) self.assertEqual(args['verbosity'], MIGRATE_VERBOSITY) self.assertEqual(args['interactive'], MIGRATE_INTERACTIVE) self.assertEqual(args['db'], 'default')