Re: [Django] #34165: migrate management command does not respect database parameter when adding Permissions.
Django
Permissions.
------------------------------+------------------------------------
Reporter: Vasanth | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.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
------------------------------+------------------------------------
Comment (by Vasanth):
This patch resolves the problem at my end. I hope it can be added in the
4.1.4 since #29843 seems to be not actively worked on at the moment.
--
Ticket URL: <https://code.djangoproject.com/ticket/34165#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Permissions.
------------------------------+------------------------------------
Reporter: Vasanth | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.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
------------------------------+------------------------------------
Comment (by Vasanth):
After diving a bit deeper it turned out that the issue was with one of the
libraries in my project which was not adapted for multi-DB. I've made a PR
with changes on the django-admin-interface which resolved my issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/34165#comment:5>
Django
Permissions.
------------------------------+------------------------------------
Reporter: Vasanth | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34165#comment:6>
Django
Permissions.
------------------------------+------------------------------------
Reporter: Vasanth | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------
* has_patch: 1 => 0
Comment:
Aryan, this ticket doesn't have submitted PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/34165#comment:7>
Django
Permissions.
------------------------------+------------------------------------
Reporter: Vasanth | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.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
------------------------------+------------------------------------
Comment (by David Wobrock):
Replying to [comment:3 Mariusz Felisiak]:
> Thanks for this report, it's related with adding missing permissions. I
was able to fix this by setting `_state.db`, however I'm not convinced
that it's the best solution:
> {{{#!diff
> diff --git a/django/contrib/auth/management/__init__.py
b/django/contrib/auth/management/__init__.py
> index 0b5a982617..27fe0df1d7 100644
> --- a/django/contrib/auth/management/__init__.py
> +++ b/django/contrib/auth/management/__init__.py
> @@ -94,12 +94,15 @@ def create_permissions(
> )
> .values_list("content_type", "codename")
> )
> -
> - perms = [
> - Permission(codename=codename, name=name, content_type=ct)
> - for ct, (codename, name) in searched_perms
> - if (ct.pk, codename) not in all_perms
> - ]
> + perms = []
> + for ct, (codename, name) in searched_perms:
> + if (ct.pk, codename) not in all_perms:
> + permission = Permission()
> + permission._state.db = using
> + permission.codename = codename
> + permission.name = name
> + permission.content_type = ct
> + perms.append(permission)
> Permission.objects.using(using).bulk_create(perms)
> if verbosity >= 2:
> for perm in perms:
>
> }}}
>
> Partly related to #29843.
I think `bulk_create` already sets the `_state.db` to the value passed in
`.using()`, right?
Or is it in `bulk_create` that we require `_state.db` to be set earlier?
In which case, we could perhaps change something inside of this method.
Replying to [comment:5 Vasanth]:
> After diving a bit deeper it turned out that the issue was with one of
the libraries in my project which was not adapted for multi-DB. I've made
a PR with changes on the django-admin-interface which resolved my issue.
So would it be relevant to close the issue or is the bug really related to
Django itself?
--
Ticket URL: <https://code.djangoproject.com/ticket/34165#comment:8>
Django
Permissions.
------------------------------+------------------------------------
Reporter: Vasanth | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.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
------------------------------+------------------------------------
* cc: David Wobrock (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34165#comment:9>
Django
Permissions.
------------------------------+------------------------------------
Reporter: Vasanth | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.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
------------------------------+------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:8 David Wobrock]:
> I think `bulk_create` already sets the `_state.db` to the value passed
in `.using()`, right?
Yes, but it's a different issue, strictly related with `Permission` and
its `content_type`. `get_content_type()` is trying to find a content type
using `obj._state.db` so when we create a `Permission()` without
`._state.db` it will first try to find a content type in the default db.
> So would it be relevant to close the issue or is the bug really related
to Django itself?
IMO we should fix this for permissions.
--
Ticket URL: <https://code.djangoproject.com/ticket/34165#comment:10>
Django
Permissions.
------------------------------+------------------------------------
Reporter: Vasanth | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.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
------------------------------+------------------------------------
Comment (by David Wobrock):
Replying to [comment:10 Mariusz Felisiak]:
> Replying to [comment:8 David Wobrock]:
> > I think `bulk_create` already sets the `_state.db` to the value passed
in `.using()`, right?
>
> Yes, but it's a different issue, strictly related with `Permission` and
its `content_type`. `get_content_type()` is trying to find a content type
using `obj._state.db` so when we create a `Permission()` without
`._state.db` it will first try to find a content type in the default db.
Okay, I understand the issue now, thanks for the details!!
First thing, it makes me wonder why we require to have a DB attribute set,
at a moment where we are not (yet) interacting with the DB.
So we are currently checking, when setting the `content_type` FK, that the
router allows this relation. I guess one option is to not do that for not-
saved model instances.
Would it make sense to defer this to when we start interacting with the
DB? But it brings a whole other lot of changes and challenges, like
changing a deep behaviour of FKs and multi-tenancy :/
Apart from that, if we don't want to set directly the internal attribute
`_state.db`, I guess we would need a proper way to pass the `db`/`using`
to the model instantiation.
What would be the most ''Django-y'' way?
- Passing it through the model constructor => this has quite a large
impact, as a keyword argument would possibly shadow existing field names:
`Permission(..., db=using)`. Quite risky in terms of backward
compatibility I guess.
- Adding a method to `Model`? Something like: `Permission(...).using(db)`,
which could perhaps then be re-used in other places also.
What do you think ? :) Or am I missing other solutions?
--
Ticket URL: <https://code.djangoproject.com/ticket/34165#comment:11>
Django
Permissions.
------------------------------+------------------------------------
Reporter: Vasanth | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.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
------------------------------+------------------------------------
Comment (by Mariusz Felisiak):
> Apart from that, if we don't want to set directly the internal attribute
`_state.db`, I guess we would need a proper way to pass the `db`/`using`
to the model instantiation.
`_state` is
[https://docs.djangoproject.com/en/4.1/ref/models/instances/#state
documented] so using it is not so bad.
> What would be the most ''Django-y'' way?
> - Passing it through the model constructor => this has quite a large
impact, as a keyword argument would possibly shadow existing field names:
`Permission(..., db=using)`. Quite risky in terms of backward
compatibility I guess.
> - Adding a method to `Model`? Something like:
`Permission(...).using(db)`, which could perhaps then be re-used in other
places also.
>
> What do you think ? :) Or am I missing other solutions?
Django doesn't support cross-db relationships and users were always
responsible for assigning related objects from the same db. I don't think
that we should add more logic to do this. The `Permission`-`content_type`
issue is really an edge case in managing relations, as for me we don't
need a generic solution for it.
--
Ticket URL: <https://code.djangoproject.com/ticket/34165#comment:12>
Django
Permissions.
Reporter: Vasanth | Owner: David Wobrock
Type: Bug | Status: assigned
Component: contrib.auth | Version: 4.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Wobrock):
* owner: nobody => David Wobrock
* status: new => assigned
* has_patch: 0 => 1
Comment:
Thanks for all the details Mariusz!
Then let's try with a basic [https://github.com/django/django/pull/16400
PR] that uses your suggested fix :)
--
Ticket URL: <https://code.djangoproject.com/ticket/34165#comment:13>
Django
Permissions.
Reporter: Vasanth | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: contrib.auth | Version: 4.1
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34165#comment:14>
Django
Permissions.
-------------------------------------+-------------------------------------
Reporter: Vasanth | Owner: David
| Wobrock
Component: contrib.auth | Version: 4.1
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"5aaad5f39c67591ba3d60c57ffa03fac12965f26" 5aaad5f]:
{{{
#!CommitTicketReference repository=""
revision="5aaad5f39c67591ba3d60c57ffa03fac12965f26"
Fixed #34165 -- Made permissions creation respect the "using" parameter.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34165#comment:15>