Create permissions using migration operations rather than using the post_migrate signal

[Petter Strandmark]

We have encountered this problem when deploying our app to a new database.

The easiest way to demonstrate the problem is with this repository: ​https://github.com/PetterS/django-demo-app

First clone the repo and execute

pipenv install

Then in a clean repository:

$ python manage.py migrate auth
$ python manage.py migrate myapp

This fails with "django.contrib.auth.models.DoesNotExist: Permission matching query does not exist."

Indeed, I can verify that the sqlite database does not contain any custom permissions. But showmigrations tells me that "0001_initial" and "0002_auto_20181012_1052" have been applied!

$ python manage.py showmigrations
...
myapp
 [X] 0001_initial
 [X] 0002_auto_20181012_1052
 [ ] 0003_auto_20181012_1054

On the other hand, in a clean repository the following succeeds:

$ python manage.py migrate auth
$ python manage.py migrate myapp 0001_initial
$ python manage.py migrate myapp 0002_auto_20181012_1052
$ python manage.py migrate myapp 0003_auto_20181012_1054
$ python manage.py migrate myapp

So the result of the migrations adding the permission is not visible until after the migrate command finished, even if there are other migrations coming after.

In a clean repository:

$ python manage.py migrate auth
$ python manage.py migrate myapp 0001_initial

"can_smell" is now a permission in the DB.

I think this bug is pretty severe, since running migrate on a new DB will put the database in a state that is not very easy to recover from. The migrations show as applied, when in fact the permissions have not been created.

[Tim Graham]

There's a recent discussion that describes ​the same problem for contenttypes.

[Arthur Rio]

I don't think we can solve the ticket for permissions unless we fix it for content types as well. The creation/deletion/renaming of permissions and content types should happen in the makemigrations phase as opposed to using a post_migrate signal.

I suggest renaming this ticket to "Manage content types and permissions using migration operations rather than using the post_migrate signal", what do you think?

The approach would be:

• Add a pre_makemigrations_write signal, triggered right before the migrations are written to file
• Content Type would register to that signal and insert operations if a model is created, deleted or renamed. If any operation is inserted, it would add itself to the dependencies of the migration
• Add a post_contenttypes_insert_migrations signal, triggered if any content type operation is inserted
• Permissions would register to that signal and insert operations if a model is created or deleted, or if any of the permissions of the model have changed (default or custom). If any operation is inserted, it would add itself to the dependencies of the migration

That way all the operations are part of the migration files and subsequent migration operations can use the new or updated permissions/content types. Also, it would handle having only django.contrib.contenttypes setup in the INSTALLED_APPS (i.e. not django.contrib.auth). I might be missing some edge cases, so any feedback would be appreciated.

Finally, I'm not sure yet how to handle adding django.contrib.auth and django.contrib.contenttypes to the INSTALLED_APPS after migrations were already created for other models, but I think that we could keep the post_migrate signal in place to perform insert operations if 0001_initial is part of the plan.

[Simon Charette]

Finally, I'm not sure yet how to handle adding django.contrib.auth and django.contrib.contenttypes to the INSTALLED_APPS after migrations were already created for other models, but I think that we could keep the post_migrate signal in place to perform insert operations if 0001_initial is part of the plan.

That's one of the reasons why RenameContentType operations are injected in the plan on pre_migrate and not baked into migration files. We must account for the fact these apps can be installed or uninstalled at any time in the state of a project.

Another reason why operations baking is problematic is migration squashing and optimizations which the operation injection technique completely works around. I don't want to deter you from trying but I spent a large amount of time working on the operation injection approach in #24067 and #24100 and it seemed like the best approach at least at that time. What's the rationale for not using a similar approach for CreateContentType, CreatePermission and RenamePermission? ​Someone give a shot at permission renaming in #27489 and it seemed to be working fine.

Something to keep in mind as well is that an automatically generated DeleteContentType operation could lead to data loss because of cascade deletion of foreign references. That's the reason behind the interactive remove_stale_contenttypes command. IMO that's a no go.

While I'm not a big fan of signals myself I can see how allowing third party apps to bake operations could be useful. For example, contrib.postgres could bake a CreateExtension if it detects an HStoreField is first added to a project state. I just don't think this is necessary to solve this issue as the operation injection approach seem to be working fine.

[Petter Strandmark]

As a workaround, it would be helpful to have a "--one-at-a-time" flag for the migrate command that runs one (the next) migration and then exists. That is what we are doing manually now.

[Arthur Rio]

