Opened 5 years ago

Closed 3 years ago

#30427 closed Bug (fixed)

Descriptors not accessible for inherited models.

Reported by: Jarek Glowacki Owned by: Carlton Gibson
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: inherited descriptor deferred
Cc: Hongtao Ma Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
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 (17)

comment:1 by Mariusz Felisiak, 5 years ago

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 by Jarek Glowacki, 5 years ago

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 5 years ago by Jarek Glowacki (previous) (diff)

comment:3 by Carlton Gibson, 5 years ago

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 5 years ago by Carlton Gibson (previous) (diff)

comment:4 by Jarek Glowacki, 5 years ago

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 5 years ago by Jarek Glowacki (previous) (diff)

comment:5 by Mariusz Felisiak, 5 years ago

Related issue #16176.

comment:6 by Carlton Gibson, 4 years ago

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 4 years ago by Carlton Gibson (previous) (diff)

comment:7 by Hongtao Ma, 4 years ago

Cc: Hongtao Ma added

comment:8 by Carlton Gibson, 4 years ago

#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 by Carlton Gibson, 4 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

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

comment:10 by Carlton Gibson, 4 years ago

Owner: changed from nobody to Jarek Glowacki
Status: newassigned

comment:11 by Mariusz Felisiak, 4 years ago

Triage Stage: Ready for checkinAccepted

comment:12 by Carlton Gibson, 4 years ago

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.

comment:13 by Jacob Walls, 3 years ago

Patch needs improvement: unset

Resetting flag to match PR discussion.

comment:14 by Carlton Gibson, 3 years ago

Needs documentation: set
Owner: changed from Jarek Glowacki to Carlton Gibson

comment:15 by Carlton Gibson, 3 years ago

Needs documentation: unset

comment:16 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:17 by Carlton Gibson, 3 years ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top