Address ListSerializer lookup field review feedback

Author: zainnadeem786

docs/api-guide/serializers.md

@@ -799,6 +799,8 @@ To support multiple updates you'll need to do so explicitly. When writing your m
 
 You will need to add an explicit `id` field to the instance serializer. The default implicitly-generated `id` field is marked as `read_only`. This causes it to be removed on updates. Once you declare it explicitly, it will be available in the list serializer's `update` method.
 
+During validation, `ListSerializer` matches each input item to an existing instance using `pk` by default. To use another identifier, such as `id` or `uuid`, set `lookup_field` on the child serializer's `Meta` class.
+
 Here's an example of how you might choose to implement multiple updates:
 
     class BookListSerializer(serializers.ListSerializer):
@@ -831,6 +833,7 @@ Here's an example of how you might choose to implement multiple updates:
 
         class Meta:
             list_serializer_class = BookListSerializer
+            lookup_field = 'id'
 
 ### Customizing ListSerializer initialization
 

rest_framework/serializers.py

@@ -673,13 +673,8 @@ def run_child_validation(self, data):
         ):
             return self.child.run_validation(data)
 
-        lookup_field = getattr(getattr(self.child, 'Meta', None), 'lookup_field', None)
-        if lookup_field is not None:
-            data_pk = data.get(lookup_field)
-        else:
-            data_pk = data.get('id')
-            if data_pk is None:
-                data_pk = data.get('pk')
+        lookup_field = getattr(getattr(self.child, 'Meta', None), 'lookup_field', 'pk')
+        data_pk = data.get(lookup_field)
 
         if data_pk is None:
             return self.child.run_validation(data)
@@ -738,15 +733,10 @@ def to_internal_value(self, data):
                 instance_map = {str(k): v for k, v in self.instance.items()}
             elif isinstance(self.instance, (list, tuple, models.query.QuerySet)):
                 instance_map = {}
-                lookup_field = getattr(getattr(self.child, 'Meta', None), 'lookup_field', None)
+                lookup_field = getattr(getattr(self.child, 'Meta', None), 'lookup_field', 'pk')
 
                 for obj in self.instance:
-                    if lookup_field is not None:
-                        pk = getattr(obj, lookup_field, None)
-                    else:
-                        pk = getattr(obj, 'id', None)
-                        if pk is None:
-                            pk = getattr(obj, 'pk', None)
+                    pk = getattr(obj, lookup_field, None)
 
                     if pk is not None:
                         key = str(pk)
@@ -831,7 +821,7 @@ def save(self, **kwargs):
             "For example: 'serializer.save(owner=request.user)'."
         )
         assert not hasattr(self, '_data'), (
-            "You cannot call `.save()` after accessing `serializer.data`."
+            "You cannot call `.save()` after accessing `serializer.data`. "
             "If you need to access data before committing to the database then "
             "inspect 'serializer.validated_data' instead. "
         )

tests/test_serializer_lists.py

@@ -204,30 +204,7 @@ def update(self, instance, validated_data):
 
 
 class TestListSerializerInstanceMatching:
-    def test_matching_with_id(self):
-        seen_instances = []
-
-        class TestSerializer(serializers.Serializer):
-            id = serializers.IntegerField()
-
-            def validate(self, attrs):
-                seen_instances.append(self.instance)
-                return attrs
-
-        instance = [
-            BasicObject(id=1),
-            BasicObject(id=2),
-        ]
-        input_data = [
-            {'id': 1},
-            {'id': 2},
-        ]
-
-        serializer = TestSerializer(instance, data=input_data, many=True)
-        assert serializer.is_valid()
-        assert seen_instances == instance
-
-    def test_matching_with_pk(self):
+    def test_matching_with_default_lookup_field(self):
         seen_instances = []
 
         class TestSerializer(serializers.Serializer):
@@ -250,59 +227,25 @@ def validate(self, attrs):
         assert serializer.is_valid()
         assert seen_instances == instance
 
-    def test_matching_with_id_against_object_with_pk_only(self):
-        seen_instances = []
-
-        class TestSerializer(serializers.Serializer):
-            id = serializers.IntegerField()
-
-            def validate(self, attrs):
-                seen_instances.append(self.instance)
-                return attrs
-
-        instance = [BasicObject(pk=1)]
-        input_data = [{'id': 1}]
-
-        serializer = TestSerializer(instance, data=input_data, many=True)
-        assert serializer.is_valid()
-        assert seen_instances == instance
-
-    def test_matching_prefers_id_when_id_and_pk_differ(self):
-        seen_instances = []
-
-        class TestSerializer(serializers.Serializer):
-            id = serializers.IntegerField()
-
-            def validate(self, attrs):
-                seen_instances.append(self.instance)
-                return attrs
-
-        instance = [BasicObject(id=1, pk=2)]
-        input_data = [{'id': 1}]
-
-        serializer = TestSerializer(instance, data=input_data, many=True)
-        assert serializer.is_valid()
-        assert seen_instances == instance
-
     def test_mapping_instance_matching(self):
         seen_instances = []
 
         class TestSerializer(serializers.Serializer):
-            id = serializers.IntegerField()
+            pk = serializers.IntegerField()
 
             def validate(self, attrs):
                 seen_instances.append(self.instance)
                 return attrs
 
-        obj1 = BasicObject(id=1)
-        obj2 = BasicObject(id=2)
+        obj1 = BasicObject(pk=1)
+        obj2 = BasicObject(pk=2)
         instance = {
             '1': obj1,
             '2': obj2,
         }
         input_data = [
-            {'id': 1},
-            {'id': 2},
+            {'pk': 1},
+            {'pk': 2},
         ]
 
         serializer = TestSerializer(instance, data=input_data, many=True)
@@ -313,13 +256,13 @@ def test_unsupported_instance_type_preserves_original_behavior(self):
         seen_instances = []
 
         class TestSerializer(serializers.Serializer):
-            id = serializers.IntegerField()
+            pk = serializers.IntegerField()
 
             def validate(self, attrs):
                 seen_instances.append(self.instance)
                 return attrs
 
-        serializer = TestSerializer(instance=123, data=[{'id': 1}], many=True)
+        serializer = TestSerializer(instance=123, data=[{'pk': 1}], many=True)
         assert serializer.is_valid()
         assert seen_instances == [123]
 
@@ -370,11 +313,11 @@ def validate(self, attrs):
 
     def test_existing_instance_map_is_restored_after_validation(self):
         class TestSerializer(serializers.Serializer):
-            id = serializers.IntegerField()
+            pk = serializers.IntegerField()
 
-        instance = [BasicObject(id=1)]
-        original_instance_map = {'sentinel': BasicObject(id=2)}
-        serializer = TestSerializer(instance, data=[{'id': 1}], many=True)
+        instance = [BasicObject(pk=1)]
+        original_instance_map = {'sentinel': BasicObject(pk=2)}
+        serializer = TestSerializer(instance, data=[{'pk': 1}], many=True)
         serializer._list_serializer_instance_map = original_instance_map
 
         assert serializer.is_valid()
@@ -1073,6 +1016,9 @@ class TestSerializer(serializers.Serializer):
         id = serializers.IntegerField()
         status = serializers.CharField()
 
+        class Meta:
+            lookup_field = 'id'
+
         def validate_status(self, value):
             if self.instance is None:
                 raise serializers.ValidationError("Instance not matched")