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 99designs#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 Sep 17, 2020
1 parent 305bed0 commit 5d50337
Show file tree
Hide file tree
Showing 36 changed files with 3,285 additions and 1,488 deletions.
8 changes: 4 additions & 4 deletions .github/workflows/integration.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,20 @@ jobs:
integration:
runs-on: ubuntu-latest
timeout-minutes: 3
container: alpine:3.10
container: golang:1.13-alpine
steps:
- uses: actions/checkout@v1
- run: apk add --no-cache --no-progress nodejs npm go musl-dev git bash
- run: apk add --no-cache --no-progress nodejs npm git bash
- run: go mod download
- run: cd integration ; npm install
- run: .github/workflows/check-integration

federation:
runs-on: ubuntu-latest
container: alpine:3.10
container: golang:1.13-alpine
steps:
- uses: actions/checkout@v1
- run: apk add --no-cache --no-progress nodejs npm go musl-dev git bash
- run: apk add --no-cache --no-progress nodejs npm git bash
- run: go mod download
- run: cd example/federation ; npm install
- run: .github/workflows/check-federation
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ jobs:
test:
strategy:
matrix:
go: [1.12, 1.14]
go: [1.13, 1.14]
os: [ubuntu-latest, windows-latest]

runs-on: ${{ matrix.os }}
Expand Down
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/field.gotpl
Original file line number Diff line number Diff line change
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 5d50337

Please sign in to comment.