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 to palliate the lack of support for unresolved lookups introspection
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.