[Django] #28198: Doesn't override existing class attributes with a deferred instance attribute
Django
attribute
-----------------------------------------+------------------------
Reporter: Ryan Hiebert | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
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 |
-----------------------------------------+------------------------
If you have a base model (whether concrete or abstract) with a generic
property set, and it is overridden by a field in a subclass.
Consider the following models:
{{{#!python
from django.db import models
class Super(models.Model):
is_super = True
class Sub(Super):
is_super = models.BooleanField(default=False)
}}}
Then, you would see this in a shell session:
{{{#!pycon
>>> sub = Sub.objects.create()
>>> sub.is_super
False
>>> sub_defer = Sub.objects.defer('is_super').get()
>>> sub_defer.is_super
True
>>> sub_defer.refresh_from_db()
>>> sub_defer.is_super
True
>>> Sub.objects.get().is_super
False
}}}
The deferred field doesn't get fixed, even by calling `refresh_from_db()`.
It also behaves this way if the Super model is an abstract model. A
particular instance of this is with `auth.User`, which displays this
behavior with the `is_active` field, which gets the value from
`AbstractBaseUser`.
{{{#!pycon
>>> from django.contrib.auth.models import User
>>> User.objects.create_user(username='spam', password='eggs',
is_active=False)
<User: spam>
>>> User.objects.get().is_active
False
>>> User.objects.defer('is_active').get().is_active
True
}}}
------
I found this bug while trying to get creative, and loading a sparse User
model with `User.from_db(None, ['id'], [1])`, to avoid a database access
when I just wanted to use it as a filter argument and I already had the
user id. It turned out, though, that it would affect any deferred access
of this property, whether using `from_db`, `defer`, or `only`.
I found this issue in Django 1.11.1, and I've confirmed that it exists in
master, all the way back through 1.8, which was the earliest version I
could use with the manage.py that was created with master Django, since
1.7 was Python 2 only.
I first sent an email to the security list out of an abundance of caution.
Because of the quite limited exposure this would cause, we decided to move
it over to a public issue.
This was the response, with some helpful information as to the cause of
the issue:
> The general problem is that Django doesn't override existing class
attributes with a deferred instance attribute:
https://github.com/django/django/blob/a87189fc5e2e874219d20700cf811345138dd365/django/db/models/fields/__init__.py#L699-L703.
>
> In the case of User.is_active, the class attribute defaulting to True
was added in Django 1.6 by
https://github.com/django/django/commit/4ea8105120121c7ef0d3dd6eb23f2bf5f55b496a
in order to fix https://code.djangoproject.com/ticket/19061.
>
> Unfortunately this looks like it isn't going to be trivial to fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/28198>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
attribute
Reporter: Ryan Hiebert | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Aymeric Augustin):
Regarding the piece of code highlighted in the report above, I don't
understand why fields shouldn't override classmethods.
If I look at this piece of code:
class A(models.Model):
@classmethod
def foo(self, bar):
raise FooError
class B(A):
foo = models.FooField()
and guess what B().foo(bar) does, I'm not guessing "FooError", I'm saying
"TypeError: ... is not callable".
Simply removing the condition breaks the tests for multiple inheritance,
for reasons I haven't quite figured out.
In `django.db.models.base.ModelBase` (the metaclass for `Model`) Django
builds inherited models child-to-parent by tracking which fields are
already defined, likely to avoid calling `contribute_to_class` on fields
from parents that are overridden in fields from children. This code is a
bit complicated and I'm having trouble figuring out how to modify it.
--
Ticket URL: <https://code.djangoproject.com/ticket/28198#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Ryan Hiebert | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
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):
* stage: Unreviewed => Accepted
* component: Uncategorized => Database layer (models, ORM)
--
Ticket URL: <https://code.djangoproject.com/ticket/28198#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam (Chainz) Johnson):
* cc: me@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/28198#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Ryan Hiebert | Owner:
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Denis.Tarykin):
* owner: nobody => Denis.Tarykin
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/28198#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Ryan Hiebert | Owner:
| Denis.Tarykin
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Denis.Tarykin):
* has_patch: 0 => 1
Comment:
Pull Request:[https://github.com/django/django/pull/9126]
--
Ticket URL: <https://code.djangoproject.com/ticket/28198#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Ryan Hiebert | Owner:
| Denis.Tarykin
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
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 Ryan Hiebert):
* stage: Accepted => Ready for checkin
Comment:
I've tested the patch against the {{{auth.User}}} issue mentioned
initially, and have reviewed the patch. It looks good to me, so I hope
it's acceptable for me to mark it as Ready for Checkin, despite being the
one who opened the issue. I anticipate that it would be appropriate to
backport to supported versions of Django.
--
Ticket URL: <https://code.djangoproject.com/ticket/28198#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Ryan Hiebert | Owner:
| Denis.Tarykin
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
I left some comments for improvement on the pull request. Please uncheck
"Patch needs improvement" after updating.
--
Ticket URL: <https://code.djangoproject.com/ticket/28198#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Ryan Hiebert | Owner:
| Denis.Tarykin
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Denis.Tarykin):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/28198#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Ryan Hiebert | Owner:
| Denis.Tarykin
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
Comment:
Feedback on patch:
* At the least, please break out the logic adjusting attributes on parent
models and add tests for those.
* Can we not do this without actually altering the parent models? (e.g.
one possibility, by noting duplicates and checking against that in
`Field.contribute_to_class`)
* This should be helped by extracting the helper methods but, it wasn't
clear what the code was doing and why. The comments should be clarified if
necessary.
Please uncheck "Patch needs improvement" after updating.
--
Ticket URL: <https://code.djangoproject.com/ticket/28198#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Ryan Hiebert | Owner:
| Denis.Tarykin
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: assigned => closed
* resolution: => duplicate
Comment:
Whilst this was earlier, closing as a duplicate of #30427.
(Thanks all.)
--
Ticket URL: <https://code.djangoproject.com/ticket/28198#comment:10>