Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#35594 closed Bug (fixed)

Add support for non-distinct NULL expressions to UniqueConstraint.validate()

Reported by: Mark Gensler Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 5.0
Severity: Release blocker Keywords: unique constraint nulls_distinct validation expressions
Cc: Simon Charette Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Mark Gensler)

Expressions which evaluate to NULL within UniqueConstraint(*expressions, nulls_distinct=False) are still treated as distinct by UniqueConstraint.validate(). This means a ValidationError is not raised when it should be.

Similarly, if the database connection uses interprets_empty_strings_as_nulls this is also ignored by .validate().

Should #35575 be merged, this problem would also extend to any GeneratedField included in UniqueConstraint(fields=[...], nulls_distinct=False).

E.g.

class Book(models.Model):
    name = CharField(max_length=255, null=True, blank=True)

    class Meta:
        constraints = [
            UniqueConstraint(F("name"), nulls_distinct=False, name="book_name_null_unique")
        ]

then

> Book.objects.create(name=None)
> book = Book(name=None)
> book.full_clean() # Should raise a `ValidationError` but doesn't.
> book.save()  # The database raises a `UniqueViolation`.

This ticket was raised following discussion in https://github.com/django/django/pull/18356#pullrequestreview-2166340541

Change History (16)

comment:1 by Mark Gensler, 5 months ago

Description: modified (diff)

comment:2 by Simon Charette, 5 months ago

Keywords: unique constraint validation expressions added; uniqueconstraint removed
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Thanks for creating the report!

This was missed in #34701. Elevating to release blocker as this is a bug in a newly released feature.

I guess solution would be that when nulls_distinct=False all expressions checks should be turned into OR (rhs IS NULL AND lhs IS NULL)

SELECT 1 FROM book WHERE lower(name) = lower(NULL)

Is turned into

SELECT 1 FROM book WHERE (lower(name) = lower(NULL)) OR (lower(name) IS NULL AND lower(NULL) IS NULL)
Version 1, edited 5 months ago by Simon Charette (previous) (next) (diff)

comment:3 by Simon Charette, 5 months ago

Cc: Simon Charette added

comment:4 by DongwookKim0823, 5 months ago

Owner: set to DongwookKim0823
Status: newassigned

comment:5 by Simon Charette, 5 months ago

Dong it would have been appreciate if you could have chimed in with Mark and myself before jumping on assignment.

Worked on a PoC here.

comment:6 by DongwookKim0823, 5 months ago

Owner: DongwookKim0823 removed
Status: assignednew

comment:7 by Simon Charette, 5 months ago

Has patch: set
Patch needs improvement: set

in reply to:  5 comment:8 by DongwookKim0823, 5 months ago

Replying to Simon Charette:

Sorry for jumping in without prior discussion. I wasn't aware and will be more cautious in the future.

comment:9 by Simon Charette, 5 months ago

No worries and no harm done, I just wanted to make sure you wouldn't waste time on this problem since both Mark and I had a pretty good idea of where to take the patch.

I guess a possible rule of thumb would be to at least wait a few days after the creation of a ticket if there seems to be active discussions and no call for someone to take the implementation work. In doubt, asking if help is required is also always welcome.

comment:10 by Simon Charette, 5 months ago

Owner: set to Simon Charette
Status: newassigned

in reply to:  9 comment:11 by DongwookKim0823, 5 months ago

Thanks for the guidance. I appreciate the advice and will follow it moving forward.

comment:12 by Simon Charette, 5 months ago

Patch needs improvement: unset
Type: New featureBug

comment:13 by Sarah Boyce, 5 months ago

Triage Stage: AcceptedReady for checkin

comment:14 by Sarah Boyce <42296566+sarahboyce@…>, 5 months ago

Resolution: fixed
Status: assignedclosed

In adc0b6aa:

Fixed #35594 -- Added unique nulls distinct validation for expressions.

Thanks Mark Gensler for the report.

comment:15 by Sarah Boyce <42296566+sarahboyce@…>, 5 months ago

In 4d8e5743:

[5.1.x] Fixed #35594 -- Added unique nulls distinct validation for expressions.

Thanks Mark Gensler for the report.

Backport of adc0b6aac3f8a5c96e1ca282bc9f46e28d20281c from main.

comment:16 by Sarah Boyce <42296566+sarahboyce@…>, 5 months ago

In fe9bf0c:

[5.0.x] Fixed #35594 -- Added unique nulls distinct validation for expressions.

Thanks Mark Gensler for the report.

Backport of adc0b6aac3f8a5c96e1ca282bc9f46e28d20281c from main.

Note: See TracTickets for help on using tickets.
Back to Top