Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add default GenericRelations for RevisionMixin and WorkflowMixin #11750

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

laymonage
Copy link
Member

Not entirely related but fixes #11746.

For models that have multi-table inheritance, accessing a
GenericRelation will not always give you the correct results. This can
occur if the content type used by the GenericRelation does not match the
instance at hand, e.g. accessing a GenericRelation that uses
base_content_type from an instance of the specific class, or vice versa.

This is a known quirk when using GenericRelation together with MTI:
https://code.djangoproject.com/ticket/31269

To work around this, we need to have a method or property that resolves
to the correct GenericRelation (or the related model's QuerySet) to use,
depending on whether the current instance is specific or not. This
allows us to use and publicly document RevisionMixin.revisions as
something that always gives you the correct queryset of Revisions for
the object, without having to deal with the MTI quirks.

This is why the RevisionMixin has a revisions property that queries
the Revision model directly, so that accessing MyModelInstance.revisions
always gives you the correct results, no matter how the GenericRelation
is set up for custom models.

For pages, we have access to the model implementation, so the
Page.revisions property is implemented using a GenericRelation that is
always accessed using a specific version of the instance.

For models that don't use MTI, we advise developers to just override the
revisions property with a GenericRelation definition. However, as of
django/django@faeb92e,
the existence of a @Property and a related field accessor of the same
name triggers a system check error.

It is unknown whether the system check error is the desired result or
not. However, the approach that we documented about shadowing the
mixins' default properties with a real GenericRelation clearly worked
before, so it is probably okay.

To work around this, we can use cached_property instead, which bypasses
the check, as Django only checks for the plain Python property.

Additionally, we can suggest developers to never override the
revisions and workflow_states with GenericRelations directly, and
instead define the GenericRelations with other names, and override the
property as a new property that returns the GenericRelation (with any
additional logic as necessary, e.g. to handle MTI).Please describe the problem you're fixing here. Include the issue number, if applicable.

Copy link

squash-labs bot commented Mar 11, 2024

Manage this branch in Squash

Test this branch here: https://laymonagemixins-default-generi-9ulxz.squash.io

…kflow_states

For models that have multi-table inheritance, accessing a
GenericRelation will not always give you the correct results. This can
occur if the content type used by the GenericRelation does not match the
instance at hand, e.g. accessing a GenericRelation that uses
base_content_type from an instance of the specific class, or vice versa.

This is a known quirk when using GenericRelation together with MTI:
https://code.djangoproject.com/ticket/31269

To work around this, we need to have a method or property that resolves
to the correct GenericRelation (or the related model's QuerySet) to use,
depending on whether the current instance is specific or not. This
allows us to use and publicly document RevisionMixin.revisions as
something that always gives you the correct queryset of Revisions for
the object, without having to deal with the MTI quirks.

This is why the RevisionMixin has a `revisions` property that queries
the Revision model directly, so that accessing MyModelInstance.revisions
always gives you the correct results, no matter how the GenericRelation
is set up for custom models.

For pages, we have access to the model implementation, so the
Page.revisions property is implemented using a GenericRelation that is
always accessed using a specific version of the instance.

For models that don't use MTI, we advise developers to just override the
revisions property with a GenericRelation definition. However, as of
django/django@faeb92e,
the existence of a @Property and a related field accessor of the same
name triggers a system check error.

It is unknown whether the system check error is the desired result or
not, as the error wasn't triggered before Django made the optimisation.
The approach that we documented about shadowing the mixins' default
properties with a real GenericRelation clearly worked before though, so
it is probably okay.

To work around the new Django behaviour, we can use cached_property
instead, which bypasses the check, as Django only checks for the plain
Python `property`.

Alternatively, if the system check error is indeed the desired result,
we can suggest developers to **never** override the `revisions` and
`workflow_states` with `GenericRelation`s directly, and instead define
the `GenericRelation`s with other names, and override the property as
a new property that returns the appropriate `GenericRelation` (with
any additional logic as necessary, e.g. to handle MTI).
Also update Page._revisions GenericRelation to be more explicit about
which field we are using for the content type.
…he base_content_type for querying

This ensures the returned QuerySet of Revisions is always correct regardless whether it's an instance of the base model or the specific model
@laymonage laymonage force-pushed the mixins-default-generic-relation branch from b93d016 to ef3d01f Compare March 21, 2024 23:56
@laymonage laymonage self-assigned this Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Django mainmodels.E025: The property 'revisions'/'workflow_states' clashes with a related field accessor
1 participant