[Django] #29843: Migrations involving permissions (in Meta class of model) are not applied immediately
Django
immediately
-----------------------------------------+------------------------
Reporter: PetterS | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 2.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29843>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
post_migrate signal
--------------------------------------+------------------------------------
Reporter: PetterS | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Bug => Cleanup/optimization
* component: Uncategorized => contrib.auth
* stage: Unreviewed => Accepted
Comment:
There's a recent discussion that describes
[https://groups.google.com/d/topic/django-
developers/HDKVHrmSMdE/discussion the same problem] for contenttypes.
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:1>
Django
post_migrate signal
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Petter Strandmark):
* cc: Petter Strandmark (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:2>
Django
post_migrate signal
--------------------------------------+------------------------------------
Reporter: Petter Strandmark | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Arthur Rio):
* cc: Arthur Rio (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:3>
Django
post_migrate signal
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution:
permissions post_migrate |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Arthur Rio):
* owner: nobody => Arthur Rio
* keywords: => contenttypes permissions post_migrate
* status: new => assigned
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:4>
Django
post_migrate signal
-------------------------------------+-------------------------------------
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution:
Keywords: contenttypes | Triage Stage: Accepted
permissions post_migrate |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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`?
[https://github.com/django/django/pull/6612/ 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:5>
Django
post_migrate signal
-------------------------------------+-------------------------------------
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution:
Keywords: contenttypes | Triage Stage: Accepted
permissions post_migrate |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:6>
Django
post_migrate signal
-------------------------------------+-------------------------------------
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution:
Keywords: contenttypes | Triage Stage: Accepted
permissions post_migrate |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:7>
Django
post_migrate signal
-------------------------------------+-------------------------------------
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution:
Keywords: contenttypes | Triage Stage: Accepted
permissions post_migrate |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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 [https://docs.djangoproject.com/en/2.1/ref/middleware
/#middleware-ordering document] and perform
[https://github.com/django/django/blob/dc5e75d419893bde33b7e439b59bdf271fc1a3f2/django/contrib/auth/middleware.py#L17-L23
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
[https://github.com/django/django/blob/dc5e75d419893bde33b7e439b59bdf271fc1a3f2/django/conf/project_template/project_name/settings
.py-tpl#L35-L36 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:8>
Django
post_migrate signal
-------------------------------------+-------------------------------------
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution:
Keywords: contenttypes | Triage Stage: Accepted
permissions post_migrate |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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
[https://github.com/django/django/blob/dc5e75d419893bde33b7e439b59bdf271fc1a3f2/django/contrib/contenttypes/management/__init__.py#L45-L84
nasty plan injection logic] and expose a somewhat reusable interface for
third party apps all that without enforcing `INSTALLED_APPS` ordering for
now.
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:9>
Django
post_migrate signal
-------------------------------------+-------------------------------------
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution:
Keywords: contenttypes | Triage Stage: Accepted
permissions post_migrate |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:10>
Django
post_migrate signal
-------------------------------------+-------------------------------------
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: contenttypes | Triage Stage: Accepted
permissions post_migrate |
Easy pickings: 0 | UI/UX: 0
Changes (by Asif Saifuddin Auvi):
* needs_better_patch: 0 => 1
* version: 2.1 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:11>
Django
post_migrate signal
-------------------------------------+-------------------------------------
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: contenttypes | Triage Stage: Accepted
permissions post_migrate |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:12>
Django
post_migrate signal
-------------------------------------+-------------------------------------
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: contenttypes | Triage Stage: Accepted
permissions post_migrate |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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)?
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:13>
Django
post_migrate signal
-------------------------------------+-------------------------------------
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: contenttypes | Triage Stage: Accepted
permissions post_migrate |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/14229 New PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:14>
Django
post_migrate signal
-------------------------------------+-------------------------------------
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: contenttypes | Triage Stage: Accepted
permissions post_migrate |
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:15>
Django
post_migrate signal
-------------------------------------+-------------------------------------
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: contenttypes | Triage Stage: Accepted
permissions post_migrate |
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:16>
Django
post_migrate signal
-------------------------------------+-------------------------------------
Reporter: Petter Strandmark | Owner: Arthur
Type: | Rio
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: contenttypes | Triage Stage: Accepted
permissions post_migrate |
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Changes (by Ryan Hiebert):
* cc: Ryan Hiebert (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29843#comment:17>