Code

Opened 6 years ago

Closed 4 years ago

Last modified 3 years ago

#7270 closed (fixed)

selected_related() can not follow reverse relations of OneToOne.

Reported by: towjzhou@… Owned by: mtredinnick
Component: Database layer (models, ORM) Version: master
Severity: Keywords: select_related onetoone reverse performance
Cc: oyvind.altvik@…, brent.hagany@…, elsdoerfer@…, gregoire@…, andrewbadr.etc@…, anossov@…, kimavr@…, dbronner@…, gav, Boobsd@…, vbmendes@…, bendavis78@…, aiev.an.tks@…, nikitka@…, glennfmaynard@…, ionel.mc@…, garrison, dexterbt1@…, jdunck@…, kmike84@…, vinilios@…, oldium.pro@…, nicola.murino@…, mike@…, daemianmack@…, hgeerts@…, powderflask@…, david, s.angel@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Here are my models:

# The base model
class User(models.Model):
    username = models.CharField()
    password = models.CharField()

# The model holds extra info to User
class UserProfile(models.Model):
    user  = models.OneToOneField(User)
    address = models.CharField()

# Then query the user with extra info:
    users = User.objects.select_related('userprofile')

Above code can't work, because select_related only accepts ForeignKey fields.

Attachments (11)

django_select_related_onetoone_r7534.patch (2.2 KB) - added by towjzhou@… 6 years ago.
a dirty patch to fix this ticket.
django_select_related_onetoone_r7534.2.patch (2.4 KB) - added by towjzhou@… 6 years ago.
A updated patch to fix this ticket. This patch only work in select(fields...) mode, not effect on select()/select(depth=?) mode.
121_reverse_r7601.patch (9.6 KB) - added by gav 6 years ago.
121_reverse_r7831.patch (21.1 KB) - added by gav 6 years ago.
Updated patch again r7831, friendlier towards FK(unique=True)
121_reverse_r8985.patch (21.5 KB) - added by gav 6 years ago.
121 reverse patch updated to r8985.
121_reverse_r10396.patch (21.2 KB) - added by bendavis78 5 years ago.
121_reverse_patch updated to 10396
django_select_related_onetoone_r10448.patch (6.9 KB) - added by aiev 5 years ago.
adaptation to the r10448
django_select_related_onetoone_r10454.patch (22.4 KB) - added by bendavis78 5 years ago.
Updated to always use LEFT OUTER JOINs, added back tests
django_select_related_onetoone.patch (9.1 KB) - added by bendavis78 5 years ago.
patches against trunk, r11479
reverse_select_related.diff (11.5 KB) - added by Alex 4 years ago.
12307-onetoone-null.patch (639 bytes) - added by s.angel@… 4 years ago.
http://code.djangoproject.com/ticket/7270#comment:67

Download all attachments as: .zip

Change History (81)

comment:1 Changed 6 years ago by anonymous

  • Component changed from Uncategorized to Database wrapper
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Changed 6 years ago by towjzhou@…

a dirty patch to fix this ticket.

comment:2 Changed 6 years ago by marc.boeker@…

thanks for the patch, but this also affects ForeignKeys and results in a extraordinary large invalid sql query:)

Changed 6 years ago by towjzhou@…

A updated patch to fix this ticket. This patch only work in select(fields...) mode, not effect on select()/select(depth=?) mode.

comment:3 Changed 6 years ago by gav

The newly attached patch is another approach to doing this, and shares some points with the original ticket creator. However, this patch also covers reverse caching, safely handles following all types of ForeignKeys attached to OneToOneFields, and includes unit tests and documentation.

Changed 6 years ago by gav

comment:4 Changed 6 years ago by gav

  • Owner changed from nobody to gav

Changed 6 years ago by gav

Updated patch again r7831, friendlier towards FK(unique=True)

comment:5 Changed 6 years ago by miracle2k

Should be in a 1.0 milestone?

comment:6 Changed 6 years ago by gav

  • milestone set to 1.0

