Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ListField validation errors are inconsistent with ListSerializer #7279

Open
markopy opened this issue Apr 18, 2020 · 6 comments
Open

ListField validation errors are inconsistent with ListSerializer #7279

markopy opened this issue Apr 18, 2020 · 6 comments

Comments

@markopy
Copy link

markopy commented Apr 18, 2020

Validation errors when using ListSerializer are returned as a list with non-failing entries empty:

[
{},
{"foo": ["error"]},
{},
{"foo": ["another error"]}
]

When using ListField the same information is unexpectedly returned as a dictionary:

{
"1": {"foo": ["error"]},
"3": {"foo": ["another error"]}
}

The current ListField error handling was implemented in #5655 along with DictField, which explains this behavior but I think it would be more obvious for ListField to behave like ListSerializer and return a list instead.

@agowa agowa mentioned this issue Sep 15, 2020
6 tasks
@sarathak
Copy link
Contributor

How to replicate it ?

i tried following

    class TestSerializer(serializers.Serializer):
        foo = serializers.ListField(required=False)

    class ObjectListSerializer(serializers.ListSerializer):
        child = TestSerializer()
    input_data = [
        {},
        {"foo": ["error"]},
        {},
        {"foo": ["another error"]}
    ]
    serializer = ObjectListSerializer(data=input_data)

@stale
Copy link

stale bot commented Apr 28, 2022

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.

@stale stale bot added the stale label Apr 28, 2022
@bost-h
Copy link

bost-h commented Nov 18, 2022

I agree with the original issue, ListField and ListSerializer errors seem inconsistent to me.

How to replicate it ?

Does this help ?

class NestedTestSerializer(serializers.Serializer):
    nested_serializer_field = serializers.CharField()


class TestSerializer(serializers.Serializer):
    list_serializer = NestedTestSerializer(many=True)
    list_field = serializers.ListField(child=serializers.DictField(allow_empty=False))


def test_errors_inconsistencies():
    serializer = TestSerializer(
        data={
            "list_serializer": [{"nested_serializer_field": ""}],
            "list_field": [{}],
        }
    )
    serializer.is_valid()
    # list_serializer error is: [{"nested_serializer_field": ["This field may not be blank."]}]
    assert isinstance(serializer.errors["list_serializer"], list)
    # list_field error is: {"0": ["This dictionary may not be empty."]}
    assert isinstance(serializer.errors["list_field"], dict)

As you can see, both list_serializer and list_field expects a list, but in case of validation errors ListField returns a dict of errors, and ListSerializer returns a list of errors.

It is caused by ListField.run_child_validation which uses errors = OrderedDict() but ListSerializer.to_internal_value has errors = [].

It is debatable if the correct error type is a list or a dict. On our side, we have chosen to use a dict in all cases because:

  • It seems to be the intention of Improve composite field child errors #5655
  • Having a dict of errors address the case where the error isn't for the first item (e.g. {"1": ["Error"]} vs [null, ["Error"]
  • I think it means the clients can be sure list are always list of messages, and there is no need to dive deeper / check the type of the items.

@gilman88
Copy link

I am also in the camp that returning a dict of errors is more likely to be useful when actually handling these errors and/or debugging, I also think it's more likely to be the behavior that developers are expecting.

However in the event that consensus cannot be reached I would propose that the documentation be updated to make this difference clear between ListSerializer (which you get by default with the many=True flag) and ListField. Especially since passing a many=True flag is strongly encouraged whenever you are dealing with nested serializers. I'm sure there are other important differences as well but I haven't dug into them (maybe the create/update behaviour?)

@stale stale bot removed the stale label Mar 11, 2024
@auvipy
Copy link
Member

auvipy commented Mar 11, 2024

a document update and later updating or aligning the errors would be the go. any takers?

@gilman88
Copy link

gilman88 commented Apr 2, 2024

I don't really have enough time, also I don't really agree with the style the documentation is written in so I would inevitably fall down a deep rabbit hole trying to update them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants