Fix TypeInfo.getInputType() for custom scalar list literals.

[yuchenshi]

Fix #4512.

Also improves test coverage of TypeInfo and the affected validation rule. See linked issue.

I've noticed an interesting existing behavior: When traversing a list literal, TypeInfo.getInputType() returns the list element type during enter(ListValue). As opposed to, the list type (such as [String]), as some may expect. I've add a comment to document this behavior. I'm assuming we don't want to introduce a breaking change here but a maintainer can let me know if we should treat it as a bug.

The two list literal tests would have been broken without the fix. Other tests are added purely for extra coverage as I don't want to break things as I fix others.

[vercel]

@yuchenshi is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel.

A member of the Team first needs to authorize it.

[linux-foundation-easycla]

• ❌ - login: @yuchenshi / name: Yuchen Shi . The commit (4589344) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[yaacovCR]

It does seem like the getInputType for the itemType should be undefined rather than the wrapping ListType if it's a custom scalar. So I think this does qualify as a bug.

Fixing this does seem to unlock allowing embedding variables inside list literals in positions expecting custom scalars (although only validation demonstrated here).

We do allow embedding variables inside object literals within a position expecting a custom scalars, although that is currently non-specified, but supported behavior within graphql-js, and has been for a long time.

I am not sure why historically we do not currently support allowing variables within list literals passed to custom scalars, as there would seem to be a reasonable parallel there, but I am a bit wary about introducing further non-specified behavior into the implementation without broader discussion from the WG.

@graphql/tsc any thoughts on this?

[yaacovCR]

Ah, I see there is a linked issue basically giving the go ahead from @benjie => i would consider this a bug fix/feature rather than a breaking change, no existing queries will become invalid.

[benjie]

That was the go ahead to essentially investigate/suggest a fix, not the go ahead to merge… just for clarity. Will review in more detail in the new year. Thanks for your work on this, both!

[benjie]

One concern here is lists have fun behaviours in GraphQL; we do auto coercion of value to list (i.e. you can feed 7 into [Int]) and i would want to ensure this doesn’t break under this fix. I think it might, purely because we would process the list first?

[benjie]

Either way, the literal behaviour ought to match the value behaviour. If we accept a list value for a custom scalar but not a list literal then that’s inconsistent and we should figure out what the correct behaviour should be. V17 is a good opportunity to introduce that fix.

[yaacovCR]

@yuchenshi based on the above comment from @benjie, this would be deferred until v17, which currently corresponds to the next branch as the PR base.

Also, a non-emergent reminder re: the CLA.

[yuchenshi]

One concern here is lists have fun behaviours in GraphQL; we do auto coercion of value to list (i.e. you can feed 7 into [Int]) and i would want to ensure this doesn’t break under this fix. I think it might, purely because we would process the list first?

Good point, let me see how I can add a test for this.

Either way, the literal behaviour ought to match the value behaviour. If we accept a list value for a custom scalar but not a list literal then that’s inconsistent and we should figure out what the correct behaviour should be. V17 is a good opportunity to introduce that fix.

Re: what the correct behavior should be, will this be addressed under graphql/graphql-spec#1156 and/or graphql/graphql-spec#1179 ? Does either of these block this PR or the v17 release?

@yuchenshi based on the above comment from @benjie, this would be deferred until v17, which currently corresponds to the next branch as the PR base.

Let me rebase the PR then.

Is there a target date for the v17 release? The linked bug is affecting us in production and even a ballpark estimation helps. I understand this repo doesn't have a fixed release schedule -- I just want to understand if we're quite close to v17 or it's more like months away.

Also, a non-emergent reminder re: the CLA.

I'm working with my employer on the CLA and I'll try to get it done before v17.

[yuchenshi]

One concern here is lists have fun behaviours in GraphQL; we do auto coercion of value to list (i.e. you can feed 7 into [Int]) and i would want to ensure this doesn’t break under this fix. I think it might, purely because we would process the list first?

Good point, let me see how I can add a test for this.

Added coverage in src/execution/__tests__/variables-test.ts. These tests pass without the TypeInfo change and continue to pass after. I'd need another Vercel approval please.

@yuchenshi based on the above comment from @benjie, this would be deferred until v17, which currently corresponds to the next branch as the PR base.

Let me rebase the PR then.

Rebased to next. Feel free to take another look or wait until...

Also, a non-emergent reminder re: the CLA.

I'm working with my employer on the CLA and I'll try to get it done before v17.

Unfortunately, I still don't have an update here. I'm not aware of any blockers right now but legal review tend to get slow these days. I'm committed to push this through since we'd very much like to start contributing to upstream.

Is the v17 release coming up soon? Will we miss the chance and have to wait until v18?