Opened 5 months ago

Last modified 23 hours ago

#28198 assigned Bug

Model attributes shouldn't override deferred fields

Reported by: Ryan Hiebert Owned by: Denis.Tarykin
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: me@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
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 (6)

comment:1 Changed 5 months ago 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.

Last edited 5 months ago by Aymeric Augustin (previous) (diff)

comment:2 Changed 5 months ago by Tim Graham

Component: UncategorizedDatabase layer (models, ORM)
Summary: Doesn't override existing class attributes with a deferred instance attributeModel attributes shouldn't override deferred fields
Triage Stage: UnreviewedAccepted

comment:3 Changed 5 months ago by Adam (Chainz) Johnson

Cc: me@… added

comment:4 Changed 6 weeks ago by Denis.Tarykin

Owner: changed from nobody to Denis.Tarykin
Status: newassigned

comment:5 Changed 4 weeks ago by Denis.Tarykin

Has patch: set

comment:6 Changed 23 hours ago by Ryan Hiebert

Triage Stage: AcceptedReady 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.

Note: See TracTickets for help on using tickets.
Back to Top