Opened 6 years ago

Closed 5 years ago

#13562 closed Bug (worksforme)

values() cannot follow reverse one-to-one relationship

Reported by: Ben Davis Owned by: nobody
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords:
Cc: bendavis78@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


This is similar to #7270, which fixed reverse-one-to-one when doing select_related, however this did not fix reverse-one-to-one when using values().

For example, given that you have a model "Place", and a model "Restaurant" which has a one-to-one relationship with "Place":

>>> Place.objects.all().values('name','restaurant__serves_hot_dogs')
[{'name':'Some restaurant','restaurant__serves_hot_dogs':True},{'name':'Not a Restaurant','restaurant__serves_hot_dogs': None}]

Attachments (2)

values-reverse-one-to-one__test-only.diff (1008 bytes) - added by Ben Davis 6 years ago.
Patch for test that should pass.
13562_test_only.diff (1.1 KB) - added by Aymeric Augustin 5 years ago.

Download all attachments as: .zip

Change History (13)

Changed 6 years ago by Ben Davis

Patch for test that should pass.

comment:1 Changed 6 years ago by Alex Gaynor

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Seems reasonable to me, but honest question: this is going to filter out all non-restaurant objs, why not just do Restaurant.objects.values("serves_hot_dogs", "name") ?

comment:2 Changed 6 years ago by Ben Davis

@Alex: I would argue that it should not filter out all non-restaurant objects; i.e., it should do a LEFT JOIN. The reasoning is that this is the same behavior when doing a .select_related(), and most people would expect all objects to be returned if the "Place" type since that is the base model of the queryset.

comment:3 Changed 6 years ago by Alex Gaynor

Do we do left outer joins for forward relations now?

comment:4 Changed 6 years ago by Ben Davis

Cc: bendavis78@… added

Yes, if you do Place.objects.all().select_related('restaurant'), it performs a LEFT OUTER JOIN. I would argue that this is how it should happen. It seems much less intuitive that a .select_related() would alter the number of results returned.

comment:5 Changed 6 years ago by Alex Gaynor

I'm not talking about select_related() (it should of course never alter result set). i'm talking about Place.objects.values("some_nullable_fkey") , we should be consistent with whatever behavior that has.

comment:6 Changed 6 years ago by Ben Davis

Alex, I'm a bit confused why you're asking about nullable foreign keys -- If Place had, eg, owner = ForeignKey(Owner,null=True,blank=True), and if we did Place.objects.values('owner'), we'd get [{'owner': None}, {'owner': 1}] . The reverse of that, of course, wouldn't make sense, since it's a many-to-one relationship.

Regardless, I would argue that in no case should .values() alter the number of rows returned in the result set. We expect the same of .select_related(), so I don't see why .values() should behave differently.

comment:7 Changed 6 years ago by Alex Gaynor

Bah you're totally right about that, sorry for my confusion :)

comment:8 Changed 6 years ago by Russell Keith-Magee

Component: UncategorizedDatabase layer (models, ORM)
Triage Stage: UnreviewedAccepted

comment:9 Changed 5 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:10 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset
UI/UX: unset

I think this was fixed in 1.3: the docs say:

you can now also refer to fields on related models with reverse relations through OneToOneField, ForeignKey and ManyToManyField attributes

The patch no longer applies, but I've ported it quickly from doctest to unittest (see attachement) and the test passes.

Changed 5 years ago by Aymeric Augustin

Attachment: 13562_test_only.diff added

comment:11 Changed 5 years ago by Aymeric Augustin

Resolution: worksforme
Status: newclosed

Closing since the problem appears to be fixed.

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