[Breaking]fix(GraphQL):Added support for parameterized cascade with variables.

[JatinDev543]

Fixes: GRAPHQL-1007

We haven't added support to pass arguments of parameterized cascade through variables when we added parameterized cascade.
In this PR we have added it and also some tests and validation checks.
Example:
We can pass cascade arguments as variables as given below.

query($fields:[String]) {
  queryPerson1 @cascade(fields:$fields){
    id
    todo
    person  { id1 }
  }
}

variables:{
  "fields": ["todo"]
}

---

This change is 

[abhimanyusinghgaur]

Reviewed 9 of 10 files at r1.
Reviewable status: 9 of 12 files reviewed, 9 unresolved discussions (waiting on @jatindevdg, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

---

graphql/resolve/query_test.go, line 41 at r1 (raw file):

GQLVariables string

Nice catch!

---

graphql/resolve/query_test.yaml, line 1963 at r1 (raw file):

Quoted 33 lines of code…

-
  name: "Parameterized Cascade directive on root field using variables"
  gqlquery: |
    query($fieldsRoot:[String]) {
      queryAuthor @cascade(fields: $fieldsRoot) {
        dob
        reputation
        posts {
          text
          title
        }
      }
    }
  gqlvariables: |
    {
        "fieldsRoot": [
            "dob",
            "reputation"
        ]
    }
  dgquery: |-
    query {
      queryAuthor(func: type(Author)) @cascade(Author.dob, Author.reputation) {
        Author.dob : Author.dob
        Author.reputation : Author.reputation
        Author.posts : Author.posts {
          Post.text : Post.text
          Post.title : Post.title
          dgraph.uid : uid
        }
        dgraph.uid : uid
      }
    }

I think we shouldn't need this as the next test covers it already.

---

graphql/schema/validation_rules.go, line 101 at r1 (raw file):

Quoted 4 lines of code…

			if directive.Arguments.ForName(cascadeArg) == nil {
				return
			}
			fieldArg := directive.Arguments.ForName(cascadeArg)

fieldArg := directive.Arguments.ForName(cascadeArg)
if fieldArg == nil {
  return
}

---

graphql/schema/validation_rules.go, line 106 at r1 (raw file):

isVariable := strings.HasPrefix(fieldArg.Value.String(), "$")

a better check is:

isVariable := fieldArg.Value.Kind == ast.Variable

---

graphql/schema/validation_rules.go, line 107 at r1 (raw file):

Quoted 4 lines of code…

			if directive.ArgumentMap(walker.Variables)[cascadeArg] == nil {
				return
			}
			var variableVal []interface{}

fields, ok := directive.ArgumentMap(walker.Variables)[cascadeArg].([]interface{})
if !ok || val == nil {
  return
}

here fields should have the value for the fields arg, irrespective of whether it came from a variable or it was in the query itself.
So, we don't need to consider variable values separately and hence shouldn't need the GqlFields function.

---

graphql/schema/validation_rules.go, line 113 at r1 (raw file):

variable

should be variables

---

graphql/schema/validation_rules.go, line 127 at r1 (raw file):

						addError(validator.Message(gqlerror.ErrorPathf(validatorPath, err).Error()), validator.At(directive.Position))
						return

let's just correctly format the error here instead of doing addError. It feels redundant. The addError below should take care of adding the error.

err = gqlerror.ErrorPathf(validatorPath, err).Error()

---

graphql/schema/wrappers.go, line 1095 at r1 (raw file):

Quoted 15 lines of code…

	arg := dir.Arguments.ForName(cascadeArg)
	// check valid arg
	if arg == nil || arg.Value == nil {
		return []string{"__all__"}
	}
	val := dir.ArgumentMap(f.op.vars)[cascadeArg].([]interface{})
	isVariable := strings.HasPrefix(arg.Value.String(), "$")
	// check length of arg
	if isVariable {
		if len(val) == 0 {
			return []string{"__all__"}
		}
	} else if len(arg.Value.Children) == 0 {
		return []string{"__all__"}
	}

we can just find val first and then just check it's length, no need for separate cases for variable and raw cases:

val := dir.ArgumentMap(f.op.vars)[cascadeArg].([]interface{})
if len(val) == 0 {
  return []string{"__all__"}
}

---

graphql/schema/wrappers.go, line 1118 at r1 (raw file):

}

