Use github.com/pkg/errors and store cause of errors in FormattedError

[dvic]

No description provided.

[coveralls]

Coverage decreased (-0.1%) to 80.829% when pulling 4c643dd on dvic:keep-cause-errors into c35670f on graphql-go:master.

[sharonjl]

+1 Certainly in need of this one. Can we get this merged into master asap?

[nicolaiskogheim]

There are a lot of changes in this PR, making it difficult to review. I think you'll have better luck getting it reviewed if you focus the PR to do what the title says it should do.
Helper functions in tests, for instance, is nice but it's just noise when trying to figure out what the impact of using pkg/errors is.
My suggestion is to try and split your work into multiple commits. It would then be easier to know the intention of your changes, and review smaller chunks at a time. You could also open other pull requests if the commits your create are valid changes of their own.

[dvic]

Thanks for the suggestion 👍 However, those helpers are related to the change because otherwise it is a lot of work to update all the tests manually. Due to the new error being present almost all of the tests failed.

Of course I could invest more time and revert those changes in the tests but to be honest I have the feeling it would not result in my PR being merged, as I have seen very little response from the maintainers on my PR's for this repo 😕

[nicolaiskogheim]

I'm working on a solution to the whole error formatting problem, which is why I'm taking interest in this PR. If this repo is stale I'll just need to vendor it myself, but I would like to not repeat others work if I don't have to.

It would be easier to discuss your approach if you split up the work in commits. That shouldn't be too hard to do. (I have no idea how proficient you are with git, so I'm just going to assume very basic knowledge here and give you the steps, just in case :) )

# cd to your fork
$ git stash # or whatever, just make sure your working directory is clean
$ git checkout keep-cause-errors
$ git reset @~ # undo's the commit, but leaves all changes
$ git diff # Take a glance and decide which change you want to commit first
$ git add -p # Only add changes/patches that belongs in the commit you're creating. Use h to view help
$ git commit -m "Refactor tests to use helper for error message matching" # Commit the changes
# repeat steps with diff, add, and commit until done
$ git push -f # overwrites this PR with your new commits

And if you do this, you might as well drop changes like adding -race flag to goveralls and adding 1.10rc1 to go in .travis.yml.

[dvic]

Sure no problem, I'll split it in logical commits, give me 5 minutes 😉

[dvic]

I have split the PR into three separate commits, let's see what TravisCI has to say 🤞

[dvic]

Updated the first commit just to include github.com/pkg/errors.

[nicolaiskogheim]

Hey, sorry for disappearing, hehe.

Storing the original error and making it available outside this library is a good idea.
It would solve my use case, which seems very similar to yours.
There's no reason to import pkg/errors though.

May I suggest we change the name of Cause()?
The reason I don't like Cause() is that the error can be something else, like in both your and my case where we wrap errors with messages and stack traces.
Your example of using this new functionality, I think, is an argument for another name:

res := graphql.Do(params)
if res.HasErrors() {
	for _, err := range res.Errors {
		if err, ok := errors.Cause(err.Cause()).(interface {
			AsPublicJSON() []byte
		}); ok {
			err.AsPublicJSON()))
		}
	}
}

Specifically this: errors.Cause(err.Cause()).
And it may not be obvious for people that aren't using pkg/errors what Cause means.
OriginalError may be a long name, but I would rather have something like that.

[dvic]

Yes Cause is probably confusing in many situations. What about Underlying()?

Regarding the use of pkg/errors: you mean that we implement our own error wrapper that implements the Causer interface? (i.e., has the method Cause() error, as explained here)

[ruben-podadera]

Hello there,

just here to support this PR. I have the same need. Can't wait to see it merged !

[nicolaiskogheim]

In the context of type FormattedError struct, cause makes sense, and I would vote to keep that name.

//  gqlerrors/formatted.go
type FormattedError struct {
	Message   string                    `json:"message"`
	Locations []location.SourceLocation `json:"locations"`
	cause     error
}

But from the perspective of the user facing api, Cause() seems weird because we also expose Error().

// gqlerrors/formatted.go
func (g FormattedError) Error() string {
	return g.Message
}

func (g FormattedError) Cause() error {
	return g.cause
}

I'm fine with Underlying(), I think. Then we would have these two:

func (g FormattedError) Error() string {
	return g.Message
}

func (g FormattedError) Underlying() error {
	return g.cause
}

---

I noticed that in FormatError you only set the cause field in the default: case.
Should we not always set cause?
In the code below I set cause to the passed error in all cases.

func FormatError(err error) FormattedError {
	switch err := err.(type) {
	case FormattedError:
		return err
	case *Error:
		return FormattedError{
			Message:   err.Error(),
			Locations: err.Locations,
                        cause: err // <-- or err.OriginalError (I discuss this in the last paragraph)
		}
	case Error:
		return FormattedError{
			Message:   err.Error(),
			Locations: err.Locations,
                        cause: err // <--
		}
	default:
		return FormattedError{
			Message:   err.Error(),
			Locations: []location.SourceLocation{},
			cause:     err,
		}
	}
}

The Error type that FormatError tries to match on looks like this:

// gqlerrors/errors.go
type Error struct {
	Message       string
	Stack         string
	Nodes         []ast.Node
	Source        *source.Source
	Positions     []int
	Locations     []location.SourceLocation
	OriginalError error
}

I don't know what the sources of Error is, i.e. are they only created when graphql queries are invalid, or can the be created after my resolvers have returned? Consequently I don't know what
would be in OriginalError. There could be reasons that we would want to unwrap the Error error, and just take err.OriginalError when we create a FormattedError.
I feel like keeping the whole error, because I think that I will be able to use the information in the Error struct, but I might be mistaken.

[tgwizard]

Any news on this PR? It seems like a great addition to me, and apparently others too (given the duplicate in #379).

The main reason I need this is two-fold: To keep error formatting of internal errors (like upstream "resource not found") in one place, and to transform panics and unexpected server errors so we don't leak internal details (which might be sensitive).

It seems to me that three things remain to be fixed:

• Rename cause to originalError and Cause() to OriginalError(). However, if we want to work fully with github.com/pkg/errors, it seems keeping the name Cause() would be nice.
• Update FormatError to set cause in all causes (except the FormattedError one).
• Fix the merge conflicts (and perhaps clean up the extra "Merge branch 'master' into keep-cause-errors" commits)

@dvic @nicolaiskogheim @chris-ramon do you think you'll get to this soon, or want to? If not, I'd be happy to take a stab at fixing the last things (probably through a new PR).

[dvic]

I'm gonna go ahead and close this PR as I think it's outdated. I will keep the branch around, feel free to fork it and continue the work from there.