Skip to content
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

Use error instead of *gqlerror.Error #99

Open
robojones opened this issue May 14, 2019 · 7 comments
Open

Use error instead of *gqlerror.Error #99

robojones opened this issue May 14, 2019 · 7 comments

Comments

@robojones
Copy link

In gqlparser all methods which are returning an error are using the explicit struct type (e.g. *gqlerror.Error or gqlerror.List).

Problem

As long as you use the explicit types everything is fine, but as soon as you convert them to the error interface type, nil checks will fail.

See the golang FAQ for information why the checks fail.
https://golang.org/doc/faq#nil_error

Example

read.go

func ReadSchema(dir string) (*ast.Schema, error) {
  var files []*ast.Source

  // ... read files, may return io errors ...

  // Here the returned *gqlerror.Error gets converted to the error return type.
  return gqlparser.LoadSchema(files...)
}

The ReadSchema function may return io errors and graphql errors. Because of this I can't use *gqlerror.Error as return type. When testing this I noticed that my nil checks failed.

read_test.go

func TestReadSchema(t *testing.T) {
  schema, err := readSchema(schemaTestDir)

  // This check always fails
  assert.Assert(t, err == nil)

  // A workaround is to convert the error back to its struct type.
  // When handling multiple error types this quickly becomes impractical.
  assert.Assert(t, err.(*gqlerror.Error) == nil)
}

It's kind of strange that gqlparser returns errors as struct types. I've never seen anywhere else. All other go packages (including the internal ones) use the error interface type.

I think this should be changed so all methods use the error interface when returning errors.

@vektah
Copy link
Owner

vektah commented May 14, 2019

Yeah this is an disappointing gotcha in go. I wish it would get fixed.

Returning error is fine, but its really only returning one type of error and gqlgen is going to reach into it on the other side. This results in interface overhead and type assertion spaghetti.

All that said, would be open to a pr if you want to do the conversion in gqlgen too.

@robojones
Copy link
Author

I've opened a PR for gqlparser but I'm having trouble adding the conversions you mentioned to gqlgen.
How do I make my gqlgen fork use my gqlparser fork?
If this doesn't work, how will I be able to test my changes?

@vektah
Copy link
Owner

vektah commented May 16, 2019

you can add something like this at the end of the go.mod

replace github.com/vektah/gqlparser => /home/you/projects/gqlparser

and you're done!

One of the best go modules features

@robojones
Copy link
Author

robojones commented May 16, 2019

Okay thanks 😄

@AndrewWPhillips
Copy link
Contributor

AndrewWPhillips commented Sep 3, 2021

I wasted a lot of time trying to work out why ParseQuery was always returning an error (it wasn't).

func Do(schema *ast.Schema, s string) (r string, err error) {
	var query *ast.QueryDocument
	query, err = parser.ParseQuery(&ast.Source{Name: "query", Input: s})
	if err != nil {
		return // err is not nil even when nil is returned for error from ParseQuery
	}
	...
}

Please just change all error return types to error.

@JohnStarich
Copy link

I hit the same issue when I first picked this up, too. It's definitely a surprising behavior, compared to most (if not all) Go libraries I've worked with over the years.

Returning error is fine, but its really only returning one type of error and gqlgen is going to reach into it on the other side. This results in interface overhead and type assertion spaghetti.

@vektah As an idea, I think most potential type assertions in gqlgen could be handled using the built-in errors package. For example:

err := gqlparser.LoadSchema(files...)
var gqlErr *gqlerror.Error
if !errors.As(err, &gqlErr) {
    return err
}
// do gqlparser-specific error handling here
gqlErr.Message

At the very least, the spaghetti could be cleaner spaghetti 😉

@fredzqm
Copy link
Contributor

fredzqm commented Sep 7, 2023

I have a counter-argument.

It looks like the parsing library never return errors other than *gqlerror.Error. It would be nice to capture this characteristics and avoid seemingly unsafe conversion from error back to *gqlerror.Error.

Can I propose a pure additive change?

  • For all private methods, return *gqlerror.Error.
  • For some public methods, add a version that returns *gqlerror.Error and make the current method wraps it and return error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants