Opened 7 years ago
Closed 5 years ago
#28198 closed Bug (duplicate)
Model attributes shouldn't override deferred fields
Reported by: | Ryan Hiebert | Owned by: | Denis.Tarykin |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | me@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
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:
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:
>>> 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
.
>>> 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.
Change History (10)
comment:2 by , 7 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|---|
Summary: | Doesn't override existing class attributes with a deferred instance attribute → Model attributes shouldn't override deferred fields |
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 7 years ago
Cc: | added |
---|
comment:4 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 7 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
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.
comment:7 by , 7 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
I left some comments for improvement on the pull request. Please uncheck "Patch needs improvement" after updating.
comment:8 by , 7 years ago
Patch needs improvement: | unset |
---|
comment:9 by , 7 years ago
Patch needs improvement: | set |
---|
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.
comment:10 by , 5 years ago
Resolution: | → duplicate |
---|---|
Status: | assigned → closed |
Whilst this was earlier, closing as a duplicate of #30427.
(Thanks all.)
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:
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 forModel
) Django builds inherited models child-to-parent by tracking which fields are already defined, likely to avoid callingcontribute_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.