-
-
Notifications
You must be signed in to change notification settings - Fork 300
One-level select_related #395
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__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__()]) |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.