Opened 6 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 |
Description
Example:
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 1 >>> 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 , 6 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 6 years ago
Has patch: | set |
---|
Issue was introduced here: https://github.com/django/django/pull/6491/files#diff-bf776a3b8e5dbfac2432015825ef8afeR699
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.
comment:3 by , 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: https://groups.google.com/forum/#!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.
comment:4 by , 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.
comment:6 by , 5 years ago
Patch needs improvement: | set |
---|
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...)
comment:7 by , 5 years ago
Cc: | added |
---|
comment:8 by , 5 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 False >>> User.objects.defer('is_active').get().is_active True
(and other discussion.)
comment:9 by , 5 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
This has had quite a journey. I think we're there. Thanks all.
comment:11 by , 5 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
comment:12 by , 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.
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:15 by , 3 years ago
Needs documentation: | unset |
---|
comment:16 by , 3 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:17 by , 3 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Thanks for the report. Described behavior of
models.Model
it's been there since at least1.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.