Code

Opened 4 years ago

Closed 20 months ago

Last modified 20 months ago

#13781 closed Bug (fixed)

select_related and multiple inheritance

Reported by: shauncutts Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: shaun@…, vzima, 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 ungenio 2 years ago.
test demonstrating error
select_related_subclass_patch.diff (9.3 KB) - added by ungenio 2 years ago.
Tests and patch (trunk)
select_related_subclass_patch_1.3.X.diff (9.0 KB) - added by ungenio 2 years ago.
Tests and patch (1.3.X)
select_related_subclass_patch_1.2.X.diff (8.9 KB) - added by ungenio 2 years ago.
Tests and patch (1.2.X)
13781-master.patch (9.4 KB) - added by Elvard 21 months ago.
Diff against latest master

Download all attachments as: .zip

Change History (35)

comment:1 Changed 4 years ago by shauncutts

  • Cc shaun@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 4 years ago by russellm

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

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 Changed 4 years ago by shauncutts

  • Resolution wontfix deleted
  • Status changed from closed to reopened

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 Changed 4 years ago by russellm

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 Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

comment:6 Changed 4 years ago by vzima

  • Cc vzima added

comment:7 Changed 3 years ago by graham_king

  • Severity set to Normal
  • Type set to Bug

comment:8 Changed 3 years ago by srmerritt

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

  • Cc david@… added
  • UI/UX unset

Changed 2 years ago by ungenio

test demonstrating error

comment:10 Changed 2 years ago by ungenio

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

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

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

Changed 2 years ago by ungenio

Tests and patch (trunk)

Changed 2 years ago by ungenio

Tests and patch (1.3.X)

Changed 2 years ago by ungenio

Tests and patch (1.2.X)

comment:13 Changed 2 years ago by ungenio

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

  • Cc mike@… added

comment:15 Changed 2 years ago by gosia

  • Cc gosia added

Changed 21 months ago by Elvard

Diff against latest master

comment:16 Changed 21 months ago by Elvard

  • Cc tomas.ehrlich@… added
  • Version changed from 1.2 to master

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

comment:17 Changed 21 months ago by akaariai

  • Easy pickings unset
  • Triage Stage changed from Accepted to Ready 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 Changed 21 months ago by akaariai

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

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 Changed 20 months ago by Anssi Kääriäinen <akaariai@…>

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

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

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

comment:22 Changed 20 months ago by charettes

Is this going to be backported to 1.5?

comment:23 Changed 20 months ago by charettes

comment:24 Changed 20 months ago by charettes

  • Cc charette.s@… added

comment:25 Changed 20 months ago by akaariai

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

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 Changed 20 months ago by Anssi Kääriäinen <akaariai@…>

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

  • Cc david@… removed

comment:29 Changed 20 months ago by Preston Holmes <preston@…>

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 Changed 20 months ago by Preston Holmes <preston@…>

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

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.