add additional runtime errors for pre-coercion OneOf errors #4195

yaacovCR · 2024-09-18T13:00:57Z

This changes the implementation to match the specification.

Adds additional tests as well.

netlify · 2024-09-18T13:01:13Z

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

Name	Link
🔨 Latest commit	`5feb200`
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/66eb32a228af7b0008abe77e
😎 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 · 2024-09-18T13:01:24Z

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

src/execution/__tests__/oneof-test.ts

benjie

Thanks for your work on this! It seems right; but let's improve the error messages

benjie · 2024-09-18T16:16:59Z

src/execution/__tests__/oneof-test.ts

+            // A nullable variable in a oneOf field position would be caught at validation-time
+            // hence the vague error message here.
+            message:
+              'Argument "input" of type "TestInputObject!" has invalid value { a: $a, b: $b }.',


I understand the errors are vague and why; but the { a: $a } error messages below are confusing. Maybe it would be better to replace all the errors with:

Argument "input" of type "TestInputObject!" has invalid value - exactly one field must be specified, and the value for that field must not be `null`.

I updated the OneOf error messages to something uniform, but the vague ones are a result of the uniform behavior within getArgumentValues()

graphql-js/src/execution/values.ts

Lines 222 to 238 in 7875552

const coercedValue = coerceInputLiteral(

valueNode,

argType,

variableValues,

fragmentVariables,

);

if (coercedValue === undefined) {

// Note: ValuesOfCorrectTypeRule validation should catch this before

// execution. This is a runtime check to ensure execution does not

// continue with an invalid argument value.

throw new GraphQLError(

`Argument "${name}" of type "${inspect(

argType,

)}" has invalid value ${print(valueNode)}.`,

{ nodes: valueNode },

);

}

which does not transmit the original error (but would be caught at validation time).

Within #3813 , part of the rebased Default Value Coercion/Validation stack, @leebyron has fixed this to provide more informative errors.

I think another way of saying what I am saying that all of the tests within the oneOf test file, including the new ones, actually belong within the tests for coerceInputValue() and coerceInputLiteral() (formerly known as valueFromAST()).

I'll set about to do that. Then these tests won't have opaque error messages in our test files that make it difficult to understand what exactly is being tested, although users who choose not to validate will still get opaque errors, at least until #3813 lands.

src/execution/__tests__/oneof-test.ts

benjie

Just being picky over capitalisation now...

Do we generally capitalise Object Type / Interface Type and so on in error messages? If so, the suggestions to reduce capitalization should be dismissed.

benjie · 2024-09-19T08:53:47Z

src/validation/rules/ValuesOfCorrectTypeRule.ts

