[Django] #30427: Descriptors not accessible for inherited models.

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[Django] #30427: Descriptors not accessible for inherited models.

Django
#30427: Descriptors not accessible for inherited models.
-------------------------------------+-------------------------------------
               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.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/050.b42f0112bd9f2e4fac8b55e4e825dc15%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30427: Descriptors not accessible for inherited models.

Django
#30427: Descriptors not accessible for inherited models.
-------------------------------------+-------------------------------------
     Reporter:  Jarek Glowacki       |                    Owner:  nobody
         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
-------------------------------------+-------------------------------------
Changes (by felixxm):

 * 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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.56ad1729bad219d21740de67ecf87b75%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30427: Descriptors not accessible for inherited models.

Django
In reply to this post by Django
#30427: Descriptors not accessible for inherited models.
-------------------------------------+-------------------------------------
     Reporter:  Jarek Glowacki       |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  master
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:  inherited            |             Triage Stage:  Accepted
  descriptor deferred                |
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Jarek Glowacki):

 * 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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.eb25f2238169f592a3a8154d531ac729%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30427: Descriptors not accessible for inherited models.

Django
In reply to this post by Django
#30427: Descriptors not accessible for inherited models.
-------------------------------------+-------------------------------------
     Reporter:  Jarek Glowacki       |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  master
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:  inherited            |             Triage Stage:  Accepted
  descriptor deferred                |
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):

 * 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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.3b6d4db8aa1b5a8d92e55762a5d65315%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30427: Descriptors not accessible for inherited models.

Django
In reply to this post by Django
#30427: Descriptors not accessible for inherited models.
-------------------------------------+-------------------------------------
     Reporter:  Jarek Glowacki       |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  master
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:  inherited            |             Triage Stage:  Accepted
  descriptor deferred                |
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Jarek Glowacki):

 * 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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.d5b5371521dc93b912d7230303846b8e%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30427: Descriptors not accessible for inherited models.

Django
In reply to this post by Django
#30427: Descriptors not accessible for inherited models.
-------------------------------------+-------------------------------------
     Reporter:  Jarek Glowacki       |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  master
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:  inherited            |             Triage Stage:  Accepted
  descriptor deferred                |
    Has patch:  1                    |      Needs documentation:  0
  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.57ebd53ee55197a8073370dc100cd9d6%40djangoproject.com.