Overriding a @property of an abstract model with a GenericRelation causes a models.E025 error.

[Sage Abdullah]

As of faeb92ea13f0c1b2cc83f45b512f2c41cfb4f02d, Django traverses the model's MRO to find the names of properties, to later be used in different places, such as ​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!

[Mariusz Felisiak]

Thanks for the detailed report and early testing!

[Adam Johnson]

Yeah thanks. I have an idea to fix this by keeping track of seen names whilst iterating. Will give it a try tonight.

[Sage Abdullah]

Thanks both! Here's a much more minimal reproduction that still somewhat makes sense.

from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation
from django.contrib.contenttypes.models import ContentType
from django.db import models


class Generic(models.Model):
    content_type = models.ForeignKey(
        ContentType,
        related_name="+",
        on_delete=models.CASCADE,
    )
    object_id = models.CharField(max_length=255)
    content_object = GenericForeignKey()


class Abstract(models.Model):
    @property
    def generics(self):
        return Generic.objects.filter(
            content_type=ContentType.objects.get_for_model(self),
            object_id=str(self.pk),
        )

    class Meta:
        abstract = True


class Concrete(Abstract):
    generics = GenericRelation(Generic, related_query_name="concrete")

[GitHub <noreply@…>]

In 7646b90:

Fixed #35301 -- Fixed Options._property_names for overriden properties.

Regression in faeb92ea13f0c1b2cc83f45b512f2c41cfb4f02d.