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

When a serializer field defined explicitly, extra_kwargs for that field are ignored #6581

Closed
ozgurakcali opened this issue Apr 10, 2019 · 8 comments · Fixed by #6588
Closed

Comments

@ozgurakcali
Copy link

Steps to reproduce

Define a field explicity on a serializer, than add extra_kwargs for that field in the Meta class

class MySerializer(serializers.ModelSerializer):
    my_field = serializer.CharField()

    class Meta:
        model = MyModel
        fields = '__all__'
        extra_kwargs = {
            'my_field ': {'required': False}
        }

Expected behavior

Should not raise validation error when my_field is not supplied in the request body

Actual behavior

Raises validation error for missing my_field

I think this behavior is caused by this part in get_fields method.

If this is the expected behavior though, which could be taking possible clashing settings into consideration, I think the documentation should be updated stating that extra_kwargs will not be taken into account if the field is defined explicitly in the serializer.

rawteech added a commit to rawteech/django-rest-framework that referenced this issue Apr 13, 2019
@rawteech
Copy link
Contributor

Added a little more description on the documentation

@xordoquy
Copy link
Collaborator

I consider the addition in the documentation to be good enough to close the issue yet.
It might be nice to raise some warning at load time if an explicitly defined field has extra_kwargs. PR will be considered based on its merit.

@rpkilby
Copy link
Member

rpkilby commented Apr 15, 2019

Note: a solution would need to consider serializer inheritance.

@maingoh
Copy link

maingoh commented Nov 29, 2019

This would be very helpful to handle extra_kwargs for serializer inheritance.

@xordoquy
Copy link
Collaborator

xordoquy commented Nov 29, 2019

Having extra_kwargs for Serializer is not a good idea. You are supposed to define your fields explicitly. Having two sources of truth is confusing. Same thing for an explicit field in ModelSerializer

@maingoh
Copy link

maingoh commented Nov 29, 2019

I will try to explain my current issue, I have a serializer with many long fields. I want to reuse this Serializer to just make them optional (required=False) or change only one or two parameters:

class BaseSerializer(Serializer):
    field_one = serializers.CharField(required=True, many=True, allow_empty=True, ...)
    field_second = serializers.CharField(required=True, many=True, allow_empty=True, ...)
    field_third = serializers.CharField(required=True, many=True, allow_empty=True, ...)

If I have to redefine all the fields, then inheritance becomes almost useless. Also if finally I realize I want to change field_one to allow_empty=False, I need to change it everywhere ..

The extra_kwargs feature exists for models, so it sounds more confusing to me to not be able to use it in this case (or maybe it should be named less generically).

@xordoquy
Copy link
Collaborator

And in less time it take to explain it, we'll see request for some weird use cases such as:

class HaveFunSerializer(Serializer):
    field_one = serializers.CharField(required=True, many=True, allow_empty=True, ...)
    class Meta:
        extra_kwargs = {'field_one': {'required': False}}

class BaseSerializer(Serializer):
    field_one = serializers.CharField(required=True, many=True, allow_empty=True, ...)

class Serializer1(BaseSerializer):
    class Meta:
        extra_kwargs = {'field_one': {'required': False}}

class Serializer2(Serializer1):
    class Meta:
        extra_kwargs = {'field_one': {'required': True}}

At which point half of the world will argue the HaveFunSerializer's field_one should be required and the other say it shouldn't. Also, should Serializer2's field_one be set to required because Serialize1 set it to False or should the Serializer's extra_kwargs be dismissed ?
Not even speaking about redefining field_one in Serializer2.

So many thing that makes me think it's a broken design.

Your options on the table being to use Python's type() function with a list of kwargs to define serializers on the flight and override the kwargs as you need or override the inherited serializer init to

@maingoh
Copy link

maingoh commented Nov 29, 2019

I understand the issue now. For me the extra_kwargs means "I want to override the fields attributes from the parent or the Model".
That means ideally, we shouldn't have a extra_kwargs in HaveFunSerializer concerning the field that got declared in the same serializer. An explicit exception if it happens would be nice (not sure if it is detectable though). Having both in the same serializer for me would be equal to declaring the same field twice with different parameters like if we did:

class HaveFunSerializer(Serializer):
    field_one = serializers.CharField(required=True, many=True, allow_empty=True, ...)
    field_one = serializers.CharField(required=False, many=True, allow_empty=True, ...)

Which would give field_one.required=False. But no one should do this, I guess .. So I don't think the HaveFunSerializer is a use case that someone would ask ?

Starting from this assumption Serializer2 would have field_one required and Serializer would have field_one optional.

Your options on the table being to use Python's type() function with a list of kwargs to define serializers on the flight and override the kwargs as you need or override the inherited serializer init to

I am not sure to understand, if you could give me a small example ?

Alternatively I could change it in the __init__ of the child, however this implies more code (less clear).
The existence of extra_kwargs made me think it could be used for this as well ..

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this issue Nov 17, 2020
sigvef pushed a commit to sigvef/django-rest-framework that referenced this issue Dec 3, 2022
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

Successfully merging a pull request may close this issue.

5 participants