Opened 3 months ago

Closed 5 days ago

Last modified 5 days ago

#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 Simon Charette, 3 months ago

Triage Stage: UnreviewedAccepted

comment:2 by Simon Charette, 3 months ago

Cc: Simon Charette added

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 through values and values_list.

With composite primary keys we've made usages of values("pk") turned into values(Tuple(*pk_field_attnames)) and not values(*pk_field_attnames) so we should be coherent here IMO which means that we likely want something like

  • tests/composite_pk/test_values.py

    diff --git a/tests/composite_pk/test_values.py b/tests/composite_pk/test_values.py
    index 03a9a85496..cc45db40bc 100644
    a b  
    33
    44from django.test import TestCase
    55
    6 from .models import Post, Tenant, User
     6from .models import Comment, Post, Tenant, User
    77
    88
    99class CompositePKValuesTests(TestCase):
    def test_values(self):  
    210210                    {"pk": self.user_3.pk, "id": self.user_3.id},
    211211                ),
    212212            )
     213
     214    def test_foreign_object_values(self):
     215        Comment.objects.create(id=1, user=self.user_1, integer=42)
     216        values = list(Comment.objects.values("user", "integer"))
     217        self.assertEqual(values[0]["user"], (self.user_1.tenant_id, self.user_1.id))
     218        self.assertEqual(values[0]["integer"], 42)

In 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 and values_list return tuples instead of flattening them as we've taken this approach for CompositePrimaryKey (which is now part of the public API) and that ForeignObject is still private and has been broken since its introduction in this regard.

comment:3 by Colleen Dunlap, 3 months ago

Owner: set to Colleen Dunlap
Status: newassigned

comment:4 by Colleen Dunlap, 4 weeks ago

Owner: Colleen Dunlap removed
Status: assignednew

comment:5 by JaeHyuckSa, 10 days ago

Owner: set to JaeHyuckSa
Status: newassigned

comment:6 by JaeHyuckSa, 10 days ago

Has patch: set

comment:7 by Simon Charette, 10 days ago

Patch needs improvement: set

comment:8 by Simon Charette, 10 days 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 JaeHyuckSa, 9 days ago

Patch needs improvement: unset

comment:10 by Jacob Walls, 9 days ago

Keywords: CompositePrimaryKey added; composite primary key removed

comment:11 by Sarah Boyce, 6 days ago

Severity: NormalRelease blocker
Version: 4.25.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

Last edited 6 days ago by Sarah Boyce (previous) (diff)

comment:12 by Sarah Boyce, 6 days ago

Patch needs improvement: set

comment:13 by JaeHyuckSa, 5 days ago

Patch needs improvement: unset

comment:14 by Jacob Walls, 5 days ago

Triage Stage: AcceptedReady for checkin

comment:15 by Jacob Walls <jacobtylerwalls@…>, 5 days ago

Resolution: fixed
Status: assignedclosed

In bb7a770:

Fixed #36431 -- Returned tuples for multi-column ForeignObject in values()/values_list().

Thanks Jacob Walls and Simon Charette for tests.

Signed-off-by: SaJH <wogur981208@…>

comment:16 by Jacob Walls <jacobtylerwalls@…>, 5 days ago

In ace59cb:

[5.2.x] Fixed #36431 -- Returned tuples for multi-column ForeignObject in values()/values_list().

Thanks Jacob Walls and Simon Charette for tests.

Signed-off-by: SaJH <wogur981208@…>

Backport of bb7a7701b1a0e8fffe14dcebf5d5bac7f176c02a from main

Note: See TracTickets for help on using tickets.
Back to Top