#36431 closed Bug (fixed)
values() query on ForeignObject discards additional columns
Reported by: | Jacob Walls | Owned by: | JaeHyuckSa |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 5.2 |
Severity: | Release blocker | Keywords: | CompositePrimaryKey |
Cc: | Simon Charette | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
For a multi-column field using ForeignObject
as modeled here, .values("user")
will discard columns after the first, and values("user", "integer")
will select the second column of user
into the integer
namespace.
Here's a failing test for tests/composite_pk/test_values.py:
def test_foreign_object_values(self): Comment.objects.create(id=1, user=self.user_1, integer=42) values = list(Comment.objects.values("user", "integer")) self.assertEqual(values[0]["integer"], 42)
====================================================================== FAIL: test_foreign_object_values (composite_pk.test_values.CompositePKValuesTests.test_foreign_object_values) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/.../django/tests/composite_pk/test_values.py", line 217, in test_foreign_object_values self.assertEqual(values[0]["integer"], 42) ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: 1 != 42
And a similar rough test for foreign_objects/tests.py that fails on Django 4.2, to remove the composite primary key from discussion. (If this model had more fields, this values query would be more realistic):
class ForeignObjectValues(TestCase): def test_foreign_object_values(self): from .models import Customer customer_1 = Customer.objects.create(customer_id=1, company="a") customer_2 = Customer.objects.create(customer_id=1, company="b") company_a_customers = Customer.objects.filter(company="a").values("address", "company") self.assertSequenceEqual( Customer.objects.exclude(company__in=[cust["company"] for cust in company_a_customers]), [customer_2], # Fails, all companies included )
Change History (16)
comment:1 by , 4 months ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 4 months ago
Cc: | added |
---|
comment:3 by , 4 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:4 by , 6 weeks ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:5 by , 4 weeks ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:6 by , 4 weeks ago
Has patch: | set |
---|
comment:7 by , 4 weeks ago
Patch needs improvement: | set |
---|
comment:8 by , 4 weeks ago
I suspect the logic will be able to be simplified through #35956 (setup_join
will most likely need to return a single multi-valued target instead of being multi-valued itself) but the proposed patch seems focused enough to be accepted as is.
comment:9 by , 4 weeks ago
Patch needs improvement: | unset |
---|
comment:10 by , 4 weeks ago
Keywords: | CompositePrimaryKey added; composite primary key removed |
---|
comment:11 by , 4 weeks ago
Severity: | Normal → Release blocker |
---|---|
Version: | 4.2 → 5.2 |
Marking as a release blocker for 5.2, even though the issue has existed within ForeignObject
for longer, since in 5.2 we've opted to start documenting ForeignObject
's usage with CompositePrimaryKey
(whereas before ForeignObject
was not documented/public)
This is roughly consistent with other decisions, see also #36116
comment:12 by , 4 weeks ago
Patch needs improvement: | set |
---|
comment:13 by , 4 weeks ago
Patch needs improvement: | unset |
---|
comment:14 by , 4 weeks ago
Triage Stage: | Accepted → Ready for checkin |
---|
On interesting dilemma we'll have to figure out here is what exactly to return when a foreign object composed of more than one
from_fields
is selected throughvalues
andvalues_list
.With composite primary keys we've made usages of
values("pk")
turned intovalues(Tuple(*pk_field_attnames))
and notvalues(*pk_field_attnames)
so we should be coherent here IMO which means that we likely want something liketests/composite_pk/test_values.py
Post, Tenant, UserIn other words I suggest we double down on making sure references to composite fields (the only two we support today is composite pk and foreign objects with multiple fields) in
values
andvalues_list
return tuples instead of flattening them as we've taken this approach forCompositePrimaryKey
(which is now part of the public API) and thatForeignObject
is still private and has been broken since its introduction in this regard.