encode / django-rest-framework

Web APIs for Django. 🎸
https://www.django-rest-framework.org
Other
28.21k stars 6.81k forks source link

`UniqueTogetherValidator` does not work with `source` kwarg #9442

Closed r-thomson closed 1 month ago

r-thomson commented 3 months ago

My team has run into a bug in UniqueTogetherValidator where it does not work properly with fields with the source attribute. If a field has a source that is different than the field's name on the serializer, then that field is effectively ignored when checking for uniqueness violations, and the ValidationError is not raised.

I am using a serializer set up like this. Note that UniqueTogetherValidator is expecting the serializer's name for the fields argument— you'll get an error on this line if you try to provide the source name instead.

class ExampleSerializer(serializers.Serializer):
    list_id = serializers.PrimaryKeyRelatedField(source='list')
    position = serializers.IntegerField(source='ordering_key')

    class Meta:
        validators = [
            UniqueTogetherValidator(
                queryset=ToDoItem.objects.all(),
                fields=['list_id', 'position']
            )
        ]

I believe the bug occurs at this position in the code:

https://github.com/encode/django-rest-framework/blob/e13688f0c0d32672d31ef3b9474c2a9f9dd12ae9/rest_framework/validators.py#L167-L178

On line 172, we check if field (which comes from source) is in self.fields (which is the name on the serializer). If we're looking at the serializer above, then attrs.keys() would be ['list', 'ordering_key'] and self.fields is ['list_id', 'position'].

This means that checked_values will always be empty, and a ValidationError will never be raised.

Checklist

REST Framework version: 3.15.1

BPC-AnonymousBeast commented 3 months ago

I tried to reproduce the same and I am able to get the expected results. As per my understanding from whatever you have mentioned, you need to replace 'ToDoItem' with your Model class name in 'queryset=ToDoItem.objects.all(),'

My code : -

class PeopleSerialiser(serializers.Serializer):

name1 = serializers.CharField(source='name',required = True)
lastname = serializers.CharField(required = True)
age = serializers.IntegerField(required = False)

class Meta():        
    validators = [
        UniqueTogetherValidator(
            queryset=Person.objects.all(),
            fields=['name1', 'lastname']
        )
    ] 

 Note : its 'name' in my model class and its 'name1' in the serializer class. So, I have changed the field name.

Output :

{ "non_field_errors": [ "The fields name1, lastname must make a unique set." ] }

The only limitation I see here is, it throws this error only when you try to post the names in the same order. For ex: you have "John Doe" in your table and try to enter "John Doe " again, it will give you this error. It will accept another entry as "Doe John" while it should not accept. I'm not sure about my understanding of UniqueTogether. Someone can correct me if I'm wrong.

If this does not resolve your issue, please provide the code of your model class that you are trying to serialize. Also, specify the name of the model class. I'll dig deeper.

r-thomson commented 3 months ago

When I have the time I'll try to open a PR adding a failing test case for this scenario.

auvipy commented 2 months ago

can you both review this PR https://github.com/encode/django-rest-framework/pull/9482, please?