add support for variables in recurse #4385

poonai · 2019-12-09T14:54:14Z

close #3301
Signed-off-by: பாலாஜி balaji@dgraph.io

This change is

Signed-off-by: பாலாஜி <balaji@dgraph.io>

martinmr

Left some comments. Also make sure the final commit message is informative.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @balajijinnah and @manishrjain)

gql/parser.go, line 409 at r1 (raw file):

// update depth

comment is not really adding much.

gql/parser.go, line 423 at r1 (raw file):

// update loop

same here

gql/parser.go, line 809 at r1 (raw file):

return fmt.Sprintf("$%s", item.Val), nil

return "$" + item.Val, nil should work.

gql/parser.go, line 839 at r1 (raw file):

// check

minor: start comments with uppercase.

gql/parser.go, line 847 at r1 (raw file):

Quoted 7 lines of code…

				varName, err := parseVarName(it)
				if err != nil {
					return err
				}
				if gq.RecurseArgs.varMap == nil {
					gq.RecurseArgs.varMap = make(map[string]string)
				}

This code is the same as below. Please look if making this into a method makes sense.

gql/parser_test.go, line 4927 at r1 (raw file):

}

func TestRecurseWithArgs(t *testing.T) {

Add a test where you pass a variable with the wrong type and see if the parser returns an error.

So far all the tests succeed. There should be tests that ensure bad inputs throw errors.

Signed-off-by: பாலாஜி <balaji@dgraph.io>

poonai

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @manishrjain and @martinmr)

gql/parser.go, line 409 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

// update depth
comment is not really adding much.

Done.

gql/parser.go, line 423 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

// update loop
same here

Done.

gql/parser.go, line 809 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

return fmt.Sprintf("$%s", item.Val), nil
return "$" + item.Val, nil should work.

Done.

gql/parser.go, line 847 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

				varName, err := parseVarName(it)
				if err != nil {
					return err
				}
				if gq.RecurseArgs.varMap == nil {
					gq.RecurseArgs.varMap = make(map[string]string)
				}
This code is the same as below. Please look if making this into a method makes sense.

Intially, I too had this thought. But, not able to think of anything

gql/parser_test.go, line 4927 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Add a test where you pass a variable with the wrong type and see if the parser returns an error.

So far all the tests succeed. There should be tests that ensure bad inputs throw errors.

Done.

pawanrawal

The PR looks functionally correct but just needs a little more work.

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @balajijinnah, @manishrjain, and @martinmr)

gql/parser.go, line 409 at r1 (raw file):

Previously, balajijinnah (balaji) wrote…

Done.

if we get
The whole comment can go on the same line.

gql/parser.go, line 423 at r1 (raw file):

Previously, balajijinnah (balaji) wrote…

Done.

if we get the loop
Also the whole comment can go on the same line.

gql/parser.go, line 809 at r1 (raw file):

Previously, balajijinnah (balaji) wrote…

Done.

We should be using the collectName function here otherwise this would not work when the graphql variable name has dashes(-).

gql/parser.go, line 847 at r1 (raw file):

Previously, balajijinnah (balaji) wrote…

Intially, I too had this thought. But, not able to think of anything

This map could have been initialized before so that you don't have to do this check all the time.

gql/parser.go, line 88 at r2 (raw file):

	Depth     uint64
	AllowLoop bool
	varMap    map[string]string

Please add a comment about what this stores.

gql/parser.go, line 419 at r2 (raw file):

			depth, err := strconv.ParseUint(val.Value, 0, 64)
			if err != nil {
				return err

Please wrap the error so that the error message is more user friendly.

errors.Wrapf(err, "couldn't parse the value for depth variable as an integer")

gql/parser.go, line 434 at r2 (raw file):

			allowLoop, err := strconv.ParseBool(val.Value)
			if err != nil {
				return err

wrap this as well

gql/parser_test.go, line 5007 at r2 (raw file):

	_, err = Parse(Request{Str: query, Variables: map[string]string{"$hello": "sd", "$hello1": "true"}})
	require.Error(t, err)
	require.Contains(t, err.Error(), "parsing \"sd\": invalid syntax")

The error should be a bit more human readable like expected $hello to have value of type int. Is that possible to be done here?

gql/parser_test.go, line 5020 at r2 (raw file):

	_, err = Parse(Request{Str: query, Variables: map[string]string{"$hello": "1", "$hello1": "tre"}})
	require.Error(t, err)
	require.Contains(t, err.Error(), "strconv.ParseBool: parsing \"tre\"")

Similarly here, could the error be a bit more easier for the user to understand?

gql/parser_test.go, line 5027 at r2 (raw file):

	{
		me(func: gt(count(~genre), 30000), first: 1) @recurse(depth: 1, loop: true) {
			name@en

I know all the queries are the same and you probably just copy pasted them but they could have been simpler, they don't need 4 predicates and even the function at root could be simpler as what you are testing is just graphql variables.

Signed-off-by: பாலாஜி <balaji@dgraph.io>

poonai

Reviewable status: 0 of 2 files reviewed, 11 unresolved discussions (waiting on @manishrjain, @martinmr, and @pawanrawal)

gql/parser.go, line 809 at r1 (raw file):