allow_null=True is not being passed to ListSerializer constructor in many_init

[delinhabit]

Hello,

We have a use case where we accept either a list of objects or null. The problem is that BaseSerializer.many_init is not passing the allow_null argument to the ListSerializer constructor. because of that I have validation errors even if the field was specified with allow_null=True.

He's an example:

class PaymentInfoSerializer(serializers.Serializer):
    url = serializer.CharField()
    amount = serializers.DecimalField()

    def validate(self, attrs):
        """
        This is never being called
        """


class Request(serializers.Serializer):
    payment_info = PaymentInfoSerializer(
        many=True, 
        required=False,
        allow_null=True)

Basically LIST_SERIALIZER_KWARGS (defined here) does not include allow_null and therefore the arguments is not passed to the ListSerializer constructor (details here).

Is there a reason why allow_null should not be sent to the list serializer?

Thanks!

[delinhabit]

Our current fix is to create a custom ListSerializer that passes allow_null=True to the default constructor:

class NullableListSerializer(serializers.ListSerializer):
    def __init__(self, *args, **kwargs):
        kwargs['allow_null'] = True
        super(NullableListSerializer, self).__init__(*args, **kwargs)


class PaymentInfoSerializer(serializers.Serializer):
    class Meta:
        list_serializer_class = NullableListSerializer

It just feels hacky, since the default implementation of ListSerializer work well if you pass it the allow_null=True keyword argument, and this is the only required changed to make it work.

[tomchristie]

Not convinced that we should be supporting this by default.
I'd assume the current behavior will be to allow this:

[{'child': 'object'}, {'child': 'object'}, None, {'child': 'object'}]

Which I think is reasonable. If you want an empty list, use an empty list [] rather than None, or use a custom list serializer as you've done. We could discuss this further and might be open to a change, but for now I'm going to consider this as closed.

[delinhabit]

I'm not saying it's necessarily a bug that needs to be addressed. You point regarding empty lists is perfectly valid. I'm not sure though that a list containing a null value at some index is more plausible use-case than specifying a null as an input for the many field.

But there are a few aspects that make sense regarding this:

1. It costs almost nothing to support this functionality (just add the allow_null value to LIST_SERIALIZER_KWARGS). It will not break backwards compatibility and whoever will want to use this behavior will have to just define the field as WhateverSerializer(many=True, allow_null=True). This will also be consistent with the other serializer usage OtherSerializer(allow_null=True).
2. There might be cases when an empty list might mean something and a null value might means something else for the same field and the API needs to handle that.
3. In my case (and it's possible in other cases) I'm not building a new API, I'm upgrading a DRF 2.x API to the latest version and I need to keep backwards compatibility with my mobile apps

If you don't consider this important enough, and I understand if you don't, we should at least document that this is not support, or even raise an exception if someone tries to use WhateverSerializer(many=True, allow_null=True). Currently it just silently drops the keyword arguments that are not supported and it's somehow confusing (it took me a while to understand why the null value is not being passed to the serializer)

Thanks for the great work you are doing!

[tomchristie]

if someone tries to use WhateverSerializer(many=True, allow_null=True)

I think it's probably more a case of trying to figure out which of [{}, None, {}, {}] vs None is more intuitive - perhaps worth taking to the discussion group?

[delinhabit]

Sounds good to me. I posted a question to the discussion group.

[tomchristie]

👍

[delinhabit]

@tomchristie There was not much of a discussion on the forum about this issue. The only comment was in favor of supporting None as a value for a list serializer field (instead of [{}, None, {}]).

Did you get a chance to think about this more throughly?

[tomchristie]

Perhaps the best thing to move this forward would be to write the most minimal possible test case that demonstrates the behavior change that you're suggesting.

Then submit that as a pull request, along with the suggested fix.

Not a garuntee that it'll get pulled in, but that'd remove any blockers aside from the design decision.

[delinhabit]

Sounds good. I'll have a PR in the next day or two.

[israellias]

Hi everyone.
The merged PR applies only for Serializers, but it does not for RelatedField's.

I guess we only need to add allow_null to MANY_RELATION_KWARGS (Found on relations.py:82)

The use case occurs we want extend a serializers.PrimaryKeyRelatedField and allow a null value for that many primary keys

class ConnectionRelatedField(serializers.PrimaryKeyRelatedField):
   # Do something
    pass

class MySerializer(serializers.ModelSerializer):
    connections = ConnectionRelatedField(
        many=True,
        queryset=Entity.objects.filter(entity_type=EntityType.COLLECTION),
        required=False,
        allow_null=True,
        write_only=True,
    )

What are you thoughts about this?