Repository: encode/django-rest-framework
Pull request: Fix validation for ListSerializer
## Description

This PR fixes #8926. It makes the instance variable of child serializer point to the correct list object instead of the entire list when validating ListSerializer. 

### Problem

As mentioned in #8926, when calling `is_valid` on a serializer that has been initialized with `many=True`, the `instance` variable within any `validate_` method points to the list of multiple model instances instead of the relevant single model instance. 

### Solution

When a serializer is initialized with `many=True`, the returned serializer is actually an instance of `ListSerializer` with the intended serializer set as the `child` of the `ListSerializer`. This PR dynamically sets the correct `instance` on the child serializer when iterating over all provided objects in the `data` list.

If both `data` and `instance` are passed to a serializer with `many=True`, the serializer raises an assertion error if their lengths are different.

### Assumptions

It is assumed that the objects within `data` and `instance` correspond to each other based on the index. 

### Further Work

We could also add the ability to pass a `key` or `field` value that matches objects within `data` and `instance`. Moreover, the API would be clearer if `instance` was actually renamed to `instances` but that might cause backwards compatibility issues.



Problem statement:
Invalid `self.instance` when validating the serializer using `many=True`
Currently it is almost impossible to use the same serializer with and without `many=True` and the problem that sometimes serializer depending on the instance state or value during serialization and validation (usually it is PATCH method and `partial=True`

This is the simplest example to understand:
```python
from django.db import models
from rest_framework import serializers


class MyClass(models.Model):
    name = models.CharField(max_length=100)
    status = models.CharField(max_length=100, blank=True)

    @property
    def is_valid(self):
        return self.name == 'valid'


class MyClassSerializer(serializers.ModelSerializer):
    class Meta:
        model = MyClass
        fields = ('id', 'name', 'status')

    def validate_status(self, value):
        if value and not self.instance.is_valid:
            raise serializers.ValidationError('Status cannot be set for invalid instance')
        return value


objs = MyClass.objects.bulk_create(
    objs=[
        MyClass(name='valid'),
        MyClass(name='invalid'),
        MyClass(name='other'),
    ]
)

serializer = MyClassSerializer(
    data=[{'status': 'set', 'id': instance.id} for instance in objs],
    instance=objs,
    many=True,
    partial=True,
)
serializer.is_valid()
print(serializer.errors)
```

Please note that it is just an example and real serializers and models can be much more complex. The case is working when we pass the single instance to the serializer but not working when we are trying to validate `bulk` operation, like update in bulk (let's say)

The following exception raised:
```
AttributeError: 'list' object has no attribute 'is_valid'
```

When expected behavior would be to get this one (pretty-printed):
```
[
    {},
    {'status': [ErrorDetail(string='Status cannot be set for invalid instance', code='invalid')]},
    {'status': [ErrorDetail(string='Status cannot be set for invalid instance', code='invalid')]},
]
```

P.S. I was able to manually patch the serializer to obtain the instance from the data provided, but I do not think that it should be done manually for every serializer. I assume that when we are validating the instance using PARTIAL, then the appropriate instance which is currently on validation, should be available inside the serializer. In this case we will be able to use the same serializer as single and multi entity-validation.

P.P.S. Also I think that if user require some logic for the list of objects, it should be placed inside the overridden `list_serializer_class`, no?

Hints from issue discussion:
I understand what you're trying to do here but I'm not sure how the values in the `data` list and the `objs` that you pass as the `instance` would correspond to each other. Would you assume that they are matched via indexes? Would you want to introduce a new keyword argument (e.g `instances`) just to make it clearer?

Also, from what I understand, `many` only works when you want serialize multiple objects into a list.

Interesting idea for a new feature, nonetheless. I know that Django does support [bulk updating](https://docs.djangoproject.com/en/4.2/ref/models/querysets/#bulk-update) so this should be possible to do. However, I'm not sure if the team behind DRF would want to add this to the core project at this point.
if anything is supported in django ORM core then that should be supported in DRF too.
My only concern is the mapping between the `data` and the `objs` in this example. Maybe instead of passing the `objs` it would be better to do something like this:

```python
serializer = MyClassSerializer(
    data=[{'status': 'set', 'id': instance.id} for instance in objs],
    key='id',
    many=True,
    partial=True,
)
```

This way the serializer can internally make a query to to fetch all objects based on the provided `key` (the argument doesn't have to have this name) and make a single query to fetch them. It could then do a bulk update as defined in the [Django Docs](https://docs.djangoproject.com/en/4.2/ref/models/querysets/)
So I did some more digging into it. The problem is caused by the following method:

```python
    def validate_status(self, value):
        if value and not self.instance.is_valid:
            raise serializers.ValidationError('Status cannot be set for invalid instance')
        return value
```

Specifically, it's caused by the `self.instance` is set to the value passed as `data`.  Also, when you set `many` to `True`, the serializer that is declared is actually a `ListSerializer` instance. I'm not sure how we could make the `self.instance` variable point to the right `dict` object within the `data` list. From the looks of it, this would require significant reworking within the validation of the serializer classes.

Commit message: fix imports sorting for list serializer tests

Patch to review:
diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py
index 01bebf5fca..9301484dc2 100644
--- a/rest_framework/serializers.py
+++ b/rest_framework/serializers.py
@@ -589,6 +589,11 @@ def __init__(self, *args, **kwargs):
         self.min_length = kwargs.pop('min_length', None)
         assert self.child is not None, '`child` is a required argument.'
         assert not inspect.isclass(self.child), '`child` has not been instantiated.'
+
+        if kwargs.get('instance', []) and kwargs.get('data', []):
+            assert len(kwargs.get("data", [])) == len(
+                kwargs.get("instance", [])), 'Data and instance should have same length'
+
         super().__init__(*args, **kwargs)
         self.child.bind(field_name='', parent=self)
 
@@ -663,7 +668,10 @@ def to_internal_value(self, data):
         ret = []
         errors = []
 
-        for item in data:
+        for idx, item in enumerate(data):
+            if hasattr(self, 'instance') and self.instance and \
+                    len(self.instance) > idx:
+                self.child.instance = self.instance[idx]
             try:
                 validated = self.child.run_validation(item)
             except ValidationError as exc:
diff --git a/tests/test_serializer.py b/tests/test_serializer.py
index 1d9efaa434..9e286babd5 100644
--- a/tests/test_serializer.py
+++ b/tests/test_serializer.py
@@ -2,6 +2,7 @@
 import pickle
 import re
 import sys
+import unittest
 from collections import ChainMap
 from collections.abc import Mapping
 
@@ -762,3 +763,63 @@ class TestSerializer(serializers.Serializer):
 
         assert (s.data | {}).__class__ == s.data.__class__
         assert ({} | s.data).__class__ == s.data.__class__
+
+
+class MyClass(models.Model):
+    name = models.CharField(max_length=100)
+    value = models.CharField(max_length=100, blank=True)
+
+    app_label = "test"
+
+    @property
+    def is_valid(self):
+        return self.name == 'valid'
+
+
+class MyClassSerializer(serializers.ModelSerializer):
+    class Meta:
+        model = MyClass
+        fields = ('id', 'name', 'value')
+
+    def validate_value(self, value):
+        if value and not self.instance.is_valid:
+            raise serializers.ValidationError(
+                'Status cannot be set for invalid instance')
+        return value
+
+
+class TestMultipleObjectsValidation(unittest.TestCase):
+    def setUp(self):
+        self.objs = [
+            MyClass(name='valid'),
+            MyClass(name='invalid'),
+            MyClass(name='other'),
+        ]
+
+    def test_multiple_objects_are_validated_separately(self):
+
+        serializer = MyClassSerializer(
+            data=[{'value': 'set', 'id': instance.id} for instance in
+                  self.objs],
+            instance=self.objs,
+            many=True,
+            partial=True,
+        )
+
+        assert not serializer.is_valid()
+        assert serializer.errors == [
+            {},
+            {'value': ['Status cannot be set for invalid instance']},
+            {'value': ['Status cannot be set for invalid instance']}
+        ]
+
+    def test_exception_raised_when_data_and_instance_length_different(self):
+
+        with self.assertRaises(AssertionError):
+            MyClassSerializer(
+                data=[{'value': 'set', 'id': instance.id} for instance in
+                      self.objs],
+                instance=self.objs[:-1],
+                many=True,
+                partial=True,
+            )


Code review comments:
diff_hunk: @@ -663,7 +668,10 @@ def to_internal_value(self, data):
         ret = []
         errors = []
 
-        for item in data:
+        for idx, item in enumerate(data):
+            if hasattr(self, 'instance') and self.instance and \
+                    len(self.instance) > idx:
path: rest_framework/serializers.py
text: @user1:
I do not know if this repo is using backlashes or parenthesis to wrap long lines:
```suggestion
            if (
                hasattr(self, 'instance')
                and self.instance
                and len(self.instance) > idx
            ):
```

diff_hunk: @@ -589,6 +589,11 @@ def __init__(self, *args, **kwargs):
         self.min_length = kwargs.pop('min_length', None)
         assert self.child is not None, '`child` is a required argument.'
         assert not inspect.isclass(self.child), '`child` has not been instantiated.'
+
+        if kwargs.get('instance', []) and kwargs.get('data', []):
+            assert len(kwargs.get("data", [])) == len(
+                kwargs.get("instance", [])), 'Data and instance should have same length'
path: rest_framework/serializers.py
text: @user1:
To avoid 2 times getting the dict value with default fallback, maybe use variables?

In python 3.8+ we are using `walrus :=` operator like this:
```suggestion
        if (instance := kwargs.get('instance', [])) and (data := kwargs.get('data', [])):
            assert len(data) == len(instance), 'Data and instance should have same length'
```

But to support previous versions of python, maybe we should do it as a separate variables:
```suggestion
        # instance, data = kwargs.get('instance', []), kwargs.get('data', [])  # if you prefer one line assignment support
        instance = kwargs.get('instance', [])
        data = kwargs.get('data', [])
        if instance and data:
            assert len(data) == len(instance), 'Data and instance should have same length'
```

@user2:
we have to support 3.6 now