Add required variable validation

[dvic]

Expected Behaviour

When using variables for query or mutation arguments:

• if a variable is required
• I expect that I get an error when I omit the variable or specify null.

Actual Behavior

No error is returned.

Minimal graphql.schema and models to reproduce

Using the starwars example, execute the following query and do not specify the id variable:

query($id:ID!) {
  droid(id:$id) {
    id
    name
  }
}

Expected behavior shown on different external playground example: https://graphqlbin.com/nZ4vTx

(message is Variable '$id' expected value of type 'ID!' but value is undefined. Reason: Expected non-null value, found null.)

[vektah]

I'm going to leave this blocked on the new parser for now, which should fix a few validation issues.

[dvic]

Sorry I missed the discussion on the new parser, you mean on https://github.com/graph-gophers/graphql-go? Is there something I can help with?

[vektah]

The parser we are currently using was copied out of the graph-gophers repo because we cant use internal pacakges across repos.

It has fallen behind the spec so it needs some work, and the internal package is hurting collaboration. One of the big blockers is that the scanner is built on go's text/scanner which enforces some go-like syntax on its strings that isn't compatible with graphql.

@tonyghita has made a start on writing a new tokenizer, I'm hoping we can spin it out into a separate repo, maybe under the gophers-graphql org and iterate there without breaking anything until it looks good enough to integrate.

@seeruk is also working on a request parser over in https://github.com/bucketd/go-graphqlparser which looks very fast, but doesn't do any schema parsing.

The validation library sits next to the parser https://github.com/graph-gophers/graphql-go/tree/master/internal/validation. Maybe it makes sense to pull some of that over too, if you have the parsed schema and the parsed request you should have all you need to validate the request.

There might still be some merit in exploring and benchmarking the various scanner generators, a grammar (bnf, g4, etc) might be easier to keep up to date with the spec and if the generator is any good, should be pretty quick.

Its still early days, but if your interested I'll ping you wherever the code ends up. The more 👀 the better.

[dvic]

Cool, thanks for the summary, I'm definitely interested in contributing to this project 👍

[mtibben]

Using g4 / antlr would be a smart move

[vektah]

I thought so too, but the go generator is pretty bad, and the benchmarks are not great:

go test -bench=. -run=none . -benchmem
goos: linux
goarch: amd64
pkg: github.com/bucketd/go-graphqlparser/lexer
BenchmarkLexer/github.com/bucketd/go-graphqlparser-8             1000000              1142 ns/op               0 B/op          0 allocs/op
BenchmarkLexer/github.com/graphql-go/graphql-8                    300000              3966 ns/op             688 B/op         17 allocs/op
BenchmarkLexer/github.com/graphql-gophers/graphql-go-8           1000000              2283 ns/op            1808 B/op         17 allocs/op
BenchmarkLexer/Antlr-8                                            100000             12105 ns/op            3864 B/op         60 allocs/op
PASS
ok      github.com/bucketd/go-graphqlparser/lexer       6.037s

[mtibben]

You can probably expect it to get better over time though, and how big are schemas anyway? Surely we're talking milliseconds in parsing right?

[vektah]

Oh sure, schema parse time is irrelevant it's done offline.

The same scanner is probably shared between both the schema and request parser though. They have the same identifier format, types, string parsing, directives, arg syntax, etc.

[seeruk]

My friend and I are still planning on getting schema parsing working with the existing lexer / parser. I believe the lexer should already be outputting the correct tokens for schemas (not actually tried it yet though!)

For us, it's always been about how this will perform in the context of an HTTP request. We wanted our parser to have the smallest impact possible, and to at least beat the Node.js version in performance. We saw that surprisingly, the reference implementation on a single core was faster than many other implementations (not only in Go, but also for example in Scala).

Personally, I'm extremely excited to see where we can take this.

[vektah]

I've started working on https://github.com/vektah/graphql-parser, the lexer is 100% compatible with graphql-js's token stream & errors (its passing their test suite)

BenchmarkLexer/github.com/bucketd/go-graphqlparser-8             1000000              1147 ns/op               0 B/op          0 allocs/op
BenchmarkLexer/github.com/graphql-go/graphql-8                    300000              3853 ns/op             688 B/op         17 allocs/op
BenchmarkLexer/github.com/graphql-gophers/graphql-go-8           1000000              2323 ns/op            1808 B/op         17 allocs/op
BenchmarkLexer/Antlr-8                                            100000             12223 ns/op            3864 B/op         60 allocs/op
BenchmarkLexer/new_lexer-8                                       1000000              1265 ns/op             176 B/op          3 allocs/op
PASS
ok      github.com/bucketd/go-graphqlparser/lexer       7.338s

Perf is good, the 3 allocs are from the whitespace processing in block strings. There could be a hot path for single line blockquotes but thats probably not the common case, so its probably a sane perf/maintainability tradeoff. Lets take further parser related talk over there.

[vektah]

Fixed in 0.4.0