List of IDs is not resolved correctly

[lonski]

Consider following schema fragment:

type Mutation {
  deleteField(fieldId: ID!): Field!
  deleteFields(fieldIds: [ID!]!): [Field!]!
}

On backend ID is of type Long.

Resolver methods for above mutations expect Long and List<Long> arguments:

public List<FieldDto> deleteField(Long fieldId) 

public List<FieldDto> deleteFields(List<Long> fieldIds) 

On version 5.7.1 everything works correctly.

On version 6.0.0 single ID is resolved correctly to Long, but list of ids [ID] is resolved to List<String> (instead List<Long>)

Here is a screenshot from the debugger:

• version 5.7.1 (Expected behaviour)
  

• version 6.0.0 (Incorrect behaviour)
  

[vojtapol]

This is a regression caused by this PR https://github.com/graphql-java-kickstart/graphql-java-tools/pull/358/files

[roschlau]

I'm having this problem too after upgrading, we're using a custom Id type which also isn't deserialized correctly, causing crashes when on version 6.0.0. Is there a fix in sight?

[vojtapol]

From GraphQL documentation:

ID: The ID scalar type represents a unique identifier, often used to refetch an object or as the key for a cache. The ID type is serialized in the same way as a String; however, defining it as an ID signifies that it is not intended to be human‐readable.

In other words ID scalar is just an alias for String. The fact that it worked was actually caused by a bug that was fixed in the PR linked above. Ideally, we should just throw on startup if you try to map the ID scalar to anything other than String.

However, many users are relying on the bug so we are considering reverting to the old behaviour of the ID scalar or making it configurable.

[roschlau]

The GraphQL specification states that an ID should indeed serialize as a String in the JSON response, but it is more lax when treating input and makes no assumptions about internal representation:

when expected as an input type, any string (such as "4") or integer (such as 4) input value should be coerced to ID as appropriate for the ID formats a given GraphQL server expects.

So, there is no restriction on how the server represents IDs internally, even if they should always serialize to a String. I think it is worthwhile to allow the implementing resolvers to determine their appropriate ID type for themselves - if you have a dedicated custom Id type like we do, which adds better type-safety than the plain String approach, it was pretty cool that GraphQL would simply deserialize that from the input string it got.

If accidentally using other types that are not meant to hold IDs is a concern, then I'd be fine with registering the custom ID type explicitly when building the schema to prevent such cases.

[oliemansm]

I've added a (failing) unit test to verify this behavior on a separate branch (see #379). We can't simply revert, because then the #240 will break again, and that should just work too. That case was actually very similar, since the single Upload scalar worked fine, while it failed when using multiple. Now we see the same behavior for the built-in ID scalar.

The isScalarType method that was changed in #358 to fix the Upload scalar and break the ID scalar is never called for a single input. So simply put if we're able to make the multiple version work the same way is the singular, we should be good in both cases.

Any help to fix the issue is appreciated!

[oliemansm]

Not the cleanest fix, but it works and all unit tests are green again. Basically all scalar types that represent a type that's not part of the java.lang package is returned as is, while the ones from that package are serialized by Jackson.

[roschlau]

The fix doesn't work in our case:

class TestMutationResolver : GraphQLMutationResolver {
    fun test1(ids: List<Id<Any>>): Boolean {
        ids.forEach { println(it.value) } // This works as expected
        return true
    }

    fun test2(ids: List<Id<SomeDomainClass>>): Boolean {
        ids.forEach { println(it.value) } // This doesn't, because ids is a List<String> at runtime, not a List<Id>
        return true
    }
}

It seems like nested generic types with type arguments other than Object still cause problems here. If it helps, our Id class basically looks like this:

data class Id<out T>(val value: String) : Comparable {
    override fun compareTo(other: Id<*>) = value.compareTo(other.value)
}

[oliemansm]

That's because in your case the ID scalar apparently is represented in code by a custom generic Id class. It's unwrapping the entire thing apparently, probably thinking that the ID is represented by a String and therefore using Jackson.

[roschlau]

It's the other way around, actually. I would want our Id type to be deserialized by Jackson, as that works just fine (we're relying on Jackson passing the whole JSON-value to a single parameter constructor by default). However, isScalarType returns true for List<Id<SomeDomainClass>> because it's not from the java.lang package, causing it to be returned as-is, and not handled by Jackson. Is there any way we could arrange to have cases like this still handled by Jackson? Alternatively, I would be perfectly fine if I was able to just override the built-in Id-Scalar with my own implementation which handles the mapping between the serialized value and our wrapper class.

Btw, I also wondered why test1 from my example still works as expected, though - funny enough, the isJavaLanguageType has a quirk that causes it to return true for Id<Any>, Id<String> and any other Id<X> where X is Java type, because for generic classes, it just takes the first type argument and checks that instead. That works for wrappers like List and Optional, but isn't really meaningful if the type parameter doesn't refer to a wrapped type, but something else instead, like with our Id. This is what caused test1 in my example to (accidentally) work as I expected it to, but not test2.