Apollo-android: Kotlin code generation uses Input for types

[zarebski-m]

Is your feature request related to a problem? Please describe.
Kotlin code generation is inconsistent between queries and types. For queries, optional types are using Kotlin nullable types – which is expected. On the other hand, generated _types_ are always using Input wrapper which is a bit cumbersome.

Describe the solution you'd like
I'd expect the GraphQLKompiler to always use Kotlin nullable types for generated code, instead of Input wrapper.

[tasomaniac]

Hi, thanks for taking time to open a PR. You could find more detail about this here: https://github.com/apollographql/apollo-android/issues/1839

We prefer using Input since we need to model the difference between absent vs null value. The practical difference is that null puts the value as null in JSON but absent skips the value completely.

It is unfortunately impossible with nullable types to model this.

[zarebski-m]

I see your point. The need to model absent vs. null is not required for all cases, though; otherwise this ticket wouldn't be here.

How about introducing a configuration property for this behaviour? After looking at #1839 ot seems that I'm not the only one that would benefit from using nullable types.

[martinbonnin]

Digging a bit into this, it looks like there is equivalence between the argument is required and the argument type is non-nullable:

https://spec.graphql.org/June2018/#sec-Required-Arguments

Arguments can be required. An argument is required if the argument type is non‐null and does not have a default value. Otherwise, the argument is optional.

So mapping everything to Kotlin nullable/non-nullable types might work ?

[tasomaniac]

It is more clear when the argument is required. For example, not accepting null as a value. But what about when it is optional? Does it define that present null value and value being absent must be treated equally?

[martinbonnin]

According to the spec,

The argument can be omitted from a field with a nullable argument.

That sounds like the intent of the spec is for null and absent to be treated equally but it could be more explicit indeed.

Do we have an example of a server handling these differently?

[martinbonnin]

Turns out it's explicitly written in the spec that null and absent are not the same:

https://spec.graphql.org/June2018/#sec-Null-Value

For example, these two field calls are similar, but are not identical:

{
  field(arg: null)
  field
}

The first has explicitly provided null to the argument “arg”, while the second has implicitly not provided a value to the argument “arg”. These two forms may be interpreted differently. For example, a mutation representing deleting a field vs not altering a field, respectively. Neither form may be used for an input expecting a Non‐Null type.

The same two methods of representing the lack of a value are possible via variables by either providing the a variable value as null and not providing a variable value at all.

That settles it I guess.

[martinbonnin]

Now there's the question whether we should introduce an option for this. Something like

apollo {
   sendNullForAbsentArguments = true
}

?

At first sight, that sounds a recipe for troubles. I think I'd prefer the callers to be aware of the difference even if it's a bit more cumbersome. But I don't have a strong opinion there.

[tasomaniac]

Thanks for digging deep in the specs. These are good insights.

Btw, to make it little easier, we have toInput extension on nullable types.

I'm not super in favor of flags to change the behavior of the whole Apollo. That's why we went for list of types as an option for sealed class generation recently. Doing this just prevents it for the whole application.

At the same time, most users do not have this distinction. But the point is that if you do, then it will be too cumbursome to go back to having Input type.

[tasomaniac]

We talked with the iOS team too. Especially when updating models in the backend, sending null values explicitly means removing those objects whereas skipping values means no update on those values.

It is an important difference. That's why we will close this issue for now.