redefine type references api

[andimarek]

Breaking change (Details about the breaking stuff below)
This tries to streamline and simplify how to provide type references (resolved when the schema is built) to a Schema.

A bit background:
I originally restricted the GraphQLTypeReference to be used just for GraphQLOutputTypes, because I thought recursive (circular) type definitions would not be allowed.

This was clarified in graphql/graphql-spec#189, that it is indeed allowed.

After that GraphQLTypeReference was changed to also implemented GraphQLInputType. See PR #172.

PR #172 added also some static factory methods which returned special internal subclasses which would be for example of type GraphQLInterfaceType AND marked as type resolver:

 private static class Reference extends GraphQLInterfaceType implements TypeReference {
        private Reference(String name) {
            super(name, "", Collections.emptyList(), new TypeResolverProxy());
        }
    }
....

 public static Reference reference(String name) {
        return new Reference(name);
    }

This enabled type referenced to be used everywhere, but it added a second way for type references handling:
Instantiate GraphQLTypeReference or use these .references factory methods, when GraphQLTypeReference could not be used.

This is not really a nice API: two very different ways how to achieve the same thing.

This PR removes the static factory methods and makes GraphQLTypeReference useable everywhere: it adds new methods to GraphQLObjectType and GraphQLUnionType so that GraphQLTypeReferences can be provided:

e.g.:

        public Builder withInterface(GraphQLTypeReference reference) {
            assertNotNull(reference, "reference can't be null");
            this.interfaces.add(new TypeOrReference<>(reference));
            return this;
        }

This methods are unfortunately necessary because GraphQLObjectType expects concrete GraphQLInterface instances and there is no interface for GraphQLInterface (a bit ironic). If GraphQLInterface would be an interface we could have provided two different implementations: one interface definition and one type reference.

Breaking changes and migration:
The .reference factory methods in GraphQLInputObject, GraphQLInterfaceType and GraphQLObjectType are removed.

Instead of using them just create a new GraphQLTypeReference with new same argument.

Also the return types of GraphQLObjectType.getInterfaces is now List<GraphQLOutputType> instead of List<GraphQLInterface> and GraphQLUnionType.getTypes() is now List<GraphQLOutputType> instead of List<GraphQLObjectType>. This is because they can return GraphQLTypeReferences before they are resolved.

[andimarek]

Ping @kaqqao (author of PR #172)

[andimarek]

@bbakerman @dminkovsky
I don't think this is a too big change and the migration is straight forward and on the other side we really simplify the API. We already had a question about it: #382

[kaqqao]

TL;DR I suggest you do away with GraphQLTypeReference class instead

My original implementation was akin to this one, but there were specific reasons I went the way I did:

• better type-safety: can't accidentally mix up references to object/interface/input types
• when references and real types are stored separately, calling objectType.getInterfaces() or unionType.getTypes() before the schema has been built would either:
  • give incomplete information (return only real types), which is a very nasty hidden gotcha
  • have to expose objectType.getUnresolvedInterfaces() and leave it to the client to consolidate the two (which IMHO is an ugly, but functional, API)
  • expose an additional method that returns everything as List<TypeOrReference> that can be safely used while the schema is being built (again, IMHO ugly but functional)

Instead, I opted to keep type safety (getInterfaces keeps returning interfaces, no instanceof checks, builder.withInterface() can't be accidentally given a reference to an object or input type), and consistency (all registered interfaces are indeed returned, no gotchas) by introducing the special implementations (which are both interfaces and references) returned by factory methods.

Whatever path you choose to take, I'd strongly advise not to give incomplete view of registered interfaces (or possible object types for unions) to the client during building as this is a surprising behavior (could cause hard-to-notice bugs in existing client code). It is much better to force the client to consolidate the references and real types themselves than not to be able to do this at all and have inconsistent view. Maybe expose an additional method that returns everything as List<TypeOrReference> that can be safely used while the schema is being built?

I'd even go as far as to suggest a completely opposite route: remove GraphQLTypeReference class altogether and enforce type safety everywhere in the same (or similar) vein I did for object interfaces. Maybe have one class with factory methods for all reference types instead of one factory method per class. I'd have done something like this in my original PR but couldn't for compatibility reasons. I'd argue this approach has all the merits: consistent view, type safety, no duplicated withInterface methods, and simple API (no two ways to create a reference).
Or perhaps, do exactly as already mentioned: make GraphQLInterfaceType an interface, with 2 implementations. I can't fully think this one through, but sounds as if it would have similar benefits.

[bbakerman]

I think you should update the README.md and the ReadmeExamples.java to explain this decision.

The original update I made was to stop people failing into the trap of how to type reference.

[bbakerman]

Looks good. I would add a readme example so consumers know how to use it

[bbakerman]

Which import rules you want to run in this library.

I personally find "import all" better but your tooling is not doing that clearly.

[andimarek]

@bbakerman what do you mean with "import all"?

[andimarek]

I must say this part is handled by IDEA and it was not really intentional :-)

[andimarek]

@bbakerman Ok ... I know what you mean: these wildcards imports are caused by wrong idea settings: I never want wildcard imports.

[andimarek]

@kaqqao Thanks for your input!

I think you have a point there: I must agree that I don't like this part either:

public List<GraphQLInterfaceType> getInterfaces() {
        if (this.interfaces == null) {
            return this.tmpInterfaces
                    .stream()
                    .filter(TypeOrReference::isType)
                    .map(TypeOrReference::getType)
                    .collect(Collectors.toList());
        }
          return new ArrayList<>(interfaces);		         
}

I will definitely think about it more, before it will get merged.

About the type safety part: I think the current implementation doesn't provide any, even if it looks like it: GraphQLObject.reference("foo") returns a GraphQLObject instance, but the resolved object could be something totally different. I wanted to tackle this problem in a separate PR.

[andimarek]

@kaqqao After thinking about it more I changed it so that getTypes() and getInterfaces() return now GraphQLTypeReferences before they are resolved. Nothing special filtering any more.

The downside now is that the return type of the Methods needed to be a more generic: e.g.GraphQLOutputType instead GraphQLInterface, because as already mentioned GraphQLTypeReference is not a GraphQLInterface.

[kaqqao]

assertNotNull(interfaces, "unresolvedInterfaces can't be null");
This is a leftover from the first implementation, should be deleted

[andimarek]

thanks

[kaqqao]

This was exactly my approach at some point as well. It is correct (in the sense it always gives correct information), so I'm ultimately fine with it.
I went the other route mainly because I'm not a fan of instanceof checks.

Just out of curiosity I must ask: if these problems stem from the fact the reference API wasn't originally intended to be used the way it is being used now, have you considered redesigning the whole thing i.e. making all problematic types like GraphQLInterfaceType interfaces with 2 implementations, as you mentioned above?

[andimarek]

@kaqqao Yes, I thought about redesigning the whole hierarchy, but my main focus was:

• break as little as possible
• streamline the API

I tried to redesign it in a better/cleaner way, but was just too much for now. So this is the best I came up with and I'm ok with it for now.

Thanks a lot for your feedback!

[bbakerman]

So are we happy to merge this?

[andimarek]

@bbakerman Documentation update is still missing ... after that I will merge it.