encode / django-rest-framework

Web APIs for Django. 🎸
https://www.django-rest-framework.org
Other
28.46k stars 6.85k forks source link

One field that is `required=False, allow_null=True` and not provided. to_representation should not explicitly set it to None #9458

Open LinanV opened 4 months ago

LinanV commented 4 months ago

Problem Statement

Currently, one field that isrequired=False, allow_null=True and not provided. to_representation should not explicitly set it to None.

Maybe

rest_framework/fields.py

def get_attribute(self, instance
        """
        Given the *outgoing* object instance, return the primitive value
        that should be used for this field.
        """
       ...

           # changed
            if not self.required: 
                raise SkipField()
            if self.allow_null:
                return None**
       ...

Example

Consider the following serializer and instance:

class MySerializer(serializers.Serializer):
    name = serializers.CharField()
    type = serializers.CharField(required=False, allow_null=True)
    desc = serializers.CharField(required=False, allow_null=True)

instance = {'name': 'example'}
serializer = MySerializer(instance, partial=True)
data = serializer.data
# Current output: {'name': 'example', 'desc': None, 'type': None}
# Proposed output: {'name': 'example'}
LinanV commented 4 months ago

My Python version is 3.7, so I am using djangorestframework 3.15.1. However, the issue still persists

sevdog commented 4 months ago

Currently the partial keyword argument provided to serializers is meant for "partial update", not for "partial representation".

Thus the use-case you are willing is not that for which that parameter was intended. The change in #9461 may cause breaking changes in applications which use DRF.

LinanV commented 4 months ago

Currently the partial keyword argument provided to serializers is meant for "partial update", not for "partial representation".

Thus the use-case you are willing is not that for which that parameter was intended. The change in #9461 may cause breaking changes in applications which use DRF.

Sorry, maybe I didn't describe it clearly. This issue is not related to the partial parameter, but rather to the configuration when both required=False and allow_null=True are set together. As described in the example, I'm getting unexpected output.

Perhaps my pull request has issues, but I hope this problem can be resolved.

sevdog commented 4 months ago

I suggest to look at #5888, because there there is a change which is the opposite of #9461 (the same change was made in #5639).

LinanV commented 4 months ago

As a user of DRF, required=False and allow_null=True indicate that this field is optional and can be null. However, this does not mean that the field's default value is None. The changes in refs #5888 have made the meaning of allow_null=True unclear, as it now encompasses two implications: 1) it allows null values, and 2) it defaults to None.

sevdog commented 4 months ago

Currently I belive that this can be work-arounded by providing a callable default which raises a SkipField exception.

def skip_default():
    raise serializers.SkipField

class MySerializer(serializers.Serializer):
    name = serializers.CharField()
    type = serializers.CharField(required=False, allow_null=True, default=skip_default)
    desc = serializers.CharField(required=False, allow_null=True, default=skip_default)

A possible solution to this could be to have a different usage of the allow_null using a more complex value check (ie: True/False -> current behaviour, None -> skip), which is a breaking change (since currently any falsy value produces the same behaviour.

An other possible solution would be to add a specific parameter to control the output when required and allow_null parameters fall into this case.

LinanV commented 4 months ago

Why not directly modify the Field class, instead of requiring users to explicitly define the default parameter?

class Field:
    def __init__(self, read_only=False, write_only=False, 
                 required=None, default=empty, initial=empty, source=None,
                 label=None, help_text=None, style=None, 
                 error_messages=None, validators=None, allow_null=False):
        ...
        if required is False and allow_null is True and default is empty:
            self.default = skip_default
Rustambek0401 commented 3 months ago

salom Qandaysiz