Less strict verification of list types

[konrad-garus]

I'm using the library with Spring and CGLIB-based proxies.

I have a mutation class that looks like this:

@Component
public class Mutation {
    public String changeIt(List<Foo> input) {
        // ...
    }
}

It works just fine, until I add something that wraps it in a CGLIB proxy (like @PreAuthorize on the mutation method). Then it blows up with:

Caused by: com.coxautodev.graphql.tools.SchemaClassScannerError: Unable to match type definition (NonNullType{type=ListType{type=TypeName{name='Foo'}}}) with java type (interface java.util.List): Java class is not a List: interface java.util.List
	at com.coxautodev.graphql.tools.TypeClassMatcher.error(TypeClassMatcher.kt:23) ~[graphql-java-tools-3.1.0.jar:na]
	at com.coxautodev.graphql.tools.TypeClassMatcher.match(TypeClassMatcher.kt:60) ~[graphql-java-tools-3.1.0.jar:na]
	at com.coxautodev.graphql.tools.TypeClassMatcher.match$default(TypeClassMatcher.kt:27) ~[graphql-java-tools-3.1.0.jar:na]
	at com.coxautodev.graphql.tools.TypeClassMatcher.match(TypeClassMatcher.kt:53) ~[graphql-java-tools-3.1.0.jar:na]
	at com.coxautodev.graphql.tools.TypeClassMatcher.match(TypeClassMatcher.kt:25) ~[graphql-java-tools-3.1.0.jar:na]
	at com.coxautodev.graphql.tools.SchemaClassScanner.matchTypeToClass(SchemaClassScanner.kt:222) ~[graphql-java-tools-3.1.0.jar:na]
	at com.coxautodev.graphql.tools.SchemaClassScanner.matchTypeToClass$default(SchemaClassScanner.kt:222) ~[graphql-java-tools-3.1.0.jar:na]
	at com.coxautodev.graphql.tools.SchemaClassScanner.handleFieldMethod(SchemaClassScanner.kt:186) ~[graphql-java-tools-3.1.0.jar:na]
	at com.coxautodev.graphql.tools.SchemaClassScanner.scanObjectForDictionaryItems(SchemaClassScanner.kt:175) ~[graphql-java-tools-3.1.0.jar:na]
	at com.coxautodev.graphql.tools.SchemaClassScanner.scanForClasses(SchemaClassScanner.kt:93) ~[graphql-java-tools-3.1.0.jar:na]
	at com.coxautodev.graphql.tools.SchemaParserBuilder.build(SchemaParserBuilder.kt:107) ~[graphql-java-tools-3.1.0.jar:na]

The thing is, without the proxy the method argument obtained with reflection is parameterized List<Foo>. However, the proxy changes it to a plain List.

---

Does the schema actually need such strict verification?

One easy-ish way to solve it could be adding support for arrays. This should not lose type information:

public String changeIt(Foo[] input)

[apottere]

I tried to reproduce this but I couldn't, would you mind providing a sample project, or creating a branch of the example?

It doesn't necessarily need strict validation, no and I might end up adding a system property to disable it.

[apottere]

@konrad-garus is this still an issue? I wasn't able to reproduce it but I'd love to fix it once I can.

[konrad-garus]

See sample app at https://github.com/konrad-garus/graphql-java-tools-bugs.

