Issue With @GraphQLId Deserialization to Integer in Queries

[austinarbor]

I think I've found a bug when using @GraphQLId on a query argument

If I have a query

@GraphQLQuery
public MyModel myModel(@GraphQLId @GraphQLNonNull Integer id) {
  var model = new MyModel();
  model.setId(id);
  return model;
}

and send the query

query MyModel($id: ID!) {
    myModel(id: $id) {
        id
    }
}

with the variables

{
  "id": "1"
}

You get the error

{"errors":[{"message":"Exception while fetching data (/myModel) : Expected a value that can be converted to type 'Int' but it was a 'String'","locations":[{"line":2,"column":2}],"path":["myModel"],"extensions":{"classification":"DataFetchingException"}}],"data":{"myModel":null}}

The error comes from https://github.com/leangen/graphql-spqr/blob/master/src/main/java/io/leangen/graphql/metadata/strategy/value/jackson/JacksonValueMapper.java#L90

I could be wrong, but I think even under the stricter typing rules, converting from ID to Number should be valid? The query with variables does pass the initial GraphQL validations and checks before it tries to execute it. I created a sample project with a test to demonstrate the issue

[kaqqao]

Seems like a bug indeed. I think this is due to recent versions of graphql-java being stricter with scalar parsing than before.

[austinarbor]

do you think this should be resolved in graphql-java library or this library? I think at a minimum this library should be calling GraphqlIDCoercing.parseValue instead of GraphqlIntCoercing.parseValue, however GraphqlIDCoercing.parseValue always returns String. Maybe if GraphqlIDCoercing returns a String value, we can then use the jackson/gson mapper to convert to the actual target type?

[kaqqao]

Yes, that sounds right. While graphql-java did stop accepting strings as legal inputs for number literals, in this specific case SPQR never should have tried coercing a string into an int directly anyway...

It's a bit convoluted because Jackson/Gson interfere with scalar parsing logic, and SPQR tries to prevent this as much as possible for inputs that are already scalars (as in your case). But since the user can map arbitrary types to scalars, there's currently no good way to detect that a Java type is a scalar at the point of deserialization reliably. Still, ID is a built-in scalar and at least those should work reliably in any case.

I'm looking into this issue now, will come with something workable for this release.

[kaqqao]

Well, I added a simple check (which should have been there already) that will suffice for the moment. But the entire scalar coercion implementation will have to change soon anyway, because:

a) graphql-java deprecated the methods SPQR is currently using b) SPQR can't reliably tell a Java type is mapped as scalar when deserializing the inputs, so flimsy workarounds like the one I added are needed

and both of these are reasons enough to warrant a rewrite. I opened a separate issue for that: https://github.com/leangen/graphql-spqr/issues/451