add additional runtime errors for pre-coercion OneOf errors

[yaacovCR]

This changes the implementation to match the specification.

Adds additional tests as well.

[netlify[bot]]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	cdd0b55529c994bfac336f25e9d4c09c80f41f33
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/6710ffe3a121e80008a3ac3b
😎 Deploy Preview	https://deploy-preview-4195--compassionate-pike-271cb3.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.

[github-actions[bot]]

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[yaacovCR]

This PR ended up turning into a lot of editing of error messages, but contains the following two cases:

1. { foo: 123, unknownField: 123 }

Assuming unknownField is an unknown field, besides the error for the unknown field, you should also get an error for OneOf, because there is more than one field specified. The query is invalid anyway, so it's kind of not a huge deal.

2. { foo: 123, field: undefined }

Per non-normative graphql-js specific behavior, when the value for field is the JavaScript value undefined, this is equivalent to { foo: 123 } and should be valid.

I should probably extract both of these cases from this PR as two separate small PRs, and then they can be merged to v16.x.x. The diffuse changes to the error messages make this PR difficult to follow.

[benjie]

Assuming unknownField is an unknown field, besides the error for the unknown field, you should also get an error for OneOf, because there is more than one field specified.

Hmmm... Thanks for calling this out! We should only get one error for each mistake. I think when oneOf performs this "one field" check, it's actually checking that exactly one of its fields is set.

From the spec edits:

a special variant of Input Objects where the type system asserts that exactly one of the fields must be set and non-null

and thus requires exactly one of its field be provided

There are some places in the spec where it suggests that it throws an error even if its not one of its fields:

• If the input object literal or unordered map does not contain exactly one entry an error must be thrown.

This is just saying that an error must be thrown, not that this must throw an additional error. An existing error being thrown for an unknown field would be suitable in this case, as clarified through the coercion table:

Literal Value	Variables	Coerced Value
{ b: 123, c: "xyz" }	{}	Error: Unexpected field {c}

This doesn't seem sufficiently clear though, so I'll edit the spec text so that oneof only applies these assertions to its own fields.

[benjie]

Upon further reading, this clarification to the spec edits is not required. The spec already states:

In either case, the input object literal or unordered map must not contain any entries with names not defined by a field of this input object type, otherwise a request error must be raised.

This is the first thing to be stated before visiting any individual rules, thus I think the coercion rules can rely on this assumption.

The coercion table also demonstrates this behavior.

[yaacovCR]

Thanks for exploring that one with me, I think you are right.

I should also call out that there is an additional failure mode added by this PR that actually does not yet have a test case, namely, from the table at https://github.com/graphql/graphql-spec/pull/825

| { a: $a, b: $b } | { a: "abc" } | Error: Exactly one key must be specified |

If I'm not mistaken, the coerced value will just be { a: 'abc' }, and without checking the pre-coercion values, this would pass, but this PR would fail it because it has that second variable pre-coercion => although I don't think I actually added a test for that case in that PR!