Opened 14 months ago
Last modified 4 months ago
#34871 assigned Bug
Validation of UniqueConstraint with Case() crashes.
Reported by: | Andrew Roberts | Owned by: | Francesco Panico |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.2 |
Severity: | Normal | Keywords: | |
Cc: | Gagaro, Simon Charette, Václav Řehák, Mark Gensler | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Consider the following model where we want to guarantee that there is only one active profile per email address:
class Profile(models.Model): active = models.BooleanField() email = models.CharField(max_length=255) class Meta: constraints = [ models.UniqueConstraint( Case(When(active=True, then=F("email"))), name="unique_active_email", ) ] app_label = "test"
Model validation indirectly calls constraint.validate(), but to simulate this we can execute:
constraint = Profile._meta.constraints[0] constraint.validate(Profile, Profile(active=True, email="test@example.com"))
This fails with:
--------------------------------------------------------------------------- AttributeError Traceback (most recent call last) Cell In[21], line 1 ----> 1 constraint.validate(Profile, Profile(active=True, email="test@example.com")) File /usr/local/lib/python3.11/site-packages/django/db/models/constraints.py:346, in UniqueConstraint.validate(self, model, instance, exclude, using) 344 if isinstance(expr, OrderBy): 345 expr = expr.expression --> 346 expressions.append(Exact(expr, expr.replace_expressions(replacements))) 347 queryset = queryset.filter(*expressions) 348 model_class_pk = instance._get_pk_val(model._meta) File /usr/local/lib/python3.11/site-packages/django/db/models/expressions.py:401, in BaseExpression.replace_expressions(self, replacements) 398 clone = self.copy() 399 source_expressions = clone.get_source_expressions() 400 clone.set_source_expressions( --> 401 [ 402 expr.replace_expressions(replacements) if expr else None 403 for expr in source_expressions 404 ] 405 ) 406 return clone File /usr/local/lib/python3.11/site-packages/django/db/models/expressions.py:402, in <listcomp>(.0) 398 clone = self.copy() 399 source_expressions = clone.get_source_expressions() 400 clone.set_source_expressions( 401 [ --> 402 expr.replace_expressions(replacements) if expr else None 403 for expr in source_expressions 404 ] 405 ) 406 return clone File /usr/local/lib/python3.11/site-packages/django/db/models/expressions.py:401, in BaseExpression.replace_expressions(self, replacements) 398 clone = self.copy() 399 source_expressions = clone.get_source_expressions() 400 clone.set_source_expressions( --> 401 [ 402 expr.replace_expressions(replacements) if expr else None 403 for expr in source_expressions 404 ] 405 ) 406 return clone File /usr/local/lib/python3.11/site-packages/django/db/models/expressions.py:402, in <listcomp>(.0) 398 clone = self.copy() 399 source_expressions = clone.get_source_expressions() 400 clone.set_source_expressions( 401 [ --> 402 expr.replace_expressions(replacements) if expr else None 403 for expr in source_expressions 404 ] 405 ) 406 return clone AttributeError: 'Q' object has no attribute 'replace_expressions'
Change History (14)
comment:1 by , 14 months ago
Cc: | added |
---|---|
Summary: | Validation fails for models with UniqueConstraints that include Case statements → Validation of UniqueConstraint with Case() crashes. |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 14 months ago
Owner: | removed |
---|---|
Status: | new → assigned |
comment:3 by , 14 months ago
Owner: | removed |
---|---|
Status: | new → assigned |
comment:4 by , 14 months ago
Owner: | set to |
---|
comment:5 by , 14 months ago
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.
comment:7 by , 14 months ago
Replying to 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.
comment:8 by , 9 months ago
Hey all, I'm running into this issue too. Is there any idea on how to work-a-round this issue?
comment:9 by , 9 months ago
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
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 documented if the backend you use support it which will also incur less storage use.
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
.
comment:10 by , 9 months ago
Cc: | added |
---|
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 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
from django.db.models.lookups import Exact models.UniqueConstraint( Case(When(Exact(F("active", True), then=F("email"))), name="unique_active_email", )
comment:11 by , 9 months ago
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.
comment:12 by , 9 months ago
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
.
comment:13 by , 6 months ago
Cc: | added |
---|
comment:14 by , 4 months ago
Cc: | added |
---|
Thanks for the report.