[Django] #36051: Count CompositePrimaryKey field targets toward function arity check

[Django]

#36051: Count CompositePrimaryKey field targets toward function arity check
-------------------------------------+-------------------------------------
Reporter: Jacob | Owner: Jacob Walls
Walls |
Type: New | Status: assigned
feature |
Component: Database | Version: dev
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Calling a database function with an incorrect number of arguments raises
`TypeError` if the function declares its
[https://docs.djangoproject.com/en/5.1/ref/models/expressions/#django.db.models.Func.arity
arity]. We could improve the DX around CompositePrimaryKey by leveraging
this feature, instead of just
[https://github.com/django/django/blob/8d9901c961bf9d5cfa6bddddbbcebfbf487a5125/docs/topics
/composite-primary-key.txt#L150 advising] to be careful.

The docs sort of imply some kind of error is raised ("# ERROR"), but this
test passes:

{{{#!diff
diff --git a/tests/composite_pk/test_aggregate.py
b/tests/composite_pk/test_aggregate.py
index b5474c5218..750618a277 100644
--- a/tests/composite_pk/test_aggregate.py
+++ b/tests/composite_pk/test_aggregate.py
@@ -1,5 +1,5 @@
from django.db import NotSupportedError
-from django.db.models import Count, Q
+from django.db.models import Count, Max, Q
from django.test import TestCase

from .models import Comment, Tenant, User
@@ -32,6 +32,10 @@ class CompositePKAggregateTests(TestCase):
cls.comment_5 = Comment.objects.create(id=5, user=cls.user_3,
text="barbaz")
cls.comment_6 = Comment.objects.create(id=6, user=cls.user_3,
text="baz")

+ def test_max_pk(self):
+ # Could raise TypeError instead.
+ self.assertEqual(Comment.objects.aggregate(Max("pk")),
{'pk__max': 1})
+
def test_users_annotated_with_comments_id_count(self):
user_1, user_2, user_3 =
User.objects.annotate(Count("comments__id")).order_by(
"pk"
}}}

Relatedly, most of the built-in aggregate functions should set `arity=1`,
which I'll include in a separate commit, but we can also discuss whether
that needs to go through a deprecation path.
--
Ticket URL: <https://code.djangoproject.com/ticket/36051>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36051: Count CompositePrimaryKey field targets toward function arity check
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/18984 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/36051#comment:1>

[Django]

#36051: Count CompositePrimaryKey field targets toward function arity check
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

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 Tim Graham):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/36051#comment:2>

[Django]

#36051: Count CompositePrimaryKey field targets toward function arity check
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
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 Simon Charette):

* cc: Simon Charette (added)

Comment:

I understand the desire to streamline the composite primary key reference
experience but this approach just seems plain wrong. Function arity is
tied to the number of argument passed to a function and `Max(F("pk"))` is
a single argument where the argument itself is composite.

In other words, there is a difference between calls of the form `foo(1,
2)` and `foo((1,2))` (or `foo(bar)` where `bar: int | tuple[int, int]`
since `F` is a reference) and trying to automatically turn one into the
other seems like it's going to cause more harm than good.

What about we merge both #36042 and this ticket under a single one that
adds a `BaseExpression.allows_composite_expression: bool = False` that we
set to `True` on `Count` and `TupleLookupMixin` and we make
`BaseExpression.resolve_expression` raise a `ValueError` when it's set to
`False` and any of its source expression is an instance of `ColPairs`?

Don't get me wrong I think we should define `arity` for `Aggregate`
subclasses to guard against improper calls but I don't think that making
`Func.resolve_expression` unpack composite expressions is something we
should do for the aforementioned reasons.
--
Ticket URL: <https://code.djangoproject.com/ticket/36051#comment:3>