That's one of the reasons why RenameContentType operations are injected in the plan on pre_migrate and not baked into migration files. We must account for the fact these apps can be installed or uninstalled at any time in the state of a project.
Another reason why operations baking is problematic is migration squashing and optimizations which the operation injection technique completely works around. I don't want to deter you from trying but I spent a large amount of time working on the operation injection approach in #24067 and #24100 and it seemed like the best approach at least at that time. What's the rationale for not using a similar approach for CreateContentType, CreatePermission and RenamePermission? ​Someone give a shot at permission renaming in #27489 and it seemed to be working fine.

The rationale was just to try to make things a bit less magical and show the set of changes to the user instead of doing the injection behind the scenes, but I hadn't considered the squashing and optimizations yet, thanks for pointing that out. Thank you also for the references to existing PRs that I can use as resources while working on this ticket.
It seems that the main issue blocking #27489 was to make sure the permission operations injection happened after the contenttypes one. What do you think about triggering a post_contenttypes_operations_injection signal so that django.contrib.auth can register to it and do its create/update permission injection then? The AlterModelOptions operations injection would happen in the pre_migrate though.

Something to keep in mind as well is that an automatically generated DeleteContentType operation could lead to data loss because of cascade deletion of foreign references. That's the reason behind the interactive remove_stale_contenttypes command. IMO that's a no go.

Sounds reasonable. Same for permissions I guess?

While I'm not a big fan of signals myself I can see how allowing third party apps to bake operations could be useful. For example, contrib.postgres could bake a CreateExtension if it detects an HStoreField is first added to a project state. I just don't think this is necessary to solve this issue as the operation injection approach seem to be working fine.

Thank you very much for your input, I will try the pre_migrate approach.

[Simon Charette]

What do you think about triggering a post_contenttypes_operations_injection signal so that django.contrib.auth can register to it and do its create/update permission injection then?

This could work but an alternative could be to add a system check or raise ImproperlyConfigured if django.contrib.auth is installed before django.contrib.contenttypes.

If this assertion holds then the pre_migrate signal registered by auth is guaranteed to run after the one registered by contenttypes as it was registered after. That would allow the auth operation injection logic to look for AddContentType and RenameContentType operations in the plan and insert AddPermission and RenamePermission after them.

We already ​document and perform ​similar checks for middleware ordering so I think doing it for INSTALLED_APPS could work as well. Thing is this check would fail for most of the projects at first ​since the project template has been ordering them the other way around for a while. I still think this is worth enforcing a form of ordering for installed apps for this purpose though and I'd volunteer to work on a PR to tackle this.

Whether we choose a system check or an ImproperlyConfigured preceded by a period deprecation warning we should still register the post_migrate signals for a few releases if apps are not ordered appropriately. This would allow the contenttypes creation and permissions to still work in most of the cases until the deprecation ends of if some users ignore or silence the system check.

Thoughts?

[Simon Charette]

Another alternative could be to introduce a post_operation signal where the sender would be the operation class, operation the Operation instance and we could include the from_state and to_state as well.

The content types app could register a signal receiver for CreateModel using post_operation.connect(sender=CreateModel, receiver) and return [CreateContentType(...)]. The executor would then execute the collected operations. Then auth app could register a post_operation.connect(sender=CreateContentType, receiver) and return [CreatePermission] which the executor would execute as well.

That would allow us get rid of the ​nasty plan injection logic and expose a somewhat reusable interface for third party apps all that without enforcing INSTALLED_APPS ordering for now.

[Arthur Rio]

I'm in favor of the second approach since it will make the release of the patch much simpler and also provide a more generic/extensible approach to the problem.

Thank you for the great suggestions.

[Taylor H]

Hi everyone 👋🏻

Copying the following from the mailing list for cross-reference (Arthur and I are working together). Hoping to get some feedback from everyone .

After some experiments and discussions it felt like while the implementation might solve the initial problem, it's a bit under the hood and will be hard to debug if something goes wrong. The idea was to inject operations at the time of running migrate.

So... we went back to the idea of having hooks during the makemigrations process instead, so that the operations would be written to the migration files, which would make it more explicit and less risky. Here is a first draft of how it would look like: ​https://github.com/django/django/pull/14229.

1. We know that the makemigrations process is complicated, so before we invest more time down that path, is there something obvious we might be missing?
2. What do you think of this approach with hooks (pre and post "add_operation")?
3. Do you think it would be a useful feature for other third party apps as well (not just content types and permissions)?

[Jacob Walls]

​New PR