Fix coercion table for list

[benjie]

The spec claims:

Expected Type	Provided Value	Coerced Value
[Int]	[1, 2, 3]	[1, 2, 3]
[Int]	1	[1]
[[Int]]	[1, 2, 3]	Error: Incorrect item value

but this isn't correct. This final line should actually be:

Expected Type	Provided Value	Coerced Value
[[Int]]	[1, 2, 3]	[[1], [2], [3]]

This is the behavior GraphQL.js has already.

Reproduction:

import { GraphQLInt, GraphQLList, GraphQLNonNull, GraphQLObjectType, GraphQLSchema, GraphQLString, graphqlSync, printSchema, validateSchema } from "graphql";

const Query = new GraphQLObjectType({
  name: "Query",
  fields: {
    field: {
      args: {
        arg: {
          type: new GraphQLList(new GraphQLList(GraphQLInt)),
        },
      },
      type: new GraphQLNonNull(GraphQLString),
      resolve(_, { arg }) {
        return JSON.stringify(arg);
      },
    },
  },
});
const schema = new GraphQLSchema({
  query: Query,
});

const result = graphqlSync({
  schema,
  source: /* GraphQL */ `
    query {
      field(arg: [1, 2, 3])
    }
  `,
  variables: {},
});
const errors = validateSchema(schema);
if (errors.length) {
  console.dir(errors);
  process.exit(1);
}
console.log(printSchema(schema));
console.log(JSON.stringify(result, null, 2));

And it follows from the spec text:

When expected as an input, list values are accepted only when each item in the list can be accepted by the list’s item type.

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've fixed this, and added another couple of examples.

I will be following up with a separate PR that fixes another issue in list type coercion; but this should be an easy merge.

[netlify]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	7e13d5c
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/6551eb6c8e536100081f469d
😎 Deploy Preview	https://deploy-preview-1057--graphql-spec-draft.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[benjie]

I think this is actually an editorial change since the text already states recursive, it's just a bug in the examples.

cc @graphql/tsc

[yaacovCR]

relevant:
graphql/graphql-js#3858 (perhaps especially graphql/graphql-js#3858 (comment)) and graphql/graphql-js#3859

This was discussed this at a WG meeting (i do not see notes for that meeting)

As far as I recall:

The consensus from the meeting was that the table was actually controlling in practice in that other implementations (Hot Chocolate?) had followed that explicit lead rather than the somewhat more ambiguous spec text. The thinking was that the single item to list coercion feature was apparently intended to only work when passed on a non-list -- even though it works to coerce to lists of lists in that case -- but never when initially passed a list of wrong order. So in some sense it works recursively, single item => list => list of lists, as the spec text clearly indicates, but only when you start from a non-list value, as the paragraph begins and the table makes clear.

So the resolution of those present was that the safest option forward would be to make an editorial change to the spec text to make it more explicitly match the table (and a bug-fix to graphql-js to remove this behavior? maybe in a major revision) and and then, if desired, re-introduce any additional automatic coercion as a new spec and graphql-js feature.

Importantly, I do not believe @leebyron nor you @benjie were present at the meeting. I think the action item we took away was to check back with @leebyron to see if there was any non-spec-preserved knowledge / recollection around the issue -- that may not have ever happened!

[benjie]

Thanks for the extra detail @yaacovCR; in which case I'll move this back to being an RFC.

Regarding the previous discussion you highlight: the GraphQL spec states (emphasis mine):

Examples in this document are non-normative, and are presented to aid understanding of introduced concepts and the behavior of normative portions of the specification. Examples are either introduced explicitly in prose (e.g. “for example”) or are set apart in example or counter-example blocks [...]

And the text leading into this table is (emphasis mine):

Following are examples of input coercion with various list types and values:

So I believe the correct interpretation is to follow the wording of the spec and to fix the mistake in the example table. Further, the spec text makes a lot more sense from the point of view of how coercion happens in general because it doesn't need to know the context of where it's running (am I inside of a list that's already being coerced?) - just the data and the thing you're coercing it to.

[yaacovCR]

Tagging @abhinand-c explicitly.

[benjie]

Cc @michaelstaib

[Shane32]

I support this PR. Although the spec is confusing at first glance (specifying that list types are not coerced), this is a rule simply to force coercion 'down a level', so it is ** not ** ambiguous whether [1,2,3] gets converted to [[1],[2],[3]] or [[1,2,3]]. If that were not stated, there would be nothing in the specification to distinguish which were correct. It does not prevent coercion from occurring, so-to-speak. This is defined within the same sentence, where it is specified that the rule may be applied recursively -- explicitly allowing the behavior demonstrated in this PR.

Besides fixing the sample, I might suggest adding an additional note to explain this rule and behavior, as it is confusing.

GraphQL.NET coerces lists pursuant to the specification and as demonstrated within this PR, ignoring the sample. I'd be curious what the behavior of graphql.js is.

I'd also like to point out that implementations that do not follow the spec can make this change as a non-breaking change to existing clients. However, changing the specification to disallow these types of coercions would be a breaking change for other implementations.

[benjie]

I totally agree with the desire to clarify this further, and have written up a detailed algorithm which also covers the ambiguity of dealing with variables:

#1058

[abhinand-c]

@benjie For a first time GraphQL developer these coercion for nested list might seem a bit confusion. So, could you also mention the coercion example for [[int]] in case of [1, [2], 3] (If I'm right it's [[1], [2], [3]]) in the table.

[benjie]

@abhinand-c Since this is just a bugfix I'll leave that out of this PR, but I'll add it to #1058 - thanks for the feedback!

[fotoetienne]

👍 I agree the table needs to be fixed

1. Given the reference impl (graphql-js) has had this behaviour all along, the impl is a better reflection of how GraphQL is used
2. Changing the impls to match the table (returning an error) would be a breaking change. This change is moving to be more permissive of inputs and thus non-breaking.

Ship it!

[benjie]

This is seen by the attendees of tonight's WG as an editorial fix. (Further clarifications are in a separate PR #1058)

I'm going to leave this open a couple of weeks, and then merge it unless there's any good reasons not to 👍