Skip to content

Conversation

@stenreijers
Copy link
Contributor

Errors thrown in the parseValue function for custom scalars are not propagated correctly using the originalError variable of the GraphQLError on invalid input. As a result, error codes from the extensions.code are not propagated correctly.

The fix is simple: Replace error.orginalError with error, since error.orginalError does not exist yet.
see:

originalError: error.originalError,

I have added test-cases to avoid this problem in the future.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 29, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@netlify
Copy link

netlify bot commented Jan 29, 2023

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit a982330
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/63d8ff12212435000845a830
😎 Deploy Preview https://deploy-preview-3837--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@stenreijers
Copy link
Contributor Author

stenreijers commented Jan 29, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@saihaj
Copy link
Member

saihaj commented Jan 30, 2023

@stenreijers maybe push an empty commit to retigger the CI ?

@stenreijers
Copy link
Contributor Author

/easycla

@yaacovCR
Copy link
Contributor

This makes sense as a problem to me and also as the solution.

Another approach could be to introduce a generic cloneError utility that does an appropriate deep clone => but I think that rather than attempting the deep clone, it makes more sense to preserve the error hierarchy appropriately...

@IvanGoncharov IvanGoncharov added the PR: bug fix 🐞 requires increase of "patch" version number label Jan 31, 2023
@IvanGoncharov
Copy link
Member

@stenreijers Thanks for fixing it.
It looks like I made a mistake in 14f260b original coercionError had exactly this behavior:

if (isScalarType(type)) {
// Scalars determine if a value is valid via parseValue(), which can
// throw to indicate failure. If it throws, maintain a reference to
// the original error.
try {
const parseResult = type.parseValue(value);
if (parseResult === undefined) {
return ofErrors([
coercionError(`Expected type ${type.name}`, blameNode, path),
]);
}
return ofValue(parseResult);
} catch (error) {
return ofErrors([
coercionError(
`Expected type ${type.name}`,
blameNode,
path,
' ' + error.message,
error,
),

function coercionError(message, blameNode, path, subMessage, originalError) {
let fullMessage = message;
// Build a string describing the path into the value where the error was found
if (path) {
fullMessage += ' at value';
for (const key of pathToArray(path)) {
fullMessage +=
typeof key === 'string' ? '.' + key : '[' + key.toString() + ']';
}
}
fullMessage += subMessage ? '.' + subMessage : '.';
// Return a GraphQLError instance
return new GraphQLError(
fullMessage,
blameNode,
undefined,
undefined,
undefined,
originalError,
);

Can you please backport this fix to https://github.com/graphql/graphql-js/tree/16.x.x ?

@stenreijers
Copy link
Contributor Author

Can you please backport this fix to https://github.com/graphql/graphql-js/tree/16.x.x ?

Please see:
#3838

ardatan added a commit to ardatan/graphql-tools that referenced this pull request Jul 1, 2024
* fix: custom scalar original error propagation (backport graphql/graphql-js#3837; graphql/graphql-js@076972e)

* Update packages/executor/src/execution/__tests__/variables-test.ts

Co-authored-by: Arda TANRIKULU <[email protected]>

* okok

---------

Co-authored-by: Arda TANRIKULU <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: bug fix 🐞 requires increase of "patch" version number

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants