mercurius-js / mercurius

Implement GraphQL servers and gateways with Fastify
https://mercurius.dev/
MIT License
2.36k stars 237 forks source link

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

Open sooxt98 opened 2 years ago

sooxt98 commented 2 years ago
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 https://github.com/mercurius-js/mercurius/pull/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 commented 2 years ago

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

Hi - can you provide some more context about this?

sooxt98 commented 2 years ago

@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 commented 2 years ago

mercurius v8.12.0 and onwards breaks the context error handling, and invalidate my PR https://github.com/mercurius-js/mercurius/pull/700 , this might due to jit

https://github.com/mercurius-js/mercurius/compare/v8.11.2...v8.12.0

sooxt98 commented 2 years ago

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

https://github.com/mercurius-js/mercurius/compare/v8.11.2...v8.12.0#diff-2119cae306240011c3c05c96fe0436eacf198677b4dc0cbe99d644a00bfbcdf4R136-R148

mcollina commented 2 years ago

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

sooxt98 commented 2 years ago

@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 commented 2 years ago

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

sooxt98 commented 2 years ago

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

sooxt98 commented 2 years ago

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 commented 2 years ago

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

sooxt98 commented 2 years ago

Here’s the question, do you think that the error throw from the context should be handled by mercurius errorFormatter or Fastify. If it’s fastify you may close the issue since it’s mercurius specs. If not please revert back the error handling from mercurius v8.12.0 to v8.11.0 in routing.js file

On Sat, 8 Jan 2022 at 11:32 PM, Matteo Collina @.***> wrote:

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

— Reply to this email directly, view it on GitHub https://github.com/mercurius-js/mercurius/issues/699#issuecomment-1008026836, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADGCEC6T3ZNYSGD4KV2D4ZDUVBKJTANCNFSM5LLJ64BQ . You are receiving this because you authored the thread.Message ID: @.***>

mcollina commented 2 years ago

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?