Fix validation for ListSerializer when many=True #9774

p-r-a-v-i-n · 2025-09-01T12:06:20Z

Added tests and ensured that ListSerializer properly validates multiple objects when many=True.
Covers both valid and invalid data scenarios.

Changes include:

Tests for validating multiple instances when many=True was passed.
Tests for handling invalid data in a list
Simplified ListSerializer usage in tests without custom run_child_validation

This aligns with the ongoing Django REST Framework issue #8926

rest_framework/serializers.py

browniebroke · 2025-09-01T15:35:10Z

rest_framework/serializers.py

+        instance = None
+        if self.instance is not None:
+            instance_map = {getattr(obj, 'pk', None): obj for obj in self.instance}
+            obj_id = data.get('id') or data.get('pk')


This line assumes that the primary key field is called "id" or "pk" but it could be called anything and DRF can't know it in advance so this fix will work in a limited subset of cases.

You are correct , but in the existing TestListSerializerContainingNestedSerializer class was using hardcoded 'pk' keys.
But your argument make sense and i have updated the it.

The difference with TestListSerializerContainingNestedSerializer is that this is demonstrating something that would be implemented by the consumers of DRF, showing how one would override run_child_validation to match up the instance with the provided data, which is highly dependant on the use case.

Here, you're doing that in the library code.

tests/models.py

browniebroke · 2025-09-01T15:38:49Z

tests/test_serializer_lists.py

+        class TestListModelSerializer(serializers.ModelSerializer):
+            class Meta:
+                model = TestListModel
+                fields = ("id", "name", "status")
+
+            def validate_status(self, value):
+                if value and not self.instance.is_valid:
+                    return False
+                return value


Minor: would be nice to not duplicate this serializer in each test function (see the other classes above for some examples on how to do that).

browniebroke · 2025-09-02T18:30:43Z

rest_framework/serializers.py

+                pk_name = model._meta.pk.name
+
+            if pk_name:
+                obj_id = data.get(pk_name, data.get("pk", data.get("id")))


I can see plenty of ways to break this: what if the PK is a UUID field called id but serialized as uuid? Sometimes it's called uid... Are we going to handle all possible field name people are using in the wild?

That’s true, but the same is already true for single updates. If the PK is serialized under a different name (e.g. uuid, uid, etc.), DRF can’t resolve it automatically there either unless the serializer is customized.

If anything works for single updates, it will also work for bulk updates, the constraints are the same.

I disagree, with a single instance, you have the instance and the data, so it's a 1 to 1 mapping and you know it should match.

With a list of dicts on one hand and a list of instances/queryset on the other, you need to map which dict corresponds to which instance.

This mapping will depend on the use case, and needs a unique identifier somewhere (which could be anything: PK, email, slug, combination of fields...). Hence why users need to do it, DRF can't do it for them.

I see your point about single updates having a direct instance - data mapping. But I’d still argue the difference is one of quantity rather than fundamentals.

Even in single updates, DRF assumes that the mapping is correct only because the caller provided the right instance. If the serializer is misaligned (e.g. PK serialized under another field, or a different uniqueness condition like email/slug), DRF doesn’t solve that, the user has to customize the serializer.

For bulk updates, the requirement is the same: there needs to be some unique identifier to match instance ↔ data. Whether that identifier is pk, uuid, email, or something else, the logic isn’t different from single updates, just applied across a list.

So I don’t see it as “DRF can’t do it at all,” but more that DRF could apply the same assumptions it already makes in the single case, and users who need different identifiers would still override/customize.

p-r-a-v-i-n · 2025-09-03T04:08:58Z

tests/test_serializer_lists.py

+        input_data = [
+            {
+                "uuid": "t3308237e-18d8-4074-9d05-79cc0fdb5bb3",
+                "name": "bar",
+            },
+        ]


I think this example is a bit overestimated. If the model’s primary key is id, it’s not a common or practical scenario to remap it to uuid and then expect DRF to resolve it automatically during updates.

In this setup, even single-object updates wouldn’t work without extra customization, since the serializer no longer exposes the real PK field. That’s not a limitation of bulk updates, it’s a limitation of how the serializer is defined. So I don’t think this example shows a specific weakness of many=True updates.

auvipy · 2025-09-04T10:03:25Z

tests/test_serializer_lists.py

            read_only_fields = ("uuid",)

        def validate_name(self, value):
+            print('SELF-INSTANCe: ', self.instance)


i'm not sure about the print here

Oh sorry, I mistakenly left after debugging.

