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

ResolverError field in QueryError not set and public errors in general #38

Closed
dvic opened this issue Mar 5, 2018 · 5 comments
Closed
Labels
enhancement New feature or request

Comments

@dvic
Copy link
Contributor

dvic commented Mar 5, 2018

I see that the ResolverError field on QueryError is not set. What is the plan for this? The Builder for errors currently only takes the error message and creates a QueryError from that.

I was a bit surprised to see that by default the generated execution context drops the original error and exposes the raw message error in the GraphQL response. What are your thoughts for this?

I can imagine one wants to be able to control how errors thrown from the resolvers are rendered. I'm using a custom written HTTP handler and would like to be able to log the original errors. Do you think it's better to do logging etc. of the errors inside the resolvers and return a custom 'public' error?

Thanks for the awesome lib! Please let us know if there are any areas where you could use some help!

@vektah
Copy link
Collaborator

vektah commented Mar 8, 2018

I guess you are expecting it to work a bit more like this?

It would let you put a String() method on your error for display to the user?

@vektah vektah added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Mar 8, 2018
@dvic
Copy link
Contributor Author

dvic commented Mar 8, 2018

Yes exactly. Storing the ResolverError will at least allow us to recover the original error and log it in the http handler. But I'm unsure how to best address the error rendering. What about checking whether the error implements

interface{ 
   PublicError() string
}

and use PublicError() as the error response message? Another option would be to pass a function publicError(err string) string(like recoverPanicFn) that extracts the error message.

Let me know what you think, I could try to make a PR for this once we decide what approach to go for.

@vektah
Copy link
Collaborator

vektah commented Mar 8, 2018

Another option would be to pass a function publicError(err string) string(like recoverPanicFn) that extracts the error message.

I really like this idea. Leave the error as an error until the very end and let the user decide. Provide a default that just does a err.Error()

Its probably a fair bit of work though.

@dvic
Copy link
Contributor Author

dvic commented Mar 8, 2018

Alright, I'll see if I can make a PR this week:

  • I'll make sure ResolverError is set
  • publicError(err error) string can be configured with a default to err.Error().

@dvic
Copy link
Contributor Author

dvic commented Mar 10, 2018

@vektah I gave it a first shot in #45. Is this something like you had in mind?

dvic added a commit to dvic/gqlgen that referenced this issue Mar 22, 2018
dvic added a commit to dvic/gqlgen that referenced this issue Mar 22, 2018
dvic added a commit to dvic/gqlgen that referenced this issue Mar 22, 2018
cgxxv pushed a commit to cgxxv/gqlgen that referenced this issue Mar 25, 2022
cgxxv pushed a commit to cgxxv/gqlgen that referenced this issue Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants