Skip to content

{types,graphql_test}: Adds Result.ErrorsJoined implementation. #717

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
31 changes: 31 additions & 0 deletions graphql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ package graphql_test

import (
"context"
"errors"
"reflect"
"testing"

"github.com/graphql-go/graphql"
"github.com/graphql-go/graphql/gqlerrors"
"github.com/graphql-go/graphql/testutil"
)

Expand Down Expand Up @@ -268,3 +270,32 @@ func TestEmptyStringIsNotNull(t *testing.T) {
t.Errorf("wrong result, query: %v, graphql result diff: %v", query, testutil.Diff(expected, result))
}
}

func TestResultErrorsJoinedFailure(t *testing.T) {
r := graphql.Result{}

if err := r.ErrorsJoined(); err != nil {
t.Fatalf("wrong result, want: nil, got: %v", err)
}
}

func TestResultErrorsJoinedSuccess(t *testing.T) {
r := graphql.Result{
Errors: []gqlerrors.FormattedError{
{Message: "first error"},
{Message: "second error"},
},
}

expected := errors.New("second error: first error")
Comment on lines +282 to +290
Copy link

Choose a reason for hiding this comment

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

It might be worth adding a test that verifies the error wrapping behavior works correctly with errors.Is or errors.As. This would ensure that the wrapping with %w is functioning as expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great call, it was addressed it via: 9bd0b67


if err := r.ErrorsJoined(); err != nil {
if !reflect.DeepEqual(err.Error(), expected.Error()) {
Copy link

Choose a reason for hiding this comment

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

Minor suggestion: When comparing error messages, you can directly compare the strings rather than using reflect.DeepEqual. It's simpler and more explicit about what you're comparing.

Suggested change
if !reflect.DeepEqual(err.Error(), expected.Error()) {
if err.Error() != expected.Error() {

Copy link
Member Author

Choose a reason for hiding this comment

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

Great call, addressed via the comment above: #717 (comment)

t.Fatalf("wrong result, want: %v, got: %v", expected, err)
}

return
}

t.Fatalf("wrong result, got: nil, want: %v", expected)
}
22 changes: 22 additions & 0 deletions types.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package graphql

import (
"fmt"

"github.com/graphql-go/graphql/gqlerrors"
)

Expand All @@ -17,3 +19,23 @@ type Result struct {
func (r *Result) HasErrors() bool {
return len(r.Errors) > 0
}

// ErrorsJoined joins and returns the result errors.
func (r *Result) ErrorsJoined() error {
if r.Errors == nil {
return nil
}

var result error
for _, err := range r.Errors {
if result == nil {
result = fmt.Errorf("%w", err)

continue
}

result = fmt.Errorf("%w: %w", err, result)
}

return result
}
Copy link

Choose a reason for hiding this comment

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

I noticed that the errors are being joined in reverse order (the first error in the slice ends up at the end of the error message). Is this intentional?

Also, it might be worth adding a bit more documentation about the behavior, particularly that:

  1. It returns nil if there are no errors
  2. The error wrapping preserves the original error types (so errors.Is would work with the wrapped errors)
  3. The formatting of the joined error (with : as separator)

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that the errors are being joined in reverse order (the first error in the slice ends up at the end of the error message). Is this intentional?

Yes because we want to show the last error first, similar how stack traces work in order to be aware what happened with the program bottom-up.


Also, it might be worth adding a bit more documentation about the behavior, particularly that:

  1. It returns nil if there are no errors
  2. The error wrapping preserves the original error types (so errors.Is would work with the wrapped errors)
  3. The formatting of the joined error (with : as separator)

Ok sounds good, answering inline:

  1. It returns nil if there are no errors

Great call, but it is redundant to add it as comment since the zero value of error is nil and is common understanding that behavior therefore not added as a part of the code comments.

  1. The error wrapping preserves the original error types (so errors.Is would work with the wrapped errors)

Ok, that's a great suggestion, for the test, on the implementation side there's no use case for leveraging errors.Is, I went ahead and updated it in the following git commit: 9bd0b67

  1. The formatting of the joined error (with : as separator)

Ok, that's great suggestion as well, I went ahead and added it as part of the code comment, added via the git commit: b2f18a1