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

discriminator property not added to components under oneOf with django-rest-polymorphic #885

Closed
Numerlor opened this issue Dec 2, 2022 · 4 comments
Labels
enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending

Comments

@Numerlor
Copy link
Contributor

Numerlor commented Dec 2, 2022

Describe the bug
When the schema for a view using the PolymorphicSerializer from django-rest-polymorphic is generated, it uses the serializer's resource_type_field_name for the discriminator but the property under the same name is not added to the components generated from the sub-serializers.
This causes it to not be included in the schema in swagger/redoc and a mismatch between what the api expects.

To Reproduce
run generation on

from django.db import models
from polymorphic.models import PolymorphicModel
from rest_framework.generics import CreateAPIView
from rest_framework.serializers import ModelSerializer
from rest_polymorphic.serializers import PolymorphicSerializer


class ExampleModel(PolymorphicModel):
    ...

class SubModel1(ExampleModel):
    field1 = models.CharField()

class SubModel2(ExampleModel):
    field2 = models.CharField()


class SubModel1Serializer(ModelSerializer):
    class Meta:
        model = SubModel1
        exclude = ("polymorphic_ctype",)

class SubModel2Serializer(ModelSerializer):
    class Meta:
        model = SubModel2
        exclude = ("polymorphic_ctype",)

class ExampleModelSerializer(PolymorphicSerializer):
    model_serializer_mapping = {
        SubModel1: SubModel1Serializer,
        SubModel2: SubModel2Serializer,
    }

class ExampleView(CreateAPIView):
    queryset = ExampleModel.objects.all()
    serializer_class = ExampleModelSerializer

Expected behavior
string property under the same name as resource_type_field_name added to components generator for polymorphic serializer so tooling picks it up

@tfranzel
Copy link
Owner

tfranzel commented Dec 2, 2022

Yes, that is the intended behavior. It is a carefully crafted trade-off between correct schema and openapi-generator building functioning models. It is working but not 100% correct though.

ExampleModelSerializer(PolymorphicSerializer) injects that resourcetype field. If we would add this field to SubModel1Serializer etc and you use that serializer also as stand-alone, the model would be wrong.

We could come up with a more complicated way of writing the ExampleModelSerializer schema but then either client generators or UI would have issues with it (last time I checked)

Lets take the test example:

https://github.com/tfranzel/drf-spectacular/blob/master/tests/contrib/test_rest_polymorphic.py
https://github.com/tfranzel/drf-spectacular/blob/master/tests/contrib/test_rest_polymorphic.yml

Written like that openapi-generator will assume the field even if the sub-component does not contain it explicitly:

export type Person = { resourcetype: 'legal' } & LegalPerson | { resourcetype: 'natural' } & NaturalPerson | { resourcetype: 'nomadic' } & NomadicPerson;

export function PersonFromJSONTyped(json: any, ignoreDiscriminator: boolean): Person {
    if ((json === undefined) || (json === null)) {
        return json;
    }
    switch (json['resourcetype']) {
        case 'legal':
            return {...LegalPersonFromJSONTyped(json, true), resourcetype: 'legal'};
        case 'natural':
            return {...NaturalPersonFromJSONTyped(json, true), resourcetype: 'natural'};
        case 'nomadic':
            return {...NomadicPersonFromJSONTyped(json, true), resourcetype: 'nomadic'};
        default:
            throw new Error(`No variant of Person exists with 'resourcetype=${json['resourcetype']}'`);
    }
}

So this just works for Person while the sub models NaturalPerson etc are still valid on their own.


If you want to have this 100% correct, I recommend adding a constant field to your sub-serializers:

class SubModel2Serializer(ModelSerializer):
    resourcetype = serializers.SerializerMethodField()

    def get_resourcetype(self, object) -> str:
        return 'SubModel2'

    class Meta:
        model = SubModel2
        exclude = ("polymorphic_ctype",)

@Numerlor
Copy link
Contributor Author

Numerlor commented Dec 2, 2022

Could components be created specifically for the polymorphic serializer with the type included to leave the ones they're using unaffected? Or would that cause confusion with their usage?

Thinking about it, doesn't the usage through PolymorphicProxySerializer have the property in the components as the discriminator should have a field defined on the serializers?

The SerializerMethodField leaves it out of the request schema which is where it's important

@tfranzel
Copy link
Owner

tfranzel commented Dec 2, 2022

Could components be created specifically for the polymorphic serializer with the type included to leave the ones they're using unaffected? Or would that cause confusion with their usage?

That would be possible, but that means duplication and we would need to come up with additional derived component names that again have the potential to collide.

It should not be too difficult to modify PolymorphicSerializerExtension to make this happen but I am not sure we should do it.

Thinking about it, doesn't the usage through PolymorphicProxySerializer have the property in the components as the discriminator should have a field defined on the serializers?

It makes sense for the PolymorphicProxySerializer, as you need to do pre-selection in the POST method anyway. So for the response side, a SerializerMethodField works just fine. For the real PolymorphicSerializer its a slightly different story yes.

The SerializerMethodField leaves it out of the request schema which is where it's important

ah sry you are right. that was from my spotty memory. This was for the PolymorphicProxySerializer case

@tfranzel
Copy link
Owner

tfranzel commented Dec 2, 2022

removed - it was not well thought out given your statements. I have to think about this.

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

No branches or pull requests

2 participants