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.

[oliemansm]

We'll have to add a (failing) unit test for your particular use case and fix it. Not sure when I have time to look at it though. PRs are also welcome in case you're up for it. Problem seems to be specific to generics now.

[roschlau]

@oliemansm Could you quickly outline what the desired behavior would be in your eyes right now? Exactly what cases should be handled by Jackson, and which should be passed through as-is? I'm thinking a minimum would be "if the input value is a String, and the type of the resolver parameter is not String, try to convert the value with Jackson" (and analogous for List, Optional etc), because I guess everything else will end up in class cast exceptions at runtime? But I might not have the complete context here.

[oliemansm]

When fixing this bug I added this unit test. This verifies the situation where the ID scalar is represented by a Long internally.

The end to end test spec also tests a couple of use cases that are relevant, like the Upload scalar, see

graphql-java-tools/src/test/groovy/graphql/kickstart/tools/EndToEndSpec.groovy

Line 200 in 8b3f431

def "generated schema should handle non nullable scalar types"() {

.

And now you're facing an issue where the ID scalar isn't represented by a simple type, but instead by a parameterized type. And the new method that I added to fix this bug apparently can't cope with that apparently.

In the end most important thing is that it's compliant with the specification. I'd have to dive deeper into scalars to be able to provide sensible input here, and not sure when I have that kind of time. This is becoming pretty complicated now for something seemingly simple. Ideally Jackson would just take care of stuff, but the Upload scalar mapped to Part wasn't compatible apparently.

[Mrman]

I'm encountering this issue with a list of java.util.UUID. As others have stated, singular it works fine.

I'm looking at the source and see special handling for genericised lists of java.lang.* types. What are your thoughts on widening this to accept java.util.UUID? UUID seems like a reasonable type to use for an ID?