Code

Opened 7 months ago

Closed 7 months ago

Last modified 7 months 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@…> 7 months ago.
shell_session.txt (2.0 KB) - added by Eivind Fonn <evfonn@…> 7 months ago.
Shell session to reproduce
reproduce.py (1.6 KB) - added by Eivind Fonn <evfonn@…> 7 months ago.
Quick executable to reproduce (assumes project and app are called bugrep)

Download all attachments as: .zip

Change History (9)

Changed 7 months ago by Eivind Fonn <evfonn@…>

Changed 7 months ago by Eivind Fonn <evfonn@…>

Shell session to reproduce

Changed 7 months ago by Eivind Fonn <evfonn@…>

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

comment:1 Changed 7 months ago by bmispelon

  • Cc bmispelon@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

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 7 months ago by akaariai

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

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

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 7 months ago by akaariai

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 Changed 7 months 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 7 months 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.

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.