Repository: graphql-java-kickstart/graphql-java-tools (kotlin)

Issue:
Best practices for handling optional arguments?
We just updated graphql-java-tools from 5.4.1 and the biggest change is around nullable input fields (I think caused by https://github.com/graphql-java-kickstart/graphql-java-tools/pull/315).

Previously, we were defining our resolver methods with `Optional<>` arguments for those that are nullable in the schema, e.g. `first` in 

```graphqls
customers(input: CustomerSearchInput!, first: Int): [Customer!]
```

... mapped to the following resolver method:

```java
public List<Customer> getCustomers(CustomerSearchInput input, Optional<Integer> first) { ... }
```

Previously, the `first` argument was `Optional.empty()` regardless of whether the client passed an explicit `first: null` or didn't pass the argument at all. However, now we are getting `null` when the client doesn't pass the argument, which requires us to handle both `null` and `Optional.empty` in all resolver methods with optional arguments.

Is there a way to opt-out of this new behavior? Or if not, a recommended way to unify handling of `null` and absence? e.g. in the object mapper configurer or something?

Pull request:
<!-- Uncomment one of the following lines as necessary. -->
<!-- Fixes #<ISSUE_NUMBER> -->
Resolves #388

## Checklist
<!-- Change [ ] to [x] to indicate you acknowledge the check. -->
- [x] Pull requests follows the [contribution guide](https://github.com/graphql-java-kickstart/graphql-java-tools/wiki/Contribution-guide)
- [x] New or modified functionality is covered by tests

## Description
<!-- Briefly describe how you resolved the issue or a bug. -->
Changes the default behavior of `Optional` mapping when used as an input parameter.
This is an adjustment to my previous PR on this topic https://github.com/graphql-java-kickstart/graphql-java-tools/pull/310.

Both options are now supported and the behavior is configurable via the boolean switch `inputArgumentOptionalDetectOmission`:
- when `true` an omitted GraphQL argument during query is converted to `null` and an explicit GraphQL `null` is converted to `Optional.empty()` (https://github.com/graphql-java-kickstart/graphql-java-tools/pull/310)
- when `false` both an omitted GraphQL argument and explicit `null` is converted to `Optional.empty()`; therefore the value is never `null`; **this is the new default!**

I am choosing to make this a breaking change and make the second option the default for these reasons:
1. Omission detection never worked in input objects which was confusing. 
2. Omitting arguments is not well supported by various client libraries such as Apollo. It is hard to store reusable query templates when omitting arguments.
3. The expectation is that an `Optional` is never `null`. 

The old behavior will be kept for those users that do not care about the reasons above and want to continue using it. But they will have to opt-in to it by setting `inputArgumentOptionalDetectOmission` to `true`.

Patch:
diff --git a/src/main/kotlin/graphql/kickstart/tools/MethodFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/MethodFieldResolver.kt
index 82fd0b79..54628faa 100644
--- a/src/main/kotlin/graphql/kickstart/tools/MethodFieldResolver.kt
+++ b/src/main/kotlin/graphql/kickstart/tools/MethodFieldResolver.kt
@@ -92,11 +92,10 @@ internal class MethodFieldResolver(
                 }
 
                 if (value == null && isOptional) {
-                    if (environment.containsArgument(definition.name)) {
-                        return@add Optional.empty<Any>()
-                    } else {
+                    if (options.inputArgumentOptionalDetectOmission && !environment.containsArgument(definition.name)) {
                         return@add null
                     }
+                    return@add Optional.empty<Any>()
                 }
 
                 if (value != null
diff --git a/src/main/kotlin/graphql/kickstart/tools/SchemaParserOptions.kt b/src/main/kotlin/graphql/kickstart/tools/SchemaParserOptions.kt
index 8e29112a..c7c9c82b 100644
--- a/src/main/kotlin/graphql/kickstart/tools/SchemaParserOptions.kt
+++ b/src/main/kotlin/graphql/kickstart/tools/SchemaParserOptions.kt
@@ -25,6 +25,7 @@ data class SchemaParserOptions internal constructor(
     val allowUnimplementedResolvers: Boolean,
     val objectMapperProvider: PerFieldObjectMapperProvider,
     val proxyHandlers: List<ProxyHandler>,
+    val inputArgumentOptionalDetectOmission: Boolean,
     val preferGraphQLResolver: Boolean,
     val introspectionEnabled: Boolean,
     val coroutineContextProvider: CoroutineContextProvider,
@@ -50,6 +51,7 @@ data class SchemaParserOptions internal constructor(
         private var allowUnimplementedResolvers = false
         private var objectMapperProvider: PerFieldObjectMapperProvider = PerFieldConfiguringObjectMapperProvider()
         private val proxyHandlers: MutableList<ProxyHandler> = mutableListOf(Spring4AopProxyHandler(), GuiceAopProxyHandler(), JavassistProxyHandler(), WeldProxyHandler())
+        private var inputArgumentOptionalDetectOmission = false
         private var preferGraphQLResolver = false
         private var introspectionEnabled = true
         private var coroutineContextProvider: CoroutineContextProvider? = null
@@ -80,6 +82,10 @@ data class SchemaParserOptions internal constructor(
             this.allowUnimplementedResolvers = allowUnimplementedResolvers
         }
 
+        fun inputArgumentOptionalDetectOmission(inputArgumentOptionalDetectOmission: Boolean) = this.apply {
+            this.inputArgumentOptionalDetectOmission = inputArgumentOptionalDetectOmission
+        }
+
         fun preferGraphQLResolver(preferGraphQLResolver: Boolean) = this.apply {
             this.preferGraphQLResolver = preferGraphQLResolver
         }
@@ -146,9 +152,18 @@ data class SchemaParserOptions internal constructor(
                 genericWrappers
             }
 
-            return SchemaParserOptions(contextClass, wrappers, allowUnimplementedResolvers, objectMapperProvider,
-                proxyHandlers, preferGraphQLResolver, introspectionEnabled, coroutineContextProvider,
-                typeDefinitionFactories, fieldVisibility
+            return SchemaParserOptions(
+                contextClass,
+                wrappers,
+                allowUnimplementedResolvers,
+                objectMapperProvider,
+                proxyHandlers,
+                inputArgumentOptionalDetectOmission,
+                preferGraphQLResolver,
+                introspectionEnabled,
+                coroutineContextProvider,
+                typeDefinitionFactories,
+                fieldVisibility
             )
         }
     }


Test patch:
diff --git a/src/test/kotlin/graphql/kickstart/tools/MethodFieldResolverTest.kt b/src/test/kotlin/graphql/kickstart/tools/MethodFieldResolverTest.kt
index 36535eb7..d2d69bd7 100644
--- a/src/test/kotlin/graphql/kickstart/tools/MethodFieldResolverTest.kt
+++ b/src/test/kotlin/graphql/kickstart/tools/MethodFieldResolverTest.kt
@@ -10,11 +10,101 @@ import org.junit.Test
 import java.lang.reflect.InvocationHandler
 import java.lang.reflect.Method
 import java.lang.reflect.Proxy
+import java.util.*
 
 class MethodFieldResolverTest {
 
     @Test
-    fun shouldHandleScalarTypesAsMethodInputArgument() {
+    fun `should handle Optional type as method input argument`() {
+        val schema = SchemaParser.newParser()
+            .schemaString("""
+                    type Query {
+                        testValue(input: String): String
+                        testOmitted(input: String): String
+                        testNull(input: String): String
+                    }
+                    """
+            )
+            .scalars(customScalarType)
+            .resolvers(object : GraphQLQueryResolver {
+                fun testValue(input: Optional<String>) = input.toString()
+                fun testOmitted(input: Optional<String>) = input.toString()
+                fun testNull(input: Optional<String>) = input.toString()
+            })
+            .build()
+            .makeExecutableSchema()
+
+        val gql = GraphQL.newGraphQL(schema).build()
+
+        val result = gql
+            .execute(ExecutionInput.newExecutionInput()
+                .query("""
+                            query {
+                                testValue(input: "test-value")
+                                testOmitted
+                                testNull(input: null)
+                            }
+                            """)
+                .context(Object())
+                .root(Object()))
+
+        val expected = mapOf(
+            "testValue" to "Optional[test-value]",
+            "testOmitted" to "Optional.empty",
+            "testNull" to "Optional.empty"
+        )
+
+        Assert.assertEquals(expected, result.getData())
+    }
+
+    @Test
+    fun `should handle Optional type as method input argument with omission detection`() {
+        val schema = SchemaParser.newParser()
+            .schemaString("""
+                    type Query {
+                        testValue(input: String): String
+                        testOmitted(input: String): String
+                        testNull(input: String): String
+                    }
+                    """
+            )
+            .scalars(customScalarType)
+            .resolvers(object : GraphQLQueryResolver {
+                fun testValue(input: Optional<String>) = input.toString()
+                fun testOmitted(input: Optional<String>?) = input.toString()
+                fun testNull(input: Optional<String>) = input.toString()
+            })
+            .options(SchemaParserOptions.newOptions()
+                .inputArgumentOptionalDetectOmission(true)
+                .build())
+            .build()
+            .makeExecutableSchema()
+
+        val gql = GraphQL.newGraphQL(schema).build()
+
+        val result = gql
+            .execute(ExecutionInput.newExecutionInput()
+                .query("""
+                            query {
+                                testValue(input: "test-value")
+                                testOmitted
+                                testNull(input: null)
+                            }
+                            """)
+                .context(Object())
+                .root(Object()))
+
+        val expected = mapOf(
+            "testValue" to "Optional[test-value]",
+            "testOmitted" to "null",
+            "testNull" to "Optional.empty"
+        )
+
+        Assert.assertEquals(expected, result.getData())
+    }
+
+    @Test
+    fun `should handle scalar types as method input argument`() {
         val schema = SchemaParser.newParser()
             .schemaString("""
                     scalar CustomScalar
@@ -47,7 +137,7 @@ class MethodFieldResolverTest {
     }
 
     @Test
-    fun shouldHandleListsOfScalarTypes() {
+    fun `should handle lists of scalar types`() {
         val schema = SchemaParser.newParser()
             .schemaString("""
                     scalar CustomScalar
@@ -80,7 +170,7 @@ class MethodFieldResolverTest {
     }
 
     @Test
-    fun shouldHandleProxies() {
+    fun `should handle proxies`() {
         val invocationHandler = object : InvocationHandler {
             override fun invoke(proxy: Any, method: Method, args: Array<out Any>): Any {
                 return when (method.name) {