Opened 14 years ago

Closed 11 years ago

Last modified 11 years ago

#13781 closed Bug (fixed)

select_related and multiple inheritance

Reported by: Shaun Cutts Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: shaun@…, Vlastimil Zíma, srmerritt, mike@…, gosia, tomas.ehrlich@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I have a "Profile" model that inherits both from auth.User and Person -- representing people who are users.
Such people also have accounts -- Account has a OneToOneField relating it to a Profile.

A particular user has an account.

>>> profile = Profile.objects.get( pk = 9819 )
>>> profile
<Profile: bob test>
>>> profile.account
<Account: bob test>

But when I use a select_related, an error occurs:

In [36]: profile = Profile.objects.select_related( 'account' ).get( pk = 9819 )
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/Users/shauncutts/<ipython console> in <module>()

/Users/shauncutts/dev/django/db/models/query.pyc in get(self, *args, **kwargs)
    334         if self.query.can_filter():
    335             clone = clone.order_by()
--> 336         num = len(clone)
    337         if num == 1:
    338             return clone._result_cache[0]

/Users/shauncutts/dev/django/db/models/query.pyc in __len__(self)
     79                 self._result_cache = list(self._iter)
     80             else:
---> 81                 self._result_cache = list(self.iterator())
     82         elif self._iter:
     83             self._result_cache.extend(list(self._iter))

/Users/shauncutts/dev/django/db/models/query.pyc in iterator(self)
    267 
    268         compiler = self.query.get_compiler(using=self.db)
--> 269         for row in compiler.results_iter():
    270             if fill_cache:
    271                 obj, _ = get_cached_row(self.model, row,

/Users/shauncutts/dev/django/db/models/sql/compiler.pyc in results_iter(self)
    670         resolve_columns = hasattr(self, 'resolve_columns')
    671         fields = None
--> 672         for rows in self.execute_sql(MULTI):
    673             for row in rows:
    674                 if resolve_columns:

/Users/shauncutts/dev/django/db/models/sql/compiler.pyc in execute_sql(self, result_type)
    715         """
    716         try:
--> 717             sql, params = self.as_sql()
    718             if not sql:
    719                 raise EmptyResultSet

/Users/shauncutts/dev/django/db/models/sql/compiler.pyc in as_sql(self, with_limits, with_col_aliases)
     53         in the query.
     54         """
---> 55         self.pre_sql_setup()
     56         out_cols = self.get_columns(with_col_aliases)
     57         ordering, ordering_group_by = self.get_ordering()

/Users/shauncutts/dev/django/db/models/sql/compiler.pyc in pre_sql_setup(self)
     27             self.query.setup_inherited_models()
     28         if self.query.select_related and not self.query.related_select_cols:
---> 29             self.fill_related_selections()
     30 
     31     def quote_name_unless_alias(self, name):

/Users/shauncutts/dev/django/db/models/sql/compiler.pyc in fill_related_selections(self, opts, root_alias, cur_depth, used, requested, restricted, nullable, dupe_set, avoid_set)
    608                 alias = root_alias
    609                 alias_chain = []
--> 610                 chain = opts.get_base_chain(f.rel.to)
    611                 if chain is not None:
    612                     for int_model in chain:

/Users/shauncutts/dev/django/db/models/options.pyc in get_base_chain(self, model)
    427             return [model]
    428         for parent in self.parents:
--> 429             res = parent._meta.get_base_chain(model)
    430             if res:
    431                 res.insert(0, parent)

/Users/shauncutts/dev/django/db/models/options.pyc in get_base_chain(self, model)
    432                 return res
    433         raise TypeError('%r is not an ancestor of this model'
--> 434                 % model._meta.module_name)
    435 
    436     def get_parent_list(self):

TypeError: 'profile' is not an ancestor of this model

Examination of the stack trace shows that the problem may be related to the fact that Profile inherits from both User and Person

>>> pdb.pm()
> /Users/shauncutts/dev/django/db/models/options.py(434)get_base_chain()
-> % model._meta.module_name)
(Pdb) u
> /Users/shauncutts/dev/django/db/models/options.py(429)get_base_chain()
-> res = parent._meta.get_base_chain(model)
(Pdb) print parent
<class 'cranedata.db.base.models.Person'>

Attachments (5)

select_related_tests.diff (2.6 KB ) - added by David Bennett 12 years ago.
test demonstrating error
select_related_subclass_patch.diff (9.3 KB ) - added by David Bennett 12 years ago.
Tests and patch (trunk)
select_related_subclass_patch_1.3.X.diff (9.0 KB ) - added by David Bennett 12 years ago.
Tests and patch (1.3.X)
select_related_subclass_patch_1.2.X.diff (8.9 KB ) - added by David Bennett 12 years ago.
Tests and patch (1.2.X)
13781-master.patch (9.4 KB ) - added by Tomáš Ehrlich 11 years ago.
Diff against latest master

Download all attachments as: .zip

Change History (35)

comment:1 by Shaun Cutts, 14 years ago

Cc: shaun@… added

comment:2 by Russell Keith-Magee, 14 years ago

Resolution: wontfix
Status: newclosed

Multiple inheritance of models isn't officially supported (see #12002, #10808, amongst others). It *may* work under certain conditions, but it's certainly not guaranteed to work.

The closest you'll be able to get is to do single inheritance, then use a OneToOne field to point at the second class.

Closing wontfix since it's a bug in a configuration that we don't support.

comment:3 by Shaun Cutts, 14 years ago

Resolution: wontfix
Status: closedreopened

Um .. ok: but may I suggest that you have a documentation bug then? See:

http://docs.djangoproject.com/en/1.2/topics/db/models/#multiple-inheritance

It certainly doesn't say its not supported.

comment:4 by Russell Keith-Magee, 14 years ago

Huh. Ok... that's interesting. Thanks for setting me straight -- I wasn't aware we explicitly said that in the docs.

In that case, there's definitely a bug here.

comment:5 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

comment:6 by Vlastimil Zíma, 14 years ago

Cc: Vlastimil Zíma added

comment:7 by Graham King, 13 years ago

Severity: Normal
Type: Bug

comment:8 by srmerritt, 13 years ago

Cc: srmerritt added
Easy pickings: unset

I am getting a separate error from the same issue (select_related failing to handle multiple inheritance), in v1.2.
My traceback looks like this:

  • /django/db/models/query.py", line 335, in get\n num = len(clone)\n',
  • /django/db/models/query.py", line 80, in len\n self._result_cache = list(self.iterator())\n',
  • /django/db/models/query.py", line 273, in iterator\n only_load=only_load)\n',
  • /django/db/models/query.py", line 1279, in get_cached_row\n setattr(rel_obj, rel_field.attname, getattr(obj, rel_field.attname))\n',

And my error is of this format: AttributeError: "'Parent2' object has no attribute 'Parent1Attribute'". This section of get_cached_row clearly assumes it is dealing with single inheritance, whereas in my case 'obj' is not an instance of 'rel_model'.

comment:9 by David Bennett, 12 years ago

Cc: david@… added
UI/UX: unset

by David Bennett, 12 years ago

Attachment: select_related_tests.diff added

test demonstrating error

comment:10 by David Bennett, 12 years ago

I've created a couple tests. The version with select_related fails, and the version without doesn't.

Description: If Child1 has a OneToOneField to Parent1 and if Child1 is a subclass of Parent2, doing Parent1.objects.select_related('child1') will produce this traceback:

======================================================================
ERROR: test_subclass_with_select_related (modeltests.select_related.tests.SelectRelatedTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/david/Projects/django-trunk/tests/modeltests/select_related/tests.py", line 171, in test_subclass_with_select_related
    p = Parent1.objects.select_related('child1').get(name="Parent1")
  File "/Users/david/Projects/django-trunk/django/db/models/query.py", line 361, in get
    num = len(clone)
  File "/Users/david/Projects/django-trunk/django/db/models/query.py", line 85, in __len__
    self._result_cache = list(self.iterator())
  File "/Users/david/Projects/django-trunk/django/db/models/query.py", line 294, in iterator
    offset=len(aggregate_select))
  File "/Users/david/Projects/django-trunk/django/db/models/query.py", line 1420, in get_cached_row
    setattr(rel_obj, rel_field.attname, getattr(obj, rel_field.attname))
AttributeError: 'Parent1' object has no attribute 'subname'

----------------------------------------------------------------------

The same issue occurs if you subclass from both Parent1 and Parent2, but I wanted to demonstrate that there is still a problem with just a single subclass.

comment:11 by David Bennett, 12 years ago

Easy pickings: set
Has patch: set

I've attached a patch that resolves the issue. Included are updated tests that check for the following:

Child1 is a subclass of both Parent1 and Parent2; Parent1.objects.select_related('child1') should not throw an error (see traceback in comment:10).

Child2 is a subclass of Parent1 and has a OneToOneField to Parent2; doing Parent2.objects.select_related('child2') should not throw an error (same traceback).

comment:12 by David Bennett, 12 years ago

Patch needs improvement: set

I've encountered an issue with my patch. If Child1 is a subclass of Parent1 and Child2 is a subclass of both Parent1 and Parent2, doing a Parent1.objects.select_related('child1', 'child2') will not return any results for Child1 because an INNER JOIN is being used with Parent2.

So, I need to figure out how to force it to a LEFT OUTER join in this instance. At this point I might just try a different approach for my project.

