Skip to content

3.15 regression: Serializer validation failed for unique together constraint #9358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
anndoc opened this issue Apr 2, 2024 · 8 comments
Closed

Comments

@anndoc
Copy link

anndoc commented Apr 2, 2024

The new 3.15.0 release introduced a bug with validation unique constraint.

Code to reproduce an error:

from django.db import models
from rest_framework import serializers

class Pet(models.Model):
    name = models.CharField(max_length=100)
    animal_type = models.CharField(max_length=100)
    can_fly = models.BooleanField(null=True)

    class Meta:
        constraints = (
            UniqueConstraint(
                fields=["name", "animal_type"],
                name="unique_pet",
                condition=Q(can_fly__isnull=True),
            ),
        )


class PetSerializer(serializers.ModelSerializer):
    class Meta:
        model = Pet
        fields = ('name', 'animal_type', 'can_fly')


Pet.objects.create(animal_type='dog', name='Fluffy', can_fly=None)
serializer = PetSerializer(data={
    'can_fly': False,
    'animal_type': 'dog',
    'name': 'Fluffy'
})
serializer.is_valid(raise_exception=True)

The last line raises the error:

Error
Traceback (most recent call last):
    rest_framework.exceptions.ValidationError: {
        'non_field_errors': [ErrorDetail(string='The fields name, animal_type must make a unique set.', code='unique')]
    }

Validation ignores that can_fly field is present in the serializer.initial_data and just runs UniqueTogetherValidator for animal_type and name fields.

@AGarrow
Copy link

AGarrow commented Apr 22, 2024

bump on this 😄 , causing some issues for us in prod after upgrading 😅

@terjekv
Copy link

terjekv commented May 27, 2024

This looks similar to what we're seeing here: unioslo/mreg#537, except that we're seeing a RelatedObjectDoesNotExist exception:

