[Django] #30427: Descriptors not accessible for inherited models.
Django
-------------------------------------+-------------------------------------
Reporter: Jarek | Owner: nobody
Glowacki |
Type: Bug | Status: new
Component: Database | Version: master
layer (models, ORM) | Keywords: inherited
Severity: Normal | descriptor deferred
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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..
--
Ticket URL: <https://code.djangoproject.com/ticket/30427>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: inherited | Triage Stage: Accepted
descriptor deferred |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: inherited | Triage Stage: Accepted
descriptor deferred |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
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 in the
first place, seems like we should be just letting the mro do its job and
always override, no matter what it is we're overriding.
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: inherited | Triage Stage: Accepted
descriptor deferred |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: inherited | Triage Stage: Accepted
descriptor deferred |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
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 `is_attname_settable` field 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: inherited | Triage Stage: Accepted
descriptor deferred |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by felixxm):
Related issue #16176.
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: inherited | Triage Stage: Accepted
descriptor deferred |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/11337#pullrequestreview-347194094
Comments on PR]
Currently MRO for field inheritance is affected. We
[https://docs.djangoproject.com/en/3.0/topics/db/models/#field-name-
hiding-is-not-permitted already advertise this as not exactly like Python
inheritance] so **maybe** this would be acceptable if suitable 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...)
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:6>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: inherited | Triage Stage: Accepted
descriptor deferred |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Hongtao Ma (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:7>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: inherited | Triage Stage: Accepted
descriptor deferred |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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
False
>>> User.objects.defer('is_active').get().is_active
True
}}}
(and other discussion.)
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:8>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
descriptor deferred | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
This has had quite a journey. I think we're there. Thanks all.
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:9>
Django
-------------------------------------+-------------------------------------
| Glowacki
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: inherited | Triage Stage: Ready for
descriptor deferred | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Jarek Glowacki
* status: new => assigned
Comment:
[https://github.com/django/django/pull/11337 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:10>
Django
-------------------------------------+-------------------------------------
| Glowacki
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
descriptor deferred |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:11>
Django
-------------------------------------+-------------------------------------
| Glowacki
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: inherited | Triage Stage: Accepted
descriptor deferred |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
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.
[https://github.com/django/django/pull/11337#pullrequestreview-454009820
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:12>
Django
-------------------------------------+-------------------------------------
| Glowacki
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: inherited | Triage Stage: Accepted
descriptor deferred |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
Resetting flag to match PR discussion.
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:13>
Django
-------------------------------------+-------------------------------------
| Gibson
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: inherited | Triage Stage: Accepted
descriptor deferred |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Jarek Glowacki => Carlton Gibson
* needs_docs: 0 => 1
Comment:
[https://github.com/django/django/pull/14508 Updated PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:14>
Django
-------------------------------------+-------------------------------------
| Gibson
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: inherited | Triage Stage: Accepted
descriptor deferred |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:15>
Django
-------------------------------------+-------------------------------------
| Gibson
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
descriptor deferred | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:16>
Django
-------------------------------------+-------------------------------------
| Gibson
Component: Database layer | Version: dev
(models, ORM) |
Keywords: inherited | Triage Stage: Ready for
descriptor deferred | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
Fixed in 225d96533a8e05debd402a2bfe566487cc27d95f
--
Ticket URL: <https://code.djangoproject.com/ticket/30427#comment:17>