Nested List Input Coercion not matching GraphQL spec

[abhinand-c]

There seems to be a deviation of Nested List Input Coercion in GraphQL library and the GraphQL specification.

The fix for the same is raised in a PR in graphql-python/graphql-core#194

(This was my first time contributing to GraphQL core library, sorry for missing out on the recommended practice of raising a issue and engaging proper discussion here)

---

For a nested List [[Int!]], providing an input value of [1, 2, 3]
Expected Behaviour: should raise validation error
Current Behaviour: coerce value to [ [1], [2], [3] ]

Refer:

• https://spec.graphql.org/October2021/#sec-List.Input-Coercion

[Cito]

I think @abhinand-c is right, the test here contradicts the above spec. Probably because the spec was changed in 2018 and the test followed the older spec of 2016.

[yaacovCR]

TLDR: I imagine the next step is to bring this to the attention of the WG at the next meeting.

The tests were added on 9/27/2018.

The spec change with the above table was made on 5/1/2018.

The explanatory text there reads:

Edit: Clarify list coercion rules (graphql/graphql-spec#436)
This adds some additional copy to list result coercion about handling errors and nullability for items, and clarified copy and new examples for input coercion to better clarify the single-item rule, recursive coercion, and null values.

Fixes graphql/graphql-spec#372

Looking back at graphql/graphql-spec#372, the spec change is attempting to clarify whether providing a single item for a recursive list should yield a nested list, and @leebyron adds that the intention had been that it should.

i.e. the change is meant to clarify the following cases:

For expected type [Int], value 1 will be converted to [1]
For expected type [[Int]], value 1 will be converted to [[1]]

As noted above, the table includes additional cases:

For expect type [Int], value [1, "b", true] will yield Error: Incorrect item value
For expect type [[Int]], value [1, 2, 3] will yield Error: Incorrect item value

That latter case about a nested list where a non-nested list is supplied does not seem to be directly addressed in the spec text, but by the fact that recursion is explicitly mandated by the issue noted above that inspired the spec change and the existing spec text, from the symmetry of the input from all the cases, from the fact that the spec change described itself as merely a clarification of existing behavior, it seems possible that the table has a typo and the cases for non-nested list input and nested list input types should be symmetrical:

For expect type [Int], value [1, "b", true] will yield Error: Incorrect item value
For expect type [[Int]], value [1, "b", true] will yield Error: Incorrect item value

In any case, as the spec text does not at all address the case in the table, either the spec text or the table must be updated.

If we have different options of whether to change this line in the table or change the spec to match the table, we may want to follow the principle of least harm, i.e. let existing implementations' behavior dictate and avoid a change that will break existing operations.

If there are multiple implementations that handle this case differently, and any standardization would break something, we actually have the choice of figuring out whether to fix the spec text or fix the table => I imagine we would favor recursion in this case as well just for consistency and fix the table, although I think the use case is much less likely.

I would love to hear from @leebyron and anybody with working memory of how we got here => I imagine the next step is to bring this to the attention of the WG at the next meeting.

[abhinand-c]

Hi @yaacovCR @Cito
Is there anything that I could do to help, and possibly speed things up?

[yaacovCR]

@abhinand-c are you interested in adding this to the agenda of the next WG meeting this Thursday?

You would add this item to the agenda by opening up a PR with the new item on:
https://github.com/graphql/graphql-wg/blob/main/agendas/2023/06-Jun/01-wg-primary.md

If you can't make that date, you can add it to a later meeting this or next month.

[abhinand-c]

Sure✨, I'll add this to the WG Meeting agenda, and join the meeting.

[yaacovCR]

My summary of the points from the meeting on 2023.06.02 --

1. In a conflict between the spec and the reference implementation, the specification is controlling.
2. Although the spec language appears ambiguous at part with how nested lists are handled, the table is crystal clear, and so we can assume the current version of the spec dictates that this coercion should only happen when no list is provided; the reference implementation is therefore indeed out of sync with the specification and should be changed.
3. @leebyron was not able to make the meeting -- notwithstanding the above, it does pay to touch base with @leebyron regarding any recollections as to original intentions.
4. Notwithstanding 3, performing additional coercions would enable more operations; it is a non-breaking change, and could be a considered a feature; if the use-case is compelling enough, it could be added.
5. Until such a feature-add, after obtaining any clarification from 3., it probably pays to tweak the text surrounding the table in an editorial-type fast-track PR to remove any ambiguity from there.

@abhinand-c et al feel free to add any recollections or correct the above :)

[abhinand-c]

I think you have covered everything @yaacovCR and I hope we can merge this PR once we get feedback from @leebyron

[abhinand-c]

@yaacovCR @leebyron Was there any further discussion on clarifying the spec?

[yaacovCR]

Not that I am aware of.

[yaacovCR]

Tracked at graphql/graphql-spec#1057

[yaacovCR]

Note: this was ultimately changed into the spec as an update to the version presented in the table, so there is no need to update the reference implementation.