[Feature request]: Support non-list variables for list arguments

[Shane32]

Currently this query which would fail validation due to the All Variable Usages Are Allowed rule's explicit validation steps:

type Query {
  dummy (arg: [String]): String
}

query ($arg: String) {
  dummy(arg: $arg)
}

This is because of the text:

Otherwise, if locationType is a list type:

• If variableType is NOT a list type, return false.

However, the following queries are fine because of list input coercion rules:

query {
  dummy(arg: "test")
}

and

query ($arg: [String]) {
  dummy(arg: $arg)
}

# variables: { "arg": "test" }

Because of the text:

If the value passed as an input to a list type is not a list and not the null value, then the result of input coercion is a list of size one, where the single item value is the result of input coercion for the list’s item type on the provided value (note this may apply recursively for nested lists).

I propose modifying the All Variable Usages Are Allowed rule to allow for passing a scalar or input object to a list type (if the child type matches of course), and thereby allowing the query at the top of this feature request. I could write a PR if desired. It would be a fully backwards-compatible feature.

[benjie]

I agree with the need for this; it would make changing a field from a non-list type to a list type a non-breaking change. Currently it's only non-breaking when using explicit values, but with variables it is a breaking change due to validation.

Your proposal to write a PR would be the best way to start - raise the PR with the necessary changes (ideally both against GraphQL-js and against GraphQL-spec, but one or the other is sufficient), and then add it to an upcoming agenda: https://github.com/graphql/graphql-wg/tree/main/agendas/2023 . If you're unable to represent this via WG attendance, let me know and I may be able to represent it on your behalf.

[Shane32]

it would make changing a field from a non-list type to a list type a non-breaking change

I hadn't thought of that, but that's potentially an even more important reason than adding the feature.

As I was thinking about how to write the PR here, I realized that the handling of null would need to be discussed. For literals, the spec currently says that this type of coercion is not allowed for null values -- so null would always be applied at the outermost layer. For variables, we have a few options, with some reasoning behind each:

1. Disallow nullable variable types when coercing to list types
   • Doesn't seem that useful; not backwards compatible for null argument types
2. Apply null/unspecified at the outermost nullable layer
   • null for arguments usually means default/undefined, so the entire argument should be default/undefined if possible
3. Apply null/unspecified at the innermost nullable layer
   • Not sure when this would be desired
4. Require that the nullability of the variable type match the innermost layer's nullability, and always apply the value there
   • Easiest to program and understand, but probably not the most useful
5. Require that the nullability of the variable type match the outermost layer's nullability, and apply null there if null
   • null for arguments usually means default/undefined, so conceptually the entire argument should be default/undefined when the argument is null

We also within GraphQL make a distinction between undefined and an explicit null. So if we assume that an undefined variable uses the argument default, then this is how coercion would work for null across the 4 options:

Arg type	Var type	Value	Option 1	Option 2	Option 3	Option 4	Option 5
[ID]	ID	null	invalid	null	[null]	[null]	null
[ID!]	ID	null	invalid	null	null	invalid	null
[ID]!	ID	null	invalid	[null]	[null]	[null]	invalid
[ID]	ID = null	undefined	invalid	null	[null]	[null]	null
[ID!]	ID = null	undefined	invalid	null	null	invalid	null
[ID]!	ID = null	undefined	invalid	[null]	[null]	[null]	invalid
[ID]	ID	undefined	invalid	(arg default)	(arg default)	(arg default)	(arg default)
[ID!]	ID	undefined	invalid	(arg default)	(arg default)	invalid	(arg default)
[ID]!	ID	undefined	invalid	???	???	???	invalid

Note that depending on which option we select, we may limit the amount of scenarios in which changing a type to a list type is backwards compatible. For instance, with option 5, changing ID to a list must be [ID] or [ID!], whereas with option 4 it must be [ID] or [ID]!. And with option 1, there is no backwards-compatible types to choose from.

We also see here that if options 2/3/4 were selected, coercion would be illogical if no variable were provided, since there is no default argument value for a non-null argument.

So as such, my suggestion is option 5. This means that to make ID to a list type, users can choose from [ID] or [ID!], and will expect null (not [null]) if the variable passed in is null. This also closely matches the behavior of passing null as a literal to [ID] -- it will never coerce to [null]. It supports nullable variables, and passing null is never much different than omitting the variable; you'll always get undefined or null, but never [null].

[benjie]

IMO as much as possible you should be able to replace a value foo: null with a variable: foo: $null and the behavior should be the same. I believe this maps to option 5. This RFC would move us closer towards this value-variable equivalence ideal.

[Shane32]

Option 5 most definitely has the least impact on the GraphQL specification. This is likely a good indicator that it presents the least changes existing codebases.

[Shane32]

I agree with the need for this; it would make changing a field from a non-list type to a list type a non-breaking change. Currently it's only non-breaking when using explicit values, but with variables it is a breaking change due to validation.

Your proposal to write a PR would be the best way to start - raise the PR with the necessary changes (ideally both against GraphQL-js and against GraphQL-spec, but one or the other is sufficient), and then add it to an upcoming agenda: https://github.com/graphql/graphql-wg/tree/main/agendas/2023 . If you're unable to represent this via WG attendance, let me know and I may be able to represent it on your behalf.

A pull request for graphql-spec has been added here:

• Support non-list variables for list arguments #1043

A corresponding pull request for GraphQL.NET has been merged here (support is locked behind an experimental option flag until the spec is finalized):

• Allow nullable variables for non-null arguments that have defaults; allow scalar variable to list types graphql-dotnet/graphql-dotnet#3662

@benjie Feel free to represent this feature at a future WG meeting. (If I can join, I will, but don't wait for me..)

[benjie]

Added to November WG: graphql/graphql-wg#1416

[Shane32]

Summary from November 2023 WG:

Two viewpoints:

• @leebyron Currently once variable coercion is complete, the remaining codebase can use variable values directly, without further coercion rules. With this change, anything that references the variable will now need to check the type and coerce it by wrapping it into a list if needed. This then affects the speed of all operations that use variables, and with adds complexity to the codebase.
• @Shane32 On the other hand, literal scalars coerce to lists, and within a variable, scalars coerce into lists. This existing coercion rule already adds significant complexity to the code, and may cover the majority of requests where scalars are coerced to lists. So the presumption from users is that any type may be changed to a list type of the same type, when in fact this is not the case (for what may be a small minority of requests), leading to unknowingly making a breaking change to their schema.

Actual scenario where this was encountered involved changing a sort-by enum field to a list field to allow for a secondary sort. @benjie and @Shane32 have encountered this issue in production schemas. While not a breaking change if the sort-by was specified as a literal, it breaks requests where it was specified by a variable.

Action items:

• Review any other areas within the spec where type coercion should be reviewed.
• Review actual impact to codebase and/or speed

[benjie]

Relevant spec text stating why non-list to list coercion exists: https://spec.graphql.org/draft/#sel-GAHjBJHABAB8H86F

[benjie]

My opinion: you should always be able to replace a literal with a variable that has the same value, and the meaning of the request should not change. Different coercion for variables vs literals w.r.t. list values breaks this. I'm not aware of anything else that breaks this specifically input coercion concern?