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

How to generate an enum of valid resourcetype values for use in schema with django-polymorphic? #958

Closed
lvlgl opened this issue Mar 7, 2023 · 7 comments
Labels
fix confirmation pending issue has been fixed and confirmation from issue reporter is pending

Comments

@lvlgl
Copy link

lvlgl commented Mar 7, 2023

# models.py
class SongMedium(PolymorphicModel):
    title = CharField(...)


class Album(SongMedium):
    pass


class EP(SongMedium):
    pass


class Song(Model):
    title = CharField(...)
    medium = ForeignKey(SongMedium, ...)


# serializers.py
class SongMediumSerializer(PolymorphicSerializer):
    model_serializer_mapping = {
        Album: ...,
        EP: ...,
    }


class SongSerializer(Serializer):
    class Meta:
        fields = ("title", "medium")

    medium = SongMediumSerializer()

Using this structure, I would be able to receive and update SongSerializer.medium.resourcetype. I was not able to generate schema for the resourcetype field though. How could I generate an enumeration of valid resourcetype values for use in the schema?

@axieum
Copy link

axieum commented Mar 8, 2023

I think this is related to the existing issue #885.

I've found that adding -

type = CharField(default="Album", required=False)

..to each serializer with the expected values does not work as it tries to serialize the "type" field to the model.

@axieum
Copy link

axieum commented Mar 8, 2023

I think this is related to the existing issue #885.

I've found that adding -

type = CharField(default="Album", required=False)

..to each serializer with the expected values does not work as it tries to serialize the "type" field to the model.

It appears in the create method you just need to pop the type field before continuing, e.g.

def create(self, validated_data: dict) -> SongMedium:
    validated_data.pop("type", None)  # do not persist type to model
    return super().create(validated_data)

@tfranzel
Copy link
Owner

I have reevaluated this issue. The blocker was that we couldn't just add the field to the sub-serializers because it would have been wrong if they were used individually outside of PolymorphicSerializer. By themselves, they simply don't have that field, because it gets injected.

This is still true, but it looks like we now have more wiggle room due to upstream improvements. We can now create 2 components without too much trouble.

  • openapi-generator seems not to choke anymore on allOf.
  • SwaggerUI properly displays allOf. Unfortunately, it does not present the type choices.
  • Redoc shows the discriminator values in a dropdown selector.

We will now generate an auxiliary component with an attached type field. The original components stay untouched and just get combined. The PolymorphicSerializer will then use auxiliary components.

@lvlgl: Generating an extra Enum for the type choices sounds reasonable but does not make much sense on closer inspection. The choices are already hard-coded into the discriminator. Furthermore, the Enum would not be usable on the sub-serializers, because each one only allows one value. So the Enum would not be used anywhere and thus would be a dangling component. Generating one-value Enums seems to not help much and create new problems.

@tfranzel tfranzel added the fix confirmation pending issue has been fixed and confirmation from issue reporter is pending label Mar 12, 2023
@axieum
Copy link

axieum commented Mar 13, 2023

This is working great! 🚀 Tested with pip install -U git+https://github.com/tfranzel/drf-spectacular.git.

My only suggestion is to insert the discriminator field at the top of the serializer rather than the bottom. Unless you know a way to re-order the serializer fields after the fact?

@tfranzel
Copy link
Owner

awesome @axieum! thx for testing.

How is inserting at the top any different than at the bottom? We gotta do it somewhere, and the next person will "prefer" it the other way round. Can you make an argument for this except preference?

Reordering is not possible afaik, which would be part of the trade-off for this construction.

@axieum
Copy link

axieum commented Mar 13, 2023

Adding the type parameter first makes it more readable in my opinion. As changing the type causes the following (underneath) fields to change.

It's a bit awkward having the type of resource drop-down show at the bottom of the payload in Redoc/Swagger, meaning you have to scroll to the bottom change the type and then scroll back up to see what fields changed.

Does this make sense? I'm happy to provide a screenshot soon.

tfranzel added a commit that referenced this issue Mar 14, 2023
@tfranzel
Copy link
Owner

Amended it. It looks slightly better in redoc but should not make much of a difference in general. It's a small concession we can make.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix confirmation pending issue has been fixed and confirmation from issue reporter is pending
Projects
None yet
Development

No branches or pull requests

3 participants