Open MarianPalkus opened 7 years ago
@ckimes89 will you find time to look at this PR or shall i merge it?
@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.
@ckimes89 No problem, thanks for your response.
@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?
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 ?
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> {}
.
I agree, that is a much nicer API. I try to change it this way.
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.
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?
The goal of this PR is to improve support of introspection which is needed to use tools (e.g. GraphiQL, apollo-codegen, ...).
Breaking Changes:
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:
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:
Related to #8, #47, #80.
TODOS:
MutationType
is missing"System.Data.Entity.DynamicProxies
in mutation results (GraphQL.Net/Executor.cs:28
): Should this be anExpressionOption
? Separate PR #85GraphQL.Parser.Test.SchemaTest.TestRecursionDepth
fails. Any help is very welcome.This PR is still work-in-progress - reviews, tests/hints related to edge cases and other remarks are very welcome.