Skip to content

Commit

Permalink
Always wrap user errors
Browse files Browse the repository at this point in the history
This is an alternative to #1305 that leans into error wrapping instead of away from it.

Requires use of go 1.13 error unwrapping.

On measure I think I prefer this approach, even though it's a bigger BC break:
- There's less mutex juggling
- It has never felt right to me that we make the user deal with path when overriding the error presenter
- The default error presenter is now incredibly simple

Questions:
- Are we comfortable with supporting 1.13 and up?
- Should we change the signature of `ErrorPresenterFunc` to `func(ctx context.Context, err *gqlerror.Error) *gqlerror.Error`?
    - It always is now, and breaking BC will force users to address the requirement for `errors.As`
  • Loading branch information
lwc committed Aug 27, 2020
1 parent 622316e commit be7c190
Show file tree
Hide file tree
Showing 31 changed files with 681 additions and 688 deletions.
6 changes: 3 additions & 3 deletions codegen/args.gotpl
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ func (ec *executionContext) {{ $name }}(ctx context.Context, rawArgs map[string]
{{- range $i, $arg := . }}
var arg{{$i}} {{ $arg.TypeReference.GO | ref}}
if tmp, ok := rawArgs[{{$arg.Name|quote}}]; ok {
ctx := graphql.WithFieldInputContext(ctx, graphql.NewFieldInputWithField({{$arg.Name|quote}}))
ctx := graphql.WithPathContext(ctx, graphql.NewPathWithField({{$arg.Name|quote}}))
{{- if $arg.ImplDirectives }}
directive0 := func(ctx context.Context) (interface{}, error) { return ec.{{ $arg.TypeReference.UnmarshalFunc }}(ctx, tmp) }
{{ template "implDirectives" $arg }}
tmp, err = directive{{$arg.ImplDirectives|len}}(ctx)
if err != nil {
return nil, err
return nil, graphql.ErrorOnPath(ctx, err)
}
if data, ok := tmp.({{ $arg.TypeReference.GO | ref }}) ; ok {
arg{{$i}} = data
Expand All @@ -20,7 +20,7 @@ func (ec *executionContext) {{ $name }}(ctx context.Context, rawArgs map[string]
arg{{$i}} = nil
{{- end }}
} else {
return nil, fmt.Errorf(`unexpected type %T from directive, should be {{ $arg.TypeReference.GO }}`, tmp)
return nil, graphql.ErrorOnPath(ctx, fmt.Errorf(`unexpected type %T from directive, should be {{ $arg.TypeReference.GO }}`, tmp))
}
{{- else }}
arg{{$i}}, err = ec.{{ $arg.TypeReference.UnmarshalFunc }}(ctx, tmp)
Expand Down
2 changes: 1 addition & 1 deletion codegen/directives.gotpl
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ func (ec *executionContext) _subscriptionMiddleware(ctx context.Context, obj *as
{{- end }}
res, err := ec.ResolverMiddleware(ctx, next)
if err != nil {
ec.Error(ctx, err)
ec.Error(ctx, err)
return nil
}
return res
Expand Down
4 changes: 2 additions & 2 deletions codegen/field.gotpl
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func (ec *executionContext) _{{$object.Name}}_{{$field.Name}}(ctx context.Contex
rawArgs := field.ArgumentMap(ec.Variables)
args, err := ec.{{ $field.ArgsFunc }}(ctx,rawArgs)
if err != nil {
ec.Error(ctx, err)
ec.Error(ctx, err)
return {{ $null }}
}
fc.Args = args
Expand Down Expand Up @@ -81,7 +81,7 @@ func (ec *executionContext) _{{$object.Name}}_{{$field.Name}}(ctx context.Contex
{{ template "implDirectives" . }}
tmp, err := directive{{.ImplDirectives|len}}(rctx)
if err != nil {
return nil, err
return nil, graphql.ErrorOnPath(ctx, err)
}
if tmp == nil {
return nil, nil
Expand Down
7 changes: 4 additions & 3 deletions codegen/input.gotpl
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@
case {{$field.Name|quote}}:
var err error

ctx := graphql.WithFieldInputContext(ctx, graphql.NewFieldInputWithField({{$field.Name|quote}}))
ctx := graphql.WithPathContext(ctx, graphql.NewPathWithField({{$field.Name|quote}}))
{{- if $field.ImplDirectives }}
directive0 := func(ctx context.Context) (interface{}, error) { return ec.{{ $field.TypeReference.UnmarshalFunc }}(ctx, v) }
{{ template "implDirectives" $field }}
tmp, err := directive{{$field.ImplDirectives|len}}(ctx)
if err != nil {
return it, err
return it, graphql.ErrorOnPath(ctx, err)
}
if data, ok := tmp.({{ $field.TypeReference.GO | ref }}) ; ok {
it.{{$field.GoFieldName}} = data
Expand All @@ -32,7 +32,8 @@
it.{{$field.GoFieldName}} = nil
{{- end }}
} else {
return it, fmt.Errorf(`unexpected type %T from directive, should be {{ $field.TypeReference.GO }}`, tmp)
err := fmt.Errorf(`unexpected type %T from directive, should be {{ $field.TypeReference.GO }}`, tmp)
return it, graphql.ErrorOnPath(ctx, err)
}
{{- else }}
it.{{$field.GoFieldName}}, err = ec.{{ $field.TypeReference.UnmarshalFunc }}(ctx, v)
Expand Down
14 changes: 7 additions & 7 deletions codegen/testserver/directive_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ func TestDirectives(t *testing.T) {

err := c.Post(`query { directiveArg(arg: "") }`, &resp)

require.EqualError(t, err, `[{"message":"invalid length","path":["directiveArg"]}]`)
require.EqualError(t, err, `[{"message":"invalid length","path":["directiveArg","arg"]}]`)
require.Nil(t, resp.DirectiveArg)
})
t.Run("when function errors on nullable arg directives", func(t *testing.T) {
Expand All @@ -210,7 +210,7 @@ func TestDirectives(t *testing.T) {

err := c.Post(`query { directiveNullableArg(arg: -100) }`, &resp)

require.EqualError(t, err, `[{"message":"too small","path":["directiveNullableArg"]}]`)
require.EqualError(t, err, `[{"message":"too small","path":["directiveNullableArg","arg"]}]`)
require.Nil(t, resp.DirectiveNullableArg)
})
t.Run("when function success on nullable arg directives", func(t *testing.T) {
Expand Down Expand Up @@ -317,7 +317,7 @@ func TestDirectives(t *testing.T) {

err := c.Post(`query { directiveInputNullable(arg: {text:"invalid text",inner:{message:"123"}}) }`, &resp)

require.EqualError(t, err, `[{"message":"not valid","path":["directiveInputNullable","arg"]}]`)
require.EqualError(t, err, `[{"message":"not valid","path":["directiveInputNullable","arg","text"]}]`)
require.Nil(t, resp.DirectiveInputNullable)
})
t.Run("when function errors on inner directives", func(t *testing.T) {
Expand All @@ -327,7 +327,7 @@ func TestDirectives(t *testing.T) {

err := c.Post(`query { directiveInputNullable(arg: {text:"2",inner:{message:""}}) }`, &resp)

require.EqualError(t, err, `[{"message":"not valid","path":["directiveInputNullable","arg","inner"]}]`)
require.EqualError(t, err, `[{"message":"not valid","path":["directiveInputNullable","arg","inner","message"]}]`)
require.Nil(t, resp.DirectiveInputNullable)
})
t.Run("when function errors on nullable inner directives", func(t *testing.T) {
Expand All @@ -337,7 +337,7 @@ func TestDirectives(t *testing.T) {

err := c.Post(`query { directiveInputNullable(arg: {text:"success",inner:{message:"1"},innerNullable:{message:""}}) }`, &resp)

require.EqualError(t, err, `[{"message":"not valid","path":["directiveInputNullable","arg","innerNullable"]}]`)
require.EqualError(t, err, `[{"message":"not valid","path":["directiveInputNullable","arg","innerNullable","message"]}]`)
require.Nil(t, resp.DirectiveInputNullable)
})
t.Run("when function success", func(t *testing.T) {
Expand Down Expand Up @@ -413,7 +413,7 @@ func TestDirectives(t *testing.T) {

err := c.WebsocketOnce(`subscription { directiveArg(arg: "") }`, &resp)

require.EqualError(t, err, `[{"message":"invalid length","path":["directiveArg"]}]`)
require.EqualError(t, err, `[{"message":"invalid length","path":["directiveArg","arg"]}]`)
require.Nil(t, resp.DirectiveArg)
})
t.Run("when function errors on nullable arg directives", func(t *testing.T) {
Expand All @@ -423,7 +423,7 @@ func TestDirectives(t *testing.T) {

err := c.WebsocketOnce(`subscription { directiveNullableArg(arg: -100) }`, &resp)

require.EqualError(t, err, `[{"message":"too small","path":["directiveNullableArg"]}]`)
require.EqualError(t, err, `[{"message":"too small","path":["directiveNullableArg","arg"]}]`)
require.Nil(t, resp.DirectiveNullableArg)
})
t.Run("when function success on nullable arg directives", func(t *testing.T) {
Expand Down
Loading

0 comments on commit be7c190

Please sign in to comment.