by David Bennett, 12 years ago

Tests and patch (trunk)

by David Bennett, 12 years ago

Tests and patch (1.3.X)

by David Bennett, 12 years ago

Tests and patch (1.2.X)

comment:13 by David Bennett, 12 years ago

Patch needs improvement: unset

Ok, I've created an additional test for the last issue I found and also figured out how to resolve it.

This patch should be ready for review.

comment:14 by Mike Fogel, 12 years ago

Cc: mike@… added

comment:15 by gosia, 12 years ago

Cc: gosia added

by Tomáš Ehrlich, 11 years ago

Attachment: 13781-master.patch added

Diff against latest master

comment:16 by Tomáš Ehrlich, 11 years ago

Cc: tomas.ehrlich@… added
Version: 1.2master

I've applied the select_related_subclass_patch.diff​ and resolved two merge conflicts against latest master branch. Tests passed.

comment:17 by Anssi Kääriäinen, 11 years ago

Easy pickings: unset
Triage Stage: AcceptedReady for checkin

Updated patch at: https://github.com/akaariai/django/commit/7ed2c5f16caa9c413002c6d9c120c657bd37fc96

I think this one is now ready for commit.

I added reference to the model from options, that is SomeM == SomeM._meta.model. This seems useful and is innocent as far as cyclic references go. There is already a cyclic reference by SomeM._meta.local_fields[0].model == SomeM.

For the record, I don't think this one was exactly easy pickings...

comment:18 by Anssi Kääriäinen, 11 years ago

I updated the work, and fixed a couple of issues affecting longer inheritance chains. Work available here: https://github.com/akaariai/django/commits/ticket_13781

The problem here is that when you fix one issue, you find two more broken issues. See added @expectedFailure tests. I think I will just go with the approach in the above branch and be done with this ticket. Fixing the @expectedFailure issues will lead to larger refactorings than what I am ready to do currently.

comment:19 by Anssi Kääriäinen, 11 years ago

I rebased the ticket_13781 branch for final commit. As the commit is somewhat complex there is also somewhat big risk of regressions so I will not backpatch this to 1.5, at least not initially. The cases where this patch is needed aren't that common.

comment:20 by Anssi Kääriäinen <akaariai@…>, 11 years ago

Resolution: fixed
Status: reopenedclosed

In f51e409a5fb34020e170494320a421503689aea0:

Fixed #13781 -- Improved select_related in inheritance situations

The select_related code got confused when it needed to travel a
reverse relation to a model which had different parent than the
originally travelled relation.

Thanks to Trac aliases shauncutts for report and ungenio for original
patch (committed patch is somewhat modified version of that).

comment:21 by Anssi Kääriäinen, 11 years ago

The commit above didn't contain some important lines in get_cached_row(), fixed in 1194a9699932088385f9f88869be28a251597f45.

comment:22 by Simon Charette, 11 years ago

Is this going to be backported to 1.5?

comment:23 by Simon Charette, 11 years ago

comment:24 by Simon Charette, 11 years ago

Cc: charette.s@… added

comment:25 by Anssi Kääriäinen, 11 years ago

Cc: charette.s@… removed

OK, I will see about adding the tests of #16572. I am planning of committing a couple more ORM fixes and then go through all the ORM tickets and close those which aren't relevant any more.

I am not yet sure if I should backpatch this to 1.5. The problem is that the ORM is somewhat fragile due to all the different query combinations and thus I don't feel too comfortable about backpatching...

comment:26 by Simon Charette, 11 years ago

Alright, didn't want to push you to backpatch, just wanted to know if you were planing to.

Good work on those patches!

comment:27 by Anssi Kääriäinen <akaariai@…>, 11 years ago

In 7cfb567e457379f52a80fe0e8d98dd8191391c6e:

Another regression fix for select_related handling

This time gis compiler.get_default_columns() wasn't up to date. Thanks
to CI another regression fixed.

Refs #13781

comment:28 by David Bennett, 11 years ago

Cc: david@… removed

comment:29 by Preston Holmes <preston@…>, 11 years ago

In a52794bc21404bc9079877db7ef5e1b8d1cafb8a:

Fixed #13781 -- Improved select_related in inheritance situations

The select_related code got confused when it needed to travel a
reverse relation to a model which had different parent than the
originally travelled relation.

Thanks to Trac aliases shauncutts for report and ungenio for original
patch (committed patch is somewhat modified version of that).

comment:30 by Preston Holmes <preston@…>, 11 years ago

In af1e499be089deebe64bd272b5c0fb80e582398a:

Another regression fix for select_related handling

This time gis compiler.get_default_columns() wasn't up to date. Thanks
to CI another regression fixed.

Refs #13781

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