[RFC] Allow singular variables in list locations

[taion]

This is a follow-up to discussion in graphql/graphql-relay-js#20 (comment).

The current coercion rules, for convenience, allow passing in singular literals in positions expecting lists. However, they do not allow passing in singular variables.

This proposal would allow passing in singular variables as well. The coercion rules already ensure reasonable behavior when this happens. Enabling this allows easier evolution of APIs, as it would enable converting singular filtering/ordering fields per the linked issues to plural ones without breaking clients.

cc @dschafer

[dschafer]

CC @leebyron — I can't recall a reason not to do this offhand. Maybe a good discussion for the next WG?

[IvanGoncharov]

@taion You also need to modify coercion of field arguments during execution:
http://facebook.github.io/graphql/June2018/#sec-Coercing-Field-Arguments

Variable values are not coerced because they are expected to be coerced before executing the operation in CoerceVariableValues(), and valid queries must only allow usage of variables of appropriate types.

It's impossible to coerce them before executing the operation since now they can be used in multiple places with and without array coercion:

query ($id: ID){
  getPerson(id: $id) { name }
  getPeople(ids: $id) { name }
}

BTW. I couldn't find coercion algorithm for directive arguments in the spec text.
Maybe it worth to generalize Coercing Field Arguments => Coercing Arguments?

[taion]

@IvanGoncharov Interesting! So the behavior in graphql-js currently doesn't follow the spec? And I imagine the same applies for Ruby, too, since that's what GitHub are using. I'll amend, thanks.

[taion]

Oh, never mind, I misunderstood what CoerceVariableValues was doing.

[IvanGoncharov]

@taion You can't remove variable coercion it's extremely important section.
+ coercing variables on every argument would affect performance.

What I meant in my comment is that with your proposal coercing variable value before executing operation is still required but not sufficient on its own. And that's why you need to add the new step to CoerceVariableValues that will wrap the variable value in List if needed.

[taion]

Per your example in #509 (comment), though, we would need to coerce for every argument, no? getPerson expects an ID, while getPeople expects an [ID]. If we wrap $id above, then it would no longer be valid for getPerson, and at any rate it's not obvious at the point of doing variable coercion just what the intended used argument type would be.

[IvanGoncharov]

@taion For example by spec ID also supports Int as an input and it's coerced to String. So in my example, if I pass { id: 1 } there are following steps:

• query ($id: ID){ coercion so it's become { id: "1" } and pass to ExecuteQuery
• getPeople(ids: $id) coercion so it's become { id: ["1"]} and pass to ResolveFieldValue

So one coercion is per operation to match type in variable definition and second is per arg to match type in argument definition.

[IvanGoncharov]

This dialog helped me to understand why such change is problematic.
Arguments from @taion can be applied to any other type of coercion beyond scalar to list. For example, integers are coerced to floats so you can write

{
  divide(dividend: 10.0, divisor: 3)
}

Does this mean we should support:

query ($value: Int) {
  divide(dividend: 10.0, divisor: $value)
}

Enabling this allows easier evolution of APIs, as it would enable converting singular filtering/ordering fields per the linked issues to plural ones without breaking clients.

You can apply exact same argument saying that it would enable converting Int to Float without breaking clients. Same goes for other conversions including custom scalar.

So I would argue it pretty big change. Moreover, it's changing the semantic of coercion, because right now coercion applied only on user-provided data when it's crossing GraphQL boundaries and become typed.

On the other hand, proposed change basically means that we process client value at operation boundary and it's already strong typed but we also allow to convert it into another type inside the query.

IMHO, this PR is not about extending coercion of user-provided data to typed GraphQL values but instead the proposal to add the first entry to completely new class of "conversion" rules between already typed values inside the query.

Thinking about it, I'm completely ok with changing validation rules but I'm not sure that it's worth to change Execute algorithms to enable this use case.

[taion]

This is bigger than I thought it would be.

The motivation here really is around concerns like graphql/graphql-relay-js#20 (comment).

Especially when using static queries that use variables for these things, such as when using Relay Modern or when using Apollo on iOS, it's pretty difficult to make these sorts of fixes to input types.

There's some real inconvenience here, and it all seems so unnecessary, as it wouldn't be a problem if we were using literals.

Perhaps given the scope, it would be better to wait to discuss this at the next WG meeting.

[taion]

As discussed at the WG, the next step here is to spike an implementation in graphql-js to allow some benchmarking, and to evaluate the incremental change in code complexity.