#172 Allow TypeReference as InputType

[kaqqao]

As described in #172 this allows TypeReferences to be used as input types, making sure they're properly replaced, in both type and argument definitions.

Worth to note is that it does not add validations that would disallow creating infinitely recursive types, as described in graphql/graphql-spec#189
I need some help/discussion for that. I see a few possible approaches here: add checks GraphQLInputObjectType.Builder#field, so they're ran as the fields are added (not really sure this is possible), add it to GraphQLFieldDefinition.Builder#argument (sounds like it's enough to just run this check for query & mutation roots?), or to GraphQLSchema.Builder#build so they're ran for the entire schema at the end.

[dminkovsky]

Some notes from #34

• I wonder what this means:

because TypeReferences are not always replaced with the real object .. —@andimarek

• Important this PR covers this issue:

can you provide in your PR some unit tests demonstrating that nothing breaks and that input refs cannot mix with output refs? —@danielkwindsor

[kaqqao]

• About GraphQLTypeReference is also a GraphQLInputType. #34

because TypeReferences are not always replaced with the real object .. —@andimarek

This means that in the current code, TypeReferences are only searched for and replaced in the output object fields, so simply adding the interface like in #34 isn't enough, as it would still leave TypeReferences used as arguments or in input object definitions unreplaced. I.e. it doesn't always replaces them, but only in some expected locations.
My PR takes care of this, and makes sure they are properly replaced regardless of where they are used.

• I can (and will) surely add some tests showing it's impossible to mix the two and that the above is true, but not really sure what you meant by demonstrating that nothing breaks... All the current tests are passing...

[dminkovsky]

but not really sure what you meant by demonstrating that nothing breaks... All the current tests are passing...

Yeah, sorry that was confusing. Yes, the existing tests are passing. I meant tests that have not yet been written :). I'll spend time in the next few days (most likely this weekend) writing tests for this PR if you don't get a chance first.

[kaqqao]

I was having trouble understanding how the current tests are organized and how they actually work... didn't have much success to be honest. I have a vague idea where to put tests for this, but could use some pointers.

[dminkovsky]

Oh cool. I just spent some time getting familiar with them too. I have
looked through most of them and am pretty comfortable with Spock
basics (Spock is really nice imo!) When you have some time ping me on
Gitter and we can discuss. Or here on this thread is good too.

среда, 21 сентября 2016 г. пользователь Bojan Tomic написал:

I was having trouble understanding how the current tests are organized and
how they actually work... didn't have much success to be honest. I have a
vague idea where to put tests for this, but could use some pointers.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#204 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AANWZacz8HhreuRXTyzWVnEafrdMpH2bks5qsUG8gaJpZM4J6ZZt
.

[JeSuisNikhil]

Based on this graphql/graphql-spec#189 it seems circular references are allowed just as long as the circularity can be broken. For e.g. (stealing code from the issue)

input Something {
  name: String,
  somethingElse: Something //ok
}

input Something {
  name: String,
  somethingElse: [Something!]! //empty list?
}

Perhaps even this is fine

input Something {
  name: String,
  blahReference: Blah
}

input Blah {
  name: String,
  somethingReference: Something! //OK
}

Just not this

input Something {
  name: String,
  somethingElse: Something! //bad!
}

Or this

input Something {
  name: String,
  blahReference: Blah!
}

input Blah {
  name: String,
  somethingReference: Something! //Bad!
}

Also I think you'll need to add a check to ensure that the type reference is to an instance of GraphQLInputType.

[kaqqao]

Right now, no implementation checks for non-broken recursion. I was asking in the chat about how to implement this and gave some ideas.

I was thinking about adding a check if the reference points to a GraphQLInputType, but since ClassCastException will already be raised if the reference points to the wrong type, I figured an additional check wouldn't add much as it can't be thrown earlier (as type reference at the time it is used, usually points to a type not created yet) nor give a more meaningful exception (ClassCastException is pretty much the correct exception you should get).

[brimworks]

These changes should probably be reverted from the pull request.

[brimworks]

Personally, I'm using this library with Java 1.8, but I'm not sure that requiring Java 1.8 will be acceptable to the maintainer of this project.

[brimworks]

Can we separate out this change regarding extension of the TypeResolver interface? That conflates the original goal: resolving TypeReferences.... I'm also curious about why you need all this extra context information when you implement your TypeResolver.

[brimworks]

I'm willing to help with the change related to supporting TypeReferences as input types, but it appears that this PR also contains changes to add extra context information to the TypeResolver interface (which requires Java 1.8).

