Opened 13 years ago

Closed 12 years ago

#16572 closed Bug (fixed)

select_related doesn't work (silent/masked TypeError) over multiple one-to-one relationships

Reported by: rfrankel Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords: select_related orm queryset
Cc: firdaus_halim, patrys@…, charette.s@…, mike@…, dvd0101, lorin@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Given this models.py:

from django.db import models

class A(models.Model):
    pass

class B(A):
    pass

class C(B):
    pass

The following select_related call returns an apparently empty queryset:

>>> A.objects.select_related('b__c')
[]

Digging a little deeper:

>>> A.objects.select_related('b__c').__iter__().next()
... 
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/opt/webapps/asdf/lib/python2.6/site-packages/django/db/models/query.py", line 107, in _result_iter
    self._fill_cache()
  File "/opt/webapps/asdf/lib/python2.6/site-packages/django/db/models/query.py", line 772, in _fill_cache
    self._result_cache.append(self._iter.next())
  File "/opt/webapps/asdf/lib/python2.6/site-packages/django/db/models/query.py", line 273, in iterator
    for row in compiler.results_iter():
  File "/opt/webapps/asdf/lib/python2.6/site-packages/django/db/models/sql/compiler.py", line 680, in results_iter
    for rows in self.execute_sql(MULTI):
  File "/opt/webapps/asdf/lib/python2.6/site-packages/django/db/models/sql/compiler.py", line 725, in execute_sql
    sql, params = self.as_sql()
  File "/opt/webapps/asdf/lib/python2.6/site-packages/django/db/models/sql/compiler.py", line 58, in as_sql
    self.pre_sql_setup()
  File "/opt/webapps/asdf/lib/python2.6/site-packages/django/db/models/sql/compiler.py", line 29, in pre_sql_setup
    self.fill_related_selections()
  File "/opt/webapps/asdf/lib/python2.6/site-packages/django/db/models/sql/compiler.py", line 661, in fill_related_selections
    used, next, restricted, new_nullable)
  File "/opt/webapps/asdf/lib/python2.6/site-packages/django/db/models/sql/compiler.py", line 617, in fill_related_selections
    chain = opts.get_base_chain(f.rel.to)
  File "/opt/webapps/asdf/lib/python2.6/site-packages/django/db/models/options.py", line 452, in get_base_chain
    % model._meta.module_name,)
TypeError: 'b' is not an ancestor of this model
>>>


This was a little too deep in Django's internals for me to be able to debug, but as far as I can tell it's a legit bug.

Attachments (1)

16572.patch (434 bytes ) - added by Patryk Zawadzki 13 years ago.
Proposed patch

Download all attachments as: .zip

Change History (15)

comment:1 by Anssi Kääriäinen, 13 years ago

Triage Stage: UnreviewedAccepted

Confirmed.

comment:2 by firdaus_halim, 13 years ago

Cc: firdaus_halim added

comment:3 by Patryk Zawadzki, 13 years ago

Cc: patrys@… added

by Patryk Zawadzki, 13 years ago

Attachment: 16572.patch added

Proposed patch

comment:4 by Patryk Zawadzki, 13 years ago

Has patch: set

I've attached a patch that fixes it for me. Here's a summary of the problem:

When asked to .select_related() a grand child django.db.models.compiler.SQLCompiler.fill_related_selections() will cascade into both the child and the grand child of the model. When it hits the child it will inevitably find a field pointing to itself. At that point a call to django.db.models.options.Options.get_base_chain() will raise an exception because (1) it has a non-empty list of parents and (2) it's being asked about its very own model that of course is not in the list.

Not sure if that's the correct solution but I've just added a check to ensure .get_base_chain() is not talking to itself and in such a case return nothing (as there are no additional intermediate classes to add to the query).

comment:5 by Preston Holmes, 13 years ago

Needs tests: set

I'm wondering if this shouldn't be fixed elsewhere.

Not super familiar with these internals either, but would you want to exclude self from the list of parents in the first place here:

https://code.djangoproject.com/browser/django/trunk/django/db/models/base.py#L34

comment:6 by Patryk Zawadzki, 13 years ago

I don't think it's a case of self.__class__ being in parents. Rather it's a case where one-to-one on a subject of .select_related() points back to the parent object (as is the case with natural inheritance). As self is in the scope of the query, Django tries to find intermediate classes to add to the query. To do so it asks self's manager for the list of classes but the manager gets confused and raises an exception.

comment:7 by Patryk Zawadzki, 13 years ago

More context:

The code reads as follows (django/db/models/sql/compiler.py):

603            for f, model in related_fields:
604                if not select_related_descend(f, restricted, requested, reverse=True):
605                    continue
606                # The "avoid" set is aliases we want to avoid just for this
607                # particular branch of the recursion. They aren't permanently
608                # forbidden from reuse in the related selection tables (which is
609                # what "used" specifies).
610                avoid = avoid_set.copy()
611                dupe_set = orig_dupe_set.copy()
612                table = model._meta.db_table
613
614                int_opts = opts
615                alias = root_alias
616                alias_chain = []
617                chain = opts.get_base_chain(f.rel.to)

In this case used is:

set(['products_childmodel'])

requested is:

{'grandchildmodel': {}}

opts is:

<Options for ChildModel>

f and model are:

(<django.db.models.fields.related.OneToOneField object at …>, <class 'products.models.GrandChildModel'>)

and f.rel.to points back to the child model:

<class 'products.models.ChildModel'>

So in the end it asks the Options instance to .get_base_chain() for its very own model. One solution is to apply my patch and have the Options instance deal with that. Another would be to add a check between lined 603 and 604 and just continue if the relation points to something that is already in used or at least when it points to the object managed by opts.

comment:8 by Simon Charette, 13 years ago

Cc: charette.s@… added

comment:9 by anonymous, 12 years ago

This patch certainly solved the problems I was having with this.

comment:10 by Mike Fogel, 12 years ago

Cc: mike@… added

comment:11 by dvd0101, 12 years ago

Cc: dvd0101 added

comment:12 by Lorin Hochstein, 12 years ago

Cc: lorin@… added

comment:13 by Lorin Hochstein, 12 years ago

Needs tests: unset

I submitted a pull request to add unit tests that reproduce this bug: https://github.com/django/django/pull/513

comment:14 by Anssi Kääriäinen, 12 years ago

Resolution: fixed
Status: newclosed

This seems to be fixed in the commits of #13781. The test in comment:13 passed on sqlite for me, and charettes reports fixed for him too. On a quick look the tests of #13781 seems to cover this ticket's issue, so I will not commit the tests.

Note: See TracTickets for help on using tickets.
Back to Top