Skip to content

feat: improve error handling #1132

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+67 −43
Open

feat: improve error handling #1132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions v2/pkg/engine/datasource/httpclient/nethttpclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
"time"

"github.com/buger/jsonparser"

pkgErrors "github.com/pkg/errors"
"github.com/wundergraph/graphql-go-tools/v2/pkg/lexer/literal"
"github.com/wundergraph/graphql-go-tools/v2/pkg/pool"
)
Expand Down Expand Up @@ -204,7 +204,7 @@ func makeHTTPRequest(client *http.Client, ctx context.Context, url, method, head

response, err := client.Do(request)
if err != nil {
return err
return pkgErrors.WithStack(err)
}
defer response.Body.Close()

Expand Down Expand Up @@ -262,6 +262,7 @@ func Do(client *http.Client, ctx context.Context, requestInput []byte, out *byte
pool.Hash64.Put(h)
ctx = context.WithValue(ctx, bodyHashContextKey{}, bodyHash)
return makeHTTPRequest(client, ctx, url, method, headers, queryParams, bytes.NewReader(body), enableTrace, out, ContentTypeJSON)

}

func DoMultipartForm(
Expand Down
30 changes: 15 additions & 15 deletions v2/pkg/engine/resolve/authorization_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func TestAuthorization(t *testing.T) {
assert.Equal(t, int64(2), authorizer.(*testAuthorizer).preFetchCalls.Load())
assert.Equal(t, int64(4), authorizer.(*testAuthorizer).objectFieldCalls.Load())

require.Nil(t, resolveCtx.subgraphErrors)
require.Nil(t, resolveCtx.error)
}
}))
t.Run("validate authorizer args", testFnWithPostEvaluation(func(t *testing.T, ctrl *gomock.Controller) (node *GraphQLResponse, ctx *Context, expectedOutput string, postEvaluation func(t *testing.T)) {
Expand Down Expand Up @@ -139,7 +139,7 @@ func TestAuthorization(t *testing.T) {
assert.Equal(t, int64(2), authorizer.(*testAuthorizer).preFetchCalls.Load())
assert.Equal(t, int64(4), authorizer.(*testAuthorizer).objectFieldCalls.Load())

require.Nil(t, resolveCtx.subgraphErrors)
require.Nil(t, resolveCtx.error)
}
}))
t.Run("disallow field with extension", testFnWithPostEvaluation(func(t *testing.T, ctrl *gomock.Controller) (node *GraphQLResponse, ctx *Context, expectedOutput string, postEvaluation func(t *testing.T)) {
Expand Down Expand Up @@ -167,7 +167,7 @@ func TestAuthorization(t *testing.T) {
assert.Equal(t, int64(2), authorizer.(*testAuthorizer).preFetchCalls.Load())
assert.Equal(t, int64(4), authorizer.(*testAuthorizer).objectFieldCalls.Load())

require.Nil(t, resolveCtx.subgraphErrors)
require.Nil(t, resolveCtx.error)
}
}))
t.Run("no authorization rules/checks", testFnWithPostEvaluation(func(t *testing.T, ctrl *gomock.Controller) (node *GraphQLResponse, ctx *Context, expectedOutput string, postEvaluation func(t *testing.T)) {
Expand All @@ -187,7 +187,7 @@ func TestAuthorization(t *testing.T) {
assert.Equal(t, int64(0), authorizer.(*testAuthorizer).preFetchCalls.Load())
assert.Equal(t, int64(0), authorizer.(*testAuthorizer).objectFieldCalls.Load())

require.Nil(t, resolveCtx.subgraphErrors)
require.Nil(t, resolveCtx.error)
}
}))
t.Run("disallow root fetch", testFnWithPostEvaluation(func(t *testing.T, ctrl *gomock.Controller) (node *GraphQLResponse, ctx *Context, expectedOutput string, postEvaluation func(t *testing.T)) {
Expand All @@ -213,7 +213,7 @@ func TestAuthorization(t *testing.T) {
assert.Equal(t, int64(0), authorizer.(*testAuthorizer).objectFieldCalls.Load())

var subgraphError *SubgraphError
require.ErrorAs(t, resolveCtx.subgraphErrors, &subgraphError)
require.ErrorAs(t, resolveCtx.error, &subgraphError)
require.Equal(t, "users", subgraphError.DataSourceInfo.Name)
require.Equal(t, "query", subgraphError.Path)
require.Equal(t, "Not allowed to fetch from users Subgraph", subgraphError.Reason)
Expand Down Expand Up @@ -244,7 +244,7 @@ func TestAuthorization(t *testing.T) {
assert.Equal(t, int64(0), authorizer.(*testAuthorizer).objectFieldCalls.Load())

var subgraphError *SubgraphError
require.ErrorAs(t, resolveCtx.subgraphErrors, &subgraphError)
require.ErrorAs(t, resolveCtx.error, &subgraphError)
require.Equal(t, "users", subgraphError.DataSourceInfo.Name)
require.Equal(t, "query", subgraphError.Path)
require.Equal(t, "", subgraphError.Reason)
Expand Down Expand Up @@ -276,11 +276,11 @@ func TestAuthorization(t *testing.T) {
assert.Equal(t, int64(2), authorizer.(*testAuthorizer).preFetchCalls.Load())
assert.Equal(t, int64(4), authorizer.(*testAuthorizer).objectFieldCalls.Load())

require.NotEmpty(t, resolveCtx.subgraphErrors)
require.EqualError(t, resolveCtx.subgraphErrors, "Failed to fetch from Subgraph 'products' at Path: 'query.me.reviews.@.product', Reason: Not allowed to fetch from products Subgraph.")
require.NotEmpty(t, resolveCtx.error)
require.EqualError(t, resolveCtx.error, "Failed to fetch from Subgraph 'products' at Path: 'query.me.reviews.@.product', Reason: Not allowed to fetch from products Subgraph.")

var subgraphError *SubgraphError
require.ErrorAs(t, resolveCtx.subgraphErrors, &subgraphError)
require.ErrorAs(t, resolveCtx.error, &subgraphError)
require.Equal(t, "products", subgraphError.DataSourceInfo.Name)
require.Equal(t, "query.me.reviews.@.product", subgraphError.Path)
require.Equal(t, "Not allowed to fetch from products Subgraph", subgraphError.Reason)
Expand Down Expand Up @@ -311,7 +311,7 @@ func TestAuthorization(t *testing.T) {
assert.Equal(t, int64(4), authorizer.(*testAuthorizer).objectFieldCalls.Load())

var subgraphError *SubgraphError
require.ErrorAs(t, resolveCtx.subgraphErrors, &subgraphError)
require.ErrorAs(t, resolveCtx.error, &subgraphError)
require.Equal(t, "products", subgraphError.DataSourceInfo.Name)
require.Equal(t, "Query.me.reviews.product.data.name", subgraphError.Path)
require.Equal(t, "Not allowed to fetch name on Product", subgraphError.Reason)
Expand Down Expand Up @@ -344,7 +344,7 @@ func TestAuthorization(t *testing.T) {
assert.Equal(t, int64(4), authorizer.(*testAuthorizer).objectFieldCalls.Load())

var subgraphError *SubgraphError
require.ErrorAs(t, resolveCtx.subgraphErrors, &subgraphError)
require.ErrorAs(t, resolveCtx.error, &subgraphError)
require.Equal(t, "products", subgraphError.DataSourceInfo.Name)
require.Equal(t, "query.me.reviews.@.product", subgraphError.Path)
require.Equal(t, "Not allowed to fetch from products Subgraph", subgraphError.Reason)
Expand Down Expand Up @@ -388,7 +388,7 @@ func TestAuthorization(t *testing.T) {
assert.Equal(t, int64(4), authorizer.(*testAuthorizer).objectFieldCalls.Load())

var subgraphError *SubgraphError
require.ErrorAs(t, resolveCtx.subgraphErrors, &subgraphError)
require.ErrorAs(t, resolveCtx.error, &subgraphError)
require.Equal(t, "reviews", subgraphError.DataSourceInfo.Name)
require.Equal(t, "Query.me.reviews.body", subgraphError.Path)
require.Equal(t, "Not allowed to fetch body on Review", subgraphError.Reason)
Expand Down Expand Up @@ -417,7 +417,7 @@ func TestAuthorization(t *testing.T) {
assert.Equal(t, int64(4), authorizer.(*testAuthorizer).objectFieldCalls.Load())

var subgraphError *SubgraphError
require.ErrorAs(t, resolveCtx.subgraphErrors, &subgraphError)
require.ErrorAs(t, resolveCtx.error, &subgraphError)
require.Equal(t, "reviews", subgraphError.DataSourceInfo.Name)
require.Equal(t, "Query.me.reviews.body", subgraphError.Path)
require.Equal(t, "", subgraphError.Reason)
Expand Down Expand Up @@ -448,7 +448,7 @@ func TestAuthorization(t *testing.T) {
assert.Equal(t, int64(4), authorizer.(*testAuthorizer).objectFieldCalls.Load())

var subgraphError *SubgraphError
require.ErrorAs(t, resolveCtx.subgraphErrors, &subgraphError)
require.ErrorAs(t, resolveCtx.error, &subgraphError)
require.Equal(t, "products", subgraphError.DataSourceInfo.Name)
require.Equal(t, "query.me.reviews.@.product", subgraphError.Path)
require.Equal(t, "Not allowed to fetch name on Product", subgraphError.Reason)
Expand Down Expand Up @@ -479,7 +479,7 @@ func TestAuthorization(t *testing.T) {
assert.Equal(t, int64(4), authorizer.(*testAuthorizer).objectFieldCalls.Load())

var subgraphError *SubgraphError
require.ErrorAs(t, resolveCtx.subgraphErrors, &subgraphError)
require.ErrorAs(t, resolveCtx.error, &subgraphError)
require.Equal(t, "products", subgraphError.DataSourceInfo.Name)
require.Equal(t, "Query.me.reviews.product.data.name", subgraphError.Path)
require.Equal(t, "Not allowed to fetch name on Product", subgraphError.Reason)
Expand Down
15 changes: 9 additions & 6 deletions v2/pkg/engine/resolve/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type Context struct {
authorizer Authorizer
rateLimiter RateLimiter

subgraphErrors error
error error
}

type ExecutionOptions struct {
Expand Down Expand Up @@ -103,12 +103,15 @@ func (c *Context) SetRateLimiter(limiter RateLimiter) {
c.rateLimiter = limiter
}

func (c *Context) SubgraphErrors() error {
return c.subgraphErrors
// ExecutionError returns the error that occurred during execution.
// You can use appendError to append errors to the context e.g. from a subgraph
func (c *Context) ExecutionError() error {
return c.error
}

func (c *Context) appendSubgraphError(err error) {
c.subgraphErrors = errors.Join(c.subgraphErrors, err)
// appendError appends the error to the context's error field. Should only be used to make the error public
func (c *Context) appendError(err error) {
c.error = errors.Join(c.error, err)
}

type Request struct {
Expand Down Expand Up @@ -168,7 +171,7 @@ func (c *Context) Free() {
c.RemapVariables = nil
c.TracingOptions.DisableAll()
c.Extensions = nil
c.subgraphErrors = nil
c.error = nil
c.authorizer = nil
c.LoaderHooks = nil
}
Expand Down
48 changes: 34 additions & 14 deletions v2/pkg/engine/resolve/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ import (
"context"
"crypto/tls"
"encoding/json"
goerrors "errors"
"fmt"
"github.com/wundergraph/graphql-go-tools/v2/pkg/errorcodes"
"net/http"
"net/http/httptrace"
"slices"
Expand All @@ -16,6 +14,8 @@ import (
"sync"
"time"

"github.com/wundergraph/graphql-go-tools/v2/pkg/errorcodes"

"github.com/buger/jsonparser"
"github.com/cespare/xxhash/v2"
"github.com/pkg/errors"
Expand Down Expand Up @@ -57,8 +57,8 @@ type ResponseInfo struct {
ResponseHeaders http.Header
}

func newResponseInfo(res *result, subgraphError error) *ResponseInfo {
responseInfo := &ResponseInfo{StatusCode: res.statusCode, Err: goerrors.Join(res.err, subgraphError)}
func newResponseInfo(res *result, err error) *ResponseInfo {
responseInfo := &ResponseInfo{StatusCode: res.statusCode, Err: err}
if res.httpResponseContext != nil {
// We're using the response.Request here, because the body will be nil (since the response was read) and won't
// cause a memory leak.
Expand Down Expand Up @@ -197,7 +197,7 @@ func (l *Loader) resolveParallel(nodes []*FetchTreeNode) error {
if l.ctx.LoaderHooks != nil && results[i].nestedMergeItems[j].loaderHookContext != nil {
l.ctx.LoaderHooks.OnFinished(results[i].nestedMergeItems[j].loaderHookContext,
results[i].nestedMergeItems[j].ds,
newResponseInfo(results[i].nestedMergeItems[j], l.ctx.subgraphErrors))
newResponseInfo(results[i].nestedMergeItems[j], l.ctx.error))
}
if err != nil {
return errors.WithStack(err)
Expand All @@ -206,7 +206,7 @@ func (l *Loader) resolveParallel(nodes []*FetchTreeNode) error {
} else {
err = l.mergeResult(nodes[i].Item, results[i], itemsItems[i])
if l.ctx.LoaderHooks != nil {
l.ctx.LoaderHooks.OnFinished(results[i].loaderHookContext, results[i].ds, newResponseInfo(results[i], l.ctx.subgraphErrors))
l.ctx.LoaderHooks.OnFinished(results[i].loaderHookContext, results[i].ds, newResponseInfo(results[i], l.ctx.error))
}
if err != nil {
return errors.WithStack(err)
Expand Down Expand Up @@ -242,8 +242,12 @@ func (l *Loader) resolveSingle(item *FetchItem) error {
return err
}
err = l.mergeResult(item, res, items)

if res.err != nil {
l.ctx.appendError(res.err)
}
if l.ctx.LoaderHooks != nil {
l.ctx.LoaderHooks.OnFinished(res.loaderHookContext, res.ds, newResponseInfo(res, l.ctx.subgraphErrors))
l.ctx.LoaderHooks.OnFinished(res.loaderHookContext, res.ds, newResponseInfo(res, l.ctx.error))
}

return err
Expand All @@ -256,9 +260,14 @@ func (l *Loader) resolveSingle(item *FetchItem) error {
return errors.WithStack(err)
}
err = l.mergeResult(item, res, items)

if res.err != nil {
l.ctx.appendError(res.err)
}
if l.ctx.LoaderHooks != nil {
l.ctx.LoaderHooks.OnFinished(res.loaderHookContext, res.ds, newResponseInfo(res, l.ctx.subgraphErrors))
l.ctx.LoaderHooks.OnFinished(res.loaderHookContext, res.ds, newResponseInfo(res, l.ctx.error))
}

return err
case *EntityFetch:
res := &result{
Expand All @@ -269,9 +278,14 @@ func (l *Loader) resolveSingle(item *FetchItem) error {
return errors.WithStack(err)
}
err = l.mergeResult(item, res, items)
if res.err != nil {
l.ctx.appendError(res.err)
}

if l.ctx.LoaderHooks != nil {
l.ctx.LoaderHooks.OnFinished(res.loaderHookContext, res.ds, newResponseInfo(res, l.ctx.subgraphErrors))
l.ctx.LoaderHooks.OnFinished(res.loaderHookContext, res.ds, newResponseInfo(res, l.ctx.error))
}

return err
case *ParallelListItemFetch:
results := make([]*result, len(items))
Expand Down Expand Up @@ -302,9 +316,14 @@ func (l *Loader) resolveSingle(item *FetchItem) error {
}
for i := range results {
err = l.mergeResult(item, results[i], items[i:i+1])

if results[i].err != nil {
l.ctx.appendError(results[i].err)
}
if l.ctx.LoaderHooks != nil {
l.ctx.LoaderHooks.OnFinished(results[i].loaderHookContext, results[i].ds, newResponseInfo(results[i], l.ctx.subgraphErrors))
l.ctx.LoaderHooks.OnFinished(results[i].loaderHookContext, results[i].ds, newResponseInfo(results[i], l.ctx.error))
}

if err != nil {
return errors.WithStack(err)
}
Expand Down Expand Up @@ -693,7 +712,7 @@ func (l *Loader) appendSubgraphError(res *result, fetchItem *FetchItem, value *a
subgraphError.AppendDownstreamError(&gErr)
}

l.ctx.appendSubgraphError(goerrors.Join(res.err, subgraphError))
l.ctx.appendError(subgraphError)

return nil
}
Expand Down Expand Up @@ -1006,7 +1025,8 @@ func (l *Loader) addApolloRouterCompatibilityError(res *result) error {
}

func (l *Loader) renderErrorsFailedToFetch(fetchItem *FetchItem, res *result, reason string) error {
l.ctx.appendSubgraphError(goerrors.Join(res.err, NewSubgraphError(res.ds, fetchItem.ResponsePath, reason, res.statusCode)))
l.ctx.appendError(NewSubgraphError(res.ds, fetchItem.ResponsePath, reason, res.statusCode))

errorObject, err := astjson.ParseWithoutCache(l.renderSubgraphBaseError(res.ds, fetchItem.ResponsePath, reason))
if err != nil {
return err
Expand All @@ -1032,7 +1052,7 @@ func (l *Loader) renderSubgraphBaseError(ds DataSourceInfo, path, reason string)

func (l *Loader) renderAuthorizationRejectedErrors(fetchItem *FetchItem, res *result) error {
for i := range res.authorizationRejectedReasons {
l.ctx.appendSubgraphError(goerrors.Join(res.err, NewSubgraphError(res.ds, fetchItem.ResponsePath, res.authorizationRejectedReasons[i], res.statusCode)))
l.ctx.appendError(NewSubgraphError(res.ds, fetchItem.ResponsePath, res.authorizationRejectedReasons[i], res.statusCode))
}
pathPart := l.renderAtPathErrorPart(fetchItem.ResponsePath)
extensionErrorCode := fmt.Sprintf(`"extensions":{"code":"%s"}`, errorcodes.UnauthorizedFieldOrType)
Expand Down Expand Up @@ -1073,7 +1093,7 @@ func (l *Loader) renderAuthorizationRejectedErrors(fetchItem *FetchItem, res *re
}

func (l *Loader) renderRateLimitRejectedErrors(fetchItem *FetchItem, res *result) error {
l.ctx.appendSubgraphError(goerrors.Join(res.err, NewRateLimitError(res.ds.Name, fetchItem.ResponsePath, res.rateLimitRejectedReason)))
l.ctx.appendError(NewRateLimitError(res.ds.Name, fetchItem.ResponsePath, res.rateLimitRejectedReason))
pathPart := l.renderAtPathErrorPart(fetchItem.ResponsePath)
var (
err error
Expand Down
10 changes: 5 additions & 5 deletions v2/pkg/engine/resolve/loader_hooks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func TestLoaderHooks_FetchPipeline(t *testing.T) {
assert.Equal(t, "errorMessage", subgraphError.DownstreamErrors[0].Message)
assert.Nil(t, subgraphError.DownstreamErrors[0].Extensions)

assert.NotNil(t, resolveCtx.SubgraphErrors())
assert.NotNil(t, resolveCtx.ExecutionError())
}
}))

Expand Down Expand Up @@ -185,7 +185,7 @@ func TestLoaderHooks_FetchPipeline(t *testing.T) {
assert.Equal(t, "errorMessage", subgraphError.DownstreamErrors[0].Message)
assert.Nil(t, subgraphError.DownstreamErrors[0].Extensions)

assert.NotNil(t, resolveCtx.SubgraphErrors())
assert.NotNil(t, resolveCtx.ExecutionError())
})

t.Run("parallel fetch with simple subgraph error", testFnWithPostEvaluation(func(t *testing.T, ctrl *gomock.Controller) (node *GraphQLResponse, ctx *Context, expectedOutput string, postEvaluation func(t *testing.T)) {
Expand Down Expand Up @@ -249,7 +249,7 @@ func TestLoaderHooks_FetchPipeline(t *testing.T) {
assert.Equal(t, "errorMessage", subgraphError.DownstreamErrors[0].Message)
assert.Nil(t, subgraphError.DownstreamErrors[0].Extensions)

assert.NotNil(t, resolveCtx.SubgraphErrors())
assert.NotNil(t, resolveCtx.ExecutionError())
}
}))

Expand Down Expand Up @@ -314,7 +314,7 @@ func TestLoaderHooks_FetchPipeline(t *testing.T) {
assert.Equal(t, "errorMessage", subgraphError.DownstreamErrors[0].Message)
assert.Nil(t, subgraphError.DownstreamErrors[0].Extensions)

assert.NotNil(t, resolveCtx.SubgraphErrors())
assert.NotNil(t, resolveCtx.ExecutionError())
}
}))

Expand Down Expand Up @@ -380,7 +380,7 @@ func TestLoaderHooks_FetchPipeline(t *testing.T) {
assert.Equal(t, "errorMessage2", subgraphError.DownstreamErrors[1].Message)
assert.Empty(t, subgraphError.DownstreamErrors[1].Extensions["code"])

assert.NotNil(t, resolveCtx.SubgraphErrors())
assert.NotNil(t, resolveCtx.ExecutionError())
}
}))

Expand Down
2 changes: 1 addition & 1 deletion v2/pkg/engine/resolve/resolvable.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,7 @@ func (r *Resolvable) addRejectFieldError(reason string, ds DataSourceInfo, field
} else {
errorMessage = fmt.Sprintf("Unauthorized to load field '%s', Reason: %s.", fieldPath, reason)
}
r.ctx.appendSubgraphError(goerrors.Join(errors.New(errorMessage),
r.ctx.appendError(goerrors.Join(errors.New(errorMessage),
Copy link
Member

@endigma endigma Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appendError calls errors.Join inside, which takes a variadic second parameter, perhaps we could just pass this through to avoid nesting errors.Join calls and this odd goerrors alias?

NewSubgraphError(ds, fieldPath, reason, 0)))
fastjsonext.AppendErrorWithExtensionsCodeToArray(r.astjsonArena, r.errors, errorMessage, errorcodes.UnauthorizedFieldOrType, r.path)
r.popNodePathElement(nodePath)
Expand Down
Loading