HackSoftware / Django-Styleguide

Django styleguide used in HackSoft projects
MIT License
4.99k stars 508 forks source link

Inline serializer doesn't work properly with openapi schema generators (spectacular as example). #105

Closed codeofmagma closed 2 years ago

codeofmagma commented 2 years ago

Hi! I have a problem with OpenAPI documentation, I use drf-spectacular for it and spectacular doesnt take inline serializer, here is an example:

class OutputSerializer(serializers.Serializer):
        id = serializers.IntegerField()
        title = serializers.CharField()
        release_date = serializers.DateField()
        credits = serializers.CharField()
        image_url = serializers.CharField()

        songs = inline_serializer(many=True, fields={
            'id': serializers.IntegerField(),
            'title': serializers.CharField(),
            'credits': serializers.CharField()
        })
        artists = inline_serializer(many=True, fields={
            'id': serializers.IntegerField(),
            'name': serializers.CharField(),
            'slug': serializers.CharField()
        })

And this is how it looks in the docs: image

RadoRado commented 2 years ago

@codeofmagma Hello :wave:

We haven't used https://github.com/tfranzel/drf-spectacular in any of our projects.

I'll take a look.

For the time being, if you heavily rely on the generated schema, better define those inline serializers as classes.

Cheers!

RadoRado commented 2 years ago

@codeofmagma Found the issue (haven't really dived in)

Basically, drf-spectacular (or DRF - not sure how the serializer resolution is done) - is looking for instances with a name.

The current inline_serializer implementation looks like this:

from rest_framework import serializers

def create_serializer_class(name, fields):
    return type(name, (serializers.Serializer, ), fields)

def inline_serializer(*, fields, data=None, **kwargs):
    serializer_class = create_serializer_class(name='', fields=fields)

    if data is not None:
        return serializer_class(data=data, **kwargs)

    return serializer_class(**kwargs)

As you can see, we are passing name="". This seems to be the case.

I changed it to the following:

    serializer_class = create_serializer_class(name='inline_serializer', fields=fields)

And it started working.

I'm not seeing any direct issues coming from that change, so give it a go :+1:

codeofmagma commented 2 years ago

@RadoRado thanks a lot! Checked both solutions, seems working without any issues, yes!

-- P. S. I'm surprised this fast reaction on issue =)

RadoRado commented 2 years ago

@codeofmagma Cheers!

--

We piped GitHub notifications to our Slack channel, so we can react faster.

Otherwise, no one on our team had the habit of checking GitHub issues often enough & replies were extremely slow.

RadoRado commented 2 years ago

Closing this for now. In case there are other problems, feel free to reopen :+1:

Farhaduneci commented 1 year ago

Hello @RadoRado,

I trust this message finds you well. I've followed your work-around on this one, and it seems to work fine. However, it has come to my attention that this solution may encounter challenges when dealing with nested applications of the inline_serializer within my APIs.

Specifically, I have encountered scenarios such as the one outlined below:

products = inline_serializer(
    fields={
        "costs": inline_serializer(
            fields={
                "price": serializers.CharField(),
                "discount": serializers.CharField(),
            },
            many=True,
        ),
    },
    many=True,
)

I've addressed the issue by generating a unique string for each inline-serializer instance, in this case using ULID just for demonstration. The following code snippet showcases the implementation:

def inline_serializer(
    *,
    fields: dict,
    data: dict | None = None,
    **kwargs,
):
    serializer_class = create_serializer_class(
        name=str(ulid.new()),
        fields=fields,
    )

    if data is not None:
        return serializer_class(data=data, **kwargs)

    return serializer_class(**kwargs)

While this approach has successfully mitigated the concern at hand, I acknowledge a sense of uncertainty regarding its alignment with best practices.

I am inclined to seek your esteemed opinion on this matter. Although the implemented solution has yielded positive outcomes thus far, I am open to receiving any general recommendations you may offer pertaining to similar scenarios.

Your insights and guidance would be greatly appreciated. Thank you for your time. :hugs:

RadoRado commented 1 year ago

@Farhaduneci Yep, this seems like a viable approach to me, especially if it solves the problem that you are having :+1:

Again, drf-spectacular seems to be relying on class names, so you need to do something about those names not beeing the same empty string = "".

I'll add a reference to that in the code.

Cheers!

RadoRado commented 1 year ago

It's here - https://github.com/HackSoftware/Django-Styleguide-Example/commit/5df008f37f3b8e3bc3dfa06043c53cb26a13b544

Farhaduneci commented 1 year ago

Perfect 👍