Descriptors not accessible for inherited models.

[Jarek Glowacki]

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..

[Mariusz Felisiak]

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.

[Jarek Glowacki]

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.

[Carlton Gibson]

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.

[Jarek Glowacki]

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.

[Mariusz Felisiak]

Related issue #16176.

[Carlton Gibson]

​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...)

[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.)

[Carlton Gibson]

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

[Carlton Gibson]

​PR

[Carlton Gibson]

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.

[Jacob Walls]

Resetting flag to match PR discussion.

[Carlton Gibson]

​Updated PR.

[Carlton Gibson]

Fixed in 225d96533a8e05debd402a2bfe566487cc27d95f