You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I currently extensively use the graphql-java-tools library with success.
But I have a use case where I would like to relax/bypass the type checking of objects returned from a resolver.
Let's take a simple example with a graphql service A which has the following resolver:
publicclassFooResolverimplementsGraphQLQueryResolver {
public [MaporJsonNode] foo(Stringid, DataFetchingEnvironmentenv) {
// make graphql call to other service BJsonObjectfoo = callRemoteService()
// here, i don't want to return an umarshalled instance of FooDto, but I want to return either// a json object or a map.returnfoo.
}
The type Foo is declared in the graphql schema B and imported in the schema A. So we have the full type definition.
But in the code, I do not want to have the burden of redeclaring all the java types coming from schema B.
I've been mulling over type checking in this library since i ran into
problems using kotlin co-routines, and the more i think about it the
more it makes sense to relax type checking in this library. It should
be that you can return different data classes for a graphql type in
different field locations, and one Data class should also be
returnable for different graphql types. I think the library should
just check that field names in data classes and resolvers correspond
with the field names on the graphql type and get out of the way.
If I remember correctly, this was the behavior in version 1.x of this library. @apottere
That's really interesting. I subscribe to your point of view. I would be really interested in having one Data class for different graphql types. Ideally, it would be great to have a way to cut the type introspection at some point and let the data go through without any type check.
@oexza That wasn't quite the behavior in 1.x - in 1.x it didn't introspect any types, but it required you to provide a map of all the types you would ever use.
I'm open to removing the introspection (it would make the library a lot simpler), but the two reasons I added it in the first place are:
I wanted all reflection to be done on startup - w/r/t app startup time reflection is fast, but w/r/t the critical path of the application, reflection is slow. If we relax the type checking, that means we'll have to be scanning classes for methods/fields in the critical path. This can be mitigated by caching the discovered field -> method mapping after the first time it's discovered, but that brings me to:
Introspecting types allows the library to do a sanity check on app startup and fail if there are situations it finds won't work, in addition to giving warnings for less serious issues. Since the schema is decoupled from the code, I think this is really valuable (when it works, historically, this has been really difficult to get right). Removing type discovery means that all those errors will be found at request time, and not app startup time.
@apottere
I don't want to completely remove the introspection, I'd just like to bypass the introspection at some point of the model tree, particularly on all fields which are intended to represent an edge.
Of course I still want to benefit from the security of typing, I think it's really valuable too.
I'd also support relaxing type checking.
I'm using an orm library where concrete types of data objects are created at runtime. When these objects are returned from a resolver, graphql-java-tools is unable to map the data object's type to the corresponding gql type, despite the fact that these runtime-generated data objects implement the interface required by the gql type.
That approach sounds reasonable and would satisfy my use-case.
FYI this is the workaround I was using https://github.com/pm-dev/graphql-java-tools/commit/ac43f74e224f7017f6f17cc17cc2e657f9e7b1e9
It's un-ideal because it changes the getType method from O(1) to O(n) where n is the size of types and there's no caching. This approach still verified that the returned data class type is an instance of the class type found at startup, which I liked.
Activity
oexza commentedon Jan 18, 2018
ccamel commentedon Jan 18, 2018
That's really interesting. I subscribe to your point of view. I would be really interested in having one Data class for different graphql types. Ideally, it would be great to have a way to cut the type introspection at some point and let the data go through without any type check.
apottere commentedon Jan 18, 2018
@oexza That wasn't quite the behavior in 1.x - in 1.x it didn't introspect any types, but it required you to provide a map of all the types you would ever use.
I'm open to removing the introspection (it would make the library a lot simpler), but the two reasons I added it in the first place are:
ccamel commentedon Jan 23, 2018
@apottere
I don't want to completely remove the introspection, I'd just like to bypass the introspection at some point of the model tree, particularly on all fields which are intended to represent an edge.
Of course I still want to benefit from the security of typing, I think it's really valuable too.
pm-dev commentedon Jan 24, 2018
I'd also support relaxing type checking.
I'm using an orm library where concrete types of data objects are created at runtime. When these objects are returned from a resolver, graphql-java-tools is unable to map the data object's type to the corresponding gql type, despite the fact that these runtime-generated data objects implement the interface required by the gql type.
apottere commentedon Feb 2, 2018
@oexza @pm-dev what about this approach:
I think that would satisfy both of your requirements, and it would make the library a little simpler to maintain and use.
oexza commentedon Feb 2, 2018
Hi @apottere
Sorry i didn't reply to your previous message, somehow i managed to miss it.
Yes this seems like a plausible middle ground. It will certainly work for my use case.
pm-dev commentedon Feb 2, 2018
That approach sounds reasonable and would satisfy my use-case.
FYI this is the workaround I was using
https://github.com/pm-dev/graphql-java-tools/commit/ac43f74e224f7017f6f17cc17cc2e657f9e7b1e9
It's un-ideal because it changes the
getTypemethod from O(1) to O(n) where n is the size oftypesand there's no caching. This approach still verified that the returned data class type is an instance of the class type found at startup, which I liked.oexza commentedon Mar 21, 2018
Hi @apottere any traction on this??
apottere commentedon Mar 21, 2018
All my spare time has been going to subscriptions in graphql-java-servlet, but this is next on the list.
oexza commentedon May 5, 2018
Hey @apottere friendly reminder. Enjoy the weekend!!
oliemansm commentedon Sep 22, 2018
Andrew seems to be pretty tied up with other projects atm. Any of you guys up for creating a PR for this one?
micheal-hill commentedon Jan 13, 2020
Did this end up going anywhere?