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

Improve introspection support #83

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

Conversation

MarianPalkus
Copy link
Collaborator

@MarianPalkus MarianPalkus commented Aug 27, 2017

The goal of this PR is to improve support of introspection which is needed to use tools (e.g. GraphiQL, apollo-codegen, ...).

Breaking Changes:

  • Types have to be handled a little bit different to match GraphQl types/abstract types. Automatic handling of inheritance as well as multi-level inheritance is not supported anymore Introspection and GraphiQL #47.
    Abstract types (interface and union types) can be defined using:
  • GraphQLSchema.AddUnionType(string name, IEnumerable<IGraphQLType> possibleTypes, Type type = null string description = null)
  • GraphQLSchema.AddInterfaceType<TInterface>(string name = null, string description = null)

Interface implementations can be defined using:

  • GraphQLTypeBuilder.AddInterface<TInterface>(GraphQLTypeBuilder<TContext, TInterface> interfaceTypeBuilder)

Example:

 var characterInterface = schema.AddInterfaceType<ICharacter>();
characterInterface.AddAllFields();

var humanType = schema.AddType<Human>();
humanType.AddAllFields();
humanType.AddInterface(characterInterface);

var droidType = schema.AddType<Droid>();
droidType.AddAllFields();
droidType.AddInterface(characterInterface);

Since those changes are quite extensive, it adds more unit tests (and fixes of minor occurring issues) which makes this PR is so large.

Changes:

  • Improve support of introspection
  • Add unit tests according to graphql-js
  • Add unit tests according to graphql-cats (GraphQL Compatibility Acceptance Tests)
  • Fix issue related to fields of type list of scalar

Related to #8, #47, #80.

TODOS:

  • Update docs (including breaking changes related to inheritance/type unions)
  • Introspection MutationType is missing
  • Handling of "System.Data.Entity.DynamicProxies in mutation results (GraphQL.Net/Executor.cs:28): Should this be an ExpressionOption? Separate PR Add expression option to use base type. #85
  • Make unit tests green: Currently, GraphQL.Parser.Test.SchemaTest.TestRecursionDepth fails. Any help is very welcome.
  • (x) Update example projects: The example projects use published nuget version of graphql-net, so they should be updated after the next release

This PR is still work-in-progress - reviews, tests/hints related to edge cases and other remarks are very welcome.

MarianPalkus and others added 30 commits June 7, 2017 20:08
Auto commit by GitBook Editor
Auto commit by GitBook Editor
@MarianPalkus MarianPalkus changed the title WIP: Full support of introspection Improve introspection support Sep 19, 2017
@MarianPalkus
Copy link
Collaborator Author

@ckimes89 will you find time to look at this PR or shall i merge it?

@chkimes
Copy link
Owner

chkimes commented Sep 27, 2017

@MarianPalkus Hey, sorry for the delay - I've been pretty busy for a while. I'll try to make some time to review this during this week. If I haven't left any comments by the weekend, feel free to merge it. Same goes for the other PR.

@MarianPalkus
Copy link
Collaborator Author

@ckimes89 No problem, thanks for your response.

@chkimes
Copy link
Owner

chkimes commented Oct 1, 2017

@MarianPalkus Thanks for all your hard work on this!

I think I'm missing some context around the WithReturnType method. I see that it has to do with the introduction of union types, but I don't fully understand why it's necessary. Could you explain that?

@MarianPalkus
Copy link
Collaborator Author

Thanks for reviewing and sharing your thoughts.

The GrapQLType of a GraphQLField is resolved lazily when accessing the Type property using its CLRType when it has not been resolved yet.

For fields with an union types of CLRTypes which are unrelated to each other, the field's CLRType might be object.
To my understanding, the resolution of those fields type will not work, because there are might be many GraphQLTypes related to CLRType object.

The idea of WithReturnType is to set the GraphQLType directly for fields with union types (theoretically this can be done for any field, but might not be reasonable) so that it is already resolved.

I had no better idea to solve this problem yet.
We could change the API and replace WithReturnType by another overload of AddField/AddListField to pass the field's resolved GraphQLType. This might be easier to understand. What do you think, @ckimes89 ?

@chkimes
Copy link
Owner

chkimes commented Oct 2, 2017

Ah, I see. So for situations like https://github.com/ckimes89/graphql-net/pull/83/files#diff-45f447cd065642a16ffbb95b53ab29acR118 where you don't have a CLR type that represents our GraphQL type.

I'll think about this one for a bit. I think both the current solution and a new overload with the return type parameter specified are less than ideal since 1) you don't get the benefits of type inference or checking and 2) the results of the schema-building API are now potentially used as inputs to the same API. To clarify on 2, previously it was CLR types and expressions going in, and GraphQL types/fields coming out. This isn't a horrible thing and I'd be willing to accept it given no other options, but it just makes the API a bit more convoluted.

One option to consider would be a special union CLR type, like Union<T1, T2> (and more parameter variants). Then to add a Union type you would just call schema.AddType<Union<T1, T2>>(). In methods that return a Union type, it would require an explicit cast but it may not be too bad. Example:

schema.AddField("search", new {text = ""},
        (db, args) => args.text == "starship"
            ? new Starship() as (Union<Starship, Droid, Human>)
            : (args.text == "droid"
                ? new Droid() as (Union<Starship, Droid, Human>)
                : new Human() as (Union<Starship, Droid, Human>)));

Admittedly it's not the prettiest creation, but that could be somewhat alleviated with a type alias like class SearchResult : Union<Starship, Droid, Human> {}.

@MarianPalkus
Copy link
Collaborator Author

I agree, that is a much nicer API. I try to change it this way.

@chkimes
Copy link
Owner

chkimes commented Oct 2, 2017

I'm also open to any other suggestions. CLR Union types are a big addition, so I want to make sure it's worth it.

Also, I think something like Entity Framework could have problems trying to deserialize into a Union type (which it might try to do since it's part of the expression). There are a number of questions to be answered here.

@MarianPalkus
Copy link
Collaborator Author

Additionally, imho the spec does allow defining two different unions with the same set of object types.
So there might be a 1:N relation between a set of CLRTypes and GraphQL unions.I might be wrong, we should check this.
If this assumption was true, we would need a reference or the name of the union to define a field which uses it, don't we?

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