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

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 this issue Jul 5, 2024 · 8 comments

Comments

@LinanV
Copy link

LinanV commented Jul 5, 2024

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 LinanV changed the title Include required=False Fields with None Value in to_representation with partial=True One field that is required=False, allow_null=True and not provided. to_representation should not explicitly set it to None Jul 5, 2024
@LinanV
Copy link
Author

LinanV commented Jul 8, 2024

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

@sevdog
Copy link
Contributor

sevdog commented Jul 8, 2024

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
Copy link
Author

LinanV commented Jul 8, 2024

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
Copy link
Contributor

sevdog commented Jul 8, 2024

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
Copy link
Author

LinanV commented Jul 9, 2024

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
Copy link
Contributor

sevdog commented Jul 9, 2024

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
Copy link
Author

LinanV commented Jul 9, 2024

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=Noneerror_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
Copy link

salom Qandaysiz

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