@@ -193,7 +193,7 @@ function validateOneOfInputObject(
  if (isNotExactlyOneField) {
    context.reportError(
      new GraphQLError(
-        `OneOf Input Object "${type}" must specify exactly one key.`,
+        `Within OneOf Input Object Type "${type}", exactly one field must be specified, and the value for that field must be non-null.`,


Suggested change

`Within OneOf Input Object Type "${type}", exactly one field must be specified, and the value for that field must be non-null.`,

`Within OneOf Input Object type "${type}", exactly one field must be specified, and the value for that field must be non-null.`,

benjie · 2024-09-19T08:54:54Z

src/validation/rules/ValuesOfCorrectTypeRule.ts

-        nodes: [node],
-      }),
+      new GraphQLError(
+        `Within OneOf Input Object Type "${type}", exactly one field must be specified, and the value for that field must be non-null.`,


Suggested change

`Within OneOf Input Object Type "${type}", exactly one field must be specified, and the value for that field must be non-null.`,

`Within OneOf input object type "${type}", exactly one field must be specified, and the value for that field must be non-null.`,

benjie · 2024-09-19T08:55:08Z

src/validation/rules/ValuesOfCorrectTypeRule.ts

@@ -221,7 +224,7 @@ function validateOneOfInputObject(
    if (isNullableVariable) {
      context.reportError(
        new GraphQLError(
-          `Variable "$${variableName}" must be non-nullable to be used for OneOf Input Object "${type}".`,
+          `Variable "$${variableName}" must be non-nullable to be used for Within OneOf Input Object Type "${type}".`,


Suggested change

`Variable "$${variableName}" must be non-nullable to be used for Within OneOf Input Object Type "${type}".`,

`Variable "$${variableName}" must be non-nullable to be used for within OneOf input object type "${type}".`,

benjie · 2024-09-19T08:57:25Z

src/execution/__tests__/oneof-test.ts

@@ -152,13 +157,13 @@ describe('Execute: Handles OneOf Input Objects', () => {
          {
            locations: [{ column: 16, line: 2 }],
            message:
-              'Variable "$input" got invalid value { a: "abc", b: 123 }; Exactly one key must be specified for OneOf type "TestInputObject".',
+              'Variable "$input" got invalid value { a: "abc", b: 123 }; Within OneOf Input Object Type "TestInputObject", exactly one field must be specified, and the value for that field must be non-null.',


This error seems to be a combination of two errors; ideally this Within should not have a capital letter since it's in the middle of a sentence. Alternatively; can the preceding semicolon be changed to a fullstop?

add runtime errors for pre-coercion OneOf errors

136cde0

yaacovCR requested review from benjie and JoviDeCroock September 18, 2024 13:00

yaacovCR requested a review from a team as a code owner September 18, 2024 13:00

yaacovCR mentioned this pull request Sep 18, 2024

move variable position validation for OneOf to VariablesInAllowedPositionRule #4194

Open

benjie reviewed Sep 18, 2024

View reviewed changes

src/execution/__tests__/oneof-test.ts Outdated Show resolved Hide resolved

only return a single error per value

70e5c4f

yaacovCR requested a review from benjie September 18, 2024 13:33

yaacovCR added the PR: bug fix 🐞 requires increase of "patch" version number label Sep 18, 2024

benjie requested changes Sep 18, 2024

View reviewed changes

combine number of fields and nullability into uniform error message

5feb200

yaacovCR requested a review from benjie September 18, 2024 20:14

benjie requested changes Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add additional runtime errors for pre-coercion OneOf errors #4195

add additional runtime errors for pre-coercion OneOf errors #4195

yaacovCR commented Sep 18, 2024

netlify bot commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024

benjie left a comment

benjie Sep 18, 2024

yaacovCR Sep 18, 2024 •

edited

Loading

yaacovCR Sep 19, 2024

benjie left a comment

benjie Sep 19, 2024

benjie Sep 19, 2024

benjie Sep 19, 2024

benjie Sep 19, 2024

	const coercedValue = coerceInputLiteral(
	valueNode,
	argType,
	variableValues,
	fragmentVariables,
	);
	if (coercedValue === undefined) {
	// Note: ValuesOfCorrectTypeRule validation should catch this before
	// execution. This is a runtime check to ensure execution does not
	// continue with an invalid argument value.
	throw new GraphQLError(
	`Argument "${name}" of type "${inspect(
	argType,
	)}" has invalid value ${print(valueNode)}.`,
	{ nodes: valueNode },
	);
	}

	`Within OneOf Input Object Type "${type}", exactly one field must be specified, and the value for that field must be non-null.`,
	`Within OneOf Input Object type "${type}", exactly one field must be specified, and the value for that field must be non-null.`,

	`Variable "$${variableName}" must be non-nullable to be used for Within OneOf Input Object Type "${type}".`,
	`Variable "$${variableName}" must be non-nullable to be used for within OneOf input object type "${type}".`,

add additional runtime errors for pre-coercion OneOf errors #4195

Are you sure you want to change the base?

add additional runtime errors for pre-coercion OneOf errors #4195

Conversation

yaacovCR commented Sep 18, 2024

netlify bot commented Sep 18, 2024 • edited Loading

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

github-actions bot commented Sep 18, 2024

benjie left a comment

Choose a reason for hiding this comment

benjie Sep 18, 2024

Choose a reason for hiding this comment

yaacovCR Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

yaacovCR Sep 19, 2024

Choose a reason for hiding this comment

benjie left a comment

Choose a reason for hiding this comment

benjie Sep 19, 2024

Choose a reason for hiding this comment

benjie Sep 19, 2024

Choose a reason for hiding this comment

benjie Sep 19, 2024

Choose a reason for hiding this comment

benjie Sep 19, 2024

Choose a reason for hiding this comment

netlify bot commented Sep 18, 2024 •

edited

Loading

yaacovCR Sep 18, 2024 •

edited

Loading