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

Since allow_null=True, the serializer can still validate and partially update even though partial=False #9501

Open
fbozhang opened this issue Aug 16, 2024 · 3 comments

Comments

@fbozhang
Copy link

fbozhang commented Aug 16, 2024

Description:

When using Django’s model and serializer as shown below, the FlowSerializer allows partial updates even when partial=False, due to allow_null=True for the flow_id field.

Model:

class Flow(models.Model):
    flow_id = models.IntegerField(null=True)
    flow_info = models.JSONField()

Serializer:

class FlowSerializer(serializers.ModelSerializer):
    flow_id = serializers.IntegerField(allow_null=True, required=False)
    
    class Meta:
        model = Flow
        fields = "__all__"

Input data:

FlowSerializer(instance, data={"flow_info": {"test":1}})

When the input data does not include the flow_id field, the serializer still passes validation, resulting in a partial update. This behavior is unexpected because partial=True is not used.

Source Code Insight:
The relevant code in the Django REST framework source is:

    def get_default(self):
        """
        Return the default value to use when validating data if no input
        is provided for this field.

        If a default has not been set for this field then this will simply
        raise `SkipField`, indicating that no value should be set in the
        validated data for this field.
        """
        if self.default is empty or getattr(self.root, 'partial', False):
            # No default, or this is a partial update.
            raise SkipField()

Questions:

  • Why does the get_default method check getattr(self.root, 'partial', False)?
  • Why does the serializer allow validation to pass in non-partial update scenarios, even when not all fields are provided?
  • Shouldn’t the serializer require all fields to be provided when partial=False?

This behavior is causing confusion and leads to unintended partial updates. Any insights or clarification on this design choice would be greatly appreciated.

@Himalaypatel75
Copy link

you are passing required=False in FlowSerializer . don't you think you should pass required=True for desired result ? for failing validation ?
@fbozhang

@fbozhang
Copy link
Author

you are passing required=False in FlowSerializer . don't you think you should pass required=True for desired result ? for failing validation ? @fbozhang
I don't think it's necessary to make it required. Because it allows nulls, it supports non-required fields, and if I use required=True, it can't be modified locally. I use flow_id = serializers.IntegerField(allow_null=True, required=False) just to express my question more clearly. In fact, when I use serializers.ModelSerializer, this field is originally a non-required parameter.
You can refer to the historical versions of Django and find that get_default is not what I expected at the beginning.From the comments, it can be seen that this is intentional. I want to know what went wrong at that time that led to such a change.
@Himalaypatel75

@Shekhrozx
Copy link

you are passing required=False in FlowSerializer . don't you think you should pass required=True for desired result ? for failing validation ? @fbozhang
I don't think it's necessary to make it required. Because it allows nulls, it supports non-required fields, and if I use required=True, it can't be modified locally. I use flow_id = serializers.IntegerField(allow_null=True, required=False) just to express my question more clearly. In fact, when I use serializers.ModelSerializer, this field is originally a non-required parameter.
You can refer to the historical versions of Django and find that get_default is not what I expected at the beginning.From the comments, it can be seen that this is intentional. I want to know what went wrong at that time that led to such a change.
@Himalaypatel75

Understanding the Behavior:

  • get_default Method and partial Check:

    • The get_default method checks if the partial attribute of the serializer's root (the serializer instance itself) is False. This logic determines whether the serializer is in partial update mode.

    • If partial is False, the method assumes that a complete set of data is expected, and any missing fields will be considered errors.

  • Allowing Validation to Pass:

    • The serializer allows validation to pass even when not all fields are provided because the flow_id field has allow_null=True. This means that the field can be null or omitted in the input data without causing validation errors.

    • The combination of partial=False and allow_null=True creates a discrepancy: while partial=False suggests a complete update, allow_null=True permits partial updates by allowing the flow_id field to be omitted.

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

3 participants