Skip to content

Add support for prefetch_related to InheritanceManager #639

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

diesieben07
Copy link

Problem

Currently, InheritanceManager does not support prefetch_related very well. This is mainly because Django's prefetch_related_objects expects the list it is given to be homogeneous, but for InheritanceIterable it is not.
This makes it so a prefetch_related('child__relation') will not work correctly in various ways, because

  • prefetch_related only looks at the first object in the list to make its decisions
  • Even if that happens to be correct, the prefetch was written from the point of view of the parent model, but now we have a child model instance.

Additionally, a prefetch child__relation will not be accessible in a grandchild, because from Django's point of view that would be child__grandchild__relation.

Solution

The solution has two parts:

  1. InheritanceQuerySetMixin now overrides _prefetch_related_objects and instead of using self._result_cache (which has the subclasses generated by InheritanceIterable) it first reconstructs the list of base objects. It then uses the base objects for prefetch_related_objects. This makes prefetch_related('child__relation') work.
  2. Copy the prefetch cache (which really has two parts, one for foreign keys and one for many to many) from parents into their children. This ensures that the field descriptors on the child instances can find the cache. Django does make some attempt to do this already, but only for foreign keys and only for one level of ancestry (a child will only look in its direct parent).

In the process I have also fixed the use of raw SQL in instance_of and simplified _get_ancestors_path by using the API Django already provides. If needed I can split these changes off into their own PR.

Commandments

  • Write PEP8 compliant code.
  • Cover it with tests.
  • Update CHANGES.rst file to describe the changes, and quote according issue with GH-<issue_number>.
  • Pay attention to backward compatibility, or if it breaks it, explain why.
  • Update documentation (if relevant).

@diesieben07
Copy link
Author

I have opened an issue for Django so that in the future hopefully the ugly copying of the caches can be removed:
https://code.djangoproject.com/ticket/36282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant