[Django] #21150: select_related and annotate won't follow nullable foreign keys
Django
----------------------------------------------+----------------------------
Reporter: Eivind Fonn <evfonn@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.6-beta-1
Severity: Normal | Keywords: select_related
Triage Stage: Unreviewed | annotate
Easy pickings: 0 | Has patch: 0
| UI/UX: 0
----------------------------------------------+----------------------------
To reproduce, first the models:
{{{#!python
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:
{{{#!python
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:
{{{#!python
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:
{{{#!python
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>]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21150>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Eivind Fonn | Owner: nobody
<evfonn@…> | Status: new
Type: Bug | Version:
Component: Database layer | 1.6-beta-1
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: select_related | Needs documentation: 0
annotate | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by bmispelon):
* severity: Normal => Release blocker
* cc: bmispelon@… (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21150#comment:1>
Django
Reporter: Eivind Fonn | Owner: nobody
<evfonn@…> | Status: new
Type: Bug | Version:
Component: Database layer | 1.6-beta-1
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: select_related | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21150#comment:2>
Django
Reporter: Eivind Fonn | Owner: nobody
Type: Bug | Version:
Component: Database layer | 1.6-beta-1
Severity: Release blocker | Triage Stage: Accepted
Keywords: select_related | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"1a922870ea07e730281c0e2a7b3a170232a81236"]:
{{{
#!CommitTicketReference repository=""
revision="1a922870ea07e730281c0e2a7b3a170232a81236"
[1.6.x] Fixed #21150 -- Improved Query.add_fields() join promotion logic
Thanks to Eivind Fonn for the report and test case.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21150#comment:3>
Django
Reporter: Eivind Fonn | Owner: nobody
<evfonn@…> | Status: closed
Type: Bug | Version:
Component: Database layer | 1.6-beta-1
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Accepted
Keywords: select_related | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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...
--
Ticket URL: <https://code.djangoproject.com/ticket/21150#comment:4>
Django
Reporter: Eivind Fonn | Owner: nobody
<evfonn@…> | Status: closed
Type: Bug | Version:
Component: Database layer | 1.6-beta-1
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Accepted
Keywords: select_related | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Anssi Kääriäinen <akaariai@…>):
In [changeset:"ed0d720b78c8f1c655ead0057d767a0712f1a6a8"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21150#comment:5>
Django
Reporter: Eivind Fonn | Owner: nobody
<evfonn@…> | Status: closed
Type: Bug | Version:
Component: Database layer | 1.6-beta-1
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Accepted
Keywords: select_related | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Anssi Kääriäinen <akaariai@…>):
In [changeset:"ecaba3602837d1e02fe1e961f7d3bf9086453259"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21150#comment:6>