-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Retain orignal resolver error and support overriding error message #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR 👍
I wonder if its time to split QueryError up into QueryError
and ResolverError
.
type ResolverError struct {
Error error `json:"-"`
Path []interface{} `json:"path,omitempty"`
}
type QueryError struct {
Message string `json:"message"`
Locations []Location `json:"locations,omitempty"`
Rule string `json:"-"` // I think this might only be used by tests.
}
QueryErrors are always "public" messages, its the result of failed validation or parsing. Resolver errors always retain the underlying error and don't try to format the message until it bubbles all the way back out to the handler.
test/resolvers_test.go
Outdated
return e.Error() | ||
} | ||
{ | ||
// return special error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather then use a comment and anonymous blocks, you can nest t.Runs, eg
t.Run("return special error", func(t *testing.T) {
} | ||
|
||
func (c *Builder) Errorf(format string, args ...interface{}) { | ||
c.Errors = append(c.Errors, Errorf(format, args...)) | ||
} | ||
|
||
func (c *Builder) Error(err error) { | ||
c.Errors = append(c.Errors, Errorf("%s", err.Error())) | ||
var gqlErrMessage string | ||
if c.ErrorMessageFn != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could eliminate this branch by setting the default error message function to func(err error) string { return err.Error() }
@@ -8,8 +8,12 @@ import ( | |||
{{ end }} | |||
) | |||
|
|||
func MakeExecutableSchema(resolvers Resolvers) graphql.ExecutableSchema { | |||
return &executableSchema{resolvers} | |||
func MakeExecutableSchema(resolvers Resolvers, opts ...ExecutableOption) graphql.ExecutableSchema { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch has failing tests.
please run go generate ./...
and go test ./...
I've think I've fixed CircleCI to run on PRs from forks but I cant find a way to retrigger it for old commits.
8bd84e6
to
156568b
Compare
156568b
to
ab4e701
Compare
I've updated the branch by addressing your comments, haven't gotten to splitting the errors into |
Retain orignal resolver error and support overriding error message
Fixes #38.