[Django] #31657: self referencing foreign key join and order_by issues
Django
-----------------------------------------+------------------------
Reporter: Jack Delany | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 3.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Initially discovered on 2.2.10 but verified still happens on 3.0.6. Given
the following models:
{{{
class OneModel(models.Model):
class Meta:
ordering = ("-id",)
id = models.BigAutoField(primary_key=True)
root = models.ForeignKey("OneModel", on_delete=models.CASCADE,
null=True)
oneval = models.BigIntegerField(null=True)
class TwoModel(models.Model):
id = models.BigAutoField(primary_key=True)
record = models.ForeignKey(OneModel, on_delete=models.CASCADE)
twoval = models.BigIntegerField(null=True)
}}}
The following queryset gives unexpected results and appears to be an
incorrect SQL query:
{{{
qs = TwoModel.objects.filter(record__oneval__in=[1,2,3])
qs = qs.order_by("record__root_id")
print(qs.query)
}}}
{{{
SELECT "orion_twomodel"."id", "orion_twomodel"."record_id",
"orion_twomodel"."twoval" FROM "orion_twomodel" INNER JOIN
"orion_onemodel" ON ("orion_twomodel"."record_id" = "orion_onemodel"."id")
LEFT OUTER JOIN "orion_onemodel" T3 ON ("orion_onemodel"."root_id" =
T3."id") WHERE "orion_onemodel"."oneval" IN (1, 2, 3) ORDER BY T3."id"
DESC
}}}
The query has an unexpected DESCENDING sort. That appears to come from
the default sort order on the OneModel class, but I would expect the
order_by() to take prececence. The the query has two JOINS, which is
unnecessary. It appears that, since OneModel.root is a foreign key to
itself, that is causing it to do the unnecessary extra join. In fact,
testing a model where root is a foreign key to a third model doesn't show
the problem behavior.
Note also that the queryset with `order_by("record__root")` gives the
exact same SQL.
This queryset gives correct results and what looks like a pretty optimal
SQL:
{{{
qs = TwoModel.objects.filter(record__oneval__in=[1,2,3])
qs = qs.order_by("record__root__id")
print(qs.query)
}}}
{{{
SELECT "orion_twomodel"."id", "orion_twomodel"."record_id",
"orion_twomodel"."twoval" FROM "orion_twomodel" INNER JOIN
"orion_onemodel" ON ("orion_twomodel"."record_id" = "orion_onemodel"."id")
WHERE "orion_onemodel"."oneval" IN (1, 2, 3) ORDER BY
"orion_onemodel"."root_id" ASC
}}}
So is this a potential bug or a misunderstanding on my part?
Another queryset that works around the issue and gives a reasonable SQL
query and expected results:
{{{
qs = TwoModel.objects.filter(record__oneval__in=[1,2,3])
qs = qs.annotate(root_id=F("record__root_id"))
qs = qs.order_by("root_id")
print(qs.query)
}}}
{{{
SELECT "orion_twomodel"."id", "orion_twomodel"."record_id",
"orion_twomodel"."twoval" FROM "orion_twomodel" INNER JOIN
"orion_onemodel" ON ("orion_twomodel"."record_id" = "orion_onemodel"."id")
WHERE "orion_onemodel"."oneval" IN (1, 2, 3) ORDER BY
"orion_onemodel"."zero_id" ASC
}}}
ASCENDING sort, and a single INNER JOIN, as I'd expect. That actually
works for my use because I need that output column anyway.
One final oddity; with the original queryset but the inverted sort
order_by():
{{{
qs = TwoModel.objects.filter(record__oneval__in=[1,2,3])
qs = qs.order_by("-record__root_id")
print(qs.query)
}}}
{{{
SELECT "orion_twomodel"."id", "orion_twomodel"."record_id",
"orion_twomodel"."twoval" FROM "orion_twomodel" INNER JOIN
"orion_onemodel" ON ("orion_twomodel"."record_id" = "orion_onemodel"."id")
LEFT OUTER JOIN "orion_onemodel" T3 ON ("orion_onemodel"."root_id" =
T3."id") WHERE "orion_onemodel"."oneval" IN (1, 2, 3) ORDER BY T3."id" ASC
}}}
One gets the query with the two JOINs but an ASCENDING sort order. I was
not under the impression that sort orders are somehow relative to the
class level sort order, eg: does specifing `order_by("-record__root_id")`
invert the class sort order? Testing that on a simple case doesn't show
that behavior at all.
Thanks for any assistance and clarification.
--
Ticket URL: <https://code.djangoproject.com/ticket/31657>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Jack Delany | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Something is definitely wrong here.
- `order_by('record__root_id')` should result in `ORDER BY root_id`
- `order_by('record__root')` should use the `Meta.ordering` in `ORDER BY
root_join.id DESC`
[https://docs.djangoproject.com/en/3.0/ref/models/querysets/#django.db.models.query.QuerySet.order_by
as documented]
- `order_by('record__root__id')` should result in `ORDER BY root_join.id`
--
Ticket URL: <https://code.djangoproject.com/ticket/31657#comment:2>
Django
field.
-------------------------------------+-------------------------------------
Reporter: Jack Delany | Owner: nobody
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jack Delany):
FWIW, I did apply the suggested fix to my local version and I do now get
what looks like correct behavior and results that match what Simon
suggested should be the sort results. Also my annotate "workaround"
continues to behave correctly. Looks promising.
--
Ticket URL: <https://code.djangoproject.com/ticket/31657#comment:4>
Django
field.
-------------------------------------+-------------------------------------
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
Jack, would you like to prepare a patch?
--
Ticket URL: <https://code.djangoproject.com/ticket/31657#comment:5>
Django
field.
-------------------------------------+-------------------------------------
| Ramezani
Type: Bug | Status: assigned
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* owner: nobody => Hasan Ramezani
* status: new => assigned
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/13033 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/31657#comment:6>
Django
field.
-------------------------------------+-------------------------------------
Reporter: Jack Delany | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jack Delany):
I did some additional work on this to verify the scope of the change. I
added the following **test** code on master and ran the entire test suite:
{{{
+++ b/django/db/models/sql/compiler.py
@@ -727,6 +727,11 @@ class SQLCompiler:
# If we get to this point and the field is a relation to another
model,
# append the default ordering for that model unless it is the pk
# shortcut or the attribute name of the field that is specified.
+ if (field.is_relation and opts.ordering and name != 'pk' and
+ ((getattr(field, 'attname', None) != name) !=
+ (getattr(field, 'attname', None) != pieces[-1]))):
+ print(f"JJD <{getattr(field, 'attname', '')}> <{name}>
<{pieces[-1]}>")
+ breakpoint()
if field.is_relation and opts.ordering and getattr(field,
'attname', None) != name and name != 'pk':
# Firstly, avoid infinite loops.
already_seen = already_seen or set()
}}}
The idea being to display every time that the change from name to
pieces[-1] in the code would make a difference in the execution. Of
course verified that when running the reproducer one does go into the test
block and outputs: {{{JJD <root_id> <record__root_id> <root_id>}}}. The
code is not triggered for any other test across the entire test suite, so
the scope of the change is not causing unexpected changes in other places.
This seems reassuring.
--
Ticket URL: <https://code.djangoproject.com/ticket/31657#comment:7>
Django
field.
-------------------------------------+-------------------------------------
Reporter: Jack Delany | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"c1f7de8acc187a3899f7f3cda47edbc2f4142d7d" c1f7de8a]:
{{{
#!CommitTicketReference repository=""
revision="c1f7de8acc187a3899f7f3cda47edbc2f4142d7d"
Refs #31657 -- Added test for ordering by self-referential ForeignKeys.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31657#comment:8>
Django
field.
-------------------------------------+-------------------------------------
Reporter: Jack Delany | Owner: Hasan
| Ramezani
Component: Database layer | Version: 3.1
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"037a624120b676858be4b93c4d0adda45916fd2a" 037a6241]:
{{{
#!CommitTicketReference repository=""
revision="037a624120b676858be4b93c4d0adda45916fd2a"
Fixed #31657 -- Fixed ordering by attnames of self-referential
ForeignKeys.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31657#comment:9>
Django
field.
-------------------------------------+-------------------------------------
Reporter: Jack Delany | Owner: Hasan
| Ramezani
Type: Bug | Status: closed
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"bdfad5731924e02aae01d799632e8fcdcbd588fd" bdfad573]:
{{{
#!CommitTicketReference repository=""
revision="bdfad5731924e02aae01d799632e8fcdcbd588fd"
[3.1.x] Refs #31657 -- Added test for ordering by self-referential
ForeignKeys.
Backport of c1f7de8acc187a3899f7f3cda47edbc2f4142d7d from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31657#comment:10>
Django
field.
-------------------------------------+-------------------------------------
Reporter: Jack Delany | Owner: Hasan
| Ramezani
Type: Bug | Status: closed
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"4385ef0119c2e800f44ae92e3215cc8e571f6e7d" 4385ef01]:
{{{
#!CommitTicketReference repository=""
revision="4385ef0119c2e800f44ae92e3215cc8e571f6e7d"
[3.1.x] Fixed #31657 -- Fixed ordering by attnames of self-referential
ForeignKeys.
Backport of 037a624120b676858be4b93c4d0adda45916fd2a from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31657#comment:11>