oxan / djangorestframework-dataclasses

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

Adding support for dataclass Union field #63

Closed panchock closed 1 year ago

panchock commented 1 year ago

Hi there!

We are starting using this package a lot in our product and it's great! One gap we reached into is support for dataclass union (typing.Union[dataclass1, dataclass2, ...]) as one of the dataclass fields.

Example case:

@dataclass
class A:
    a: int

@dataclass
class B:
    b: int

@dataclass
class Response:
    obj: A | B

class ResponseSerializer(DataclassSerializer)
    class Meta:
        dataclass = Response

I solved this by extending DataclassSerializer and add support using rest-polymorphic. We also use drf-spectacular for openapi scheme and it also supports rest-polymorphic, so all good!

Before I open PR I wanted to check that this feature is useful, I would be happy to get a feedback :)

oxan commented 1 year ago

I'm happy to consider a PR to add support for unions, provided that it doesn't add a hard dependency (i.e. if you don't use unions, you shouldn't need the dependency).

panchock commented 1 year ago

Happy to hear it will be useful. My implementation based on polymorphic serializer so it will require rest-polymorphic packege as requirement. I'll open the PR and we will discuss there.

rooterkyberian commented 1 year ago

@panchock care to share your solution? a "dirty" DataclassSerializer modification (shared through gist or something) will be nice as well if you don't have a time for a proper PR.

johnthagen commented 1 year ago

This would be a really great feature, especially given that it would plug into drf-spectacular.

In order to avoid concerns about other users being required to add a new dependencies, django-rest-polymorphic could be added as a optional dependency:

Something like:

[project.optional-dependencies]
union = ["django-rest-polymorphic"]

And then users could opt-in to this feature/dependency:

pip install djangorestframework-dataclasses[union]
johnthagen commented 1 year ago

I'll open the PR and we will discuss there.

@panchock Were you ever able to open a PR?

panchock commented 1 year ago

I'm planning to open PR soon. @oxan Do I need any permissions to push my branch?

johnthagen commented 1 year ago

@panchock You don't need permissions to open a PR. You will create your own local fork, create a branch there, and then GitHub will help you set up the pull request back to this original repository.

oxan commented 1 year ago

Fixed by #82.