[Django] #29386: Meta Inheritance for default_permissions
Django
-----------------------------------------+------------------------
Reporter: Clayton Daley | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.11
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 |
-----------------------------------------+------------------------
I add Read/Browse permissions to my models that I enforce in my views.
For some reason, `default_permissions` isn't inheriting in the way I would
expect. Nor does the Meta Inheritance section of the documentation say
anything to suggest this should not be valid.
Given something like:
{{{
class BreadMetaMixin(object):
# ACTION_EDIT aliases "change" for BREAD semantics, see
http://paul-m-jones.com/archives/291
default_permissions = (ACTION_BROWSE, ACTION_READ, ACTION_EDIT,
ACTION_ADD, ACTION_DELETE)
class Encounter(AbstractParentModel):
class Meta(BreadMetaMixin, AbstractParentModel.Meta):
pass
class Admission(Encounter):
class Meta(Encounter.Meta):
# also reproduced with non-proxy model
proxy = True
}}}
When I `makemigrations`, Encounter detects the new permissions...
{{{
...
operations = [
migrations.CreateModel(
name='Encounter',
...
options={
'default_permissions': ('browse', 'read', 'change', 'add',
'delete'),
'abstract': False,
},
...
}}}
but Admissions does not:
{{{
...
operations = [
migrations.CreateModel(
name='Admission',
...
options={
'abstract': False,
'proxy': True, # also verified with non-proxy model
'indexes': [],
},
...
}}}
It's possible that this hasn't come up because it's masked by #11154. I
noticed it because I'm working around #11154 using a migration like:
{{{
def add_proxy_permissions(apps, schema_editor):
"""Workaround https://code.djangoproject.com/ticket/11154 by manually
creating the required permissions."""
ContentType = apps.get_model('contenttypes', 'ContentType')
Permission = apps.get_model('auth', 'Permission')
for model in ['Admission']:
class_ = apps.get_model('task_manager', model)
content_type = ContentType.objects.get_for_model(class_,
for_concrete_model=False)
for perm in class_._meta.default_permissions:
Permission.objects.get_or_create(
content_type=content_type,
codename='{}_{}'.format(perm, model.lower()),
)
}}}
This works great on two meta models where `BreadMetaMixin` is included
directly (e.g. `UserProxy` wraps `auth.models.User` and includes
`BreadMetaMixin`). When I moved the code over to `Admission`, however, it
wasn't working correctly. If I mixin `BreadMetaMixin` directly, it works
fine so I've narrowed it down to a Meta Inheritance issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/29386>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Clayton Daley | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.11
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Clayton Daley:
Old description:
New description:
Given something like:
but Admissions does not:
It's possible that this hasn't come up because it's masked by #11154, but
it affects non-proxy models as well. I noticed it because I'm working
around #11154 using a migration like:
{{{
def add_proxy_permissions(apps, schema_editor):
"""Workaround https://code.djangoproject.com/ticket/11154 by manually
creating the required permissions."""
ContentType = apps.get_model('contenttypes', 'ContentType')
Permission = apps.get_model('auth', 'Permission')
for model in ['Admission']:
class_ = apps.get_model('task_manager', model)
content_type = ContentType.objects.get_for_model(class_,
for_concrete_model=False)
for perm in class_._meta.default_permissions:
Permission.objects.get_or_create(
content_type=content_type,
codename='{}_{}'.format(perm, model.lower()),
)
}}}
This works great on two meta models where `BreadMetaMixin` is included
directly (e.g. `UserProxy` wraps `auth.models.User` and includes
`BreadMetaMixin`). When I moved the code over to `Admission`, however, it
wasn't working correctly. If I mixin `BreadMetaMixin` directly, it works
fine so I've narrowed it down to a Meta Inheritance issue.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29386#comment:1>
Django
Reporter: Clayton Daley | Owner: nobody
Component: Uncategorized | Version: 1.11
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => invalid
Comment:
This is expected behaviour.
The [https://docs.djangoproject.com/en/2.0/topics/db/models/#proxy-models
Proxy models docs] say:
> Proxy models inherit Meta attributes in the same way as regular models.
Here they link to the
[https://docs.djangoproject.com/en/2.0/topics/db/models/#meta-and-multi-
table-inheritance Meta and and multi-table inheritance section] which
says:
> ... it doesn’t make sense for a child class to inherit from its parent’s
Meta class... However, there are a few limited cases where the child
inherits behavior from the parent: if the child does not specify an
`ordering` attribute or a `get_latest_by attribute`, it will inherit these
from its parent.
`default_permissions` is not amongst the listed exceptions, so there is no
reason to expect it would be inherited.
--
Ticket URL: <https://code.djangoproject.com/ticket/29386#comment:2>
Django
Reporter: Clayton Daley | Owner: nobody
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* component: Uncategorized => Database layer (models, ORM)
--
Ticket URL: <https://code.djangoproject.com/ticket/29386#comment:3>
Django
Reporter: Clayton Daley | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Clayton Daley):
* status: closed => new
* resolution: invalid =>
Comment:
The section you cited says "child class to inherit from its parent’s Meta
class" which I interpret as the following:
{{{
class Parent(models.Model):
class Meta:
<stuff here>
class Child(Parent):
# no explicit meta definition
}}}
In my case, the Meta class is explicitly a subclass of the parent's Meta:
{{{
class Encounter(AbstractParentModel):
class Meta(BreadMetaMixin, AbstractParentModel.Meta):
pass
class Admission(Encounter):
class Meta(Encounter.Meta): # EXPLICIT inheritance
pass
}}}
So this should be covered by the Meta Inheritance documentation
([https://docs.djangoproject.com/en/2.0/topics/db/models/#meta-
inheritance]). This section specifically says:
> If the child wants to extend the parent’s Meta class, it can subclass
it... Django does make one adjustment to the Meta class of an abstract
base class: before installing the Meta attribute, it sets abstract=False.
It seems to me that it's also ignoring `default_permissions`. At minimum,
this is a documentation issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/29386#comment:4>
Django
Reporter: Clayton Daley | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Replying to [comment:4 Clayton Daley]:
> The section you cited says "child class to inherit from its parent’s
Meta class" ...
Those words go " it doesn’t make sense for a child class to inherit from
its parent’s Meta class". Don't you think the "it doesn't make sense" is
clear enough?
It follows "All the Meta options have already been applied to the parent
class and applying them again would normally only lead to contradictory
behavior".
What else do you want it to say?
--
Ticket URL: <https://code.djangoproject.com/ticket/29386#comment:5>
Django
Reporter: Clayton Daley | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Clayton Daley):
I only cited that section to point out that it describes a different
case... implicit inheritance of a parent's Meta Class. In that case, the
documentation is fine and I believe the behavior is expected.
I followed the Meta Inheritance section which states "If the child wants
to extend the parent’s Meta class, it can subclass it". I wanted to
extend it. I subclassed it. In this section, it states that only one
change is made... `abstract` is set to `False`. It does not state that
`default_permissions` are ignored. But they seem to be.
--
Ticket URL: <https://code.djangoproject.com/ticket/29386#comment:6>
Django
Reporter: Clayton Daley | Owner: nobody
Component: Database layer | Version: 1.11
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => invalid
Comment:
That section if for abstract inheritance, one of the three types (the
others being multi-table and proxy). It's saying that when you inherit
from an abstract model Meta is inherited (and can be overridden.)
Your case is Proxy inheritance, which links to the multi-table rules for
inheritance (which I cited above). It's important not to conflate the
different types of inheritance.
I appreciate it's complex and not always as expected every time,
especially when the examples become more developed, as are yours.
This still seems to be expected behaviour. I'm going to close this as
such. If you have a concrete suggestion for the docs—there's always the
possibility of improvement—we'd be very happy to review a PR referencing
this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/29386#comment:7>
Django
Reporter: Clayton Daley | Owner: nobody
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution: invalid
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Clayton Daley):
Thanks. I finally see the chain of references that would have brought me
to that conclusion. It sounds like Meta Inheritance is supported under
few, very specific conditions... something like:
- A is the parent of B
- A.Meta is the parent of B.Meta
- A.Meta is abstract
If a concise set of conditions could be described, would you be open to a
PR that threw `ImproperlyConfigured` under other conditions? This seems
like an ideal case for an exception because Meta Inheritance is a best
practice in some cases (i.e. abstract inheritance) and deliberately
ineffective in others (Multi-Table, Proxy). A reasonably informative
error message would eliminate the need to even go to the documentation.
--
Ticket URL: <https://code.djangoproject.com/ticket/29386#comment:8>
Django
Reporter: Clayton Daley | Owner: nobody
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution: invalid
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hi Clayton.
I'm struggling to think what such a PR would look like — but yes — happy
to look at one. (It's always easier to think about with actual code.)
My only initial thought would be to use a system check, rather than an
`ImproperlyConfigured`.
If you come up with something call the PR `Ref #29386 -- ...` and
@carltongibson me on GitHub. I'll have a look.
--
Ticket URL: <https://code.djangoproject.com/ticket/29386#comment:9>