...I think @dminkovsky was hoping that the TypeReference as input change would include unit tests, and I think @IamCornholio was hoping that a schema validation check would be added to detect when a circular input type is declared as "non null" (and thus it would be impossible to provide such an input that would be valid).

@kaqqao, if you don't think you address the above two items, I can extract out the TypeReference parts of this pull request and address the above two items as a separate pull request from me, but I don't want to rudely undermine your pull request. Please let me know your plan by Monday, otherwise I'll just open a separate PR that addresses these issues.

Thanks,
-Brian

[kaqqao]

@brimworks Wooow, the past commits were completely unrelated and I did not intend to add them to this PR! Will fix immediately! Only the first commit was intentional. Sorry for the confusion!

Regarding the tests, I can add them, but I need some help as to where to put the check for non-null circular references. I was thinking it should be somewhere at the end, once the schema has been already built. What do you think?
I also absolutely don't mind you taking it over if you'd like to.

And, as to why I need extra context for TypeResolver, I'll provide the rationale in a separate PR for #122. But in short, because of generic type erasure, having the resulting object alone is often not enough to resolve its GraphQL type, as multiple generic types can be mapped to multiple GraphQL types (and if you introduce Java8's type-use annotations that influence the mapping, it gets even more complex) and having extra context can help with disambiguation.

[kaqqao]

Ok, I reverted to the initial state, with just the first commit.

Not sure if I should also re-apply the commits that were adding support for TypeReferences as possible types in GraphQLUnionType and as interface types in GraphQLObjectType. I think at least the latter is a rather important feature, as certain legal recursive types are still impossible to create without it. The former is not as important, but highly desirable as it eliminates the need to handle yet another special case.
I just don't like the way I implemented it. I'll try coming up with something better over the weekend.

[brimworks]

Not sure if I should also re-apply the commits that were adding support for TypeReferences as possible types in GraphQLUnionType and as interface types in GraphQLObjectType.

Yes, I think we need to do that. For implementation, perhaps a minor refactor can be performed, although I'd love to get @dminkovsky's opinion. Specifically, if we introduce this interface:

public interface ResolveTypeReferences {
    public void resolveTypeReferences(SchemaUtil util, Map<String, GraphQLType> typeMap);
}

...then SchemaUtils gets this change:

    void replaceTypeReferences(GraphQLSchema schema) {
        Map<String, GraphQLType> typeMap = allTypes(schema, schema.getDictionary());
        for (GraphQLType type : typeMap.values()) {
            if (type instanceof ResolveTypeReferences) {
                ((ResolveTypeReferences)type).resolveTypeReferences(this, typeMap);
            }
        }
    }

...and then modify the existing (and new places) where replaceTypeReferences is defined so they all implement the ResolveTypeReferences interface.

...this way the logic for resolving the type references can be pushed down into the individual types.

For "bonus points", the SchemaUtil.resolveTypeReference and SchemaUtil.resolveTypeReferences methods could be turned into generic methods so we can avoid all this type casting:

    <T extends GraphQLType> T resolveTypeReference(T type, Map<String, GraphQLType> typeMap) {
        if (type instanceof GraphQLTypeReference || typeMap.containsKey(type.getName())) {
            GraphQLType resolvedType = typeMap.get(type.getName());
            if (resolvedType == null) {
                throw new GraphQLException("type " + type.getName() + " not found in schema");
            }
            return (T)resolvedType;
        }
        if (type instanceof ResolveTypeReferences) {
            ((ResolveTypeReferences) type).replaceTypeReferences(this, typeMap);
        }
        return type;
    }

    <T extends GraphQLType> List<T> resolveTypeReferences(List<T> types, Map<String, GraphQLType> typeMap) {
        List<T> resolvedTypes = new ArrayList<T>();
        for (T type : types) {
            resolvedTypes.add((T)resolveTypeReference(type, typeMap));
        }
        return resolvedTypes;
    }

[brimworks]

I need some help as to where to put the check for non-null circular references. I was thinking it should be somewhere at the end, once the schema has been already built. What do you think?

It seems the GraphQLSchema constructor is the logical place to do this check. For implementation, I'd do what graphql.validation.Validator does for graphql.language.Document. Specifically, it defines a generic "visitor" interface (graphql.validation.AbstractRule) that various rules implement, and if a rule is "broken", then a ValidationError is thrown. However, given that there is only one check being performed, perhaps this is overkill. Maybe @dminkovsky or @IamCornholio have a better suggestion.

[kaqqao]

