Opened 15 months ago

Last modified 5 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 Mariusz Felisiak, 15 months ago

Cc: Gagaro added
Summary: Validation fails for models with UniqueConstraints that include Case statementsValidation of UniqueConstraint with Case() crashes.
Triage Stage: UnreviewedAccepted

Thanks for the report.

comment:2 by Francesco Panico, 15 months ago

Owner: nobody removed
Status: newassigned

comment:3 by Francesco Panico, 15 months ago

Owner: nobody removed
Status: newassigned

comment:4 by Francesco Panico, 15 months ago

Owner: set to Francesco Panico

comment:5 by David Sanders, 15 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:6 by David Sanders, 15 months ago

fyi for the uninitiated: #34805 is related to this

in reply to:  6 comment:7 by Mariusz Felisiak, 15 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 Nick, 10 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 Simon Charette, 10 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 Simon Charette, 10 months ago

Cc: Simon Charette 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",
    )
)
Last edited 10 months ago by Simon Charette (previous) (diff)

comment:11 by David Sanders, 10 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 Simon Charette, 10 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 Václav Řehák, 7 months ago

Cc: Václav Řehák added

comment:14 by Mark Gensler, 5 months ago

Cc: Mark Gensler added
Note: See TracTickets for help on using tickets.
Back to Top