Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add better error checking for fetched GraphQL requests #919

Merged
merged 3 commits into from
May 23, 2023

Conversation

MartinMinkov
Copy link
Contributor

@MartinMinkov MartinMinkov commented May 22, 2023

Description

This PR introduces enhanced error handling for GraphQL network requests within the Fetch module. Previously, if a GraphQL request failed, the error output would be presented as statusText in an array format. This made error reporting from the user's application less informative, as it logged [object Object]. An example of this less descriptive error logging is as follows:

> npm run build && node build/src/index.js

> test-graphql-errors@0.1.0 build
> tsc
/home/martin/Code/o1/mina/src/lib/snarkyjs/dist/node/_node_bindings/snarky_js_node.bc.cjs:7427
         throw err;
         ^
Error: [object Object]
    at fetchEvents (file:///home/martin/Code/o1/mina/src/lib/snarkyjs/dist/node/lib/fetch.js:475:15)
    at process.processTicksAndRejections (node:internal/process/task_queue:95:5)
    at async main (file:///tmp/test-graphql-errors/build/src/index.js:3:20)

This error as produced by building snarkyjs with an invalid GraphQL schema in the request for fetchEvents.

With the enhancements made in this PR, the error reporting becomes more informative and user-friendly. The new error messages provide specific details about the issues at hand. An example of the improved error message is:

> npm run build && node build/src/index.js                         

> test-graphql-errors@0.1.0 build
> tsc
/home/martin/Code/o1/mina/src/lib/snarkyjs/dist/node/_node_bindings/snarky_js_node.bc.cjs:7427
         throw err;
         ^
Error: Cannot query field "events1" on type "Query". Did you mean "events"?
    at fetchEvents (file:///home/martin/Code/o1/mina/src/lib/snarkyjs/dist/node/lib/fetch.js:475:15)
    at process.processTicksAndRejections (node:internal/process/task_queue:95:5)
    at async main (file:///tmp/test-graphql-errors/build/src/index.js:3:20)

Testing

The improvements introduced in this PR have been verified using a local test script.

@MartinMinkov MartinMinkov linked an issue May 22, 2023 that may be closed by this pull request
@MartinMinkov MartinMinkov marked this pull request as ready for review May 22, 2023 16:56
Copy link
Member

@shimkiv shimkiv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +1094 to +1099
} else if (jsonResponse.data === undefined) {
return [
undefined,
{
statusCode: response.status,
statusText: `GraphQL response data is undefined`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just double-checking -- are we sure that undefined response data always means that the request failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I believe this to be the case since we check that jsonResponse.data exists when we try to do some processing on later functions:

I'm not sure of a case where we would make a GraphQL request and expect the data to be undefined. Even if we make a GraphQL mutation instead of a query, we would probably want some data returned (e.g. an ID).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I guess we can look at our current queries to check that we always want something returned, and this is currently the case 👍🏻

@MartinMinkov MartinMinkov merged commit f2257f6 into main May 23, 2023
9 checks passed
@MartinMinkov MartinMinkov deleted the fix/graphql-errors-check branch May 23, 2023 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Swallowed errors during comms with Archive-Node-API
3 participants