I definitely agree on the approaches you suggested.

Just to clarify why I said I didn't like my own impl for TypeReferences as possible types in GraphQLUnionType and as interface types in GraphQLObjectType.

GraphQLObjectType holds its interfaces in a List<GraphQLInterfaceType> interfaces, and since TypeReference is a concrete class on the same level as GraphQLInterfaceType there's no way to make one inherit the other. This would mean an extra List<TypeReferences> unresolvedInterfaces would have to be introduced, which complicates List<GraphQLInterfaceType> getInterfaces() as it couldn't return both the resolved and unresolved interfaces.
My impl introduced 2 extra methods List<TypeReference> getUnresolvedInterfaces() and List<GraphQLOutputType> getAllInterfaces() to deal with this, and that's what I found iffy.

At some point I was thinking of making TypeReference an interface which would have one generic impl that would replace the current class, and additional impls that subclass GraphQLInterfaceType, GraphQLObjectType (and whatever other class might need to be replaced by a reference while maintaining their exact type). But my brain started looping endlessly before I got anywhere with this thought 😞

[brimworks]

Yes, I see exactly what you mean @kaqqao !

Perhaps changing the List<GraphQLInterfaceType> to be a List<GraphQLType>. The GraphQLObjectType.getInterfaces() is already making a copy of the list, so might as well make a copy and cast to GraphQLInterfaceType at the same time. By keeping the API the same as it is now (only adding a public Builder withInterfaces(GraphQLTypeReference...) method to the builder, and a singular version too), then we keep things "type safe".

...similar thing with the GraphQLUnionType.

[kaqqao]

@brimworks Wouldn't it make GraphQLObjectType.getInterfaces() kind of dangerous? I mean, inconspicuous looking code like this:

GraphQLObjectType type = newObject()
            .withInterface(anInterface)
            .withInterface(aReference)
            .build();
type.getInterfaces();

would actually break with a ClassCastException without an obvious reason.

[brimworks]

...if it has not been resolved, then you are correct. We could implement getInterfaces() so it throws an UnresolvedTypeException (or something like that) so users of that interface understand that getInterfaces() can only be called after resolving the types.

[brimworks]

Great work @kaqqao ! I'd love to see this change get merged in.

Thanks!
-Brian

[kaqqao]

@brimworks
I think I understood why replaceTypeReferences is not defined on an interface. It would require the method to be public instead of package-private, breaking the immutability 😞, which I think is a big deal, so I decided to leave it as is. Maybe it could go to an abstract class instead... not sure how desirable that would be.

I've now pushed the changes I had on my branch, including ubiquitous support for references. The important detail I found out is that order in which types are discovered is important for correct replacement of references, leading to rather surprising behaviors e.g. if the query/mutation type only uses references, and the real types are provided in the dictionary, they'll never get correctly replaced. For this reason, I changed SchemaUtil.collectTypesForXX to always update references.

Regarding the talk above, I went with the approach of creating dedicated private types for references to Object types, Interface types etc. Seems to maintain type-safety nicely without requiring special case handling, but still feels somewhat iffy.

On the topic of schema validation, I added something more-or-less similar to query validation, alas simpler. It still feels like an overkill, to be honest 😯 It currently only checks for infinitely recursive input types, as expected.

I also added some tests, but I feel I am misunderstanding a lot when it comes to test organization in the project (and it sure does not help that I'm not exactly good with Groovy or know anything about Spock).

All in all, I'm not particularly happy with my code, especially the tests, so please feel 100% welcome to change anything and everything you don't like. I'll be OK with you even completely throwing it away in lieu of your own implementation. Just do pay attention to the changes in SchemaUtil.collectTypesForXX, they're easy to overlook and are IMHO important.

[dminkovsky]

@kaqqao I don't know: I think your code might be fine and this just might be a particularly rough corner of the implementation, with these mutable fields that need to be updated with types that might be recursive and have certain restrictions...

If you and @brimworks feel good enough about this in the sense that it adds spec functionality and doesn't break anything, then we should merge it.

[brimworks]

I don't think the changes should break anything, and they fix much needed functionality, so I'd love to see the changes get merged in.

With that said, if it is important to have "pure" code, then I think all the schema types should essentially hold "type references". To get a concrete type you would pass in a map of type names into concrete types.

However, the above suggestion would break the API, so it would require us to wait for a major version update.

[dminkovsky]

Cool, let's merge this then.

Yes, going forward I think we'll want something like that, that also addresses the other issues with the definition types (hashCode/equals, circularity, validation). That can be for a breaking revision.