Opened 3 weeks ago

Closed 2 weeks ago

Last modified 2 weeks ago

#36648 closed Bug (fixed)

"pk" exception when using first() on unordered queryset with aggregation does not consider composite pk fields provided separately

Reported by: Jacob Walls Owned by: Jacob Walls
Component: Database layer (models, ORM) Version: 5.2
Severity: Release blocker Keywords: compositepk
Cc: 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

Discovered while poking into the source for first() that we have a special case for "pk" that does not behave equivalently for composite primary key fields specified individually.

A test for CompositePKAggregateTests:

The commented out assertion shows the point of the guard -- it should raise a TypeError in most cases of using first() on an unordered queryset, see #33772.

The second assertion shows the exception for values("pk") -- this does not raise.
The third assertion tries to provide the values for a composite pk separately -- this should not raise...

    def test_group_by_guard(self):
        # This assertion passes, only included to demonstrate the behavior.
        # with self.assertRaises(TypeError):
        #     Comment.objects.values("text").annotate(max=Max("id")).first()
        Comment.objects.values("pk").annotate(max=Max("id")).first()  # passes
        Comment.objects.values("tenant", "id").annotate(max=Max("id")).first()  # fails

... but does:

TypeError: Cannot use QuerySet.first() on an unordered queryset performing aggregation. Add an ordering with order_by().

Change History (11)

comment:1 by Natalia Bidart, 3 weeks ago

Keywords: compositepk added
Triage Stage: UnreviewedAccepted

comment:2 by Natalia Bidart, 3 weeks ago

Summary: "pk" exception from guard against using first() on unordered queryset with aggregation does not consider composite pk fields provided separately"pk" exception when using first() on unordered queryset with aggregation does not consider composite pk fields provided separately

comment:3 by Jacob Walls, 3 weeks ago

Owner: set to Jacob Walls
Status: newassigned

comment:4 by Jacob Walls, 2 weeks ago

Has patch: set

comment:5 by Natalia Bidart, 2 weeks ago

Triage Stage: AcceptedReady for checkin

comment:6 by Jacob Walls <jacobtylerwalls@…>, 2 weeks ago

Resolution: fixed
Status: assignedclosed

In 02eed4f:

Fixed #36648, Refs #33772 -- Accounted for composite pks in first()/last() when aggregating.

comment:7 by Jacob Walls <jacobtylerwalls@…>, 2 weeks ago

In 28c95a35:

[6.0.x] Fixed #36648, Refs #33772 -- Accounted for composite pks in first()/last() when aggregating.

Backport of 02eed4f37879b2077496f86bb1378a076b981233 from main.

comment:8 by Jacob Walls <jacobtylerwalls@…>, 2 weeks ago

In 8baee53:

[5.2.x] Fixed #36648, Refs #33772 -- Accounted for composite pks in first()/last() when aggregating.

Backport of 02eed4f37879b2077496f86bb1378a076b981233 from main.

comment:9 by Jacob Walls <jacobtylerwalls@…>, 2 weeks ago

In bee6456:

Refs #36648 -- Removed hardcoded pk in CompositePKAggregateTests.

comment:10 by Jacob Walls <jacobtylerwalls@…>, 2 weeks ago

In 2cc9072:

[6.0.x] Refs #36648 -- Removed hardcoded pk in CompositePKAggregateTests.

Backport of bee64561a6e8cd22995c2b1254bab66dae892a6d from main.

comment:11 by Jacob Walls <jacobtylerwalls@…>, 2 weeks ago

In dc61f20:

[5.2.x] Refs #36648 -- Removed hardcoded pk in CompositePKAggregateTests.

Backport of bee64561a6e8cd22995c2b1254bab66dae892a6d from main.

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