encode / django-rest-framework

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

ListSerializer doesn't handle unique_together constraints #3970

Closed hanssto closed 8 years ago

hanssto commented 8 years ago

Checklist

As referenced in https://github.com/tomchristie/django-rest-framework/pull/2575 https://github.com/miki725/django-rest-framework-bulk/issues/30

ListSerializer attempts to apply unique together validation on the queryset passed to it, but some part of the code is passed the full queryset, where it attempts to look up attributes on what it expects to be individual instances.

Steps to reproduce

I've modified a test case found in the above links, and run them against master. I'm not sure what the file should be named, and if it follows your quality standards, so I have not submitted a PR with it.

from django.db import models
from django.test import TestCase

from rest_framework import serializers

class ManyUniqueTogetherModel(models.Model):
    foo = models.IntegerField()
    bar = models.IntegerField()
    class Meta:
        unique_together = ("foo", "bar")

class ManyUniqueTogetherSerializer(serializers.ModelSerializer):
    class Meta:
        model = ManyUniqueTogetherModel

class ManyUniqueTogetherTestCase(TestCase):

    def test_partial_false(self):
        obj = ManyUniqueTogetherModel.objects.create(foo=1, bar=2)
        serializer = ManyUniqueTogetherSerializer(
            instance=ManyUniqueTogetherModel.objects.all(),
            data=[{
                "id": obj.pk,
                "foo": 5,
                "bar": 6,
            }],
            many=True
        )
        self.assertTrue(serializer.is_valid())

    def test_partial_true(self):
        obj = ManyUniqueTogetherModel.objects.create(foo=1, bar=2)
        serializer = ManyUniqueTogetherSerializer(
            instance=ManyUniqueTogetherModel.objects.all(),
            data=[{
                "id": obj.pk,
                "foo": 5,
            }],
            partial=True,
            many=True
        )
        self.assertTrue(serializer.is_valid())

Expected behavior

The code path looks up properties on individual instances in the queryset, is_valid() returns either True or False depending on the actual data, the tests pass.

Actual behavior

====================================================================== FAILURES ======================================================================
___________________________________________________ ManyUniqueTogetherTestCase.test_partial_false ____________________________________________________
tests/test_unique_together.py:35: in test_partial_false
    self.assertTrue(serializer.is_valid())
rest_framework/serializers.py:213: in is_valid
    self._validated_data = self.run_validation(self.initial_data)
rest_framework/serializers.py:557: in run_validation
    value = self.to_internal_value(data)
rest_framework/serializers.py:593: in to_internal_value
    validated = self.child.run_validation(item)
rest_framework/serializers.py:409: in run_validation
    self.run_validators(value)
rest_framework/fields.py:499: in run_validators
    validator(value)
rest_framework/validators.py:142: in __call__
    queryset = self.exclude_current_instance(attrs, queryset)
rest_framework/validators.py:135: in exclude_current_instance
    return queryset.exclude(pk=self.instance.pk)
E   AttributeError: 'QuerySet' object has no attribute 'pk'
____________________________________________________ ManyUniqueTogetherTestCase.test_partial_true ____________________________________________________
tests/test_unique_together.py:50: in test_partial_true
    self.assertTrue(serializer.is_valid())
rest_framework/serializers.py:213: in is_valid
    self._validated_data = self.run_validation(self.initial_data)
rest_framework/serializers.py:557: in run_validation
    value = self.to_internal_value(data)
rest_framework/serializers.py:593: in to_internal_value
    validated = self.child.run_validation(item)
rest_framework/serializers.py:409: in run_validation
    self.run_validators(value)
rest_framework/fields.py:499: in run_validators
    validator(value)
rest_framework/validators.py:141: in __call__
    queryset = self.filter_queryset(attrs, queryset)
rest_framework/validators.py:120: in filter_queryset
    attrs[field_name] = getattr(self.instance, field_name)
E   AttributeError: 'QuerySet' object has no attribute 'bar'
xordoquy commented 8 years ago

Note that I've been performing something similar but handled it outside of generic DRF. Reason is, even if UniqueTogetherConstraint would work with QuerySet it would still fail in some cases such as:

data=[{
                "id": id1,
                "foo": 5,
                "bar": 6,
            }, {
                "id": id2,
                "foo": 5,
                "bar": 6,
            },],

I suspect there are some other specific use cases that would prove hard to solve with validators in their current forms.

LouWii commented 8 years ago

Could it be related to the issue I'm having ?

I got

author = models.ForeignKey(User)
created_at = models.DateTimeField(auto_now_add=True)

class Meta:
    get_latest_by = "created_at"
    ordering = ['-created_at']
    unique_together = [
        ["author", "created_at"]
    ]

in my Model. User is a cms.models.User object.

For some reason, Swagger is throwing an error when trying to list available API endpoints 'CreateOnlyDefault' object has no attribute 'is_update'

Traceback:
File "/Users/louis/Documents/WebProjects/Python/SiteTestlib/python2.7/site-packages/django/core/handlers/base.py" in get_response
  132.                     response = wrapped_callback(request, *callback_args, **callback_kwargs)
File "/Users/louis/Documents/WebProjects/Python/SiteTestlib/python2.7/site-packages/django/views/decorators/csrf.py" in wrapped_view
  58.         return view_func(*args, **kwargs)
File "/Users/louis/Documents/WebProjects/Python/SiteTestlib/python2.7/site-packages/django/views/generic/base.py" in view
  71.             return self.dispatch(request, *args, **kwargs)
File "/Users/louis/Documents/WebProjects/Python/SiteTestlib/python2.7/site-packages/rest_framework/views.py" in dispatch
  466.             response = self.handle_exception(exc)
File "/Users/louis/Documents/WebProjects/Python/SiteTestlib/python2.7/site-packages/rest_framework/views.py" in dispatch
  463.             response = handler(request, *args, **kwargs)
File "/Users/louis/Documents/WebProjects/Python/SiteTestlib/python2.7/site-packages/rest_framework_swagger/views.py" in get
  164.             'models': generator.get_models(apis),
File "/Users/louis/Documents/WebProjects/Python/SiteTestlib/python2.7/site-packages/rest_framework_swagger/docgenerator.py" in get_models
  139.             data = self._get_serializer_fields(serializer)
File "/Users/louis/Documents/WebProjects/Python/SiteTestlib/python2.7/site-packages/rest_framework_swagger/docgenerator.py" in _get_serializer_fields
  328.                 'defaultValue': get_default_value(field),
File "/Users/louis/Documents/WebProjects/Python/SiteTestlib/python2.7/site-packages/rest_framework_swagger/introspectors.py" in get_default_value
  54.         default_value = default_value()
File "/Users/louis/Documents/WebProjects/Python/SiteTestlib/python2.7/site-packages/rest_framework/fields.py" in __call__
  225.         if self.is_update:

I know Django REST Swagger is not part of that project, but the call is made from Django REST Framework if I understand the stack trace correctly.

tomchristie commented 8 years ago

Related: #2996.

jkszymans commented 11 months ago

Hey guys, I found maybe not the best looking solution, but it works.

remove UniqueTogetherValidator validator from serializer by adding:

    def run_validators(self, value):
        for validator in self.validators:
            if isinstance(validator, validators.UniqueTogetherValidator):
                self.validators.remove(validator)
        super(Yourclass, self).run_validators(value)

If user will try to create object that breaks unique_together rule, django will throw model level exception. So you can just catch it by overriding create method and then raise validation error

    def create(self, validated_data):
        try:
            with transaction.atomic():
                instance = super().create(validated_data)
        except IntegrityError as e:
            raise serializers.ValidationError(e)
        return instance