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

Union types documentation #1426

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ramonwenger
Copy link
Contributor

Adds some documentation and examples to help those struggling with the type resolution of Unions (like I myself was just recently).

As this is my first real contribution to this project I am glad for any pointers or improvements to get my PR accepted without an unnecessary delay. Thanks!

Also add an additional test case, update existing ones, to cover
`resolve_type` and `Meta.possible_types` more
@erikwrede
Copy link
Member

Thanks for the effort!

Shouldn't defining the union work without having to specify possible_types for each ObjectType in the union, just like the current example suggests?

@ramonwenger
Copy link
Contributor Author

Thanks for the effort!

Shouldn't defining the union work without having to specify possible_types for each ObjectType in the union, just like the current example suggests?

Thank you for the question @erikwrede.
I think one of the three methods mentioned is necessary, as I have tried to document here:

This can be achieved by:

  • defining an is_type_of-method on each ObjectType
  • defining the attribute possible_types on the Meta class
  • defining a resolve_type on the Union

I don't think the current example in the documentation works as-is (at least for the Union, may be different for Interfaces), although I'd need to check with a fresh project to be sure.

@ramonwenger
Copy link
Contributor Author

@erikwrede

I just tried this out, I took one of the test methods from the code base and just defined the Unions, without any of the methods mentioned in my update, just like in the current example.
The query fails with an error, telling the user what he has to provide.

Here's the adjusted test I just wrote:

def test_query_union_from_documentation():                                                                                                                                          [420/1163]
    class one_object:                                                                          
        one = 'I am one'                                                                       
                                                                                               
    class two_object:                                                                          
        two = 'I am two'                                                                       
                                                                                               
    class One(ObjectType):                                                                     
        one =  String()                                                                        
                                                                                               
    class Two(ObjectType):                                                                     
        two = String()                                                                         
                                                                                               
    class MyUnion(Union):                                                                      
        class Meta:                                                                            
            types = (One, Two,)                                                                
                                     
    class Query(ObjectType):                                                                   
        unions = List(MyUnion)                                                                 
                                                                                               
        def resolve_unions(self, info):
            return [one_object(), two_object()] 

    hello_schema = Schema(Query)

    executed = hello_schema.execute('{ unions { __typename } }')
    assert not executed.errors
    assert executed.data == {"unions": [{"__typename": "One"}, {"__typename": "Two"}]}

Here's the specific error:

[GraphQLError("Abstract type 'MyUnion' must resolve to an Object type at runtime for field 'Query.unions'. Either the 'MyUnion' type should provide a 'resolve_type' function or each possible
 type should provide an 'is_type_of' function.", locations=[SourceLocation(line=1, column=3)], path=['unions', 0]), GraphQLError("Abstract type 'MyUnion' must resolve to an Object type at runtime for field 'Query.unions'. Either the 'MyUnion' type should provide a 'resolve_type' function or each possible type should provide an 'is_type_of' function.", locations=[SourceLocation(
line=1, column=3)], path=['unions', 1])] 

@erikwrede
Copy link
Member

erikwrede commented Jul 15, 2022

What version of Graphene are you on? This works for me (3.0):

from fastapi import FastAPI
import graphene
from starlette_graphene3 import GraphQLApp, make_playground_handler


class A(graphene.ObjectType):
    a = graphene.String()

class B(graphene.ObjectType):
    b = graphene.String()



class BCD(graphene.Union):
    class Meta:
        types=[A,B]
        name="ABC"

def create_app() -> FastAPI:
    app = FastAPI()

    # graphql endpoint

    class Query(graphene.ObjectType):
        abc = graphene.Field(BCD)

        @classmethod
        def resolve_abc(cls,a,b):
            return A(a="Hi")

    schema = graphene.Schema(query=Query)

    app.mount("/graphql", GraphQLApp(schema, on_get=make_playground_handler()))

    return app


app = create_app()

@erikwrede
Copy link
Member

erikwrede commented Jul 15, 2022

And it should be working just like that due to this method:

if isinstance(instance, ObjectType):

EDIT: I see what you're doing, you're initializing one_object instead of the objecttype while I'm creating a new instance of the ObjectType. These are two separate cases, but both are valid so both should be documented. Because doing it like this is also viable. What do you think?

@ramonwenger
Copy link
Contributor Author

@erikwrede Agreed, and yes, there are different use cases. I have run into problems with this using the Django integration with the example as provided in this repository, after I've started creating some custom types for use in an Union.

I myself would have been glad to have a clearer distinction between using an ObjectType and something "taking the form" of one, with the implications of the latter. Also, I was not quite sure if it's idiomatic or "the right thing" to instance ObjectTypes like you do in your example, or if the other way with plain object is preferred. As I'm now aware, the way to go would probably to just use an ObjectType, but that could also be made a bit clearer in the documentation, in my opinion.

Would you agree to the above, @erikwrede? Or did I make some false assumption somewhere?

@erikwrede
Copy link
Member

Well, docs need a general overhaul so issues like you described with the object types have clear solution.
I like your documentation on unions, but I think it should include the case I mentioned since it is the simplest of all cases not requiring any classmethods or additional fields. If you want to add that, I'll happily merge this PR.
If you want to get involved with more docs overhaul, please contact me on slack so we can discuss things further 🙂

Best
Erik

Copy link
Member

@erikwrede erikwrede left a comment

Choose a reason for hiding this comment

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

See comment

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

Successfully merging this pull request may close these issues.

None yet

2 participants