ERROR    django.request:log.py:241 Internal Server Error: /api/v1/hosts/
Traceback (most recent call last):
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/views/decorators/csrf.py", line 65, in _view_wrapper
    return view_func(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/views.py", line 509, in dispatch
    response = self.handle_exception(exc)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/views.py", line 469, in handle_exception
    self.raise_uncaught_exception(exc)
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
    raise exc
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/views.py", line 506, in dispatch
    response = handler(request, *args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/mreg/api/v1/views.py", line 354, in post
    if ipserializer.is_valid():
       ^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/serializers.py", line 223, in is_valid
    self._validated_data = self.run_validation(self.initial_data)
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/serializers.py", line 444, in run_validation
    self.run_validators(value)
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/serializers.py", line 477, in run_validators
    super().run_validators(to_validate)
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/fields.py", line 553, in run_validators
    validator(value, self)
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/validators.py", line 169, in __call__
    checked_values = [
                     ^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/validators.py", line 172, in <listcomp>
    if field in self.fields and value != getattr(serializer.instance, field)
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/db/models/fields/related_descriptors.py", line 264, in __get__
    raise self.RelatedObjectDoesNotExist(
mreg.models.host.Ipaddress.host.RelatedObjectDoesNotExist: Ipaddress has no host.

The issue comes from this bit of code: https://github.com/unioslo/mreg/blob/fa6ca20a41bd486cc8053a116f412b1d526a72ef/mreg/api/v1/views.py#L345-L351 combined with the unique_together constraint in Ipaddress: https://github.com/unioslo/mreg/blob/fa6ca20a41bd486cc8053a116f412b1d526a72ef/mreg/models/host.py#L25-L36.

This (very old) code works fine if we use 3.14, but running 3.15.* gives the above.

terjekv added a commit to unioslo/mreg that referenced this issue May 29, 2024
Django 5 support.

* Add python 3.12 to github actions.
* Drop python 3.7 support due to djangorestframework and django-auth-ldap having dropped support.
* Drop Django 3 support due to django-filter 24.* needing 4.2+.
* Add tzdada dependency (Needed for docker unit tests)
* Clean up implementation of manually finding conflicts from requests via get_object_from_request.
* "Fix" and test filtering for HostFilterSet. :(

Note: This relies on django-rest-framework 3.14.0 and not 3.15.1 (which has formal Django 5 support)... This is due to changes in 3.15.* with regards to unique_together in models. For us, that hits Ipaddress when creating a host, leading to, where we an error as follows (this is reported as part of encode/django-rest-framework#9358).

```python
ERROR    django.request:log.py:241 Internal Server Error: /api/v1/hosts/
Traceback (most recent call last):
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/views/decorators/csrf.py", line 65, in _view_wrapper
    return view_func(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/views.py", line 509, in dispatch
    response = self.handle_exception(exc)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/views.py", line 469, in handle_exception
    self.raise_uncaught_exception(exc)
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
    raise exc
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/views.py", line 506, in dispatch
    response = handler(request, *args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/mreg/api/v1/views.py", line 354, in post
    if ipserializer.is_valid():
       ^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/serializers.py", line 223, in is_valid
    self._validated_data = self.run_validation(self.initial_data)
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/serializers.py", line 444, in run_validation
    self.run_validators(value)
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/serializers.py", line 477, in run_validators
    super().run_validators(to_validate)
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/fields.py", line 553, in run_validators
    validator(value, self)
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/validators.py", line 169, in __call__
    checked_values = [
                     ^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/validators.py", line 172, in <listcomp>
    if field in self.fields and value != getattr(serializer.instance, field)
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/db/models/fields/related_descriptors.py", line 264, in __get__
    raise self.RelatedObjectDoesNotExist(
mreg.models.host.Ipaddress.host.RelatedObjectDoesNotExist: Ipaddress has no host.
```
@dfn-certling
Copy link

We see the same issue as @terjekv . This is due to #9154 . Before you could create a bare model of the serializer's instance class in memory and provide it to the serializer, resulting in a model stored to the DB. Now the UniqueTogetherValidator tries to check for changes in the unique values in the provided instance that has never been stored to the DB and does not have the corresponding fields initialized.

terjekv added a commit to terjekv/mreg that referenced this issue Aug 7, 2024
  - We are stuck on drf 3.14.0 due to encode/django-rest-framework#9358 until encode/django-rest-framework#9483 goes into prod, hopefully 3.15.3.
oyvindhagberg pushed a commit to unioslo/mreg that referenced this issue Aug 22, 2024
* Add query string to logging output.
* Type checking fix (type->isinstance)
* Fix filters for Mreg (HostPolicy outstanding).
* Safer version of JSON filter to avoid SQL injections.
* Refactor, cleanup.
* Hostpolicy filter "fixes", also bump dependencies.

  - We are stuck on drf 3.14.0 due to encode/django-rest-framework#9358 until encode/django-rest-framework#9483 goes into prod, hopefully 3.15.3.

* Skeleton for testing filters.
* Add iexact support, add cases. Fix toml.
* Move to ruff formater.
* More tests.

  - Add ip support.
  - Add reverse lookups.

* Hostpolicy filter tests.
  - Also reformat as per ruff.
* Add a test for filtering on ?id= for hosts.
  - This should catch the generic issue of filtering on IDs. Ideally we'd do this for every model that supports ID...
* support `__in`.
* Support CIDR matching.
   - Match exact CIDR or IP within a CIDR.
@eddielu
Copy link

eddielu commented Oct 2, 2024

Is this a recent change? When did DRF start adding UniqueTogetherValidator for ModelSerializer?

Edit: I know it's broken now, but I'm wondering if this feature was always there or just added recently

@rogelho-junior
Copy link

Is this a recent change? When did DRF start adding UniqueTogetherValidator for ModelSerializer?

Edit: I know it's broken now, but I'm wondering if this feature was always there or just added recently

Has been added on DRF 3.15.0 (15th March 2024)

@beruic
Copy link
Contributor

beruic commented Feb 12, 2025

I am pretty sure this is related.

I have a model with a UniqueConstraint that is conditional on one of the fields being non-null.
The difference here is that the condition is on one of the fields that is part of the constraint fields.

Here is a simplified model to illustrate my issue:

class TestModel(models.Model):
    fielda = models.CharField(
        null=True,
        blank=True,
        max_length=80,
    )
    fieldb = models.ForeignKey("OtherTestModel", on_delete=models.PROTECT)

    class Meta:
        constraints = [
            models.UniqueConstraint(
                name="%(app_label)s_%(class)s_fielda_unique_on_fieldb",
                fields=("fielda", "fieldb"),
                condition=models.Q(fielda__isnull=False),
            ),
        ]

Because of the unique constraint fielda becomes required in a model serializer:

class AbstractIssueSerializer(ModelSerializer):
    class Meta:
        fields: str = "__all__"

I have tried adding extra_kwargs = {"reference": {"required": False}} to the serializer Meta and overriding fielda with fielda = CharField(allow_null=True, required=False). Both makes the field look like it is not required in my schema generation, but the requirement is still enforced by the serializer.

The only way I have found to remove the constraint is to add the following to the serializer:

    def get_unique_together_constraints(self, model):
        for fields, queryset in super().get_unique_together_constraints(model):
            if set(fields) != {"fielda", "fieldb"}:
                yield fields, queryset

Can anyone confirm that these are related, or should I post this as another issue?

@anndoc
Copy link
Author

anndoc commented Feb 13, 2025

@beruic you can try this workaround instead of the get_unique_together_constraints

constraints = [
    models.UniqueConstraint(
        "fielda",
        "fieldb",
        name="%(app_label)s_%(class)s_fielda_unique_on_fieldb",
        condition=models.Q(fielda__isnull=False),
    ),
]

@auvipy
Copy link
Member

auvipy commented Feb 17, 2025

we got a new pr merged to fix this issue. can you guys please try it and report back?

dewi-tik pushed a commit to goauthentik/authentik that referenced this issue Apr 28, 2025
upgrade `django-rest-framework` to `3.16.0`

The reverted commit is purely an optimization which unfortunately breaks authentik, specifically Blueprints. It adds `getattr(serializer.instance, field)` to a validator. If `field` is a `RelatedObject`, that invocation queries the database.

When authentik creates objects using Blueprints, it doesn't place related objects into the database before the validator tries to get them from there, so with the reverted commit, it produces `RelatedObjectDoesNotExist`.

Perhaps a long-term solution is to revise how Blueprints work, or perhaps it is to change upstream. But in the meantime, Django 5.0 support ended and upgrading to Django 5.1 requires an upgrade of `django-rest-framework` to `3.16.0`, hence this workaround.

See
- encode/django-rest-framework#9154
- encode/django-rest-framework#9358
- encode/django-rest-framework#9482
- encode/django-rest-framework#9483
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants