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

Allow polymorphic serializer to be defined as dotted string in PolymorphicResourceRelatedField #1087

Open
Elawphant opened this issue Sep 12, 2022 · 9 comments
Labels
good first issue Good for newcomers

Comments

@Elawphant
Copy link

PolymorphicResourceRelatedField serializer throws 'str' object has no attribute 'get_polymorphic_types', if the referenced serializer is declared before the referencing serializer.

class ASerializer(ModelSerializer):
    ...

class BSerializer(ModelSerializer):
    A = PolymorphicResourceRelatedField(
        'module.serializers.ASerializer',
    ) # <- this one raises  'str' object has no attribute 'get_polymorphic_types', to avoid it we have to reference the class directly ASerializer

    C = PolymorphicResourceRelatedField(
        'module.serializers.ASerializer', # <- this one does not raise error
    )
    
class CSerializer(ModelSerializer):
    ...

'module.serializers.ASerializer' should be the preferred way of referencing other serializers to avoid circular imports.

@Elawphant Elawphant added the bug label Sep 12, 2022
@sliverc sliverc changed the title PolymorphicResourceRelatedField serializer import throws 'str' object has no attribute 'get_polymorphic_types' Allow polymorphic serializer to be defined as dotted string in PolymorphicResourceRelatedField Sep 12, 2022
@sliverc sliverc removed the bug label Sep 12, 2022
@sliverc
Copy link
Member

sliverc commented Sep 12, 2022

Thanks for reporting. It can be debated whether this is a bug or a feature request. Currently, PolymorphicResourceRelatedField does not support serializer to be defined as dotted string. It would certainly be more consistent to allow it.

PR is very welcome. Code line which I guess needs to be adjused is around this line.

@sliverc sliverc added the good first issue Good for newcomers label Sep 12, 2022
@Elawphant
Copy link
Author

Setting a dynamic import on self.polymorphic_serializer will not work on inter-dependent serializers. SerializerA might have PolymorphicResourceRelatedField to SerializerB and vice versa, which results in circular import anyways. I have no idea how to handle it.
I believe we need to overwrite get_polymorphic_types(), or the ordinary ResourceRelatedField should just handle polymorphism.

@Elawphant
Copy link
Author

What does LazySerializersDict do at L176 in DRFJSONAPI.serializers?

@Elawphant
Copy link
Author

if there was no circular import issue, we could just do

from rest_framework_json_api.serializers import import_class_from_dotted_path

if isinstance(polymorphic_serializer, str):                
    self.polymorphic_serializer = import_class_from_dotted_path(polymorphic_serializer) 
else:
    self.polymorphic_serializer = polymorphic_serializer #allow the plugin to handle other errors with the type

@sliverc
Copy link
Member

sliverc commented Oct 22, 2022

The LazySerializerDict only resolves the serializer from dotted string when accessed the first time. This should also be used in this case, it needs to be checked whether this resolves the issue fully in terms of circular imports though as the polymorphic meta class already accesses the serializer.

@Elawphant
Copy link
Author

Elawphant commented Oct 22, 2022

It does not unfortunately. Since LazySerializerDict requires serializer, instead I used import_class_from_dotted_path. but it fails with circular imports

I tried to extend PolymorphicResourceRelatedField. Here's my code.

class ImprovedPolymorphicResourceRelatedField(PolymorphicResourceRelatedField):
    def __init__(self, polymorphic_serializer, *args, **kwargs):
        if isinstance(polymorphic_serializer, str):
            polymorphic_serializer = import_class_from_dotted_path(polymorphic_serializer)
        super().__init__(polymorphic_serializer, *args, **kwargs)

@sliverc
Copy link
Member

sliverc commented Oct 25, 2022

I haven't dived into this issue enough to really see what is the best approach. What I recommend you to do is to work on a PR with a test which setups the serializer the way you would like it to support it (including recursion and dotted strings).

Then, if you find a solution which makes the test green, great and attach it to the PR. If you do not, simply open a PR with the failing test. This will be helpful to investigate this issue further.

@sliverc
Copy link
Member

sliverc commented May 8, 2024

We want to think about how to continue supporting Django Polymorphic in the future. As this issue is about polymorphic support, I assume you are or have been using Django Polymorphic and DJA together. It would be very valuable if you could add your use case and feedback to our discussion at #1194 so we can make an informed decision. Thanks.

@Elawphant
Copy link
Author

My use case was Django Polymorphic with DJA with Ember.js data, which supports a local ORM with polymorphic model types (soon DSL instead of model classes). In my case polymorphism was helping reduce number of queries to the server for a number of data types which were essentially sub-types of single type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants