Crash when using ListSerializer for an update on a model that has Unique together constraints

[PhilipGarnero]

Checklist

• [x] I have verified that that issue exists against the master branch of Django REST framework.
• [x] I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
• [x] This is not a usage question. (Those should be directed to the discussion group instead.)
• [x] 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.)
• [x] I have reduced the issue to the simplest possible case.
• [ ] I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

When using a custom ListSerializer to update data and providing the instance as a queryset, if the model has some unique together constraint, the is_valid call crashes

Steps to reproduce

Create a model with a unique together constraint Create a serializer for this model (Probably not needed) Create a custom list serializer with the update method overwritten and provide it to your model serializer with list_serializer_class Now use the model serializer to serialize a list of data and provide both many=True and instance=YourModel.objects.all() to your serializer __init__ Call serializer.is_valid(raise_exception=True)

Expected behavior

It should pass is_valid or at least not crash

Actual behavior

It crashes in the unique together validator because instance was not popped in the many_init so the child will receive a queryset instead of a model instance

How to fix

Popping instance from the kwargs in many_init should do the trick but I'm not sure this won't introduce unforeseen problems Moreover, the validators will probably fail because they will consider the call as a create rather than an update so here is that

[lovelydinosaur]

provide both many=True and instance=YourModel.objects.all()

I don't think that's supported usage. instance should be an instance, not a queryset.

[xordoquy]

Documentation suggests it should be: http://www.django-rest-framework.org/api-guide/serializers/#customizing-multiple-update

[lovelydinosaur]

Note that the best way to address this would be to declare the fields explicitly, so that you don't end up with an auto-generated uniqueness validator (Ideally just use a fully explicit Serializer class rather than leaning on ModelSerializer)

[PhilipGarnero]

The quick and dirty fix on my side was to put validators = () on the serializer for things to be working correctly.
I would find it a bit weird to have to define a Serializer instead of a ModelSerialier to serialize models Moreover, it would be nearly the same as my actual model serializer

I think the usage right now is fine. We just need to prevent the crash from occuring or to warn people about it in the docs. As this is a pretty advanced usage, not many of us are going to encounter this issue anyways.

[xordoquy]

thinking about this, the unique validator will have some nasty side effects with many anyway. Like sending a value twice in the set. If it's not already in the db that would probably pass the validation and yet fail the DB insert. One thing I had to overcome a couple of years ago now that I think of it.

[lovelydinosaur]

@PhilipGarnero Yup I think you've got exactly the right assessment there.

We just need to prevent the crash from occuring or to warn people about it in the docs.

I'd suggest we raise a RuntimeError in this case with an explicit message, and note in the docs that unique_together validation cannot automatically work on bulk updates, and that users should override validators on any ModelSerializer, and handle the validation explicitly.

[xordoquy]

agreed

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.