Fix enum resolution

[technoweenie]

This fixes an obscure issue of enum resolution. It currently uses (*reflect.Value) String(), which only works when the underlying type is a string. Otherwise it returns <%T value>, even if the type implements the Stringer interface. The tests passed before and after the change. However, it wouldn't be a terrible idea to have more tests for enums implemented in different ways in Go.

Let me know if there's anything I can do to get this merged. Happy to write more tests, or even look into a schema validation check that ensures all enums are implemented as []string (in case you disagree that this is a bug).

[tonyghita]

@technoweenie this is a good find, thank you very much for the contribution.

A direct test would be great. Can you expand a bit more on how you ran into this issue and how you've tested the solution?

I wonder how performance would differ if instead of parsing a format string, we tried to assert the Stringer interface.

- fmt.Fprintf(out, "%s", resolver)
+ if str, ok := resolver.(Stringer); ok { // can we do this on *reflect.Value?
+  out.WriteString(str)
+ }

My gut feeling is that we'd come out slightly ahead if we didn't have to parse a format string. Thoughts?

[technoweenie]

A direct test would be great.

Got it 👍

Can you expand a bit more on how you ran into this issue and how you've tested the solution?

I ran into it by trying to support an existing int type that tracked the state of an object. I haven't done exhaustive testing outside of my work in this PR and using it in my app.

My gut feeling is that we'd come out slightly ahead if we didn't have to parse a format string. Thoughts?

No clue, worth a shot though.

EDIT: I'll work on adding a test and experimenting with manually asserting Stringer this week.

[technoweenie]

I've revised this pull with the following changes:

1. Added IntEnum tests to TestInput() in the graphql_test package.
2. Checked for the fmt.Stringer interface as suggested.
3. Added a benchmark to confirm it's the better approach.

I can rip out the benchmark if you don't want it. It's pretty clear your suggestion was better than my initial version though:

$ go test -bench=Enum -benchtime=20s
goos: darwin
goarch: amd64
pkg: github.com/graph-gophers/graphql-go
BenchmarkEnumStringStringer-8   	2000000000	        20.1 ns/op
BenchmarkEnumStringFmt-8        	200000000	       168 ns/op
BenchmarkEnumIntStringer-8      	1000000000	        25.0 ns/op
BenchmarkEnumIntFmt-8           	100000000	       253 ns/op
PASS

[tdb-alcorn]

+1 to this PR. I just ran into this issue and was considering implementing it myself until I saw this.

To anyone else with this problem, fyi my current workaround is to change the resolver's return type to string, converting the enum right before the resolver returns.

[pavelnikolov]

Please, resolve the conflicts in the PR.

[pavelnikolov]

Thank you for your contribution