Conversation

ericls

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

@@ -711,7 +711,7 @@ def to_representation(self, data):
iterable = data.all() if isinstance(data, models.manager.BaseManager) else data

return [
self.child.to_representation(item) for item in iterable
self.child.to_representation(item) if item is not None else None for item in iterable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will need extensive tests for changes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely. I didn't expect this to be merged as-is, just want to start the conversation.

In terms of tests, what kind of test do we want? I'm thinking just adding new tests for the new behavior.

I think the tricky thing is that we don't know if the current behavior is relied upon, because there's no existing tests for that, do you know if there's a way to get that infromation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please refer to this comment #9386 (comment)

@sevdog

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?

@staleStale

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.

@stalestale bot added the stale label Apr 27, 2025
@auvipyauvipy removed the stale label Apr 28, 2025
Sign up for free to join this conversation on . Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.