graphql-go
graphql-go copied to clipboard
Fix bug: make Schema.Validate accept arguments
Make Schema.Validate
accept arguments and pass them over to validation.Validate
to allow parameterized queries to be validated correctly.
I'm having a problem with func (s *Schema) Validate(queryString string) []*errors.QueryError
. It doesn't take the arguments as a parameter and thus fails to validate a parameterized query like this one:
query($uid: ID!) {
customer(id: $uid) {
id
firstName
lastName
}
}
graphql: Variable "uid" has invalid value null.
Expected type "ID!", found null. (line 1, column 8);
Without parameterization it validates just fine:
query {
customer(id: "foo") {
id
firstName
lastName
}
}
Now I'm wondering why func (s *Schema) Validate
won't actually take (queryString string, args map[string]interface{})
instead since func Validate
takes nil
arguments right there: validation.Validate(s.schema, doc, nil, s.maxDepth)
.
I just tried passing the argument through and parameterized queries are validated correctly now. Is not taking the args as parameter to func (s *Schema) Validate
a deliberate design decision or just a misunderstanding that's fixed by this PR?
@romshark thank you for your contribution. I am happy with your PR and I'd be happy to merge it. However, there is one problem with it - it introduces a breaking change. Ideally, I want to avoid introducing breaking changes. I would suggest using optional functional parameters to make this feature opt-in.
Before:
func (s *Schema) Validate(queryString string) []*errors.QueryError {
(Potentially) After:
type ValidationConfig struct {
args map[string]interface{}
}
type ValidationOpt func(cfg *ValidationConfig)
func WithArgs(args map[string]interface{}) {
return func(cfg *ValidationConfig) {
cfg.args = args
}
}
func (s *Schema) Validate(queryString string, opts ...ValidationOpt) []*errors.QueryError {
var cfg ValidationConfig
for _, o := range opts {
o(cfg)
}
doc, qErr := query.Parse(queryString)
if qErr != nil {
return []*errors.QueryError{qErr}
}
return validation.Validate(s.schema, doc, cfg.args, s.maxDepth)
}
This way none of the existing clients of the public API would be affect.
// existing users
errs := s.Validate(query)
// users who would like to opt-in for this feature
errs := s.Validate(query, graphql.WithArgs(args))
In addition to that, please, include unit test coverage. Thank you.
@pavelnikolov breaking the API is never a good option. However, ValidationOpt
feels unnecessarily complex. I can't imagine what kind of validation options there'd be other than parameters.
Why not just provide a new simple function instead?
// existing users
errs := s.Validate(query)
// users who would like to opt-in for this feature
errs := s.ValidateWithArgs(query, args)
@romshark I'd be happy with that idea. And then have Validate
call ValidateWithArgs
passing nil
for args.
ValidateWithVariables is implemented now.