[Django] #35301: Overriding a @property of an abstract class with a GenericRelation causes a models.E025 error.

[Django]

#35301: Overriding a @property of an abstract class with a GenericRelation causes a
models.E025 error.
-------------------------------------+-------------------------------------
Reporter: Sage | Owner: nobody
Abdullah |
Type: Bug | Status: new
Component: Database | Version: dev
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
As of faeb92ea13f0c1b2cc83f45b512f2c41cfb4f02d, Django traverses the
model's MRO to find the names of properties, to later be used in different
places, such as
[https://github.com/django/django/blob/80fe2f439102dc748ff8ddd661d94935915bd3e7/django/db/models/base.py#L1976-L1995
the check for models.E025]. However, this does not take into account any
properties that may have been overridden by the final concrete model into
something else (that's not a `@property`).

The previous logic only checks for attributes in `dir(self.model)`, so if
the `@property` has been overridden, it will not trigger the error.

As an example use case, in Wagtail we have a `Revision` model that uses a
`GenericForeignKey` to allow saving revisions of any model. For its
companion, we have an abstract model called `RevisionMixin` that gives you
methods like `save_revision`, as well as a `revisions` property to query
the revisions. The default `revisions` property is implemented as a
`@property` instead of a proper `GenericRelation`, because we need to
handle the case where the model may use multi-table inheritance (#31269).

In Wagtail, we handle it by having two content types in the `Revision`
model: the `base_content_type` (the first non-abstract model) and the
`content_type` (of the most specific model). In the base `RevisionMixin`
abstract class, we define a `revisions` `@property` that queries the
`Revision` model directly using the most basic content type, e.g.
`Revision.objects.filter(base_content_type=self.get_base_content_type(),
object_id=str(self.pk))`. This ensures that the `revisions` property
always returns the correct items, regardless which model (parent vs.
child) is used for querying.

For models that don't use multi-table inheritance, we've been suggesting
developers to override the `revisions` `@property` with a
`GenericRelation` directly (e.g. `revisions = GenericRelation(...)`). This
allows them to define the `related_query_name`, without having to use a
different name for the `GenericRelation` itself (e.g. `_revisions`) and
without having to override the `revisions` `@property` to return that
`GenericRelation`.

Now that I'm aware of the system check, I'm also not sure if it's safe to
override a `@property` with a `GenericRelation` in a subclass. There might
be quirks of `@property` that would interfere with how `GenericRelation`
works, that I didn't know of. But if it's safe, then the error shouldn't
have been raised.

It looks like the new Django behaviour might not be intended, as the PR
and ticket for that commit seem to suggest it was only meant as an
optimisation.

If Django would like to keep its new behaviour, I could see a few options
for us to proceed:
a) Use `cached_property` instead to bypass the system check (not sure if
this is a good idea)
b) Communicate to developers that they should not override the `@property`
directly with a `GenericRelation`, and should instead define the
`GenericRelation` with a different name e.g. `_revisions` and override the
default `@property` to return that `GenericRelation`.

I have created a simpler reproduction here:
https://github.com/laymonage/django-e025-repro, with models that use tags
instead of revisions. It also simulates how we worked around #31269.

Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/35301>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.