When you uncomment @Transactional in Mutation (https://github.com/konrad-garus/graphql-java-tools-bugs/blob/master/src/main/java/pl/squirrel/graphqlbug/Mutation.java), it blows up on startup. The root exception looks like:

Caused by: com.coxautodev.graphql.tools.ResolverError: No method found with any of the following signatures (in priority order):
  com.sun.proxy.$Proxy74.saveTodos(~todos [, graphql.schema.DataFetchingEnvironment])
  com.sun.proxy.$Proxy74.getSaveTodos(~todos [, graphql.schema.DataFetchingEnvironment])
	at com.coxautodev.graphql.tools.Resolver.getDataClassMethod(Resolver.kt:76) ~[graphql-java-tools-3.2.0.jar:na]
	at com.coxautodev.graphql.tools.Resolver.getMethod(Resolver.kt:65) ~[graphql-java-tools-3.2.0.jar:na]
	at com.coxautodev.graphql.tools.SchemaClassScanner.scanObjectForDictionaryItems(SchemaClassScanner.kt:192) ~[graphql-java-tools-3.2.0.jar:na]
	at com.coxautodev.graphql.tools.SchemaClassScanner.scanForClasses(SchemaClassScanner.kt:113) ~[graphql-java-tools-3.2.0.jar:na]
	at com.coxautodev.graphql.tools.SchemaParserBuilder.build(SchemaParserBuilder.kt:107) ~[graphql-java-tools-3.2.0.jar:na]
	at pl.squirrel.graphqlbug.Application.graphQLSchema(Application.java:34) [classes/:na]
	at pl.squirrel.graphqlbug.Application$$EnhancerBySpringCGLIB$$7ac6f11e.CGLIB$graphQLSchema$0(<generated>) ~[classes/:na]
	at pl.squirrel.graphqlbug.Application$$EnhancerBySpringCGLIB$$7ac6f11e$$FastClassBySpringCGLIB$$60871ca5.invoke(<generated>) ~[classes/:na]
	at org.springframework.cglib.proxy.MethodProxy.invokeSuper(MethodProxy.java:228) ~[spring-core-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.context.annotation.ConfigurationClassEnhancer$BeanMethodInterceptor.intercept(ConfigurationClassEnhancer.java:358) ~[spring-context-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at pl.squirrel.graphqlbug.Application$$EnhancerBySpringCGLIB$$7ac6f11e.graphQLSchema(<generated>) ~[classes/:na]
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:1.8.0_131]
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[na:1.8.0_131]
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:1.8.0_131]
	at java.lang.reflect.Method.invoke(Method.java:498) ~[na:1.8.0_131]
	at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:162) ~[spring-beans-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	... 49 common frames omitted

There is a proxy created by Spring getting in the way. However, this mechanism is so common that it should just work out of the box.

[konrad-garus]

While we're at it, it would be nice if the "list" types were relaxed on input, query and return types, supporting at least Foo[] and Iterable<Foo>. This would greatly improve interoperability with other libraries, which frequently operate on types different than List.

Things like:

class Query {
    Iterable<Foo> foos(FooId[] ids) { /* ... */ }
}

class Mutation {
    Foo[] createFoos(Iterable<FooInput> inputs) { /* ... */ }
}

[RhysXia]

So how to solve the problem.When I use spring aop on the resolver,I get the same problem.
Or you know the the probable cause of the problem?

[apottere]

It looks like I can unwrap the generated proxy to get the real class, which has the correct method signature:

With that, I could update the scanner to:

1. Treat Iterable<T>, Array<T>, and T[] all as list types
2. If AopUtils and Advised are on the classpath, test all classes for being proxies and unwrap them if possible.

Does that sound like an OK solution?

[konrad-garus]

I think supporting Iterable<T> and T[] is reasonable either way, but it does not fix the problem with proxy for generics (Iterable<T> and List<T>).

The idea with Advised could be a nice fix for Spring (which is pretty popular and all), but shouldn't there be a more generic fix? Not just this one framework/library/implementation?

I noticed one thing. If my class implements an interface:

public class Mutation implements IMutation, GraphQLMutationResolver {
    private final TodoRepository todos;

    public Mutation(TodoRepository todos) {
        this.todos = todos;
    }

    public String saveTodos(List<TodoInput> input) {
        input.stream().map(i -> new Todo(i.getWhat())).forEach(todos::save);
        return "ok";
    }
}

interface IMutation {
    public String saveTodos(List<TodoInput> input);
}

... then resolverProxy.getClass().getInterfaces() includes this interface (IMutation). The error changes, from:

com.coxautodev.graphql.tools.ResolverError: No method found with any of the following signatures (in priority order):
pl.squirrel.graphqlbug.$Proxy74.saveTodos(~todos [, graphql.schema.DataFetchingEnvironment])
pl.squirrel.graphqlbug.$Proxy74.getSaveTodos(~todos [, graphql.schema.DataFetchingEnvironment])

To:

com.coxautodev.graphql.tools.SchemaClassScannerError: Unable to match type definition (ListType{type=TypeName{name='TodoInput'}}) with java type (interface java.util.List): Java class is not a List: interface java.util.List

This seems to be type erasure related.

If the library treated T[] as list (which it should), that could be used as a workaround.

Either, it seems like generics need better support as well. Perhaps an argument annotation to pass the generic type, like @ElementType(TodoInput.class)?

It still sounds somewhat messy, that when dealing with proxies you have to put some extra annotations, have an interface, etc. But I don't see a better solution right now. Make sure the exception thrown when either of these is missing is more informative and points to docs that explain these extra measures you have to take when dealing with proxies?

[apottere]

It doesn't look like a problem with type erasure, since in my screenshot above I was able to get the wrapped class and the method on that class had a signature of List<TodoInput>. The issue is that the proxy doesn't care enough to generate proxy methods with all of the type information, since it doesn't affect the runtime behavior of the proxy.

I could add an annotation, but it does seem pretty messy and so far the only issue with proxies has been with spring proxies. Since that seems fixable, I'd rather fix the spring issue cleanly and only resort to adding an annotation if another case has no solution.

[konrad-garus]

Maybe it is a better approach. Silently make the library do the right thing out of the box, adapting to a popular framework if needed. Rather than make every user do something on their end.

It feels dirty from the standpoint of graphql-java-tools maintenance (should not need to care about Spring, right?), but I do love it as a user!

[apottere]

It's not the cleanest, but as long as it's abstracted from the rest of the code it shouldn't be too big of an issue to maintain. Spring is big enough that adding a few lines of code to handle their special cases is OK. I think as long as I add a way for end-users to add their own proxy handlers then small projects can implement their own workarounds.

[konrad-garus]

Sounds good. Thanks!

[RhysXia]

Based on your way,I add some code in it.

Of course,it's a better way you give a handler interface.I hope I can use the solution soon!
Thank you!

[apottere]

I got it working with the example project you provided:

{
  "data": {
    "saveTodos": "ok"
  }
}

Note I also had to update graphql-java-servlet to latest to match the graphql-java version.

I'll release this soon so you can try it out.