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

User error implementing PolymorphicProxySerializer #995

Closed
ItsCalebJones opened this issue May 30, 2023 · 13 comments
Closed

User error implementing PolymorphicProxySerializer #995

ItsCalebJones opened this issue May 30, 2023 · 13 comments
Labels
bug Something isn't working enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending

Comments

@ItsCalebJones
Copy link

ItsCalebJones commented May 30, 2023

Describe the bug
Our API uses a query parameter to allow users to swap between three different serializers, this query parameter is not a field of the model or anything of the sort. We then check this in the viewset to see which serializer to use. The schema thats generated seems to only pick the default generator LaunchSerializerCommon. I know this is likely a user error of me simply not understanding how this works, i'm sort of lost right now and any push in any direction would be helpful.

    def get_serializer_class(self):
        mode = self.request.query_params.get("mode", "normal")
        if self.action == "retrieve" or mode == "detailed":
            return LaunchDetailedSerializer
        elif mode == "list":
            return LaunchListSerializer
        else:
            return LaunchSerializerCommon

I've tried setting the @extend_schema as follows:

class LaunchSerializerCommon(serializers.HyperlinkedModelSerializer):
     .....
class LaunchListSerializer(serializers.HyperlinkedModelSerializer):
     .....
class LaunchSerializerCommon(serializers.HyperlinkedModelSerializer):
     .....
     
@extend_schema(
    request=PolymorphicProxySerializer(
        component_name='Launch',
        serializers=[
            LaunchDetailedSerializer, LaunchListSerializer, LaunchSerializerCommon
        ],
        resource_type_field_name="mode",
    ),
...
)
class LaunchViewSet(BaseModelViewSet):
    ....

Expected behavior
This issue reporter RTFM'd and is still confused af, needs more coffee and a helping hand.

@tfranzel
Copy link
Owner

tfranzel commented May 30, 2023

You should get some rest, it's late 😆 Let's start with what is actually going wrong. I see what you are doing, but not what is going wrong exactly!? I have a few thoughts though.

  • why are you using a ModelViewset with list and retrieve if you override both to return both a list and non-list response.
  • you may want to use resource_type_field_name=None, unless you use code generators. Otherwise, your sub-serializers need a discriminator field named mode. That is likely emitting a warning currently.
  • @extend_schema(request=PolymorphicProxySerializer(...)), I think you meant responses and not request
  • You can directly annotate @extend_schema on a view, that is a short-hand to repeat those overrides for all endpoints in that view. People usually annotate the view method or use @extend_schema_view (on the view) for more granular control. Just so you know that is an option.

When trying this out, I think I found a bug, but please elaborate first what your problem is exactly.

@tfranzel
Copy link
Owner

The schema thats generated seems to only pick the default generator LaunchSerializerCommon

Ahh forgot about that. We run get_serializer_class for each action, but are oblivious to the non-standard mode, since we do not inspect method bodies. On schema generation time, there will be "no params" given, so it will always be the default normal

@ItsCalebJones
Copy link
Author

ItsCalebJones commented May 31, 2023

why are you using a ModelViewset with list and retrieve if you override both to return both a list and non-list response.

Requirements 😂 this API was made before GQL was known to the devs - those devs was me, I didn't know about GQL.

The API is a heavily nested object with tons of data per object in the detailed view - it can be stupid big so there's a 'minified' list, a normal list and a detailed view that which some users would die without in the list repsonse.

On schema generation time, there will be "no params" given, so it will always be the default normal

That makes sense.... lets say we were to introduce a breaking change for the next release to solve code generation what would you recommend for an overall re-architecture, new paths so that each serializer is available but also properly reportable through codegen?

normal serializer
/launch/
/launch/<id>

minified serializer
/launch/list

detailed serializer
/launch/detailed/

With the above it would seem we would end up having duplicate paths for specific objects /launch/list/<id>

@tfranzel
Copy link
Owner

tfranzel commented May 31, 2023

Well, the query parameter option seems cleaner from an API perspective, though you have to do more work with annotation.

Having more endpoints will require almost no extra decoration, I think, because everything is covered by the default behavior.

Given your setup, I would probably use PolymorphicProxySerializer(...,resource_type_field_name=None) in conjunction with @extend_schema_view():

reportable through codegen

are you using generated clients (python,js)? Then use resource_type_field_name and add a static resource type field to each sub-serializer. Otherwise generated clients get confused because they don't know which object type they just received.

example:

class LegalPersonSerializer(serializers.ModelSerializer):

@tfranzel
Copy link
Owner

closing this issue for now. feel free to comment if anything is missing or not working and we will follow-up.

@ItsCalebJones
Copy link
Author

Hi there - apologies on reopening it but i'm hitting an error attempting to use the PolymorphicSerializer in the responses field of a extend_schema.

Here's a gist of where I am currently. And here is the error I get:

 File "...\drf_spectacular\utils.py", line 85, in __new__
    instance = cls.many_init(*args, **kwargs)
  File "...\rest_framework\serializers.py", line 149, in many_init
    child_serializer = cls(*args, **kwargs)
TypeError: PolymorphicProxySerializer.__init__() missing 3 required positional arguments: 'component_name', 'serializers', and 'resource_type_field_name'

@tfranzel tfranzel reopened this Jun 20, 2023
@tfranzel
Copy link
Owner

I think I'm starting to understand, but I expected a different error. Are you using an old version by any chance?

@tfranzel
Copy link
Owner

I think you are using a version < 0.25.0, as there should be this error instead:

PolymorphicProxySerializer.__init__() got an unexpected keyword argument 'context'

which is regression in this case. You are seeing the actual issue currently, which is a missing feature. You are using this on a ModelViewSet and for the list method the PolymorphicProxySerializer gets converted to a list, but at that point in time, the args are not available anymore, hence the exception.

If you split your definition and use many=True explicitly on list() this missing functionality is circumvented. Trying to think of a solution currently.

@tfranzel
Copy link
Owner

this should fix the issue. I closed the gap where required parameters were lost in the DRF magic.

@tfranzel tfranzel added bug Something isn't working enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending labels Jun 20, 2023
tfranzel added a commit that referenced this issue Jun 21, 2023
allow implicit list expansion of PolymorphicProxySerializer #995
@ItsCalebJones
Copy link
Author

ItsCalebJones commented Jun 21, 2023

are you using generated clients (python,js)?

To answer this, the idea is that customers or consumers of our data should be able to generate client code using the schema.

Well - I think this looks right, not sure how the client generated code will handle this but that'll be what I start testing.

Schema -> Component
image
Path
image

@tfranzel
Copy link
Owner

You neither confirmed which spectacular version you were using prior nor whether the merged fix solved the issue for you. Instead you chose to answer the the least important side-question from 3 weeks ago. Yes, the schema looks sensible with the limited context I have. Please stay on point, I'm losing interest here.

@ItsCalebJones
Copy link
Author

My apologies - for the original comment I was using version 0.23.0 this latest comment, with no code changes in the gist, I pulled in the changes from #1012 and im no longer getting an issue and the schema appears to be generating properly.

@tfranzel
Copy link
Owner

Excellent! Thank you for clarifying. 👍 Planning to do a release soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 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