Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixes #16855 #124

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

@akaariai akaariai commented on the diff Jun 8, 2012

django/db/models/sql/query.py
@@ -1745,7 +1745,10 @@ def add_select_related(self, fields):
certain related models (as opposed to all models, when
self.select_related=True).
"""
- field_dict = {}
+ if self.select_related is False:
+ field_dict = {}
+ else:
+ field_dict = self.select_related
@akaariai

akaariai Jun 8, 2012

Member

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

@oinopion

oinopion Jun 8, 2012

Contributor

So the question now is: what to do when select_related is True? Two use cases:

# will work ok
Species.objects.select_related(depth=4).select_related('genus') 

# should get all immediate relations AND the Order
Species.objects.select_related(depth=1).select_related('genus__family__order') 

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.

@akaariai akaariai commented on the diff Jun 8, 2012

tests/modeltests/select_related/tests.py
@@ -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 = []
@akaariai

akaariai Jun 8, 2012

Member

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.

@oinopion

oinopion Jun 8, 2012

Contributor

I don't think additional reading is required -- test without fix is failing. I guess the lists are not needed.

Member

akaariai commented Jun 8, 2012

I am closing this pull request, as the

  Species.objects.select_related(depth=1).select_related('genus__family__order')

case needs some more consideration (more details in the ticket). Please reopen when this issue is dealt with.

@akaariai akaariai closed this Jun 8, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment