#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. -- You received this message because you are subscribed to the Google Groups "Django updates" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-updates+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/0107018e3c933139-5bbdde61-79e2-4fe6-bed4-fbd0812be13f-000000%40eu-central-1.amazonses.com.