oxan / djangorestframework-dataclasses

Dataclasses serializer for Django REST framework
BSD 3-Clause "New" or "Revised" License
431 stars 28 forks source link

Support for types like frozenset? #68

Closed adonig closed 1 year ago

adonig commented 1 year ago

I have a dataclass that contains a frozenset field. When I use save() on my serializer the created object contains a list though:

@dataclass
class Foo:
    bar: frozenset[str] | None = None

class FooSerializer(DataclassSerializer):
    class Meta:
        dataclass = Foo

class FooView(APIView):
    def post(self, request, format=None):
        serializer = FooSerializer(data=request.data)
        if serializer.is_valid():
            foo = serializer.save()
            # type of foo.bar is list

I already had a look at serializer_field_mapping and tried to add frozenset to it, but found no way to make it work.

What can I do to make the serializer automatically turn the list into a frozenset?

adonig commented 1 year ago

Maybe I'm completely off but this is what I tried:

from rest_framework.fields import ListField
from rest_framework_dataclasses.serializers import DataclassSerializer

class FrozensetField(ListField):
    def to_representation(self, value):
        return list(value)

    def to_internal_value(self, data):
        return frozenset(data)

class FooSerializer(DataclassSerializer):
    serializer_field_mapping = DataclassSerializer.serializer_field_mapping | {frozenset: FrozensetField}

    class Meta:
        dataclass = Foo

Sadly to_internal_value didn't get executed, so I changed it to {list: FrozensetField} and it worked, but of course it replaced all JSON arrays with frozensets, which is not what I'm trying to achieve.

adonig commented 1 year ago

I did some debugging into the code and found out that composite fields can either be a mapping, in which case they become a dict, or they will become a list:

    def build_composite_field(self, field_name: str, type_info: TypeInfo,
                              extra_kwargs: KWArgs) -> SerializerFieldDefinition:
        """
        Create a composite (mapping or list) field.
        """
        # Lookup the types from the field mapping, so that it can easily be changed without overriding the method.
        if type_info.is_mapping:
            field_class = self.serializer_field_mapping[dict]
        else:
            field_class = self.serializer_field_mapping[list]

So if I understand it correctly one approach to make sets and frozensets work would be to extend the TypeInfo:

@dataclasses.dataclass
class TypeInfo:
    is_many: bool
    is_mapping: bool
    is_set: bool
    is_frozen: bool
    is_final: bool
    is_nullable: bool
    base_type: type

And then we would have to extend the if-else in build_composite_field to handle those types, but wouldn't it even be better to take the type of the dataclass field and put it into the TypeInfo? That way users can use the serializer_field_mapping to add support for more types by themselves.

oxan commented 1 year ago

wouldn't it even be better to take the type of the dataclass field and put it into the TypeInfo? That way users can use the serializer_field_mapping to add support for more types by themselves.

Yeah, that sounds like a good idea.

adonig commented 1 year ago

I committed a version that works for me in my fork of your repo, but it probably needs some polishing.

https://github.com/oxan/djangorestframework-dataclasses/compare/master...adonig:djangorestframework-dataclasses:master

oxan commented 1 year ago

I'd prefer a generic solution that doesn't special case (frozen) sets, such as #69.

adonig commented 1 year ago

Nice 👍

oxan commented 1 year ago

If you can try out #69 and see if it works for you, I'd appreciate that.

adonig commented 1 year ago

Thank you! Works like a charm. 🎉