p-r-a-v-i-n · 2025-10-12T12:07:48Z

Hi @browniebroke @auvipy ,
my tone in previous messages might have come across differently than I intended, and I apologize if that was the case.
I’m happy to make any changes if needed, or if you’re not happy with my changes, you can close this PR.
Either way i'm happy. Thanks for your time!

Copilot

Pull request overview

This PR adds instance-aware validation support to ListSerializer.run_child_validation() to fix validation when many=True is used with existing model instances. The implementation resolves the correct instance for each item in the data list by matching primary keys, enabling field validators to access self.instance during validation.

Key Changes:

Enhanced ListSerializer.run_child_validation() to automatically match data items to their corresponding instances using primary key lookup
Added comprehensive test coverage for validation with multiple primary key types (integer, email, UUID)
Created test models (ListModelForTest, EmailPKModel, PersonUUID) to support the new validation tests

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 15 comments.

File	Description
rest_framework/serializers.py	Implements instance resolution logic in `run_child_validation()` to match data items to model instances via primary key lookup and caching
tests/test_serializer_lists.py	Adds `TestManyTrueValidationCheck` test class with tests for integer, email, and UUID primary keys in many=True scenarios
tests/models.py	Adds three test models with different primary key types and `is_valid` properties for validation testing

Comments suppressed due to low confidence (1)

rest_framework/serializers.py:681

'except' clause does nothing but pass and there is no explanatory comment.

                except Exception:

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-14T15:09:39Z

tests/models.py

+    @property
+    def is_valid(self):
+        return self.name == 'valid'


The is_valid property returns a boolean based on whether name equals 'valid'. However, this creates confusion because 'is_valid' is a common method name in Django REST Framework serializers. Consider renaming this property to something more specific like 'has_valid_name' or 'is_name_valid' to avoid confusion.

Copilot · 2025-12-14T15:09:40Z

rest_framework/serializers.py

+        child_instance = getattr(self.child, "instance", None)
+
+        if self.instance is not None:
+            pk_name = None
+            child_meta = getattr(self.child, "Meta", None)
+            model = getattr(child_meta, "model", None) if child_meta else None
+
+            if model is not None:
+                pk_name = model._meta.pk.name
+
+            obj_id = None
+            if pk_name:
+                for field_name, field in self.child.fields.items():
+                    if getattr(field, "source", None) == pk_name:
+                        obj_id = data.get(field_name)
+                        if obj_id is not None:
+                            break
+
+                if obj_id is None:
+                    obj_id = data.get(pk_name) or data.get("pk") or data.get("id")
+
+            resolved_instance = None
+
+            if obj_id is not None and pk_name:
+                try:
+                    obj_id = model._meta.pk.to_python(obj_id)
+                except Exception:
+                    pass
+
+                if not hasattr(self, "_instance_index"):
+                    self._instance_index = {
+                        getattr(obj, pk_name): obj for obj in self.instance
+                    }
+
+                resolved_instance = self._instance_index.get(obj_id)
+
+            if resolved_instance is None:
+                if model is not None and self.context.get("allow_create", True):
+                    resolved_instance = model()
+                else:
+                    resolved_instance = child_instance
+
+            child_instance = resolved_instance
+
+        self.child.instance = child_instance
+        self.child.initial_data = data
        return self.child.run_validation(data)


There's no test coverage for serializers without a Meta.model (non-ModelSerializer) when using many=True with instances. The code checks if model is None at line 662 and 692, but no tests verify this path works correctly when using plain Serializers instead of ModelSerializers.

Copilot · 2025-12-14T15:09:40Z

tests/test_serializer_lists.py

+            def validate_status(self, value):
+                if value and not self.instance.is_valid:
+                    return False
+                return value


The validate_status method returns False when the instance is not valid, but field validators should raise ValidationError instead of returning False. Returning False will be used as the validated value, which is likely not the intended behavior. This should raise a ValidationError with an appropriate error message.

Copilot · 2025-12-14T15:09:40Z

tests/models.py

+    @property
+    def is_valid(self):
+        return self.name == 'valid'


The is_valid property returns a boolean based on whether name equals 'valid'. However, this creates confusion because 'is_valid' is a common method name in Django REST Framework serializers. Consider renaming this property to something more specific like 'has_valid_name' or 'is_name_valid' to avoid confusion.

Copilot · 2025-12-14T15:09:40Z

rest_framework/serializers.py

+                resolved_instance = self._instance_index.get(obj_id)
+
+            if resolved_instance is None:
+                if model is not None and self.context.get("allow_create", True):


