graphql-go icon indicating copy to clipboard operation
graphql-go copied to clipboard

Fix bug: make Schema.Validate accept arguments

Open romshark opened this issue 5 years ago • 3 comments

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 avatar Jul 17 '19 00:07 romshark

@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 avatar Sep 10 '19 14:09 pavelnikolov

@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 avatar Sep 10 '19 15:09 romshark

@romshark I'd be happy with that idea. And then have Validate call ValidateWithArgs passing nil for args.

pavelnikolov avatar Sep 12 '19 22:09 pavelnikolov

ValidateWithVariables is implemented now.

pavelnikolov avatar Feb 08 '23 22:02 pavelnikolov