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.
