Opened 16 months ago

Last modified 2 weeks ago

#30427 assigned Bug

Descriptors not accessible for inherited models.

Reported by: Jarek Glowacki Owned by: Jarek Glowacki
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: inherited descriptor deferred
Cc: Hongtao Ma Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no



class Base(models.Model):
    A = 1

class Inherited(Base):
    A = models.IntegerField(default=0)
    B = models.IntegerField(default=0)

And now take a look at this:

>>> Inherited.A
>>> Inherited.B
<django.db.models.query_utils.DeferredAttribute object at 0x110ed5470>

Descriptor A is not accessible.
Behaviour is correct when accessing instance attributes, which is why I think this has gone under the radar..

Real use case:
We try to apply a FieldTracker (from Django Model Utils) onto a custom user model:

class User(AbstractBaseUser):
    is_active = models.BooleanField(_('active'), default=True)

    tracker = FieldTracker()

FieldTracker falls over wrapping the is_active field, because instead of getting a DeferredAttribute when accessing User.is_active, it gets a mouthful of True, which is the value assigned to is_active in AbstractBaseUser.

Happy to submit a failing test and propose a fix if issue is accepted.
Issue replicated on Django2.1.5, but I suspect it's like this on master still..

Change History (12)

comment:1 Changed 16 months ago by felixxm

Triage Stage: UnreviewedAccepted

Thanks for the report. Described behavior of models.Model it's been there since at least 1.8 (I didn't check older releases). I'm not sure how fixable it is so, I tentatively accept this for future investigation. Patch will help with the final decision.

comment:2 Changed 15 months ago by Jarek Glowacki

Has patch: set

Issue was introduced here:

Fixing it will not be backwards compatible (obviously), but we can remain true to the in-code comment and protect class methods from being overridden.
That check currently also lets other falseys slip through. ie if my above example had been setting A = 0 instead of A = 1, the deferred attribute would've come into effect properly. Such behaviour feels very wishy-washy (it should've been comparing to None at least), so I feel it would be safe to introduce a change to this into the next release (or perhaps even as a bugfix into this one), with a oneline statement in the release notes to warn anyone who might for some strange reason be relying on this behaviour..

I've submitted a PR. Works, but it leaves a question around what we should be doing about @attribute-decorated methods. These slip under the radar of the callable check. So either we need to check for them separately, or we should rethink whether there's a point to preventing overriding of class methods in the first place.

Thoughts? Does anyone know why we were protecting classmethods? Seems like we should be just letting the mro do its job and always override, no matter what it is we're overriding.

Last edited 15 months ago by Jarek Glowacki (previous) (diff)

comment:3 Changed 14 months ago by Carlton Gibson

Patch needs improvement: set

Patch as is needs updating to pass with @property decorators, but it's quite minimal.

There's a mailing list thread:!topic/django-developers/zXB0oJ8tD3E/discussion

I've asked for input from anyone who can remember the Why of how it is now.

Update: Ryan Hiebert points out that #28198 looks related.

Last edited 14 months ago by Carlton Gibson (previous) (diff)

comment:4 Changed 14 months ago by Jarek Glowacki

Patch needs improvement: unset

Updated patch to get tests passing.
The proposed patch addresses the problem presented in this ticket, but feels dirty -- there'll be similar problems if users try to define fields that clash with method names. But maybe we cross that bridge when we hit it.

FYI, having the new is_attname_settable method always return True causes the following tests to fail:

test_model_check_method_not_shadowed (check_framework.tests.CheckFrameworkReservedNamesTests)
test_property_and_related_field_accessor_clash (invalid_models_tests.test_models.OtherModelTests)

Interestingly, these tests only ensure checks are firing properly. Nothing else seems affected. So I guess if we wanted to, we could tweak those checks and get away with always overriding.
Would you be interested in a competing PR for that, to compare? Would involve more decisions being made about whether we just drop the checks that no longer work, or try to rejig them.

Last edited 14 months ago by Jarek Glowacki (previous) (diff)

comment:5 Changed 10 months ago by felixxm

Related issue #16176.

comment:6 Changed 7 months ago by Carlton Gibson

Patch needs improvement: set

Comments on PR

Currently MRO for field inheritance is affected. We already advertise this as not exactly like Python inheritance so maybe this would be acceptable if suitably documented.

More tests are needed to see if we can resolve all/most/some of #16176, #27807, #28198 with the change here, without further regressions. (If so then it seems the me that there's a good case to be made...)

Last edited 7 months ago by Carlton Gibson (previous) (diff)

comment:7 Changed 6 months ago by Hongtao Ma

Cc: Hongtao Ma added

comment:8 Changed 5 months ago by Carlton Gibson

#28198 was a duplicate of this, but it had the good User example:

>>> 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
>>> User.objects.defer('is_active').get().is_active

(and other discussion.)

comment:9 Changed 5 months ago by Carlton Gibson

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

This has had quite a journey. I think we're there. Thanks all.

comment:10 Changed 5 months ago by Carlton Gibson

Owner: changed from nobody to Jarek Glowacki
Status: newassigned

comment:11 Changed 4 months ago by felixxm

Triage Stage: Ready for checkinAccepted

comment:12 Changed 2 weeks ago by Carlton Gibson

Patch needs improvement: set

Reviewing again the change here does seem OK.

  • Apparent MRO is changed to the "strictly-depth-first" ordering.
  • That's OK, because the case it affects is previously ruled-out by the check framework.

See PR for longer comment.

Tests need a little clarification, just to document the exact behaviour clearly, but with that in place I think this should be good to go.

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