UniqueTogetherValidator fails validating partial unique constraint when there is a nullable FK

[sevdog]

In one of my projects I have a model which defines a set of nullable foreign-keys and a value which is unique according to how FKs are defined, like the following:

class UniqueConstraintWithRelationModel(models.Model):
    value = models.IntegerField()
    first_related = models.ForeignKey(FistModel, related_name='+', on_delete=models.CASCADE, null=True, blank=True)
    second_related = models.ForeignKey(SecondModel, related_name='+', on_delete=models.CASCADE, null=True, blank=True)

    class Meta:
        constraints = [
            models.UniqueConstraint(
                fields=('value', 'first_related'),
                condition=models.Q(second_related__isnull=True),
                name='unique_constraint_with_relationa',
            ),
            models.UniqueConstraint(
                fields=('value', 'second_related'),
                condition=models.Q(first_related__isnull=True),
                name='unique_constraint_with_relationb',
            ),
        ]

When validating this model using a ModelSerializer providing an input which triggers an error on one of those constraint I am not getting any error in serializer.is_valid().

Here is a commit which adds a test case which fails with the current implementation: a4a3093

By debugging the problem seems to be how the query is build, because when executing the same query as in qs_exists_with_condition I get the following error (which is masked by the function and thus the validation prodives a false negative!):

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/opt/django-rest-framework/.venv/lib/python3.12/site-packages/django/db/models/query_utils.py", line 135, in check
    query.add_q(Q(Coalesce(self, True, output_field=BooleanField())))
  File "/opt/django-rest-framework/.venv/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1609, in add_q
    clause, _ = self._add_q(q_object, self.used_aliases)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/django-rest-framework/.venv/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1641, in _add_q
    child_clause, needed_inner = self.build_filter(
                                 ^^^^^^^^^^^^^^^^^^
  File "/opt/django-rest-framework/.venv/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1482, in build_filter
    condition = filter_expr.resolve_expression(
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/django-rest-framework/.venv/lib/python3.12/site-packages/django/db/models/expressions.py", line 1073, in resolve_expression
    c.source_expressions[pos] = arg.resolve_expression(
                                ^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/django-rest-framework/.venv/lib/python3.12/site-packages/django/db/models/query_utils.py", line 90, in resolve_expression
    clause, joins = query._add_q(
                    ^^^^^^^^^^^^^
  File "/opt/django-rest-framework/.venv/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1641, in _add_q
    child_clause, needed_inner = self.build_filter(
                                 ^^^^^^^^^^^^^^^^^^
  File "/opt/django-rest-framework/.venv/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1509, in build_filter
    condition = self.build_lookup(lookups, reffed_expression, value)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/django-rest-framework/.venv/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1375, in build_lookup
    lookup_class = lhs.get_lookup(lookup_name)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/django-rest-framework/.venv/lib/python3.12/site-packages/django/db/models/expressions.py", line 390, in get_lookup
    return self.output_field.get_lookup(lookup)
           ^^^^^^^^^^^^^^^^^
  File "/opt/django-rest-framework/.venv/lib/python3.12/site-packages/django/utils/functional.py", line 47, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
                                         ^^^^^^^^^^^^^^^^^^^
  File "/opt/django-rest-framework/.venv/lib/python3.12/site-packages/django/db/models/expressions.py", line 317, in output_field
    raise FieldError("Cannot resolve expression type, unknown output_field")
django.core.exceptions.FieldError: Cannot resolve expression type, unknown output_field

This problems forces me to rewrite the validation for the constraint which results in an extra query which is performed against the database.

[auvipy]

is this PR related #9766 ?

[sevdog]

That PR does not involves partial constraints, it is just for error message.

I see that that PR does not take into account constraint's condition, since there are no references of this term in the newly added code.

[sevdog]

PS: in my project the setup is a bit more complex because I have 4 FKs (A,B,C,D) which should form two families of unique sets (A,B,C) when D is NULL and (A,B,D) when C is NULL. In my branch I have reduced to two FK with a value for simplicity.

[auvipy]

would you mind come with a PR containing the failing test?

[sevdog]

Here it is #9779.

Has the action logs shows the constraint will fail and yet the serializer, which has every fields and every element is provided in input, validates without errors.

[p-r-a-v-i-n]

I might be wrong but here are my thoughts but please let me know if i'm missing something:

• When we keep cascading on null=True and blank=True , i suppose django won't allow you migrate. we have to set on_delete=model.SET_NULL if we want to use null=True and blank=True .

• for curiousity why this instead of unique_together ? i assume bcs you used cascading

constraints = [
            models.UniqueConstraint(
                fields=('value', 'first_related'),
                condition=models.Q(second_related__isnull=True),
                name='unique_constraint_with_relationa',
            ),
            models.UniqueConstraint(
                fields=('value', 'second_related'),
                condition=models.Q(first_related__isnull=True),
                name='unique_constraint_with_relationb',
            ),
        ]

• could you have written like this ?

 
class UniqueConstraintWithRelationModel(models.Model):
    value = models.IntegerField()
    first_related = models.ForeignKey(IntegerFieldModel, related_name='+', on_delete=models.SET_NULL, null=True, blank=True)
    second_related = models.ForeignKey(IntegerFieldModel, related_name='+', on_delete=models.SET_NULL, null=True, blank=True)

    class Meta:
        unique_together = (('value', 'first_related'), ('value', 'second_related'))
        # constraints = [
        #     # models.UniqueConstraint(fields=('value', 'first_related'), condition=models.Q(second_related__isnull=True), name='unique_constraint_with_relationa'),
        #     # models.UniqueConstraint(fields=('value', 'second_related'), condition=models.Q(first_related__isnull=True), name='unique_constraint_with_relationb'),
        # ]

• I have modified test in PR UniqueTogheterValidator fails with partial unique indexes with ForeignKey #9779 as below:

class UniqueConstraintWithRelationModel(models.Model):
    value = models.IntegerField()
    first_related = models.ForeignKey(IntegerFieldModel, related_name='+', on_delete=models.SET_NULL, null=True, blank=True)
    second_related = models.ForeignKey(IntegerFieldModel, related_name='+', on_delete=models.SET_NULL, null=True, blank=True)

    class Meta:
        unique_together = (('value', 'first_related'), ('value', 'second_related'))
        # constraints = [
        #     # models.UniqueConstraint(fields=('value', 'first_related'), condition=models.Q(second_related__isnull=True), name='unique_constraint_with_relationa'),
        #     # models.UniqueConstraint(fields=('value', 'second_related'), condition=models.Q(first_related__isnull=True), name='unique_constraint_with_relationb'),
        # ]


class UniqueConstraintWithRelationModelSerializer(serializers.ModelSerializer):
    class Meta:
        model = UniqueConstraintWithRelationModel
        fields = serializers.ALL_FIELDS


class TestUniqueConstraintWithRelationValidation(TestCase):
    def setUp(self):
        self.related_model = IntegerFieldModel.objects.create(integer=1)
        self.instance_first = UniqueConstraintWithRelationModel.objects.create(
            value=1,
            first_related=self.related_model,
            second_related=None
        )
        self.instance_second = UniqueConstraintWithRelationModel.objects.create(
            value=1,
            first_related=None,
            second_related=self.related_model
        )

    def test_unique_constraint_with_relation_first(self):
        instance = UniqueConstraintWithRelationModel(value=1, first_related=self.related_model, second_related=None)
        # with pytest.raises(DjangoValidationError) as excinfo:
        instance.validate_constraints()

        # assert str(excinfo.value) == "{'__all__': ['Constraint “unique_constraint_with_relationa” is violated.']}"
        serializer = UniqueConstraintWithRelationModelSerializer(data={
            'value': 1,
            'first_related': self.related_model.pk,
            'second_related': None
        })
        assert not serializer.is_valid()
        assert serializer.errors == {
            'non_field_errors': [
                'The fields value, first_related must make a unique set.'
            ]
        }

    def test_unique_constraint_with_relation_second(self):
        instance = UniqueConstraintWithRelationModel(value=1, second_related=self.related_model, first_related=None)
        # with pytest.raises(DjangoValidationError) as excinfo:
        instance.validate_constraints()

        # assert str(excinfo.value) == "{'__all__': ['Constraint “unique_constraint_with_relationb” is violated.']}"
        serializer = UniqueConstraintWithRelationModelSerializer(data={
            'value': 1,
            'first_related': None,
            'second_related': self.related_model.pk
        })
        assert not serializer.is_valid()
        assert serializer.errors == {
            'non_field_errors': [
                'The fields value, second_related must make a unique set.'
            ]
        }

• here is the result

[sevdog]

Hi @p-r-a-v-i-n, let me answer to your questions:

When we keep cascading on null=True and blank=True , i suppose django won't allow you migrate. we have to set on_delete=model.SET_NULL if we want to use null=True and blank=True .

This is not an issue which regards the migration system, migrations work fine and this is not an issue. This is a misleading way to view this issue.

for curiousity why this instead of unique_together ? i assume bcs you used cascading

The unique_together may be deprecated in the future (see django docs) and it is way less flexible than UniqueConstraint like in this case where there is a condition. To be completed in my code there is also an other condition which states that first_related and second_related cannot be both non-NULL, this is why the uniqueness is only tested againsta one when the other is NULL. Also this is mandatory due to how many databases threats NULLs (try the query Select NULL = NULL;, this usually returns NULL).

could you have written like this ?

No, because that would cause the unique-indexes to also account for NULL values which would be useless in my case.

The problem is that the ORM/forms can handle this situation yet DRF is not able to validate it.

[p-r-a-v-i-n]

Hi @sevdog ,

Your investigation was right, no doubt.

When we keep cascading on null=True and blank=True , i suppose django won't allow you migrate. we have to set on_delete=model.SET_NULL if we want to use null=True and blank=True .

I vaguely remember about this , not sure , was it giving me warning or stopping me to migrate. please ignore this.

The unique_together may be deprecated in the future

TIL! Thanks for the this, I didn't know about this.

in my project the setup is a bit more complex because I have 4 FKs (A,B,C,D) which should form two families of unique sets (A,B,C) when D is NULL and (A,B,D) when C is NULL. In my branch I have reduced to two FK with a value for simplicity.

for this situation , when I read , instantly i thought using unique_together would been more simpler bcs I was focusing on code rather the use case .

so please forgive me here, I might have shifted the discussion to off side.