-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
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
Fixes #16855 #124
Fixes #16855 #124
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
|
||
from django.test import TestCase | ||
|
||
from .models import Domain, Kingdom, Phylum, Klass, Order, Family, Genus, Species | ||
from .models import Domain, Kingdom, Phylum, Klass, Order, Family, Genus, Species, AlternativeName | ||
|
||
|
||
class SelectRelatedTests(TestCase): | ||
|
@@ -160,3 +160,22 @@ def test_depth_fields_fails(self): | |
Species.objects.select_related, | ||
'genus__family__order', depth=4 | ||
) | ||
|
||
def test_select_related_chaining(self): | ||
''' | ||
Regression for #16855 | ||
''' | ||
name = AlternativeName.objects.create(name='Human') | ||
human = Species.objects.get(name='sapiens') | ||
human.alternative_name = name | ||
human.save() | ||
|
||
qs = Species.objects.select_related('genus') | ||
qs = qs.select_related('alternative_name') | ||
with self.assertNumQueries(1): | ||
names = [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't these names and genus lists unused - you add to them but never read from them? BTW above in the tests.py add an empty line before the unicode method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think additional reading is required -- test without fix is failing. I guess the lists are not needed. |
||
genus = [] | ||
for s in qs: | ||
genus.append(s.genus.name) | ||
if s.alternative_name: | ||
names.append(s.alternative_name.name) |
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 query.select_related has numerous states: it can be False (for no select_related), it can be True (which seems to indicate that depth based select_related should be used) or it can be a dict (for user-selected restriction). I think here field_dict could end up being True here.
In the long run it would be better to split the query.select_related to two variables: one would be query.select_related = True/False, another would ne query.select_related_restriction = None or dict_of_asked_selections.
For the time being maybe
field_dict = isinstance(self.select_related, dict) and self.select_related or {}
would do? (naturally, not tested).
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.
So the question now is: what to do when select_related is True? Two use cases:
I seems to me we need this split on select_related and we need to handle it intelligently, ie. 'count' depth of select related field and conditionally add it or not.