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

ErrorWithProps extensions missing when throw in mercurius context/single error #699

Open
sooxt98 opened this issue Jan 6, 2022 · 12 comments

Comments

@sooxt98
Copy link
Contributor

sooxt98 commented Jan 6, 2022

app.register(mercurius, {
    schema,
    resolvers,
    context: async (req, res) => {
        throw new mercurius.ErrorWithProps('Not Authenticated!', {code: 'notAuthenticated'}) 
    },
})

app.graphql.addHook('preParsing') doesnt work either

Updates:
the patch #700 works fine with "mercurius": "^8.9.0" , context handling is perfect, but not "mercurius": "^9.1.0" , someone might change the context error handling

@jonnydgreen
Copy link
Contributor

app.graphql.addHook('preParsing') doesnt work either

Hi - can you provide some more context about this?

@sooxt98
Copy link
Contributor Author

sooxt98 commented Jan 6, 2022

@jonnydgreen trying throw ErrorWithProps in app.graphql.addHook('preParsing'), but ErrorWithProps's extensions.code missing in graphql query result, i have already fix it in the pull request.

but the context issue still persist

@sooxt98
Copy link
Contributor Author

sooxt98 commented Jan 6, 2022

mercurius v8.12.0 and onwards breaks the context error handling, and invalidate my PR #700 , this might due to jit

v8.11.2...v8.12.0

@sooxt98
Copy link
Contributor Author

sooxt98 commented Jan 7, 2022

I think i found it, the ErrorWithProps in contextFn is handled by fastify now instead of mercurius, im not sure whether this is better or not, but its not consistent over graphql api

v8.11.2...v8.12.0#diff-2119cae306240011c3c05c96fe0436eacf198677b4dc0cbe99d644a00bfbcdf4R136-R148

@mcollina
Copy link
Collaborator

mcollina commented Jan 7, 2022

I don't understand the issue. Could you please include a reproducible example? Or send a fresh PR to fix it.

@sooxt98
Copy link
Contributor Author

sooxt98 commented Jan 7, 2022

@mcollina its related to mercurius specs, before v8.12.0, errorFormatter could handle and format all of the error which occurs in mercurius context function, but after it all error are handled by fastify, the error format has changed as table shown below, thats why my PR doesnt work for newer version, as mentioned in issue title

8.11.2 8.12.0
image image

the code is provided as above, try plug it into the basic example and check the difference between

@mcollina
Copy link
Collaborator

mcollina commented Jan 7, 2022

I would encourage you to open a PR because I do not understand your use case.

@sooxt98
Copy link
Contributor Author

sooxt98 commented Jan 7, 2022

@mcollina for example fastify-jwt decode failed, but i need to return in graphql format with ErrorWithProps extensions instead of 500

@sooxt98
Copy link
Contributor Author

sooxt98 commented Jan 7, 2022

using mercurius hook to handle this error would be abit messy, although it works but i cant makesure it wont change in the future

@mcollina
Copy link
Collaborator

mcollina commented Jan 8, 2022

I'm not exactly sure what you are looking for right now and what you are asking us to do.

@sooxt98
Copy link
Contributor Author

sooxt98 commented Jan 8, 2022 via email

@mcollina
Copy link
Collaborator

mcollina commented Jan 8, 2022

What PR actually broke you? I don't think that was intended. Unfortunately a full revert is not on the table as those changes were needed to fix other bugs. However I think it should be possible to support this use case and the rest of the features. Would you like to give it a spin and send a PR?

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

No branches or pull requests

3 participants