Support for union types when using buildSchema

[jonbretman]

I'm aware that this is possible with https://github.com/apollographql/graphql-tools but it seems like something which should work out of the box with just this library.

I came up with two ways in which this could work but went for this one as an initial implementation.

Idea 1 (this PR):

For union types a __type field is added to the resolved objects for which the value is a string corresponding to the correct type to use. You can see the test for an example of this.

Idea 2:

A second argument to buildSchema which takes some options one of which could be custom resolveType functions for union types e.g.

buildSchema(schema, {
  resolveType: {
      Fruit(result) {
          // some logic to return correct type
      }
  }
});

This would then get passed through to buildASTSchema and used here instead of the default function which throws an error.

Open to suggestions. I believe this should work for interfaces too but didn't add a test as wanted to get some feedback first.

[IvanGoncharov]

@jonbretman I like the idea 👍
I would suggest renaming __type since this name is already used for different purpose. I think __typename is ideal candidate since it's already used in very similar mechanism.

I also think resolveType and isTypeOf should take priority over __typename so instead of modifying completeAbstractValue it's better to add return value.__typename after this line.

[jonbretman]

Thanks for the feedback @IvanGoncharov. Agree that __typename makes more sense.

I've made some changes to how this works, not exactly what you were suggesting but avoids the if-else-elseif complexity in completeAbstractValue.

[IvanGoncharov]

@jonbretman I think such functionality makes sense for both Union and Interfaces.
How about renaming resolveUnionType to resolveType and use it for both?

[jonbretman]

@IvanGoncharov makes sense. Updated to support Interface types too and added a test for that.

[jonbretman]

@leebyron @wincent any thoughts on this?

[IvanGoncharov]

@jonbretman Just reviewed this PR one more time and notice that you implemented this behavior only for buildASTSchema function and missed extendSchema & buildClientSchema. On this thought, I think this change should leave inside src/execution/execute.js so it will affect any schema no matter how it was built, even if it's built without utility function.

I think type resolvers should be handled the same way as field resolvers. That mean you can pass typeResolver to execute function. And functionality you suggested live as defaultTypeResolver similar to defaultFieldResolver.

[jonbretman]

@IvanGoncharov sorry for the delay in updating this PR. I've now made the changes you suggested ( think). Can you have another look please?

[IvanGoncharov]

@jonbretman NP. Can you do exact same changes as in 956d58b in that case you don't need change signature of GraphQLTypeResolver, plus it will make code more consistent.

[jonbretman]

@IvanGoncharov I'm not sure what you mean by "exact same". I needed to change the signature of GraphQLTypeResolver as it needs the returnType argument for the default implementation.

[IvanGoncharov]

@jonbretman I meant that you should use __typename only as last resort when resolveType and isTypeOf are not available.

In your current implementation you first check presence of __typename and only if it's absent proceed with resolveType. It will trigger bugs in existing servers when __typename is just a filed name unrelated to GraphQL. Moreover if will required additional sanitation of user input to prevent injection of __typename property with malicious intent.

I made a commit on top of your PR which solves above problem and also doesn't require change to GraphQLTypeResolver: IvanGoncharov@497e762
It also make defaultTypeResolver consistent wit .

If you agree with proposed changes please merge this commit into your PR.

[jonbretman]

@IvanGoncharov ok I understand now, I had tried removing the resolveType implementations in buildASTSchema and extendSchema like you did in IvanGoncharov@497e762 but that broke a few existing tests. I can see you just removed those tests 😄

It seems like you are more familiar with the codebase than I am but I'm not sure I agree with what you are suggesting. I think that there should be a sensible default for resolving interface and union types with a generated schema and allowing it to be provided via a __typename property of the result seems pretty reasonable. In addition to this, being able to provide a custom type resolver to execute may also be useful but I would suggest this might be something to consider later down the line if it's something people want e.g. Y.A.G.N.I.

I've updated this PR with something that supports both buildASTSchema and extendSchema and doesn't break any existing tests. I have not supported buildClientSchema as the docs for the function state that the result "cannot be used to execute a query" so it seems like it shouldn't matter.

@IvanGoncharov I appreciate you taking the time to help review this and I'd love to see something get merged that adds this support. Hopefully we can get something that everyone is happy with.

@leebyron / @wincent any thoughts on this?

[IvanGoncharov]

@jonbretman Got carried away 😄 Patching buildASTSchema and extendSchema is also enough for my use case.

In theory, it's better to have same runtime behavior for schemas created by calling GraphQLSchema constructor. But I agree that manually assigning resolveTypes after you build schema with buildASTSchema causing the most pain.

Would be great if @leebyron comment on this issue so we know which direction to go.

[jonbretman]

@leebyron I'd still like to try to get this moved forward if possible - any thoughts?

[leebyron]

Thanks for this great work and your patience. I really like this idea though I think it can be generalized further - no need to limit this to only generated schemas instead of explicitly defined ones.

I'll amend this PR to generalize the concept and remove some cleaned up code in the process!

[jonbretman]

@leebyron no worries, thanks for getting this landed 👍