func GqlFields(val []interface{}, arg *ast.Argument, varFlag bool) []string {

shouldn't be required now

[abhimanyusinghgaur]

Reviewable status: 9 of 12 files reviewed, 9 unresolved discussions (waiting on @jatindevdg, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

---

graphql/schema/wrappers.go, line 1095 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	arg := dir.Arguments.ForName(cascadeArg)
	// check valid arg
	if arg == nil || arg.Value == nil {
		return []string{"__all__"}
	}
	val := dir.ArgumentMap(f.op.vars)[cascadeArg].([]interface{})
	isVariable := strings.HasPrefix(arg.Value.String(), "$")
	// check length of arg
	if isVariable {
		if len(val) == 0 {
			return []string{"__all__"}
		}
	} else if len(arg.Value.Children) == 0 {
		return []string{"__all__"}
	}

we can just find val first and then just check it's length, no need for separate cases for variable and raw cases:

val := dir.ArgumentMap(f.op.vars)[cascadeArg].([]interface{})
if len(val) == 0 {
  return []string{"__all__"}
}

val, _ := dir.ArgumentMap(f.op.vars)[cascadeArg].([]interface{})
if len(val) == 0 {
  return []string{"__all__"}
}

[JatinDev543]

Reviewable status: 5 of 12 files reviewed, 9 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

---

graphql/resolve/query_test.yaml, line 1963 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

-
  name: "Parameterized Cascade directive on root field using variables"
  gqlquery: |
    query($fieldsRoot:[String]) {
      queryAuthor @cascade(fields: $fieldsRoot) {
        dob
        reputation
        posts {
          text
          title
        }
      }
    }
  gqlvariables: |
    {
        "fieldsRoot": [
            "dob",
            "reputation"
        ]
    }
  dgquery: |-
    query {
      queryAuthor(func: type(Author)) @cascade(Author.dob, Author.reputation) {
        Author.dob : Author.dob
        Author.reputation : Author.reputation
        Author.posts : Author.posts {
          Post.text : Post.text
          Post.title : Post.title
          dgraph.uid : uid
        }
        dgraph.uid : uid
      }
    }

I think we shouldn't need this as the next test covers it already.

removed.

---

graphql/schema/validation_rules.go, line 101 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

			if directive.Arguments.ForName(cascadeArg) == nil {
				return
			}
			fieldArg := directive.Arguments.ForName(cascadeArg)

fieldArg := directive.Arguments.ForName(cascadeArg)
if fieldArg == nil {
  return
}

added.

---

graphql/schema/validation_rules.go, line 106 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

isVariable := strings.HasPrefix(fieldArg.Value.String(), "$")

a better check is:

isVariable := fieldArg.Value.Kind == ast.Variable

added.

---

graphql/schema/validation_rules.go, line 107 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

			if directive.ArgumentMap(walker.Variables)[cascadeArg] == nil {
				return
			}
			var variableVal []interface{}

fields, ok := directive.ArgumentMap(walker.Variables)[cascadeArg].([]interface{})
if !ok || val == nil {
  return
}

here fields should have the value for the fields arg, irrespective of whether it came from a variable or it was in the query itself.
So, we don't need to consider variable values separately and hence shouldn't need the GqlFields function.

removed GQLFields.

---

graphql/schema/validation_rules.go, line 113 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

variable

should be variables

done.

---

graphql/schema/validation_rules.go, line 127 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

						addError(validator.Message(gqlerror.ErrorPathf(validatorPath, err).Error()), validator.At(directive.Position))
						return

let's just correctly format the error here instead of doing addError. It feels redundant. The addError below should take care of adding the error.

err = gqlerror.ErrorPathf(validatorPath, err).Error()

formated.

---

graphql/schema/wrappers.go, line 1095 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

val, _ := dir.ArgumentMap(f.op.vars)[cascadeArg].([]interface{})
if len(val) == 0 {
  return []string{"__all__"}
}

done.

---

graphql/schema/wrappers.go, line 1118 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

shouldn't be required now

removed.

[abhimanyusinghgaur]

Reviewed 6 of 7 files at r2.
Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @jatindevdg, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

---

graphql/schema/validation_rules.go, line 105 at r2 (raw file):

Quoted 4 lines of code…

			fieldsVal, ok := directive.ArgumentMap(walker.Variables)[cascadeArg].([]interface{})
			if !ok || fieldsVal == nil {
				return
			}

we can make it more simpler:

fieldsVal, _ := directive.ArgumentMap(walker.Variables)[cascadeArg].([]interface{})
if len(fieldsVal) == 0 {
  return
}

its better to use len() == 0 checks for slices that to use slice == nil

---

graphql/schema/wrappers.go, line 1086 at r2 (raw file):

Quoted 4 lines of code…

	fieldArg := dir.Arguments.ForName(cascadeArg)
	if fieldArg == nil {
		return []string{"__all__"}
	}

this shouldn't be needed too.
This case would be taken care of by the next few lines where ArgumentMap()[cascadeArg] will return an empty slice if there were no fields arg.

---

graphql/schema/wrappers.go, line 1091 at r2 (raw file):

	fieldsVal, ok := dir.ArgumentMap(f.op.vars)[cascadeArg].([]interface{})
	if !ok || fieldsVal == nil {

can simplify this too to just use len(slice) == 0 and replace ok with _.

[JatinDev543]

Reviewable status: 10 of 12 files reviewed, 4 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

---

graphql/schema/validation_rules.go, line 105 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

			fieldsVal, ok := directive.ArgumentMap(walker.Variables)[cascadeArg].([]interface{})
			if !ok || fieldsVal == nil {
				return
			}

we can make it more simpler:

fieldsVal, _ := directive.ArgumentMap(walker.Variables)[cascadeArg].([]interface{})
if len(fieldsVal) == 0 {
  return
}

its better to use len() == 0 checks for slices that to use slice == nil

done.

---

graphql/schema/wrappers.go, line 1086 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	fieldArg := dir.Arguments.ForName(cascadeArg)
	if fieldArg == nil {
		return []string{"__all__"}
	}

this shouldn't be needed too.
This case would be taken care of by the next few lines where ArgumentMap()[cascadeArg] will return an empty slice if there were no fields arg.

removed.

---

graphql/schema/wrappers.go, line 1091 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	fieldsVal, ok := dir.ArgumentMap(f.op.vars)[cascadeArg].([]interface{})
	if !ok || fieldsVal == nil {

can simplify this too to just use len(slice) == 0 and replace ok with _.

done.