Opened 11 years ago

Closed 11 years ago

Last modified 11 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

Description

To reproduce, first the models:

from django.db import models

class Alfa(models.Model):
    pass

class Bravo(models.Model):
    pass

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)

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

Download all attachments as: .zip

Change History (9)

by Eivind Fonn <evfonn@…>, 11 years ago

Attachment: models.py added

by Eivind Fonn <evfonn@…>, 11 years ago

Attachment: shell_session.txt added

Shell session to reproduce

by Eivind Fonn <evfonn@…>, 11 years ago

Attachment: reproduce.py added

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

comment:1 by Baptiste Mispelon, 11 years ago

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 by Anssi Kääriäinen, 11 years ago

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 by Anssi Kääriäinen <akaariai@…>, 11 years ago

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 by Anssi Kääriäinen, 11 years ago

While working on adding tests for this ticket in master I noticed a couple of possibilities for improved join promotion logic. The PR at https://github.com/django/django/pull/1677 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 by Anssi Kääriäinen <akaariai@…>, 11 years ago

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 by Anssi Kääriäinen <akaariai@…>, 11 years ago

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