encode / django-rest-framework

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

Return None values as None in ListSerializer.to_representation #9386

Open ericls opened 2 months ago

ericls commented 2 months ago

Description

I recently encountered some unexpected behavior when using ListSerializer when there are None values in the list.

Here's a snippet that describes the issue and provides a way to reproduce it:

from rest_framework import serializers

class Bar(serializers.Serializer):
    a = serializers.ListSerializer(
        child=serializers.IntegerField(allow_null=True)
    )

class Foo(serializers.Serializer):
    a = serializers.IntegerField(allow_null=True)
    b = Bar()

foo = Foo(data={'a': 1, 'b': {'a': [None]}})

print(foo.is_valid(raise_exception=True)) # this passes
print(foo.data)  # this triggers the error

traceback:

File ~/workspace/django-rest-framework/rest_framework/serializers.py:714, in <listcomp>(.0)
    709 # Dealing with nested relationships, data can be a Manager,
    710 # so, first get a queryset from the Manager if needed
    711 iterable = data.all() if isinstance(data, models.manager.BaseManager) else data
    713 return [
--> 714     self.child.to_representation(item) for item in iterable
    715 ]

File ~/workspace/django-rest-framework/rest_framework/fields.py:923, in IntegerField.to_representation(self, value)
    922 def to_representation(self, value):
--> 923     return int(value)

TypeError: int() argument must be a string, a bytes-like object or a number, not 'NoneType'

Another thing to note is that ListField already handles it properly (in #4118 ), in fact I'm just copying what's been done to ListField to ListSerializer.

So I think there are two issues that kinda throws me off:

  1. If a serializer instance passes the validation, it should not then throw an error when getting the .data attribute.
  2. The behavior should be the same as ListField
sevdog commented 2 months ago

Another thing to note is that ListField already handles it properly (in #4118 ), in fact I'm just copying what's been done to ListField to ListSerializer.

Since the use case is already handled in ListField, I belive that it would be better having ListSerializer raising an error if its child is not a Serializer, suggesting to use ListField instead.

What is the point of using ListSerialier instead of ListField in this case?