Re: [Django] #34871: Validation of UniqueConstraint with Case() crashes.

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: (none)
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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 Francesco Panico):

* owner: nobody => (none)
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: (none)
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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 Francesco Panico):

* owner: nobody => (none)
* status: new => assigned

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

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------

Reporter: Andrew Roberts | Owner: Francesco
| Panico

Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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 Francesco Panico):

* owner: (none) => Francesco Panico

--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:4>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Francesco
| Panico
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by David Sanders):

Hi Francesco, please note that while you're free to assign yourself to
the ticket, the design has not yet been discussed on what the best
approach is.

--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:5>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Francesco
| Panico
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by David Sanders):

fyi for the uninitiated: #34805 is related to this

--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:6>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Francesco
| Panico
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

Replying to [comment:6 David Sanders]:

> fyi for the uninitiated: #34805 is related to this

This situation is slightly different, as we have a `Case()` expression
that returns a field, not a check for condition uniqueness.

--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:7>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Francesco
| Panico
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Nick):

Hey all, I'm running into this issue too. Is there any idea on how to
work-a-round this issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:8>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Francesco
| Panico
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

I would suggest using a `Func` instead of `Case` / `When` or `Q` if you
really need to achieve the exact SQL that `CASE WHEN` would generated

Something like

{{{#!python
models.UniqueConstraint(
Func(F("email")), template="CASE WHEN active = true THEN
%(expressions)s END"),
name="unique_active_email",
)
}}}

Or to simply use `UniqueConstraint.condition` as
[https://docs.djangoproject.com/en/5.0/ref/models/constraints/#condition
documented] if the backend you use support it which will also incur less
storage use.

{{{#!python
models.UniqueConstraint(
fields=["email"],
name="unique_active_email",
condition=Q(active=True),
)
}}}

I think we should spend the time to implement `Q.replace_expressions`
though as it's pretty trivial to do so and would cover for the cases that
cannot be expressed using `.condition` such as `CASE WHEN last_name IS
NULL THEN first_name ELSE first_name || last_name END`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:9>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Francesco
| Panico
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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 Simon Charette):

* cc: Simon Charette (added)

Comment:

Playing a bit with trying to implement `Q.replace_expressions` I have to
retract myself; it's not trivial.

The only solution I can think of is have the validate logic pass a special
type of dict as `replacement` to `replace_expression` that would default
missing key lookups for tuples of the form `('active', True)` to
`query.resolve_lookup_value` so it gets turned into `Exact(F("active"),
True).replace_expression(replacements)`.

It's doable but certainly not trivial
[https://github.com/charettes/django/pull/new/ticket-34871 as this branch
demonstrate].

I think this has some ties into #24267 where the idea would be that we'd
introduce an `UnresolvedLookup(BaseExpression)` class initialized from
`__init__(self, field, **lookup)` and that would store `F(field)` in its
source expressions and thus allow `replace_expressions` to take place
normally as `Q` would be storing `UnresolvedLookup(key_parts[0],
key_parts[1:])` instead of tuple of the form `tuple[str, Any]` as
children. All `UnresolvedLookup` would do in its `resolve_expressions` is
what can be found in the trailing part of `Query.build_filter` today that
calls `solve_lookup_type`, `resolve_lookup_value`, and `build_lookup` but
it would behave like a normal expression and avoids all the hacks we have
in place today that special case `Q` objects.

Until work towards ^^ can be made the best solution here is to use lookups
directly

{{{#!python
from django.db.models.lookups import Exact

models.UniqueConstraint(
Case(When(Exact(F("active", True), then=F("email"))),
name="unique_active_email",
)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:10>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Francesco
| Panico
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by David Sanders):

I was almost going to ask whether turning `Q` object into expressions
might be a path here, because it seems to borrow a lot from `Expression`
already being "combinable" and implementing a few expression-like
attributes. Then I saw your comment about implementing
replace_expressions() being non-trivial.
--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:11>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Francesco
| Panico
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Making `Q` more `Expression`-like is definitely the way to approach this
problem so you're right. The way to make it more like so proposed above
would be to have it store `Expression`-like objects (`UnresolvedLookup`
instances) instead of `tuple[str, Any]` it its children AKA ''source
expressions''.

In other words, having `Q(name__lower__exact="david")` turned into
`Q(UnresolvedLookup(F("name"), "lower__exact", "david"))` internally and
having `UnresolvedLookup.resolve_expressions` do what `Query.build_filter`
does when dealing with `("name__lower__exact", "david")` would make `Q`
more `Expression`-like and allow methods such as `replace_expressions`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:12>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Francesco
| Panico
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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 Václav Řehák):

* cc: Václav Řehák (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:13>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Francesco
| Panico
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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 Mark Gensler):

* cc: Mark Gensler (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:14>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------

Reporter: Andrew Roberts | Owner: Simon
| Charette

Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(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):

* has_patch: 0 => 1
* owner: Francesco Panico => Simon Charette

Comment:

Working on a solution for #36198 paved the way for a less involved than
expected solution here.

Both patches can be found [https://github.com/django/django/pull/19190 in
this MR].
--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:15>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(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
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Marking as needs improvement as I'd like to find a way to avoid
duplicating `sql.Query.build_lookup` logic.
--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:16>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:17>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(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 Sarah Boyce):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:18>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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/34871#comment:19>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Simon
| Charette

Type: Bug | Status: closed

Component: Database layer | Version: 4.2
(models, ORM) |

Severity: Normal | 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:"079d31e698fa08dd92e2bc4f3fe9b4817a214419" 079d31e6]:
{{{#!CommitTicketReference repository=""
revision="079d31e698fa08dd92e2bc4f3fe9b4817a214419"
Fixed #34871, #36518 -- Implemented unresolved lookups expression
replacement.

This allows the proper resolving of lookups when performing constraint
validation involving Q and Case objects.

Thanks Andrew Roberts for the report and Sarah for the tests and review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:20>

[Django]

#34871: Validation of UniqueConstraint with Case() crashes.
-------------------------------------+-------------------------------------
Reporter: Andrew Roberts | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"b3bb7230e1225861b5c1f08931f2d82c2b04133a" b3bb723]:
{{{#!CommitTicketReference repository=""
revision="b3bb7230e1225861b5c1f08931f2d82c2b04133a"
[5.2.x] Fixed #34871, #36518 -- Implemented unresolved lookups expression

replacement.

This allows the proper resolving of lookups when performing constraint
validation involving Q and Case objects.

Thanks Andrew Roberts for the report and Sarah for the tests and review.

Backport of 079d31e698fa08dd92e2bc4f3fe9b4817a214419 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34871#comment:21>