graphql-java-kickstart / graphql-java-tools

A schema-first tool for graphql-java inspired by graphql-tools for JS
https://www.graphql-java-kickstart.com/tools/
MIT License
810 stars 174 forks source link

Issue with map-based resolver #228

Closed jhaber closed 5 years ago

jhaber commented 5 years ago

I'm having an issue with a map-based resolver when the value type is also used elsewhere in the schema. I've pushed a simplified example here: https://github.com/jhaber/graphql-java-tools-issue

The tests fail with:

com.coxautodev.graphql.tools.SchemaClassScannerError: Two different classes used for type Greeting:
- class com.graphql.MapTest$Greeting:
|   return type of method public com.graphql.MapTest$Greeting com.graphql.MapTest$QueryResolver.greetOne()

- class java.lang.Object:
|   type of field greetTwo

The conflict appears to arise because there is a normal resolver method which returns Greeting, but there is also a map-based resolver which returns Greeting. It detects the return type correctly for the normal resolver, but detects java.lang.Object for the map-based resolver, leading to this conflict.

If the type is only used by a map-based resolver, then adding it to the dictionary works around the issue. But this workaround doesn't appear to help if the type is also used by a normal resolver.

Is this behavior expected or am I doing something wrong? Thanks in advance

jhaber commented 5 years ago

Based on poking around a bit it seems like the issue is in PropertyMapResolver: https://github.com/graphql-java-kickstart/graphql-java-tools/blob/5bbc37ebcd2afeca8559eff8a5028b5401b2d744/src/main/kotlin/com/coxautodev/graphql/tools/PropertyMapResolver.kt#L17-L23

This function is looking at the type arguments of the resolver class, when it should be looking at the type arguments of the map interface implemented by that resolver class. I'm not sure if there are utilities in the codebase already for dealing with this, but I know it's something that ClassMate excels at (zero-dep library written by the Jackson folks).

I updated my fork to use ClassMate: https://github.com/graphql-java-kickstart/graphql-java-tools/compare/master...jhaber:master This fixed the test cases that prompted me to open the issue, but oddly broke the test case that was passing before (complains about Error creating bimap of type => class). I wanted to make sure you thought this is the right approach before digging in any further.

jhaber commented 5 years ago

Ignore that last part, it seems that Error creating bimap of type => class is because I used one Java type to represent two different GraphQL types in my simplified example, which obviously blows up when you try to create a bimap. Once I switched that to use two separate Java types it started working.

The confusing part is that the same test passes without my changes to graphql-java-tools, when using the dictionary to point the same Java type at two different GraphQL types: https://github.com/jhaber/graphql-java-tools-issue/blob/41736f5ef87d33b88906771dea447e2124f3f9d9/src/test/java/com/graphql/MapTest.java#L76 Is the dictionary separate from this bimap creation or somehow special-cased?

oliemansm commented 5 years ago

Fixed by #229