ModelSerializer UniqueTogetherValidator is incompatible with field source

[andrkhr]

Checklist

• I have verified that that issue exists against the master branch of Django REST framework.
• I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
• This is not a usage question. (Those should be directed to the discussion group instead.)
• This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
• I have reduced the issue to the simplest possible case.

Steps to reproduce

In DRF == 3.10.3 and below it was a valid case

# models.py
class MyModel(models.Model):
    user = models.ForeignKey(
        settings.AUTH_USER_MODEL,
        on_delete=models.CASCADE
    )
    date_from = models.DateField()
    date_to = models.DateField()
    
    class Meta:
        unique_together = (
            ('date_from', 'user'),
            ('date_to', 'user'),
        )

# serializers.py
class MyModelSerializer(serializers.ModelSerializer):
    date_start = serializers.DateField(source='date_from')
    date_end = serializers.DateField(source='date_to')
    
    class Meta:
        model = MyModel
        fields = 'date_start', 'date_end', 'user'

data = {
    'user': 1,
    'date_start': '2019-11-13',
    'date_end': '2019-11-14'
}

serializer = MyModelSerializer(data=data)
serializer.is_valid(raise_exception=True)
serializer.save()

In DRF == 3.11.0 I get an error in is_valid() method

~/PycharmProjects/env/staff.3.7.5/lib/python3.7/site-packages/rest_framework/utils/serializer_helpers.py in __getitem__(self, key)
    146 
    147     def __getitem__(self, key):
--> 148         return self.fields[key]
    149 
    150     def __delitem__(self, key):

KeyError: 'date_from'

This case is no longer valid?
It was a convenient way to redirect field names.

[napsterv]

Why aren't your fields in a list?

[xordoquy]

@napsterv it's equivalent to a tuple so it's fine anyway.
@hauworkarea could you paste the entire traceback please ?

[andrkhr]

@xordoquy that's entire traceback

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-3-7ffae1e9ca9d> in <module>
      6 
      7 serializer = UserVacationSer(data=data)
----> 8 serializer.is_valid(raise_exception=True)
      9 serializer.save()

~/PycharmProjects/env/staff.3.7.5/lib/python3.7/site-packages/rest_framework/serializers.py in is_valid(self, raise_exception)
    232         if not hasattr(self, '_validated_data'):
    233             try:
--> 234                 self._validated_data = self.run_validation(self.initial_data)
    235             except ValidationError as exc:
    236                 self._validated_data = {}

~/PycharmProjects/env/staff.3.7.5/lib/python3.7/site-packages/rest_framework/serializers.py in run_validation(self, data)
    433         value = self.to_internal_value(data)
    434         try:
--> 435             self.run_validators(value)
    436             value = self.validate(value)
    437             assert value is not None, '.validate() should return the validated data'

~/PycharmProjects/env/staff.3.7.5/lib/python3.7/site-packages/rest_framework/serializers.py in run_validators(self, value)
    466         else:
    467             to_validate = value
--> 468         super().run_validators(to_validate)
    469 
    470     def to_internal_value(self, data):

~/PycharmProjects/env/staff.3.7.5/lib/python3.7/site-packages/rest_framework/fields.py in run_validators(self, value)
    586             try:
    587                 if getattr(validator, 'requires_context', False):
--> 588                     validator(value, self)
    589                 else:
    590                     validator(value)

~/PycharmProjects/env/staff.3.7.5/lib/python3.7/site-packages/rest_framework/validators.py in __call__(self, attrs, serializer)
    146 
    147     def __call__(self, attrs, serializer):
--> 148         self.enforce_required_fields(attrs, serializer)
    149         queryset = self.queryset
    150         queryset = self.filter_queryset(attrs, queryset, serializer)

~/PycharmProjects/env/staff.3.7.5/lib/python3.7/site-packages/rest_framework/validators.py in enforce_required_fields(self, attrs, serializer)
    106         missing_items = {
    107             field_name: self.missing_message
--> 108             for field_name in self.fields
    109             if serializer.fields[field_name].source not in attrs
    110         }

~/PycharmProjects/env/staff.3.7.5/lib/python3.7/site-packages/rest_framework/validators.py in <dictcomp>(.0)
    107             field_name: self.missing_message
    108             for field_name in self.fields
--> 109             if serializer.fields[field_name].source not in attrs
    110         }
    111         if missing_items:

~/PycharmProjects/env/staff.3.7.5/lib/python3.7/site-packages/rest_framework/utils/serializer_helpers.py in __getitem__(self, key)
    146 
    147     def __getitem__(self, key):
--> 148         return self.fields[key]
    149 
    150     def __delitem__(self, key):

KeyError: 'date_from'

It seems that the problem occurs when there is a unique_together in the meta class of the model.
Yes, the model contains unique_together.

