Review needed: Proposed behavior change in Field.contribute_to_class()

[Carlton Gibson]

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

[Adam Johnson]

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

--
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 django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/d64e540e-7a55-4e13-8077-baa4b7f08805%40googlegroups.com.

--

Adam

[Mariusz Felisiak]

+1 from me.

Best,
Mariusz