[Django] #26515: trim_joins bug with nested related models using ForeignObject
Django
-------------------------------------+-------------------------------------
Reporter: ornoone | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) | Keywords: ForeignObject,
Severity: Normal | trim_join, orm
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
the order in which we declare the `from_fields` and `to_fields` does
matter if we make a nested lookup.
it is clear that the fields in from_fields and to_fields must be on the
same orders, but if 3 models use ForeignObject to links themselves, and
the fields are the sames on all 3 models, the order in which they are
declared in both fields must be the same.
a example app that can trigger the bug follow :
{{{#!python
class Address(models.Model):
company = models.CharField(max_length=1)
tiers_id = models.IntegerField()
city = models.CharField(max_length=255)
postcode = models.CharField(max_length=32)
class Meta(object):
unique_together = [
("company", "tiers_id"),
]
class Customer(models.Model):
company = models.CharField(max_length=1)
customer_id = models.IntegerField()
name = models.CharField(max_length=255)
address = ForeignObject(
Address, on_delete=CASCADE, null=True,
from_fields=["customer_id", "company"],
to_fields=["tiers_id", "company"]
)
class Meta(object):
unique_together = [
("company", "customer_id"),
]
class Contact(models.Model):
company_code = models.CharField(max_length=1)
customer_code = models.IntegerField()
surname = models.CharField(max_length=255)
# virtual field
customer = ForeignObject(
Customer, on_delete=CASCADE, related_name='contacts',
# not the same order as for Customer -> address which is
(customer, company)
to_fields=["company", "customer_id"],
from_fields=["company_code", "customer_code"]
# with same order as for Customer, the bug does not trigger
# to_fields = ["customer_id", "company"],
# from_fields = ["customer_code", "company_code"]
)
class PhoneNumber(models.Model):
num = models.CharField(max_length=32)
type_number = models.IntegerField()
contact = models.ForeignKey(Contact, on_delete=CASCADE,
related_name='phonenumbers')
}}}
with this models.py, the different orders of the fields Contact.customer
can break all query like
`PhoneNumber.objects.filter(contact__customer__address=a)`.(see comment in
Customer)
I found the problem is in django.db.models.query.Query.trim_joins line
1444 on release 1.9.5
{{{#!python3
targets = tuple(r[0] for r in info.join_field.related_fields if
r[1].column in cur_targets)
}}}
we see that the new targets is created using the previous used targets,
but the order of the previous target is not kept, and the order of
to_fields is used to define the new targets.
this lead to a query like :
{{{#!sql
SELECT "buggyapp_phonenumber"."id", "buggyapp_phonenumber"."num",
"buggyapp_phonenumber"."type_number", "buggyapp_phonenumber"."contact_id"
FROM "buggyapp_phonenumber" INNER JOIN "buggyapp_contact" ON
("buggyapp_phonenumber"."contact_id" = "buggyapp_contact"."id") WHERE
("buggyapp_contact"."company_code" = 10 AND
"buggyapp_contact"."customer_code" = a)
}}}
instead of
{{{#!sql
SELECT "buggyapp_phonenumber"."id", "buggyapp_phonenumber"."num",
"buggyapp_phonenumber"."type_number", "buggyapp_phonenumber"."contact_id"
FROM "buggyapp_phonenumber" INNER JOIN "buggyapp_contact" ON
("buggyapp_phonenumber"."contact_id" = "buggyapp_contact"."id") WHERE
("buggyapp_contact"."customer_code" = 10 AND
"buggyapp_contact"."company_code" = a)
}}}
the where part have the values order kept, but the fields order is
inconsistent and it end with customer_code = 'a' instead of 10.
the attached files contains a basic app that trigger the bug, and a patch
in trim_query that will build the new targets with same order as the
previous.
the patch is made for django 1.9.5
--
Ticket URL: <https://code.djangoproject.com/ticket/26515>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: ornoone | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: ForeignObject, trim_join, orm | Triage Stage: Unreviewed
Has patch: 1 | Easy pickings: 0
UI/UX: 0 |
Changes (by ornoone):
* Attachment "ForeignObject_from_fields__bug_26515.tar.gz" added.
simple app to trigger the bug
Django
Reporter: ornoone | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Has patch: 1 | Easy pickings: 0
UI/UX: 0 |
Changes (by ornoone):
* Attachment "ForeignObject_from_fields__bug_26515.patch" added.
patch that keep order of targets in trim_joins
Django
-------------------------------------+-------------------------------------
Reporter: ornoone | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: ForeignObject, | Triage Stage:
trim_join, orm | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Can you create a test for this in Django's test suite? See
tests/foreign_object/ directory for some similar tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/26515#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: ornoone | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: ForeignObject, | Triage Stage:
trim_join, orm | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by ornoone):
Replying to [comment:1 akaariai]:
> Can you create a test for this in Django's test suite? See
tests/foreign_object/ directory for some similar tests.
I just forked the django repo (hope it was what you expected) and wrote
down my tests in tests/foreign_object/.
the test `test_very_deep_optimized_forward` fail without the patch, and
after patch, all tests pass on my computer.
--
Ticket URL: <https://code.djangoproject.com/ticket/26515#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: ornoone | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: ForeignObject, | Triage Stage:
trim_join, orm | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
Could you please send a pull request against master with those changes?
See our PatchReviewChecklist.
--
Ticket URL: <https://code.djangoproject.com/ticket/26515#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: ornoone | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: ForeignObject, | Triage Stage:
trim_join, orm | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by ornoone):
done right now.
https://github.com/django/django/pull/6473
thanks
--
Ticket URL: <https://code.djangoproject.com/ticket/26515#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: ornoone | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: ForeignObject, | Triage Stage: Accepted
trim_join, orm |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Left comments for cosmetic improvements on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/26515#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: ornoone | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: ForeignObject, | Triage Stage: Accepted
trim_join, orm |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26515#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: ornoone | Owner: nobody
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: ForeignObject, | Triage Stage: Accepted
trim_join, orm |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"a7ad473ad27995736b07750ac64e2651ff790529" a7ad473a]:
{{{
#!CommitTicketReference repository=""
revision="a7ad473ad27995736b07750ac64e2651ff790529"
Fixed #26515 -- Fixed Query.trim_joins() for nested ForeignObjects.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26515#comment:7>