Code

Opened 3 years ago

Closed 20 months 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 patrys 3 years ago.
Proposed patch

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Confirmed.

comment:2 Changed 3 years ago by firdaus_halim

  • Cc firdaus_halim added

comment:3 Changed 3 years ago by patrys

  • Cc patrys@… added

Changed 3 years ago by patrys

Proposed patch

comment:4 Changed 3 years ago by patrys

  • 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 Changed 3 years ago by ptone

  • 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 Changed 3 years ago by patrys

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 Changed 3 years ago by patrys

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 Changed 2 years ago by charettes

  • Cc charette.s@… added

comment:9 Changed 2 years ago by anonymous

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

comment:10 Changed 2 years ago by carbonXT

  • Cc mike@… added

comment:11 Changed 21 months ago by dvd0101

  • Cc dvd0101 added

comment:12 Changed 20 months ago by lorinh

  • Cc lorin@… added

comment:13 Changed 20 months ago by lorinh

  • 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 Changed 20 months ago by akaariai

  • Resolution set to fixed
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.