# models.py
class MyModel(models.Model):
    user = models.ForeignKey(
        settings.AUTH_USER_MODEL,
        on_delete=models.CASCADE
    )
    date_from = models.DateField()
    date_to = models.DateField()
    
    class Meta:
        unique_together = (
            ('date_from', 'user'),
            ('date_to', 'user'),
        )

Corrected the original description.

[MattFisher]

I'm having the same issue.
UniqueTogetherValidator.enforce_required_fields assumes the presence of its listed fields in the dict comprehension:

missing_items = {
    field_name: self.missing_message
    for field_name in self.fields
    if serializer.fields[field_name].source not in attrs
}

I can make my failing tests pass by replacing this with:

fields_from_serializer = [
    field
    for field in serializer.fields.values()
    if field.source in self.fields
]
missing_items = {
    field.field_name: self.missing_message
    for field in fields_from_serializer
    if field.source not in attrs
}

# or just 
missing_items = {
    field.field_name: self.missing_message
    for field in serializer.fields.values()
    if field.source in self.fields
    and field.source not in attrs
}

and similarly, in UniqueTogetherValidator.filter_queryset, replacing

sources = [
    serializer.fields[field_name].source
    for field_name in self.fields
]
# with:
sources = [
    field.source
    for field in serializer.fields.values()
    if field.source in self.fields
]

I can't be 100% sure the different way of finding the fields doesn't break any assumptions about when the fields should be present.

[harshal-dhumal]

I'm getting the same issue with DRF 3.11
I have unique_together constraint where one field is ForeignKey.

Serializer

 class SomeSerializer(SomeBaseSerializer):
        xxxx_id = serializers.PrimaryKeyRelatedField(source='xxxx', write_only=True,
                                                        queryset=XXXmodel.objects.all())
        class Meta:
            model = SomeModel
            fields = ('field_1', 'field_2', 'xxxx_id')

Model

class SomeModel(BaseModel):
    xxxx = models.ForeignKey(
        'app.App',
        null=False,
        blank=False,
        on_delete=models.CASCADE
    )
    field_1 = CleanCharField(max_length=64, null=False, blank=False)
    field_2 = models.DecimalField(max_digits=5, decimal_places=1, null=False, blank=False)
    class Meta:
        app_label = 'app'
        db_table = 'app_table'
        unique_together = ('xxxx', 'field_2')

[rpkilby]

Thanks - I'll look into this. This stems from #7086, which fixed a related bug. It looks like those changes in turn broke UniqueTogetherValidators that are generated by ModelSerializers.

For now, a workaround would be to declare the UniqueTogetherValidators directly on your serializer class. Using the example the issue description...

class MyModelSerializer(serializers.ModelSerializer):
    date_start = serializers.DateField(source='date_from')
    date_end = serializers.DateField(source='date_to')
    
    class Meta:
        model = MyModel
        fields = ['date_start', 'date_end', 'user']
        validators = [
            UniqueTogetherValidator(queryset=MyModel.objects.all(), fields=['user', 'date_start']),
            UniqueTogetherValidator(queryset=MyModel.objects.all(), fields=['user', 'date_end']),
        ]

[rpkilby]

Hi all, just opened #7143. If you could test those changes to see if they fix the issue, any feedback would be appreciated.

[lerela]

Hi @rpkilby! Your branch does not seem to fix the issue, at least with manually instantiated UniqueTogetherValidators. This is our test case (working well with DRF 3.10.3):

from django.test import TestCase
from rest_framework import serializers
from rest_framework.validators import UniqueTogetherValidator

from app import models, factories

class MyNestedSerializer(serializers.Serializer):
    object_ref = serializers.CharField(
        source='customer_object_ref', max_length=64, required=False, allow_null=True,
    )


class MySerializer(serializers.ModelSerializer):
    customer = serializers.IntegerField(default=1)
    customer_info = MyNestedSerializer(source='*', required=False)

    class Meta:
        model = models.MyObject
        fields = ('customer', 'customer_info')

        validators = [
            UniqueTogetherValidator(
                queryset=models.MyObject.objects.all(),
                fields=('customer', 'customer_object_ref')
            )
        ]


class DRFIssue(TestCase):
    fixtures = []

    def test_issue(self):
        obj = factories.MyObjectFactory()
        serializer = MySerializer(instance=obj, data={"customer_info": {"object_ref": "test"}})
        self.assertTrue(serializer.is_valid())

This test fails even with your branch uniquetogether-field-source:

