One-level select_related#395
Conversation
|
You can try something like that: from typing import List
from polymorphic.models import PolymorphicModel
from polymorphic.query import PolymorphicQuerySet
class SelectRelatedPolymorphicQuerySet(PolymorphicQuerySet):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.used_select_related_cache = False
def use_select_related(self):
qs = self._clone()
qs.used_select_related_cache = True
return qs
def _clone(self, *args, **kwargs):
qs = super()._clone(*args, **kwargs)
qs.used_select_related_cache = self.used_select_related_cache
return qs
def _get_real_instances(self, base_result_objects: List[PolymorphicModel]):
if self.used_select_related_cache:
return [item.get_real_instance() for item in base_result_objects]
return super()._get_real_instances(base_result_objects)from polymorphic.managers import PolymorphicManager
from polymorphic.models import PolymorphicModel
from vas.utils.models.polymorhic.querysets import SelectRelatedPolymorphicQuerySet
class SelectRelatedPolymorphicModel(PolymorphicModel):
class Meta:
abstract = True
objects = PolymorphicManager.from_queryset(SelectRelatedPolymorphicQuerySet)()
def get_real_instance(self):
real_model = self.get_real_instance_class()
if real_model == self.__class__:
return self
# allow to use cache:
# return getattr(self, real_model.__name__.lower())
return self._state.fields_cache[real_model.__name__.lower()] # todo |
a48cebd to
1977128
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #395 +/- ##
==========================================
- Coverage 74.71% 74.21% -0.50%
==========================================
Files 21 21
Lines 1337 1346 +9
Branches 211 214 +3
==========================================
Hits 999 999
- Misses 261 269 +8
- Partials 77 78 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for this start @OskarPersson! I am the new leader of this project. It will take me a bit to work through the backlog of issues but this feature is also a desire of mine so I plan to revisit this as time allows. If you want to take another look that would be great too! |
There was a problem hiding this comment.
Pull request overview
This PR attempts to add select_related() support for polymorphic models, addressing issue #198. The implementation modifies the _get_real_instances() method to copy select_related configuration from the base queryset to the real object querysets for each concrete polymorphic subclass.
Key changes:
- Adds logic to handle
select_relatedconfiguration when fetching real instances of polymorphic models - Filters select_related to exclude sibling model paths and isolate child-specific relations
- Preserves select_related=False behavior
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sub_model_names = set([m._meta.model_name for m in self.model.__subclasses__()]) | ||
| sub_model_names.remove(concrete_model_name) |
There was a problem hiding this comment.
__subclasses__() only returns direct subclasses, not all descendants in the inheritance hierarchy. For a model hierarchy like Model2A -> Model2B -> Model2C -> Model2D, calling __subclasses__() on Model2A only returns [Model2B], missing Model2C and Model2D. This could cause issues when concrete_model_name is a deeper descendant not in sub_model_names, leading to a KeyError on remove(). Consider using a recursive helper to collect all descendants, similar to the pattern in query_translate.py:_get_mro_content_type_ids().
| real_objects.query.select_related = False | ||
| else: | ||
| concrete_model_name = real_concrete_class._meta.model_name | ||
| sub_model_names = set([m._meta.model_name for m in self.model.__subclasses__()]) |
There was a problem hiding this comment.
Unnecessary list comprehension brackets. Use a set comprehension or generator expression directly: sub_model_names = {m._meta.model_name for m in self.model.__subclasses__()}
| sub_model_names = set([m._meta.model_name for m in self.model.__subclasses__()]) | |
| sub_model_names = {m._meta.model_name for m in self.model.__subclasses__()} |
| if self.query.select_related is False: | ||
| real_objects.query.select_related = False | ||
| else: | ||
| concrete_model_name = real_concrete_class._meta.model_name | ||
| sub_model_names = set([m._meta.model_name for m in self.model.__subclasses__()]) | ||
| sub_model_names.remove(concrete_model_name) | ||
|
|
||
| for sub_name in sub_model_names: | ||
| self.query.select_related.pop(sub_name, None) | ||
|
|
||
| if concrete_model_name in self.query.select_related: | ||
| real_objects.query.select_related = self.query.select_related[ | ||
| concrete_model_name | ||
| ] | ||
| else: | ||
| real_objects.query.select_related = self.query.select_related |
There was a problem hiding this comment.
The new select_related handling logic lacks test coverage. The test suite in test_orm.py has comprehensive tests for other queryset operations (defer, prefetch_related, annotate, etc.) but no tests for select_related. Consider adding tests that cover: 1) basic select_related on polymorphic models, 2) select_related with nested relations through child models, 3) select_related with multiple inheritance levels, and 4) edge cases like empty select_related or select_related(None).
| else: | ||
| concrete_model_name = real_concrete_class._meta.model_name | ||
| sub_model_names = set([m._meta.model_name for m in self.model.__subclasses__()]) | ||
| sub_model_names.remove(concrete_model_name) | ||
|
|
||
| for sub_name in sub_model_names: | ||
| self.query.select_related.pop(sub_name, None) | ||
|
|
||
| if concrete_model_name in self.query.select_related: | ||
| real_objects.query.select_related = self.query.select_related[ | ||
| concrete_model_name | ||
| ] | ||
| else: | ||
| real_objects.query.select_related = self.query.select_related |
There was a problem hiding this comment.
The code assumes self.query.select_related is a dict when it's not False, but Django's select_related can also be True (when called with no arguments like .select_related()), or an empty dict {}. Attempting operations like .pop(), in, or [] access on True will raise TypeError or AttributeError. Add a check: elif self.query.select_related is True: or elif not isinstance(self.query.select_related, dict): to handle these cases.
| for sub_name in sub_model_names: | ||
| self.query.select_related.pop(sub_name, None) | ||
|
|
||
| if concrete_model_name in self.query.select_related: | ||
| real_objects.query.select_related = self.query.select_related[ | ||
| concrete_model_name | ||
| ] | ||
| else: | ||
| real_objects.query.select_related = self.query.select_related |
There was a problem hiding this comment.
Mutating self.query.select_related directly with pop() modifies the original queryset's configuration. Since this code is inside a loop over idlist_per_model.items(), the first iteration will remove entries from the dict, affecting subsequent iterations for other concrete classes. This should operate on a copy instead: select_related_copy = self.query.select_related.copy()
| for sub_name in sub_model_names: | |
| self.query.select_related.pop(sub_name, None) | |
| if concrete_model_name in self.query.select_related: | |
| real_objects.query.select_related = self.query.select_related[ | |
| concrete_model_name | |
| ] | |
| else: | |
| real_objects.query.select_related = self.query.select_related | |
| # Make a shallow copy to avoid mutating the original select_related dict | |
| select_related_copy = self.query.select_related.copy() | |
| for sub_name in sub_model_names: | |
| select_related_copy.pop(sub_name, None) | |
| if concrete_model_name in select_related_copy: | |
| real_objects.query.select_related = select_related_copy[ | |
| concrete_model_name | |
| ] | |
| else: | |
| real_objects.query.select_related = select_related_copy |
This is an attempt at adding support for
select_relatedand thus closing #198. So far it has no unit tests or documentation and has mostly been tested with Graphene-Django which is where I found my need for this.(Getting graphene-django-optimizer to work with this is the next step!)
With my limited testing it seems to work at the first level (e.g.
Parent.objects.select_related('childa__related')) but not any deeper than that (e.g.Parent.objects.select_related('childa__related__childb'))This PR is not in a mergeable state and instead is currently mostly created for bringing the discussion further for implementing
select_relatedwith a more complete solution.