Exception is thrown during multipart upload when field initially is not empty.

[vsfedorenko]

Is your feature request related to a problem? Please describe.

I was trying to write a test for the Mutation using an apollo-android client. It has native Upload support [proof].

Their FileUpload class is a simple POJO. The problem is their client doesn't follow https://github.com/jaydenseric/graphql-multipart-request-spec specification fully: instead of passing null placeholders for fields, it passes mime type like on the screenshot below:

And it is a plain string:

The problem is graphql.kickstart.servlet.core.internal.VariableMapper expects this value to be null. Otherwise it throws exception.

Exception:

java.lang.RuntimeException: expected null value when mapping variables.avatar
	at graphql.kickstart.servlet.core.internal.VariableMapper.mapVariable(VariableMapper.java:51) ~[graphql-java-servlet-9.2.0.jar:na]
	at graphql.kickstart.servlet.GraphQLMultipartInvocationInputParser.lambda$null$8(GraphQLMultipartInvocationInputParser.java:109) ~[graphql-java-servlet-9.2.0.jar:na]
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1510) ~[na:na]
	at graphql.kickstart.servlet.GraphQLMultipartInvocationInputParser.lambda$mapMultipartVariables$9(GraphQLMultipartInvocationInputParser.java:109) ~[graphql-java-servlet-9.2.0.jar:na]
	at java.base/java.util.LinkedHashMap.forEach(LinkedHashMap.java:723) ~[na:na]
	at graphql.kickstart.servlet.GraphQLMultipartInvocationInputParser.mapMultipartVariables(GraphQLMultipartInvocationInputParser.java:103) ~[graphql-java-servlet-9.2.0.jar:na]
	at graphql.kickstart.servlet.GraphQLMultipartInvocationInputParser.lambda$getGraphQLInvocationInput$2(GraphQLMultipartInvocationInputParser.java:77) ~[graphql-java-servlet-9.2.0.jar:na]
	at java.base/java.util.Optional.ifPresent(Optional.java:176) ~[na:na]
	at graphql.kickstart.servlet.GraphQLMultipartInvocationInputParser.getGraphQLInvocationInput(GraphQLMultipartInvocationInputParser.java:77) ~[graphql-java-servlet-9.2.0.jar:na]

And there is not any simple solution to tune this process because:

1. It is a very deep internal logic. Most of the classes in the hierarchy are package-scoped => I can't override behavior. The topmost class I can override is AbstractGraphQLHttpServlet, which is an entire servlet. I don't want to do this, its over-engineering to tune only 1 line of code.
2. graphql.kickstart.servlet.core.internal.VariableMapper.mapVariable is static => I can't override it
3. graphql.kickstart.servlet.core.internal.VariableMapper.determineMapper is private => I can't add my own mappers there or override behaviour.

Describe the solution you'd like

I can suggest you add an ability to override this behavior somehow. I don't want an exception to be thrown when these values at multipart fields are not empty initially, but I understand, that some people want it and expect it to be strict. So I don't want this exception to be eliminated — leave it as is.

You can make some setters to provide custom implementation and avoid using static calls because they can't be overridden.

Describe alternatives you've considered

I also will leave reference on that ticket at https://github.com/apollographql/apollo-android project. I hope, they will also make this multipart field optional to follow https://github.com/jaydenseric/graphql-multipart-request-spec fully. Anyway, I think both of you and they should do some steps to make this configurable on both sides.

[vsfedorenko]

Submitted bug at client side: apollographql/apollo-kotlin#2409

[vsfedorenko]

Found workaround

import com.fasterxml.jackson.databind.ObjectMapper
import graphql.kickstart.execution.GraphQLObjectMapper
import graphql.kickstart.execution.GraphQLRequest
import graphql.kickstart.execution.config.ObjectMapperProvider
import graphql.kickstart.execution.error.GraphQLErrorHandler
import java.util.function.Supplier

class GraphQLObjectMapper(objectMapper: ObjectMapper, errorHandler: GraphQLErrorHandler? = null) :
    GraphQLObjectMapper(ObjectMapperProvider { objectMapper }, Supplier { errorHandler }) {

    override fun readGraphQLRequest(text: String?): GraphQLRequest =
        super.readGraphQLRequest(text).apply {
            if (variables.isNotEmpty()) {
                variables = TrickyHashMap(variables)
            }
        }

    private class TrickyHashMap<K, V>(m: MutableMap<K, V>, private val delegate: MutableMap<K, V> = HashMap()) :
        MutableMap<K, V> by delegate {
        override fun put(key: K, value: V): V? {
            this.delegate[key] = value
            return null
        }
    }
}

and then

    @Bean
    fun graphQLObjectMapper(objectMapper: ObjectMapper): GraphQLObjectMapper =
        GraphQLObjectMapper(objectMapper, errorHandler())

[m-o]

Hi guys, this bug is really a bad one. Could anybody look into it?

[oliemansm]

@meiblorn You mention that you understand that some people might want it strict. I'm just wondering what the effect could be if that null check would simply not be done?

        if (null != mapper.set(currentLocation, segmentName, part)) {
          throw new RuntimeException("expected null value when mapping " + objectPath);
        }

Making it configurable and/or overridable will mean considerable effort mainly to support a library that isn't sticking to the specification in the first place. I've noticed they've fixed it on their end in the mean time, so apparently it's not a problem anymore?

[oliemansm]

@meiblorn @m-o As I indicated a month ago it looks like they solved the issue on their end making their library in line with the specification. Can you confirm? Since our implementation is according to specification too, then I'd say we can close this issue.

[oliemansm]

This issue has been fixed client side in release 2.4.0. See also: apollographql/apollo-android#2409