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

Fix error of multiple inputs with the same type. When using same seri… #530

Merged

Conversation

Hispar
Copy link
Contributor

@Hispar Hispar commented Oct 8, 2018

…alizer.

Fix #478

@coveralls
Copy link

coveralls commented Oct 8, 2018

Coverage Status

Coverage increased (+0.04%) to 92.755% when pulling 97461d1 on Hispar:bugfix/multiple_model_serializers into b0cba39 on graphql-python:master.

@danpalmer
Copy link
Collaborator

I'm afraid I don't have any experience with Django Rest Framework, so I'm going to opt-out of reviewing this one.

@jkimbo
Copy link
Member

jkimbo commented Mar 31, 2019

I don't have any experience with Django Rest Framework either unfortunately.

@jkimbo jkimbo requested review from BossGrand and zbyte64 and removed request for jkimbo March 31, 2019 11:25
Copy link
Collaborator

@zbyte64 zbyte64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this change require existing code bases to update their client? If I understand correctly the input type label will change.

@Hispar
Copy link
Contributor Author

Hispar commented Apr 3, 2019

Will this change require existing code bases to update their client? If I understand correctly the input type label will change.

I'm not sure, the types are generated every time the schema is created. I have been using this fix several months without problems.

@dani0805
Copy link

We ran into this problem. The pull request solves the problem, but is it necessary to create 2 Input types? would't be more appropriate to reuse the first one that gets created? After all if its the same Serializer, should be also the same serializer input type right?

@Hispar
Copy link
Contributor Author

Hispar commented May 23, 2019

We ran into this problem. The pull request solves the problem, but is it necessary to create 2 Input types? would't be more appropriate to reuse the first one that gets created? After all if its the same Serializer, should be also the same serializer input type right?

The correct way should be to reuse the first type as you are saying. But when I encountered this problem I didn't find the way to do it, so I implemented this workaround meanwhile. I haven't had time to look for a better solution.

@dani0805
Copy link

Yes, I hear you ... we commented the assertion :-)

something like this? (Did not test it as I don't have the project setup)

def convert_serializer_to_input_type(serializer_class):
    cachedType = convert_serializer_to_input_type.cache.get(serializer_class.__name__, None)
    if cachedType:
        return cachedType
    serializer = serializer_class()

    items = {
        name: convert_serializer_field(field)
        for name, field in serializer.fields.items()
    }
    retType = type(
        "{}Input".format(serializer.__class__.__name__),
        (graphene.InputObjectType,),
        items,
    )
    convert_serializer_to_input_type.cache[serializer_class.__name__] = retType
    return retType
convert_serializer_to_input_type.cache = {}

@Hispar
Copy link
Contributor Author

Hispar commented May 23, 2019

Yes, I hear you ... we commented the assertion :-)

something like this? (Did not test it as I don't have the project setup)

def convert_serializer_to_input_type(serializer_class):
    cachedType = convert_serializer_to_input_type.cache.get(serializer_class.__name__, None)
    if cachedType:
        return cachedType
    serializer = serializer_class()

    items = {
        name: convert_serializer_field(field)
        for name, field in serializer.fields.items()
    }
    retType = type(
        "{}Input".format(serializer.__class__.__name__),
        (graphene.InputObjectType,),
        items,
    )
    convert_serializer_to_input_type.cache[serializer_class.__name__] = retType
    return retType
convert_serializer_to_input_type.cache = {}

It could work, at this moment I do not have the project configured to test it, but as soon as I can I will see if it works and I will update the PR accordingly.

Thanks!

@Hispar Hispar force-pushed the bugfix/multiple_model_serializers branch from aa8f1ff to 1e76903 Compare May 24, 2019 07:03
@Hispar
Copy link
Contributor Author

Hispar commented May 24, 2019

@dani0805 I have updated the PR with your solution, I haven't tested it in our projects yet, but the test of the schema works.

@Hispar Hispar force-pushed the bugfix/multiple_model_serializers branch from f20fbc2 to 97461d1 Compare May 24, 2019 08:30
@kimbriancanavan
Copy link
Contributor

This would solve headaches for us. Let's get this merged 👍

@Shehab-Muhammad
Copy link

why no one reviews and merges this, please merge this ASAP

@phalt
Copy link
Contributor

phalt commented Jul 9, 2019

Hey @Shehab-Muhammad please appreciate that this project is worked on my volunteers in their spare time!

@phalt phalt merged commit a2103c1 into graphql-python:master Jul 9, 2019
@Shehab-Muhammad
Copy link

@phalt I appreciate your work, of course. thank you

@jkimbo jkimbo mentioned this pull request Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SerializerMutation creates excess types for fields with same serializer class
10 participants