Some conversations I've had with core devs make me think so, so I'm updating the milestone appropriately.

comment:7 Changed 6 years ago by mtredinnick

This is kind of "1.0 maybe if we get time", which really means before the beta release next week. It'd be nice to have in and it's probably very close to right, but if we don't get it in, the world won't end either.

comment:8 Changed 6 years ago by mtredinnick

  • Triage Stage changed from Unreviewed to Accepted

comment:9 Changed 6 years ago by mtredinnick

  • milestone changed from 1.0 to post-1.0

Moving this to post-1.0, in line with my above comment. I know gav has done a lot of work on this (thanks for that, George!) and it would be nice to get it in, but I've just reread the patch and, although it looks correct, it does hit a lot of code that is used frequently and can hide subtle bugs. In the interests of stability (and feature freeze), it can wait,

comment:10 Changed 6 years ago by oyvind

  • Cc oyvind.altvik@… added
  • Keywords select_related onetoone reverse added

Changed 6 years ago by gav

121 reverse patch updated to r8985.

comment:11 Changed 6 years ago by gav

The newest uploaded patch brings this to r8985, but in every other way is the same as the previous patch.

comment:12 Changed 6 years ago by oyvind

  • Has patch set

Seems sound to me, and the tests pass.

comment:13 Changed 6 years ago by robin

How would I go about making the patch use LEFT JOIN (or LEFT OUTER JOIN) instead of INNER JOIN?

comment:14 Changed 6 years ago by gav

This patch automatically makes the LEFT JOIN for any FK relationships where null=True. You should not have to do anything beyond this.

comment:15 Changed 6 years ago by robin

Since this is a one-to-one relationship, the foreign key is also the primary key, so null is never true, hence LEFT JOIN will never occur unless forced somehow.

comment:16 Changed 6 years ago by mtredinnick

There's a bug in the patch, robin. Reverse one-to-one's always have to use an outer join, since there's no guarantee that the related object exists (they are all nullable relationships when followed in reverse). I'll fix this when I commit the change; I noticed it before and have a note about it. That will happen, soon.

comment:17 Changed 6 years ago by mtredinnick

(Oh, I should point out that it doesn't have to be an outer join if we're already filtering against that relation and didn't need an outer join the first time.)

comment:18 Changed 5 years ago by bhagany

  • Cc brent.hagany@… added

comment:19 Changed 5 years ago by miracle2k

  • Cc elsdoerfer@… added

comment:20 Changed 5 years ago by gregoire

  • Cc gregoire@… added

comment:21 Changed 5 years ago by andrewbadr

  • Cc andrewbadr.etc@… added

comment:22 Changed 5 years ago by Anossov

  • Cc anossov@… added

comment:23 Changed 5 years ago by kiriyama

  • Cc kimavr@… added

comment:24 Changed 5 years ago by dbronner

  • Cc dbronner@… added

comment:25 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:26 Changed 5 years ago by mtredinnick

  • Cc gav added
  • Owner changed from gav to mtredinnick

Assigning to me so that it appears on my list of tickets to deal with. Will reassign back to gav if any insurmountable problem emerges, but I think this is pretty close to what we can use.

comment:27 Changed 5 years ago by Boo

  • Cc Boobsd@… added

comment:28 Changed 5 years ago by vbmendes

  • Cc vbmendes@… added

comment:29 Changed 5 years ago by mtredinnick

  • milestone set to 1.2

comment:30 Changed 5 years ago by anonymous

  • Cc bendavis78@… added

comment:31 Changed 5 years ago by aiev

  • Cc aiev.an.tks@… added
  • Needs tests set
  • Patch needs improvement set

I use the svn version and when I apply the patch with command "patch -p0 -i ~/121_reverse_r8985.patch" ocurrence some errors because the merge dont work correctly.

But when I aplly the patch manually and debug some lines it work! :D

Why it has not yet been implemented in current version? :( Just curiosity

Thanks.

Changed 5 years ago by bendavis78

121_reverse_patch updated to 10396

comment:32 Changed 5 years ago by bendavis78

I've attached a version of the patch against r10396.

Also, I modified the self.join() call to set promote=True so that LEFT OUTER joins are always used -- although I'm not sure if that was all I needed to do, as I'm seeing a problem with the returned queryset when some of the joined values are null.

For example, let's say we have some User records that don't have associated UserProfile records. When we do:

>>> print User.objects.select_related('userprofile').query.as_sql()[0]
'SELECT `auth_user`.`id`, `myapp_userprofile`.`id`, FROM `auth_user` LEFT OUTER JOIN `myapp_userprofile` ON (`auth_user`.`id` = `myapps_userprofile`.`user_id`)'

... we get the correct SQL, and running this in dbshell returns the correct results, with the columns from UserProfile set to NULL.

However, when we try to output the queryset object, we get an empty list:

>>> q = User.objects.select_related('user_profile')
>>> q
[]
>>> 

Any ideas on why this is occurring?

Also, I've noticed some other problems that come up when we .values() or .only() on the query set, so I think this patch needs a little more refining before we can get it to work.

comment:33 Changed 5 years ago by anonymous

  • Cc niktika@… added

comment:34 Changed 5 years ago by anonymous

  • Cc nikitka@… added; niktika@… removed

Changed 5 years ago by aiev

adaptation to the r10448

comment:35 Changed 5 years ago by aiev

A updated patch, no bug like:

>>> q = User.objects.select_related('user_profile')
>>> q
[]
>>> 

But the problem persists when we .only() on the query set.

The .values() works fine, right?

comment:36 Changed 5 years ago by bendavis78

  • Patch needs improvement unset

aiev, thanks. That seemed to fix the problem I was having :-) Although you left out the change I made in the last patch that forces the join to always be LEFT OUTER, as mtredinnick explained above. I've attached an updated patch.

Changed 5 years ago by bendavis78

Updated to always use LEFT OUTER JOINs, added back tests

comment:37 Changed 5 years ago by bendavis78

Oops, just realized the tests somehow didn't make it into that patch. Patch updated.

comment:38 Changed 5 years ago by bendavis78

  • Needs tests unset

comment:39 Changed 5 years ago by Glenn

  • Cc glennfmaynard@… added

Recursive select_related() (with no fields specified) doesn't seem to work yet.

comment:40 Changed 5 years ago by bendavis78

  • Patch needs improvement set

comment:41 Changed 5 years ago by bendavis78

  • Needs tests set
  • Patch needs improvement unset

Ok, I've updated the patch and fixed some code that was mysteriously duplicated, as well as fixed the tests that were written for the select_related change (they just needed init files).

I also believe I have the .values() query working with reverse one-to-one relationships, though I'd really like mtredinnick to take a look at the change since he's the one that wrote the original code for this. The change was fairly simple. In the setup_joins function in db/models/sql/query.py:

                     raise FieldError("Cannot resolve keyword %r into field. "
                             "Choices are: %s" % (name, ", ".join(names)))

-            if not allow_many and (m2m or not direct):
+            if not allow_many and m2m:
                 for alias in joins:
                     self.unref_alias(alias)
                 raise MultiJoin(pos + 1)

Looking at the original code, I'm not sure why "indirect" fields were not allowed if they weren't many-to-many relationships. This is basically what was keeping the reverse one-to-one lookups from working. I'm not 100% sure of the consequences of this change, but all model tests seemed to pass with this change.

@mtredinnick: thoughts?

We still need a test for this particular change, so I'm leaving "Needs tests" checked for now.

comment:42 Changed 5 years ago by IonelMaries

  • Owner changed from mtredinnick to IonelMaries
  • Status changed from new to assigned

comment:43 Changed 5 years ago by IonelMaries

  • Owner changed from IonelMaries to mtredinnick
  • Status changed from assigned to new

comment:44 Changed 5 years ago by IonelMaries

  • Cc ionel.mc@… added

comment:45 Changed 5 years ago by garrison

  • Cc garrison added

comment:46 Changed 5 years ago by garrison

Why not make this (following reverse relationships as well as forward ones) the default behavior if select_related() is given no arguments?

comment:47 Changed 5 years ago by bendavis78

I'm honestly not sure why the attached patch hasn't made it in -- I've been using it in my production environments and haven't had any problems. I was waiting on some feedback from mtredinnick, as to what the "if not allow_many and (m2m or not direct)" was for, but I'm not sure if he's following this ...

comment:48 Changed 5 years ago by SmileyChris

It's not ready for checkin, because it's missing the test (you marked it as such). After the test is added, feel free to bring it up in the dev google group.

comment:49 Changed 5 years ago by anonymous

  • Cc dexterbt1@… added

Changed 5 years ago by bendavis78

patches against trunk, r11479

comment:50 Changed 5 years ago by bendavis78

  • Needs tests unset

Ok, I've update the patch for the r11479, and have included a basic test (it's my first time for this, so please advise if it needs to be better).

