+1 from me Afraid I don't know any of the why.
Also worth noting from the PR description: Previously: DeferredAttributes would not get stapled onto models where the > model (or an ancestor) already had an existing *non-falsey* attribute. > Non-falsey! I almost spat out my tea. Thanks for the detailed write up with reference links. On Wed, 4 Mar 2020 at 11:10, Carlton Gibson <[email protected]> wrote: > Hi all. > > Especially from those with long memories, can I request thoughts on a > proposed > adjustment to `Field.contribute_to_class()`, which affects the manner in > which > fields override attributes from parent classes? > > The entails a breaking change. As such... 😬 — but I think it's acceptable > and > worth it. > > I'm posting here to ensure we get sufficient review on that. > > Sorry for the long post. I've tried to lay it out as clearly as possible. > Thank you for your time and effort. > > The pull request for all this is here: > > https://github.com/django/django/pull/11337 > > Details > ------- > > There are a cluster of tickets pertaining to the behavior of > `Field.contribute_to_class()`: > > * Descriptors not accessible for inherited models. > https://code.djangoproject.com/ticket/30427 > * Overwriting a property with field during model inheritance. > https://code.djangoproject.com/ticket/16176 > * Model attributes shouldn't override deferred fields > https://code.djangoproject.com/ticket/28198 > > The are all related. Ultimately, they're different versions of the same > bug. > > The essence of it looks like this: > > ``` > >>> 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 > ``` > > Which happens for two reasons: > > 1. `AbstractBaseUser` provides a scalar default for the `is_active` field: > > class AbstractBaseUser(models.Model): > ... > is_active = True > ... > > 2. `Field.contribute_to_class()` will then not set the field descriptor on > the > subclass: > > if self.column: > # Don't override classmethods with the descriptor. This means > that > # if you have a classmethod and a field with the same name, > then > # such fields can't be deferred (we don't have a check for > this). > if not getattr(cls, self.attname, None): > setattr(cls, self.attname, self.descriptor_class(self)) > > This leads to all the issues above, plus additional problems in the > eco-system. > e.g. https://github.com/jazzband/django-model-utils/issues/382 > > The payoff of the proposed change is fixing all these issues. > > § > > The history didn't tell me much so, I mailed the list in June last year to > ask > if anybody had any recollection of the _Why?_ to that _"Don't override > classmethods with the descriptor"_. > > https://groups.google.com/d/msg/django-developers/zXB0oJ8tD3E/nH8yx_FpBAAJ > > Nobody could remember that _Why_. > > * One post, saying: > > > I’ve run into this a few times, and the current behaviour has felt > > jarring and unpythonic. > > * One post pointing to one of the related tickets above. > > Again then, maybe last chance, do you have a long memory? Do you know that > _Why_? :) > > § > > Discussing #16176, Carl Meyer made a comment that was never picked up on: > > > [The] suggestion to set all fields as class attributes so they override > > descriptors from a parent class is certainly worth considering, as it > would > > make models behave more like regular Python classes. This could result in > > some backwards-incompatibility; would need to look into it more to get a > > clearer sense of what cases might cause trouble, but I think it's > probably > > acceptable if documented in the release notes. > > https://code.djangoproject.com/ticket/16176#comment:6 > > The idea is to implement that, always setting the descriptor in the `if > self.column` block: > > if self.column: > setattr(cls, self.attname, self.descriptor_class(self)) > > § > > Implementing that in a PR, leads to **three** failures in the current > tests. > > PR: https://github.com/django/django/pull/11337 > > Two failures are tests for system checks, which cover edge cases arising > from > the existing behavior. I think the tests can be adjusted safely: > > * `models.E020`. > > Originally added for: `Fixed #23615 -- Validate that a Model > instance's "check" attribute is a method` > > https://github.com/django/django/commit/a5c77417a651c93036cf963e6da518653115be7e > > PR Diff: > > https://github.com/django/django/pull/11337/files#diff-dc8c55ebf8072120b505dd8dade78efcR289-R294 > > * `models.E025`. > > Originally added for: ` Fixed #28867 -- Added system check for a model > property that clashes with a related field accessor.` > > https://github.com/django/django/commit/cc6bcc6ff5cab320c5e5ae2760549a6c732067d8 > > PR Diff: > > https://github.com/django/django/pull/11337/files#diff-bbed6332ed721c634e1b48b0eddd04a0R917-R918 > > § > > The third failure concerns the apparent MRO of fields inherited from > abstract > parent classes. > > This is the breaking change. > > Python MRO: > > * For simple inheritance structures, MRO follows a depth-first strategy. > * For complex structures, with diamond-like multiple-inheritance, MRO is > "depth-first-with-pull-up". That is, siblings with shared ancestors are > inserted into the MRO before their ancestors, in violation of a strict > depth-first ordering. > > (If anybody has a better wording than "depth-first-with-pull-up", I would > be > glad to hear it. 🙂) > > With the proposed change apparent field inheritance from abstract parent > models > goes from following Python MRO rules to being strictly depth-first, even > in the > cases of diamond-like multiple inheritance. > > This is a breaking change, but it's in line with what we already say about > model field inheritance: > > > Overriding fields in a parent model leads to difficulties in areas such > as > > initializing new instances (specifying which field is being initialized > in > > Model.__init__) and serialization. These are features which normal Python > > class inheritance doesn’t have to deal with in quite the same way, so the > > difference between Django model inheritance and Python class inheritance > > isn’t arbitrary. > > > https://docs.djangoproject.com/en/3.0/topics/db/models/#field-name-hiding-is-not-permitted > > The work-around for this is that folks affected will need to _push-down_ > model > field declarations to child classes. > > Test originally for `Fixed #24305 -- Allowed overriding fields on abstract > models.` > > https://github.com/django/django/commit/85ef98dc6ec565b1add417bd76808664e7318026 > > Diff in new PR: > > https://github.com/django/django/pull/11337/files#diff-36659ace33d899c344c6d08f29b6a530R64-R65 > > § > > Conclusion > ---------- > > Is dropping the system check test cases acceptable? > > Is the MRO breaking change acceptable? For me yes: > > * It fixes the bunch of tickets here. (See the PR for the **added** tests.) > * In most cases it will make models behave more like regular Python > classes. > * In complex inheritance case, the push-down field refactoring is > available. > > Despite no other breaking tests, is there another reason we can't make > this change? > (No doubt someone, somewhere is **using** the existing behavior, but...) > > Again, the PR: https://github.com/django/django/pull/11337 > > § > > Thank you for reading this far. Sorry again for the length but... > > Thank you for your time and thought. > > Kind Regards, > > Carlton > > -- > You received this message because you are subscribed to the Google Groups > "Django developers (Contributions to Django itself)" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/django-developers/d64e540e-7a55-4e13-8077-baa4b7f08805%40googlegroups.com > <https://groups.google.com/d/msgid/django-developers/d64e540e-7a55-4e13-8077-baa4b7f08805%40googlegroups.com?utm_medium=email&utm_source=footer> > . > -- Adam -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMyDDM3SToJV_AFqVJPxhKYy5WXMr1%2B44gU_ZHVRnZD0xJALHg%40mail.gmail.com.
