Skip to content

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.

Sign up for GitHub

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

Closed
OskarPersson wants to merge 2 commits into from
Closed

One-level select_related #395

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1977128
Initial attempt at select related
OskarPersson Jul 30, 2019
f02f609
run ruff
bckohan Dec 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/polymorphic/query.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,23 @@ class self.model, but as a class derived from self.model. We want to re-fetch
**{(f"{pk_name}__in"): idlist}
)
# copy select related configuration to new qs
real_objects.query.select_related = self.query.select_related

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__()])
Copy link

Copilot AI Dec 8, 2025

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__()}

Suggested change
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__()}

Copilot uses AI. Check for mistakes.
sub_model_names.remove(concrete_model_name)
Comment on lines +411 to +412
Copy link

Copilot AI Dec 8, 2025

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().

Copilot uses AI. Check for mistakes.

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
Comment on lines +407 to +422
Copy link

Copilot AI Dec 8, 2025

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).

Copilot uses AI. Check for mistakes.
Comment on lines +409 to +422
Copy link

Copilot AI Dec 8, 2025

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.

Copilot uses AI. Check for mistakes.
Comment on lines +414 to +422
Copy link

Copilot AI Dec 8, 2025

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()

Suggested change
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

Copilot uses AI. Check for mistakes.

# Copy deferred fields configuration to the new queryset
deferred_loading_fields = []
Expand Down
Loading