comment:51 Changed 5 years ago by dchristian

Patch fails when the relationship is held on a base class.

class A(models.Model):

pass

class B(A):

pass

class C(models.Model):

a_ref = models.OneToOneField('A', related_name='c_list')

B.objects.select_related('c_list')

fails. Reproducing from memory as I am not using this patch now. In any case it's pretty clear that the patch ignores base classes.

comment:52 Changed 5 years ago by anonymous

  • Cc jdunck@… added

comment:53 follow-up: Changed 5 years ago by bendavis78

@dchristian: I'm a bit confused by your use case there, as I don't see how that particular setup would be used in a real-world situation. That is, I'm having trouble seeing a situation where I would have a one-to-one referencing a base model. If you could provide a more real-world example that would help.

comment:54 in reply to: ↑ 53 Changed 5 years ago by dchristian

We have a generic Content type, and a Article type that is a more specific type of content. Content is linked to by other sub-objects, one of which is a OneToOne but is kept in a different app to keep things separated nicely.

But in any case, the point is that this patch, if applied, will result in tracebacks when used in that situation, so I wouldn't recommend approving it until the situation is at least handled in some way that doesn't crash.

comment:55 Changed 5 years ago by anonymous

  • Cc kmike84@… added

comment:56 Changed 5 years ago by anonymous

  • Cc vinilios@… added

comment:57 Changed 5 years ago by oldium

  • Cc oldium.pro@… added

comment:58 Changed 4 years ago by drakkan <nicola.murino@…>

  • Cc nicola.murino@… added

comment:59 Changed 4 years ago by anonymous

  • Cc mike@… added

comment:60 Changed 4 years ago by daemianmack

  • Cc daemianmack@… added

comment:61 Changed 4 years ago by Harm Geerts <hgeerts@…>

  • Cc hgeerts@… added

comment:62 Changed 4 years ago by powderflask

  • Cc powderflask@… added

comment:63 Changed 4 years ago by anonymous

  • Keywords performance added

comment:64 Changed 4 years ago by david

  • Cc david added

Changed 4 years ago by Alex

comment:65 Changed 4 years ago by Alex

This new version passes all tests (both the ones it introduces and the existing ones), I didn't implement the reverse-fkey support because it seemed a bit hacky to me. Any other test cases would be great.

comment:66 Changed 4 years ago by russellm

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

(In [12307]) Fixed #7270 -- Added the ability to follow reverse OneToOneFields in select_related(). Thanks to George Vilches, Ben Davis, and Alex Gaynor for their work on various stages of this patch.

comment:67 Changed 4 years ago by s.angel@…

  • Resolution fixed deleted
  • Status changed from closed to reopened

The commit [12307] has a bug. If i use select_related with a OneToOneField which can be null, if for the FIRST object, this relation is null, then it's give me an error

A path is joined

comment:68 Changed 4 years ago by anonymous

  • Cc s.angel@… added

comment:69 Changed 4 years ago by Alex

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

The original issue here was fixed, please open a new bug for the new issue.

comment:70 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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.