Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#21150 closed Bug (fixed)

select_related and annotate won't follow nullable foreign keys

Reported by: Eivind Fonn <evfonn@…> Owned by: nobody
Component: Database layer (models, ORM) Version: 1.6-beta-1
Severity: Release blocker Keywords: select_related annotate
Cc: bmispelon@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


To reproduce, first the models:

from django.db import models

class Alfa(models.Model):

class Bravo(models.Model):

class Charlie(models.Model):
    alfa = models.ForeignKey(Alfa, null=True)
    bravo = models.ForeignKey(Bravo, null=True)

Then in shell:

In [2]: b = Bravo.objects.create()

In [3]: c = Charlie.objects.create(bravo=b)

In [4]: qsboth = Charlie.objects.select_related('alfa').annotate(Count('bravo__charlie')); qsboth
Out[4]: []

In [5]: qsselrel = Charlie.objects.select_related('alfa'); qsselrel
Out[5]: [<Charlie: Charlie object>]

In [6]: qsanno = Charlie.objects.annotate(Count('bravo__charlie')); qsanno
Out[6]: [<Charlie: Charlie object>]

As you can see, select_related and annotate both work as expected, but not together. Queries:

In [7]: print(str(qsboth.query))
SELECT "bugrep_charlie"."id", "bugrep_charlie"."alfa_id", "bugrep_charlie"."bravo_id", COUNT(T4."id") AS "bravo__charlie__count", "bugrep_alfa"."id" FROM "bugrep_charlie" INNER JOIN "bugrep_alfa" ON ( "bugrep_charlie"."alfa_id" = "bugrep_alfa"."id" ) LEFT OUTER JOIN "bugrep_bravo" ON ( "bugrep_charlie"."bravo_id" = "bugrep_bravo"."id" ) LEFT OUTER JOIN "bugrep_charlie" T4 ON ( "bugrep_bravo"."id" = T4."bravo_id" ) GROUP BY "bugrep_charlie"."id", "bugrep_charlie"."alfa_id", "bugrep_charlie"."bravo_id", "bugrep_alfa"."id"

In [8]: print(str(qsselrel.query))
SELECT "bugrep_charlie"."id", "bugrep_charlie"."alfa_id", "bugrep_charlie"."bravo_id", "bugrep_alfa"."id" FROM "bugrep_charlie" LEFT OUTER JOIN "bugrep_alfa" ON ( "bugrep_charlie"."alfa_id" = "bugrep_alfa"."id" )

Using only select_related yields an outer join on the alfa table, while adding annotate in the mix gives an inner join. Indeed, if we make an alfa object, it works fine:

In [9]: a = Alfa.objects.create()

In [10]: Charlie.objects.update(alfa=a)
Out[10]: 1

In [11]: Charlie.objects.annotate(Count('bravo__charlie')).select_related('alfa')
Out[11]: [<Charlie: Charlie object>]

Attachments (3) (226 bytes) - added by Eivind Fonn <evfonn@…> 5 years ago.
shell_session.txt (2.0 KB) - added by Eivind Fonn <evfonn@…> 5 years ago.
Shell session to reproduce (1.6 KB) - added by Eivind Fonn <evfonn@…> 5 years ago.
Quick executable to reproduce (assumes project and app are called bugrep)

Download all attachments as: .zip

Change History (9)

Changed 5 years ago by Eivind Fonn <evfonn@…>

Attachment: added

Changed 5 years ago by Eivind Fonn <evfonn@…>

Attachment: shell_session.txt added

Shell session to reproduce

Changed 5 years ago by Eivind Fonn <evfonn@…>

Attachment: added

Quick executable to reproduce (assumes project and app are called bugrep)

comment:1 Changed 5 years ago by Baptiste Mispelon

Cc: bmispelon@… added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

The issue seems to have been fixed in master by commit c21e86ab9e3e5ebd6d245d038cb0cb352cd84c3a.

I'm marking it as a regression since it's not present in 1.5 either (it was introduced by edf93127bf2f9dc35b45cdea5d39a1b417ab1638).

Thanks for the detailed report.

comment:2 Changed 5 years ago by Anssi Kääriäinen

It seems the problem is that setup_joins() arguments have changed. It is called with positional arguments in many places, and as the arguments for setup_joins() change, the flags passed to setup_joins() as positional arguments end up targeting different parameters.

I recommend to go through all uses of setup_joins() in the ORM, and change the calls to use kwarg=someval for all optional arguments. Otherwise further changes will again cause similar regressions.

The patch doesn't need to be backpatched, just checking that correct arguments are passed to setup_joins() is enough. Quickly thinking I am not sure if backpatching could cause further regressions. Seems safe to backpatch, but I can't guarantee it is actually safe to do so.

comment:3 Changed 5 years ago by Anssi Kääriäinen <akaariai@…>

Resolution: fixed
Status: newclosed

In 1a922870ea07e730281c0e2a7b3a170232a81236:

[1.6.x] Fixed #21150 -- Improved Query.add_fields() join promotion logic

Thanks to Eivind Fonn for the report and test case.

comment:4 Changed 5 years ago by Anssi Kääriäinen

While working on adding tests for this ticket in master I noticed a couple of possibilities for improved join promotion logic. The PR at can now do join demotion (that is, joins that are promoted to outer joins earlier in the query can be demoted back to inner joins later on), and some cases of annotate() and .values() will create correctly inner joins instead of outer join.

There are a couple of added tests for .annotate() and .values() cases, and a couple of expectedFailures are solved by this patch.

I wonder if the added comment for promote_filter_joins() makes any sense to anybody else than me. If not, please notify me.

I have added comments explaining the changes done to the PR. To me it seems a good way to explain why things are changed the way they are changed...

comment:5 Changed 5 years ago by Anssi Kääriäinen <akaariai@…>

In ed0d720b78c8f1c655ead0057d767a0712f1a6a8:

Fixed #21150 -- select_related + annotate join promotion failure

Added tests for a .annotate().select_related() join promotion failure.
This happened to work on master but was currently untested.

comment:6 Changed 5 years ago by Anssi Kääriäinen <akaariai@…>

In ecaba3602837d1e02fe1e961f7d3bf9086453259:

Improved Query join promotion logic

There were multiple cases where join promotion was a bit too aggressive.
This resulted in using outer joins where not necessary.

Refs #21150.

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