Opened 17 months ago

Last modified 8 months 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: 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 (9)

comment:1 Changed 17 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 17 months ago by Aymeric Augustin (previous) (diff)

comment:2 Changed 17 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 17 months ago by Adam (Chainz) Johnson

Cc: me@… added

comment:4 Changed 13 months ago by Denis.Tarykin

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

comment:5 Changed 12 months ago by Denis.Tarykin

Has patch: set

comment:6 Changed 11 months 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.

comment:7 Changed 11 months ago by Tim Graham

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

I left some comments for improvement on the pull request. Please uncheck "Patch needs improvement" after updating.

comment:8 Changed 11 months ago by Denis.Tarykin

Patch needs improvement: unset

comment:9 Changed 8 months ago by Carlton Gibson

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.

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