======================================================================
ERROR: test_issue (project.src.app.tests.drf.DRFIssue)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/project/src/app/tests/drf.py", line 39, in test_issue
    self.assertTrue(serializer.is_valid())
  File "/git/rpkilby/django-rest-framework/rest_framework/serializers.py", line 219, in is_valid
    self._validated_data = self.run_validation(self.initial_data)
  File "/git/rpkilby/django-rest-framework/rest_framework/serializers.py", line 420, in run_validation
    self.run_validators(value)
  File "/git/rpkilby/django-rest-framework/rest_framework/serializers.py", line 453, in run_validators
    super().run_validators(to_validate)
  File "/git/rpkilby/django-rest-framework/rest_framework/fields.py", line 589, in run_validators
    validator(value, self)
  File "/git/rpkilby/django-rest-framework/rest_framework/validators.py", line 150, in __call__
    queryset = self.filter_queryset(attrs, queryset, serializer)
  File "/git/rpkilby/django-rest-framework/rest_framework/validators.py", line 121, in filter_queryset
    for field_name in self.fields
  File "/git/rpkilby/django-rest-framework/rest_framework/validators.py", line 121, in <listcomp>
    for field_name in self.fields
  File "/git/rpkilby/django-rest-framework/rest_framework/utils/serializer_helpers.py", line 148, in __getitem__
    return self.fields[key]
KeyError: 'customer_object_ref'

This issue is breaking a lot of our code base as we have some of those UniqueTogetherValidator pretty much everywhere.

[cepbuch]

@lerela Seems like you should try use serializer field name (not source model field name) in UniqueTogetherValidator.

i.e. in your case this would probably be like this

validators = [
            UniqueTogetherValidator(
                queryset=models.MyObject.objects.all(),
                fields=('customer', 'object_ref')
            )
        ]

Not sure if it'll work in your case, but in my case where I had

1. Model

class Candidate(models.Model):
    vacancy =  models.ForeignKey(...)
    profile = models.CharField(...)

    class Meta:
        unique_together = ('vacancy', 'profile')

2. Serializer

class CandidateSerializer(serializers.ModelSerializer):
    folder = FolderRelatedSerializer(queryset=Vacancy.objects, source='vacancy', required=True)
    profile = serializers.CharField(...)

=> the following validator in CandidateSerializer meta solved the issue:

class Meta:
        validators = [
            UniqueTogetherValidator(queryset=Candidate.objects.all(), fields=['folder', 'profile']),
        ]

[lerela]

@cepbuch Note that customer_object_ref is a child of the nested serializer customer_info, therefore object_ref does not refer to a field of MySerializer. I can't seem to fix it that way (customer_info.order_ref would be a correct reference but it does not work either).

[rpkilby]

Hi @lerela. As @cepbuch mentioned, the fields on the UniqueTogetherValidator should correspond to the serializer field names, not the names of the source model fields. From the docs:

fields required - A list or tuple of field names which should make a unique set. These must exist as fields on the serializer class.

Validating fields across a nested serializer was not intended to be supported and only worked due to the previous bug (validator incorrectly checked source model fields instead of serializer fields).

That all said, I'm not entirely sure what to recommend. If possible, it would be best if you could move your nested serializer fields to their parent serializers, but I realize this may not be possible. An alternative would be to recreate the old UniqueTogetherValidator from 3.10 and roll back the change to Serializer._read_only_defaults.

[lerela]

Hi @rpkilby. This is a huge breaking change for us (but I understand that our code base makes use of an old loophole!) and it feels like it should at least be part of the 3.11 announcement (but maybe it'll be in the release notes when they are published).

I'll need more time to assess the impact on our project as we cannot break public APIs and move the fields onto their parent serializers. I assume we'll have either to follow your advice with the old UniqueTogetherValidator or move this logic to the serializers validate method.

Thanks @rpkilby and @cepbuch for your insights!

[lerela]

@rpkilby we observed the initial issue in addition to the undefined behavior I reported and I can confirm that your branch fixes it.

[MattFisher]

@rpkilby I can confirm your branch fixes our use case, thanks!

[andrkhr]

@rpkilby +1
your branch fixes our problem, thanks!

[lane-eb]

@rpkilby +1
Cool.

• First step, I keep the unique_together unchanged.

class UserRepo(models.Model):
    user = models.ForeignKey('users.User', on_delete=models.CASCADE)
    repo = models.ForeignKey(GitRepo, on_delete=models.CASCADE)

    class Meta:
        unique_together = (('user', 'repo'),)

• Then I added

class CreateUserRepoSerializer(serializers.ModelSerializer):
    user_id = UserFilteredPrimaryKeyRelatedField(source='user')
    repo_id = RepoFilteredPrimaryKeyRelatedField(source='repo')

    class Meta:
        model = UserRepo
        fields = [
            'user_id',
            'repo_id',
        ]
        validators = [
            UniqueTogetherValidator(queryset=UserRepo.objects.all(), fields=['user_id', 'repo_id']),
        ]

Then the UniqueTogetherValidator will return 400 Bad Request before I got the if serializer.fields[field_name].source not in attrs keyError('user').

Great. Perfect.