The context.get("allow_create", True) defaults to True, which means a new model instance will be created even when it's not explicitly allowed. This could lead to unintended model creation. Consider whether the default should be False for safer behavior, or document this behavior clearly.

Suggested change

if model is not None and self.context.get("allow_create", True):

if model is not None and self.context.get("allow_create", False):

Copilot · 2025-12-14T15:09:42Z

rest_framework/serializers.py

+            if resolved_instance is None:
+                if model is not None and self.context.get("allow_create", True):
+                    resolved_instance = model()
+                else:
+                    resolved_instance = child_instance


The new code in run_child_validation handles the case when resolved_instance is None by creating a new model instance or using child_instance. However, there's no test coverage for the scenario where obj_id is found but doesn't match any instance in self._instance_index, and allow_create is False. This edge case should be tested to ensure proper behavior.

Copilot · 2025-12-14T15:09:42Z

tests/models.py

+    @property
+    def is_valid(self):
+        return self.name == 'valid'


The is_valid property returns a boolean based on whether name equals 'valid'. However, this creates confusion because 'is_valid' is a common method name in Django REST Framework serializers. Consider renaming this property to something more specific like 'has_valid_name' or 'is_name_valid' to avoid confusion.

Copilot · 2025-12-14T15:09:43Z

tests/test_serializer_lists.py

+        def validate_name(self, value):
+            if value and not self.instance.is_valid:
+                return False
+            return value


The validate_name method returns False when the instance is not valid, but field validators should raise ValidationError instead of returning False. Returning False will be used as the validated value, which is likely not the intended behavior. This should raise a ValidationError with an appropriate error message.

Copilot · 2025-12-14T15:09:43Z

tests/test_serializer_lists.py

+            def validate_name(self, value):
+                if value and not self.instance.is_valid:
+                    return False
+                return value


The validate_name method returns False when the instance is not valid, but field validators should raise ValidationError instead of returning False. Returning False will be used as the validated value, which is likely not the intended behavior. This should raise a ValidationError with an appropriate error message.

Copilot · 2025-12-14T15:09:43Z

rest_framework/serializers.py

+                try:
+                    obj_id = model._meta.pk.to_python(obj_id)
+                except Exception:
+                    pass


There's no test coverage for the scenario where the model's pk field throws an exception in to_python() (line 680) and the exception is silently caught. This could lead to obj_id remaining in an unconverted state, potentially causing lookup failures. A test should verify behavior when pk conversion fails.

browniebroke reviewed Sep 1, 2025

View reviewed changes

rest_framework/serializers.py Outdated Show resolved Hide resolved

p-r-a-v-i-n force-pushed the fix/bulk-validation-serializer branch from aec3126 to 5502965 Compare September 1, 2025 14:38

browniebroke reviewed Sep 1, 2025

View reviewed changes

tests/models.py Outdated Show resolved Hide resolved

browniebroke reviewed Sep 1, 2025

View reviewed changes

p-r-a-v-i-n force-pushed the fix/bulk-validation-serializer branch from 5502965 to 06ebf29 Compare September 2, 2025 05:56

updated child validation for ListSerializer

c3a8ad9

p-r-a-v-i-n force-pushed the fix/bulk-validation-serializer branch from 06ebf29 to c3a8ad9 Compare September 2, 2025 06:03

browniebroke reviewed Sep 2, 2025

View reviewed changes

Add another failing test

33b4977

p-r-a-v-i-n commented Sep 3, 2025

View reviewed changes

Add a test to show how single updates work

395cebe

auvipy reviewed Sep 4, 2025

View reviewed changes

Fix child instance resolution in ListSerializer

7ef2f1d

p-r-a-v-i-n force-pushed the fix/bulk-validation-serializer branch from 87a7ea5 to 7ef2f1d Compare September 4, 2025 10:13

Merge branch 'main' into fix/bulk-validation-serializer

44c3552

auvipy requested a review from Copilot December 14, 2025 15:04

Copilot started reviewing on behalf of auvipy December 14, 2025 15:04 View session

Copilot AI reviewed Dec 14, 2025

View reviewed changes

	if model is not None and self.context.get("allow_create", True):
	if model is not None and self.context.get("allow_create", False):

Uh oh!

Fix validation for ListSerializer when many=True #9774

Are you sure you want to change the base?

Fix validation for ListSerializer when many=True #9774

Conversation

p-r-a-v-i-n commented Sep 1, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-r-a-v-i-n commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

p-r-a-v-i-n commented Oct 12, 2025 •

edited

Loading