[Django] #36048: CompositePrimaryKey lookup guards raise NotSupportedError instead of a more appropriate exception

9 views
Skip to first unread message

Django

unread,
Dec 31, 2024, 5:45:30 PM12/31/24
to django-...@googlegroups.com
#36048: CompositePrimaryKey lookup guards raise NotSupportedError instead of a more
appropriate exception
-------------------------------------+-------------------------------------
Reporter: Jacob | Owner: Jacob Walls
Walls |
Type: Bug | Status: assigned
Component: Database | Version: dev
layer (models, ORM) |
Severity: Release | Keywords:
blocker |
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Django mostly(*) raises `NotSupportedError` as documented in
[https://peps.python.org/pep-0249/#notsupportederror PEP 249], to signify
that a database feature is missing. Instead, some of the
`CompositePrimaryKey` sanity checks raise regardless of db feature flags.
Given that, I'm suggesting that `ValueError` etc. would be more
appropriate in these two locations:

-
[https://github.com/django/django/blob/8d9901c961bf9d5cfa6bddddbbcebfbf487a5125/django/db/models/fields/related_lookups.py#L120
RelatedLookupMixin.as_sql]
-
[https://github.com/django/django/blob/8d9901c961bf9d5cfa6bddddbbcebfbf487a5125/django/db/models/aggregates.py#L185
Count.resolve_expression]

Our
[https://github.com/django/django/blob/8d9901c961bf9d5cfa6bddddbbcebfbf487a5125/django/db/backends/base/operations.py#L663
guidance] says "specific backends":
> This is used on specific backends to rule out known expressions that
have problematic or nonexistent implementations. If the expression has a
known problem, the backend should raise NotSupportedError.

Essentially the inverse of #28665. Marking as a release blocker for now to
ensure we consider this before finalizing the feature.

----
*: The handful of places we're raising `NotSupportedError` regardless of
db feature flags include:
- Query.check_filterable
- QuerySet._not_support_combined_queries
- QuerySet.get
- Field.slice_expression
- ResolvedOuterRef.resolve_expression

I'm not suggesting to do anything about those at the moment.
--
Ticket URL: <https://code.djangoproject.com/ticket/36048>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,
Jan 2, 2025, 4:13:36 AM1/2/25
to django-...@googlegroups.com
#36048: CompositePrimaryKey lookup guards raise NotSupportedError instead of a more
appropriate exception
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* cc: Csirmaz Bendegúz, Simon Charette (added)
* stage: Unreviewed => Accepted

Comment:

I think I agree
--
Ticket URL: <https://code.djangoproject.com/ticket/36048#comment:1>

Django

unread,
Jan 2, 2025, 8:43:40 PM1/2/25
to django-...@googlegroups.com
#36048: CompositePrimaryKey lookup guards raise NotSupportedError instead of a more
appropriate exception
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | 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 Jacob Walls):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/18993 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/36048#comment:2>

Django

unread,
Jan 3, 2025, 8:20:18 AM1/3/25
to django-...@googlegroups.com
#36048: CompositePrimaryKey lookup guards raise NotSupportedError instead of a more
appropriate exception
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36048#comment:3>

Django

unread,
Jan 6, 2025, 3:33:39 AM1/6/25
to django-...@googlegroups.com
#36048: CompositePrimaryKey lookup guards raise NotSupportedError instead of a more
appropriate exception
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"46b3e7dd8cc1792a40bb8b4d0c267f3d1ceef68c" 46b3e7dd]:
{{{#!CommitTicketReference repository=""
revision="46b3e7dd8cc1792a40bb8b4d0c267f3d1ceef68c"
Fixed #36048 -- Preferred ValueError to NotSupportedError for composite pk
sanity checks.

These checks are not backend-dependent.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36048#comment:4>
Reply all
Reply to author
Forward
0 new messages