Create ExecutionInput via builder

plm commented 5 years ago

I'm experimenting with GraphQL via Ktor 1.2.3 and Kotlin 1.3.41 using version 0.2.4 of this library. I'm generating schemas using com.expedia:graphql-kotlin:0.6.1 which depends upon com.graphql-java:graphql-java:13.0.

When running this library's code against GraphQL Java version 13.0, an exception is thrown for any query run:

500 Internal Server Error: POST - /graphql
java.lang.AbstractMethodError: Method ktor/graphql/HttpGraphQLError.getErrorType()Lgraphql/ErrorClassification; is abstract
    at ktor.graphql.HttpGraphQLError.getErrorType(HttpGraphQLError.kt)
    at graphql.GraphqlErrorHelper.toSpecification(GraphqlErrorHelper.java:29)
    at graphql.GraphQLError.toSpecification(GraphQLError.java:59)
    at ktor.graphql.FormatResultKt.formatResult(formatResult.kt:25)
    at ktor.graphql.RequestHandler.sendResponse(RequestHandler.kt:114)
    at ktor.graphql.RequestHandler.doRequest(RequestHandler.kt:48)
    at ktor.graphql.RouteKt$graphQL$graphQLRoute$1.invokeSuspend(route.kt:19)
    at ktor.graphql.RouteKt$graphQL$graphQLRoute$1.invoke(route.kt)
    at io.ktor.util.pipeline.SuspendFunctionGun.loop(PipelineContext.kt:268)
...

I found that GraphQL Java's ExecutionInput#transform method is called during it's internal request handling logic and, in version 13.0, this triggers a failed assertion since the Ktor Graphql provided ExecutionInput's CacheControl instance is null.

The null CacheControl check was introduced in version 13.0 in graphql-java/graphql-java#1493. Per the GraphQL Java tests, the tested approach for creating an ExecutionInput is via its builder.

This patch changes ExecutionInput creation to use the builder, which should be compatible with GraphQL Java version 9.0 as used by this library as well as 13.0. The version 13.0 builder provides a default CacheControl instance, thus avoiding the assertion failure.

plm commented 5 years ago

I added another fix to avoid a similar assertion error in DocumentAndVariables due to a null variables map.

This second case may actually be something DocumentAndVariables.Builder could handle as it doesn't validate the builder arguments, but the constructor does when build() is invoked. The workaround in Ktor Graphql is to pass an empty map to ExecutionInput.Builder.

excitement-engineer commented 5 years ago

Thanks so much for the pull requests. I am wondering what the best method is to start supporting newer versions of GraphQL whilst keeping support going for version 9 of GraphQL-Java. I updated the dependency of GraphQL-java in the project from 9.0 to 13.0 and I still got some errors. I am wondering what the best method is to support multiple versions of the library. Perhaps good to create a separate branch for v0.13 that can be kept in sync with the master branch to perform the tests for the different versions of the library. Any ideas on how best to do this?

I really want to maintain a single codebase (I want to avoid having to maintain slightly different versions of the software for each version of GraphQL-Java). Your PR is nice because it follows this philosophy.

rosenk commented 5 years ago

Because of this issue I would not be able to convince my manager to use Kotlin :) I don't have direct dependency to graphql-java in my project. I wander how to downgrade the version to 9.0?

rosenk commented 5 years ago

Never mind, I used this merge request branch and it works

excitement-engineer commented 4 years ago

V1.0.0 of the library was just released. This fixes this issue.

excitement-engineer / ktor-graphql

Create ExecutionInput via builder #4