Opened 5 years ago

Closed 3 years ago

#13562 closed Bug (worksforme)

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

Reported by: bendavis78 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

Description

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 bendavis78 5 years ago.
Patch for test that should pass.
13562_test_only.diff (1.1 KB) - added by aaugustin 4 years ago.

Download all attachments as: .zip

Change History (13)

Changed 5 years ago by bendavis78

Patch for test that should pass.

comment:1 Changed 5 years ago by Alex

  • 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 5 years ago by bendavis78

@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 5 years ago by Alex

Do we do left outer joins for forward relations now?

comment:4 Changed 5 years ago by bendavis78

  • 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 5 years ago by Alex

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 5 years ago by bendavis78

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 5 years ago by Alex

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

comment:8 Changed 5 years ago by russellm

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Triage Stage changed from Unreviewed to Accepted

comment:9 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:10 Changed 4 years ago by aaugustin

  • 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 4 years ago by aaugustin

comment:11 Changed 3 years ago by aaugustin

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

Closing since the problem appears to be fixed.

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