From 70e17692c4773f9b02f0c8dda9c1642a1627f26d Mon Sep 17 00:00:00 2001 From: Pawan Rawal Date: Tue, 23 Jul 2019 13:10:33 +1000 Subject: [PATCH 01/10] Allow parsing uid function in from/to arguments for shortest path query. --- gql/parser.go | 92 +++++++++++++++++++++++++++++++++++++--------- gql/parser_test.go | 23 +++++++++++- 2 files changed, 95 insertions(+), 20 deletions(-) diff --git a/gql/parser.go b/gql/parser.go index fec5f4e60b8..86dd3b6c534 100644 --- a/gql/parser.go +++ b/gql/parser.go @@ -53,21 +53,22 @@ type GraphQuery struct { Args map[string]string // Query can have multiple sort parameters. - Order []*pb.Order - Children []*GraphQuery - Filter *FilterTree - MathExp *MathTree - Normalize bool - Recurse bool - RecurseArgs RecurseArgs - Cascade bool - IgnoreReflex bool - Facets *pb.FacetParams - FacetsFilter *FilterTree - GroupbyAttrs []GroupByAttr - FacetVar map[string]string - FacetOrder string - FacetDesc bool + Order []*pb.Order + Children []*GraphQuery + Filter *FilterTree + MathExp *MathTree + Normalize bool + Recurse bool + RecurseArgs RecurseArgs + ShortestPathArgs ShortestPathArgs + Cascade bool + IgnoreReflex bool + Facets *pb.FacetParams + FacetsFilter *FilterTree + GroupbyAttrs []GroupByAttr + FacetVar map[string]string + FacetOrder string + FacetDesc bool // Internal fields below. // If gq.fragment is nonempty, then it is a fragment reference / spread. @@ -90,6 +91,11 @@ type RecurseArgs struct { AllowLoop bool } +type ShortestPathArgs struct { + From *Function + To *Function +} + // GroupByAttr stores the arguments needed to process the @groupby directive. type GroupByAttr struct { Attr string @@ -2365,7 +2371,8 @@ func getRoot(it *lex.ItemIterator) (gq *GraphQuery, rerr error) { key = item.Val expectArg = false } else if item.Typ == itemRightRound { - if gq.Func == nil && len(gq.NeedsVar) == 0 && len(gq.Args) == 0 { + if gq.Func == nil && len(gq.NeedsVar) == 0 && len(gq.Args) == 0 && + gq.ShortestPathArgs.From == nil && gq.ShortestPathArgs.To == nil { // Used to do aggregation at root which would be fetched in another block. gq.IsEmpty = true } @@ -2392,7 +2399,8 @@ func getRoot(it *lex.ItemIterator) (gq *GraphQuery, rerr error) { return nil, item.Errorf("Expecting a colon. Got: %v", item) } - if key == "func" { + switch key { + case "func": // Store the generator function. if gq.Func != nil { return gq, item.Errorf("Only one function allowed at root") @@ -2406,7 +2414,55 @@ func getRoot(it *lex.ItemIterator) (gq *GraphQuery, rerr error) { } gq.Func = gen gq.NeedsVar = append(gq.NeedsVar, gen.NeedsVar...) - } else { + case "from", "to": + if gq.Alias != "shortest" { + return gq, item.Errorf("from/to only allowed for shortest path queries") + } + + fn := &Function{} + // from, to can have a uid or a uid function as the argument. + // 1. from: 0x01 + // 2. from: uid(0x01) + // 3. from: uid(p) // a variable + peekIt, err := it.Peek(1) + if err != nil { + return nil, item.Errorf("Invalid query") + } + + if peekIt[0].Val != uid { + // This means its not a uid function, so it has to be an actual uid. + it.Next() + item := it.Item() + val := collectName(it, item.Val) + uid, err := strconv.ParseUint(val, 0, 64) + switch e := err.(type) { + case nil: + fn.UID = append(fn.UID, uid) + case *strconv.NumError: + if e.Err == strconv.ErrRange { + return nil, item.Errorf("The uid value %q is too large.", val) + } + } + if key == "from" { + gq.ShortestPathArgs.From = fn + } else { + gq.ShortestPathArgs.To = fn + } + continue + } + + gen, err := parseFunction(it, gq) + if err != nil { + return gq, err + } + fn.NeedsVar = gen.NeedsVar + fn.Name = gen.Name + if key == "from" { + gq.ShortestPathArgs.From = fn + } else { + gq.ShortestPathArgs.To = fn + } + default: var val string if !it.Next() { return nil, it.Errorf("Invalid query") diff --git a/gql/parser_test.go b/gql/parser_test.go index 0cd060aa068..b9c8e075235 100644 --- a/gql/parser_test.go +++ b/gql/parser_test.go @@ -1071,13 +1071,32 @@ func TestParseShortestPath(t *testing.T) { require.NoError(t, err) require.NotNil(t, res.Query) require.Equal(t, 1, len(res.Query)) - require.Equal(t, "0x0a", res.Query[0].Args["from"]) - require.Equal(t, "0x0b", res.Query[0].Args["to"]) + require.Equal(t, uint64(0xa), res.Query[0].ShortestPathArgs.From.UID[0]) + require.Equal(t, uint64(0xb), res.Query[0].ShortestPathArgs.To.UID[0]) require.Equal(t, "3", res.Query[0].Args["numpaths"]) require.Equal(t, "3", res.Query[0].Args["minweight"]) require.Equal(t, "6", res.Query[0].Args["maxweight"]) } +func TestParseShortestPathWithVariables(t *testing.T) { + query := ` + { + shortest(from: uid(p), to: uid(q)) { + friends + name + } + } +` + res, err := Parse(Request{Str: query}) + require.NoError(t, err) + require.NotNil(t, res.Query[0].ShortestPathArgs.From) + require.Equal(t, 1, len(res.Query[0].ShortestPathArgs.From.NeedsVar)) + require.Equal(t, "p", res.Query[0].ShortestPathArgs.From.NeedsVar[0].Name) + require.Equal(t, "uid", res.Query[0].ShortestPathArgs.From.Name) + require.NotNil(t, res.Query[0].ShortestPathArgs.To) + require.Equal(t, 1, len(res.Query[0].ShortestPathArgs.To.NeedsVar)) +} + func TestParseMultipleQueries(t *testing.T) { query := ` { From 8374d0a6e6f8938ebcafe9c835233a8e6ea95b46 Mon Sep 17 00:00:00 2001 From: Pawan Rawal Date: Tue, 23 Jul 2019 13:31:19 +1000 Subject: [PATCH 02/10] Parse From/To from ShortestPathArgs rather than Args map in query.go. --- query/query.go | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/query/query.go b/query/query.go index 43c9a494dc4..a716d28041c 100644 --- a/query/query.go +++ b/query/query.go @@ -134,6 +134,7 @@ type params struct { Cascade bool IgnoreReflex bool + // From and To are uids of the nodes between which we run the shortest path query. From uint64 To uint64 Facet *pb.FacetParams @@ -858,22 +859,6 @@ func (args *params) fill(gq *gql.GraphQuery) error { args.numPaths = int(numPaths) } - if v, ok := gq.Args["from"]; ok { - from, err := strconv.ParseUint(v, 0, 64) - if err != nil { - return err - } - args.From = uint64(from) - } - - if v, ok := gq.Args["to"]; ok { - to, err := strconv.ParseUint(v, 0, 64) - if err != nil { - return err - } - args.To = uint64(to) - } - if v, ok := gq.Args["maxweight"]; ok { maxWeight, err := strconv.ParseFloat(v, 64) if err != nil { @@ -893,6 +878,18 @@ func (args *params) fill(gq *gql.GraphQuery) error { } else if !ok { args.MinWeight = -math.MaxFloat64 } + + if gq.ShortestPathArgs.From == nil || gq.ShortestPathArgs.To == nil { + return errors.Errorf("Unexpected error: from/to can't be nil for shortest path") + } + // TODO - Add validation, either length of UID slice should be greater than zero or + // NeedsVar. + if len(gq.ShortestPathArgs.From.UID) > 0 { + args.From = gq.ShortestPathArgs.From.UID[0] + } + if len(gq.ShortestPathArgs.To.UID) > 0 { + args.To = gq.ShortestPathArgs.To.UID[0] + } } if v, ok := gq.Args["first"]; ok { From d25083219c99f040f05bb52ad95a752200f8d7da Mon Sep 17 00:00:00 2001 From: Pawan Rawal Date: Tue, 23 Jul 2019 14:01:50 +1000 Subject: [PATCH 03/10] Collect NeedsVar from shortest path as well so that checkDependency doesn't complain. --- gql/parser.go | 10 ++++++++++ gql/parser_test.go | 15 +++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/gql/parser.go b/gql/parser.go index 86dd3b6c534..b396936ea8d 100644 --- a/gql/parser.go +++ b/gql/parser.go @@ -659,6 +659,16 @@ func (gq *GraphQuery) collectVars(v *Vars) { if gq.MathExp != nil { gq.MathExp.collectVars(v) } + if gq.ShortestPathArgs.From != nil && len(gq.ShortestPathArgs.From.NeedsVar) > 0 { + for _, uidVar := range gq.ShortestPathArgs.From.NeedsVar { + v.Needs = append(v.Needs, uidVar.Name) + } + } + if gq.ShortestPathArgs.To != nil && len(gq.ShortestPathArgs.To.NeedsVar) > 0 { + for _, uidVar := range gq.ShortestPathArgs.To.NeedsVar { + v.Needs = append(v.Needs, uidVar.Name) + } + } } func (f *MathTree) collectVars(v *Vars) { diff --git a/gql/parser_test.go b/gql/parser_test.go index b9c8e075235..b3d5d85dbc9 100644 --- a/gql/parser_test.go +++ b/gql/parser_test.go @@ -1097,6 +1097,21 @@ func TestParseShortestPathWithVariables(t *testing.T) { require.Equal(t, 1, len(res.Query[0].ShortestPathArgs.To.NeedsVar)) } +func TestParseShortestPathWithUidVars(t *testing.T) { + query := `{ + a as var(func: uid(0x01)) + b as var(func: uid(0x02)) + + shortest(from: uid(a), to: uid(b)) { + password + friend + } + + }` + _, err := Parse(Request{Str: query}) + require.NoError(t, err) +} + func TestParseMultipleQueries(t *testing.T) { query := ` { From 6b44948d82dfc7f065a88bab10b504c98b9b75e9 Mon Sep 17 00:00:00 2001 From: Pawan Rawal Date: Tue, 23 Jul 2019 14:50:28 +1000 Subject: [PATCH 04/10] Fill vars for shortest path query. --- query/query.go | 79 ++++++++++++++++++++++++++++++++------------ query/query3_test.go | 16 +++++++++ 2 files changed, 73 insertions(+), 22 deletions(-) diff --git a/query/query.go b/query/query.go index a716d28041c..ddad4332f9a 100644 --- a/query/query.go +++ b/query/query.go @@ -128,11 +128,12 @@ type params struct { Langs []string // directives. - Normalize bool - Recurse bool - RecurseArgs gql.RecurseArgs - Cascade bool - IgnoreReflex bool + Normalize bool + Recurse bool + RecurseArgs gql.RecurseArgs + ShortestPathArgs gql.ShortestPathArgs + Cascade bool + IgnoreReflex bool // From and To are uids of the nodes between which we run the shortest path query. From uint64 @@ -958,23 +959,24 @@ func newGraph(ctx context.Context, gq *gql.GraphQuery) (*SubGraph, error) { // For the root, the name to be used in result is stored in Alias, not Attr. // The attr at root (if present) would stand for the source functions attr. args := params{ - Alias: gq.Alias, - Cascade: gq.Cascade, - GetUid: isDebug(ctx), - IgnoreReflex: gq.IgnoreReflex, - IsEmpty: gq.IsEmpty, - Langs: gq.Langs, - NeedsVar: append(gq.NeedsVar[:0:0], gq.NeedsVar...), - Normalize: gq.Normalize, - Order: gq.Order, - ParentVars: make(map[string]varValue), - Recurse: gq.Recurse, - RecurseArgs: gq.RecurseArgs, - Var: gq.Var, - groupbyAttrs: gq.GroupbyAttrs, - isGroupBy: gq.IsGroupby, - uidCount: gq.UidCount, - uidCountAlias: gq.UidCountAlias, + Alias: gq.Alias, + Cascade: gq.Cascade, + GetUid: isDebug(ctx), + IgnoreReflex: gq.IgnoreReflex, + IsEmpty: gq.IsEmpty, + Langs: gq.Langs, + NeedsVar: append(gq.NeedsVar[:0:0], gq.NeedsVar...), + Normalize: gq.Normalize, + Order: gq.Order, + ParentVars: make(map[string]varValue), + Recurse: gq.Recurse, + RecurseArgs: gq.RecurseArgs, + ShortestPathArgs: gq.ShortestPathArgs, + Var: gq.Var, + groupbyAttrs: gq.GroupbyAttrs, + isGroupBy: gq.IsGroupby, + uidCount: gq.UidCount, + uidCountAlias: gq.UidCountAlias, } for argk := range gq.Args { @@ -1692,6 +1694,39 @@ func (sg *SubGraph) recursiveFillVars(doneVars map[string]varValue) error { } func (sg *SubGraph) fillVars(mp map[string]varValue) error { + if sg.Params.Alias == "shortest" { + if sg.Params.ShortestPathArgs.From != nil && len(sg.Params.ShortestPathArgs.From.NeedsVar) > 0 { + fromVar := sg.Params.ShortestPathArgs.From.NeedsVar[0].Name + uidVar, ok := mp[fromVar] + if !ok { + return errors.Errorf("value of from var(%s) should have already been populated", + fromVar) + } + if uidVar.Uids != nil { + if len(uidVar.Uids.Uids) > 1 { + return errors.Errorf("from variable(%s) should only expand to 1 uid", fromVar) + } + sg.Params.From = uidVar.Uids.Uids[0] + } + } + + if sg.Params.ShortestPathArgs.To != nil && len(sg.Params.ShortestPathArgs.To.NeedsVar) > 0 { + toVar := sg.Params.ShortestPathArgs.To.NeedsVar[0].Name + uidVar, ok := mp[toVar] + if !ok { + return errors.Errorf("value of to var(%s) should have already been populated", + toVar) + } + if uidVar.Uids != nil { + if len(uidVar.Uids.Uids) > 1 { + return errors.Errorf("to variable(%s) should only expand to 1 uid", toVar) + } + sg.Params.To = uidVar.Uids.Uids[0] + } + fmt.Println("to: ", sg.Params.To) + } + } + var lists []*pb.List for _, v := range sg.Params.NeedsVar { if l, ok := mp[v.Name]; ok { diff --git a/query/query3_test.go b/query/query3_test.go index 1a3036d684a..c22be7e83fb 100644 --- a/query/query3_test.go +++ b/query/query3_test.go @@ -535,6 +535,22 @@ func TestShortestPathPassword(t *testing.T) { "me":[{"name":"Michonne"},{"name":"Andrea"}]}}`, js) } +func TestShortestPathWithUidVariable(t *testing.T) { + query := ` + { + a as var(func: uid(0x01)) + b as var(func: uid(31)) + + shortest(from: uid(a), to: uid(b)) { + password + friend + } + }` + js := processQueryNoErr(t, query) + require.JSONEq(t, + `{"data": {"_path_":[{"uid":"0x1", "_weight_": 1, "friend":{"uid":"0x1f"}}]}}`, js) +} + func TestFacetVarRetrieval(t *testing.T) { query := ` From 3d7600cc47a95676a1546fa4467772a9ed6cacb5 Mon Sep 17 00:00:00 2001 From: Pawan Rawal Date: Tue, 23 Jul 2019 15:31:37 +1000 Subject: [PATCH 05/10] Add a couple of more test cases for variables with shortest path. --- query/query3_test.go | 56 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/query/query3_test.go b/query/query3_test.go index c22be7e83fb..e17a5759ee0 100644 --- a/query/query3_test.go +++ b/query/query3_test.go @@ -551,6 +551,38 @@ func TestShortestPathWithUidVariable(t *testing.T) { `{"data": {"_path_":[{"uid":"0x1", "_weight_": 1, "friend":{"uid":"0x1f"}}]}}`, js) } +func TestShortestPathWithUidVariableAndFunc(t *testing.T) { + query := ` + { + a as var(func: eq(name, "Michonne")) + b as var(func: eq(name, "Andrea")) + + shortest(from: uid(a), to: uid(b)) { + password + friend + } + }` + js := processQueryNoErr(t, query) + require.JSONEq(t, + `{"data": {"_path_":[{"uid":"0x1", "_weight_": 1, "friend":{"uid":"0x1f"}}]}}`, js) +} + +func TestShortestPathWithUidVariableError(t *testing.T) { + query := ` + { + a as var(func: eq(name, "Alice")) + b as var(func: eq(name, "Andrea")) + + shortest(from: uid(a), to: uid(b)) { + password + friend + } + }` + + _, err := processQuery(context.Background(), t, query) + require.Error(t, err) +} + func TestFacetVarRetrieval(t *testing.T) { query := ` @@ -698,6 +730,30 @@ func TestShortestPath_filter2(t *testing.T) { require.JSONEq(t, `{"data": { "me": []}}`, js) } +func TestTwoShortestPathVariable(t *testing.T) { + + query := ` + { + a as var(func: uid(1)) + b as var(func: uid(1002)) + + A as shortest(from: uid(a), to: uid(b), numpaths: 2) { + path + } + + me(func: uid(A)) { + name + } + }` + js := processQueryNoErr(t, query) + require.JSONEq(t, + `{"data": {"_path_":[ + {"uid":"0x1","_weight_":3,"path":{"uid":"0x1f","path":{"uid":"0x3e8","path":{"uid":"0x3ea"}}}}, + {"uid":"0x1","_weight_":4,"path":{"uid":"0x1f","path":{"uid":"0x3e8","path":{"uid":"0x3e9","path":{"uid":"0x3ea"}}}}}], + "me":[{"name":"Michonne"},{"name":"Andrea"},{"name":"Alice"},{"name":"Matt"}]}}`, + js) +} + func TestUseVarsFilterMultiId(t *testing.T) { query := ` From 2906e97d075be157162c0d1d3bde894745649baf Mon Sep 17 00:00:00 2001 From: Pawan Rawal Date: Tue, 23 Jul 2019 16:23:20 +1000 Subject: [PATCH 06/10] Self review of parser.go and some refactoring. --- gql/parser.go | 75 ++++++++++++++++++++++++---------------------- gql/parser_test.go | 40 ++++++++++++------------- 2 files changed, 59 insertions(+), 56 deletions(-) diff --git a/gql/parser.go b/gql/parser.go index b396936ea8d..503e18da36b 100644 --- a/gql/parser.go +++ b/gql/parser.go @@ -92,6 +92,10 @@ type RecurseArgs struct { } type ShortestPathArgs struct { + // From, To can have a uid or a uid function as the argument. + // 1. from: 0x01 + // 2. from: uid(0x01) + // 3. from: uid(p) // a variable From *Function To *Function } @@ -659,15 +663,14 @@ func (gq *GraphQuery) collectVars(v *Vars) { if gq.MathExp != nil { gq.MathExp.collectVars(v) } - if gq.ShortestPathArgs.From != nil && len(gq.ShortestPathArgs.From.NeedsVar) > 0 { - for _, uidVar := range gq.ShortestPathArgs.From.NeedsVar { - v.Needs = append(v.Needs, uidVar.Name) - } + + shortestPathFrom := gq.ShortestPathArgs.From + if shortestPathFrom != nil && len(shortestPathFrom.NeedsVar) > 0 { + v.Needs = append(v.Needs, shortestPathFrom.NeedsVar[0].Name) } - if gq.ShortestPathArgs.To != nil && len(gq.ShortestPathArgs.To.NeedsVar) > 0 { - for _, uidVar := range gq.ShortestPathArgs.To.NeedsVar { - v.Needs = append(v.Needs, uidVar.Name) - } + shortestPathTo := gq.ShortestPathArgs.To + if shortestPathTo != nil && len(shortestPathTo.NeedsVar) > 0 { + v.Needs = append(v.Needs, shortestPathTo.NeedsVar[0].Name) } } @@ -2430,48 +2433,48 @@ func getRoot(it *lex.ItemIterator) (gq *GraphQuery, rerr error) { } fn := &Function{} - // from, to can have a uid or a uid function as the argument. - // 1. from: 0x01 - // 2. from: uid(0x01) - // 3. from: uid(p) // a variable peekIt, err := it.Peek(1) if err != nil { return nil, item.Errorf("Invalid query") } - if peekIt[0].Val != uid { - // This means its not a uid function, so it has to be an actual uid. - it.Next() - item := it.Item() - val := collectName(it, item.Val) - uid, err := strconv.ParseUint(val, 0, 64) - switch e := err.(type) { - case nil: - fn.UID = append(fn.UID, uid) - case *strconv.NumError: - if e.Err == strconv.ErrRange { - return nil, item.Errorf("The uid value %q is too large.", val) - } - } + assignShortestPathFn := func(fn *Function, key string) { if key == "from" { gq.ShortestPathArgs.From = fn } else { gq.ShortestPathArgs.To = fn } - continue } - gen, err := parseFunction(it, gq) - if err != nil { - return gq, err + if peekIt[0].Val == uid { + gen, err := parseFunction(it, gq) + if err != nil { + return gq, err + } + fn.NeedsVar = gen.NeedsVar + fn.Name = gen.Name + assignShortestPathFn(fn, key) + continue } - fn.NeedsVar = gen.NeedsVar - fn.Name = gen.Name - if key == "from" { - gq.ShortestPathArgs.From = fn - } else { - gq.ShortestPathArgs.To = fn + + // This means its not a uid function, so it has to be an actual uid. + it.Next() + item := it.Item() + val := collectName(it, item.Val) + uid, err := strconv.ParseUint(val, 0, 64) + switch e := err.(type) { + case nil: + fn.UID = append(fn.UID, uid) + case *strconv.NumError: + if e.Err == strconv.ErrRange { + return nil, item.Errorf("The uid value %q is too large.", val) + } + return nil, + item.Errorf("from/to in shortest path can only accept uid function or an uid. Got: %s", + val) } + assignShortestPathFn(fn, key) + default: var val string if !it.Next() { diff --git a/gql/parser_test.go b/gql/parser_test.go index b3d5d85dbc9..c9f5f1f7532 100644 --- a/gql/parser_test.go +++ b/gql/parser_test.go @@ -1078,25 +1078,6 @@ func TestParseShortestPath(t *testing.T) { require.Equal(t, "6", res.Query[0].Args["maxweight"]) } -func TestParseShortestPathWithVariables(t *testing.T) { - query := ` - { - shortest(from: uid(p), to: uid(q)) { - friends - name - } - } -` - res, err := Parse(Request{Str: query}) - require.NoError(t, err) - require.NotNil(t, res.Query[0].ShortestPathArgs.From) - require.Equal(t, 1, len(res.Query[0].ShortestPathArgs.From.NeedsVar)) - require.Equal(t, "p", res.Query[0].ShortestPathArgs.From.NeedsVar[0].Name) - require.Equal(t, "uid", res.Query[0].ShortestPathArgs.From.Name) - require.NotNil(t, res.Query[0].ShortestPathArgs.To) - require.Equal(t, 1, len(res.Query[0].ShortestPathArgs.To.NeedsVar)) -} - func TestParseShortestPathWithUidVars(t *testing.T) { query := `{ a as var(func: uid(0x01)) @@ -1108,8 +1089,27 @@ func TestParseShortestPathWithUidVars(t *testing.T) { } }` - _, err := Parse(Request{Str: query}) + res, err := Parse(Request{Str: query}) require.NoError(t, err) + q := res.Query[2] + require.NotNil(t, q.ShortestPathArgs.From) + require.Equal(t, 1, len(q.ShortestPathArgs.From.NeedsVar)) + require.Equal(t, "a", q.ShortestPathArgs.From.NeedsVar[0].Name) + require.Equal(t, "uid", q.ShortestPathArgs.From.Name) + require.NotNil(t, q.ShortestPathArgs.To) + require.Equal(t, 1, len(q.ShortestPathArgs.To.NeedsVar)) +} + +func TestParseShortestPathInvalidFnError(t *testing.T) { + query := `{ + shortest(from: eq(a), to: uid(b)) { + password + friend + } + + }` + _, err := Parse(Request{Str: query}) + require.Error(t, err) } func TestParseMultipleQueries(t *testing.T) { From e7d1d9d8148eb0d7b0df31fc4ed33024c4e34bce Mon Sep 17 00:00:00 2001 From: Pawan Rawal Date: Tue, 23 Jul 2019 16:37:10 +1000 Subject: [PATCH 07/10] Add some more test cases and do some refactoring. --- query/query.go | 62 +++++++++++++++++++++++--------------------- query/query3_test.go | 15 +++++++++++ query/shortest.go | 3 +++ 3 files changed, 51 insertions(+), 29 deletions(-) diff --git a/query/query.go b/query/query.go index ddad4332f9a..3f1c7165cd3 100644 --- a/query/query.go +++ b/query/query.go @@ -883,8 +883,6 @@ func (args *params) fill(gq *gql.GraphQuery) error { if gq.ShortestPathArgs.From == nil || gq.ShortestPathArgs.To == nil { return errors.Errorf("Unexpected error: from/to can't be nil for shortest path") } - // TODO - Add validation, either length of UID slice should be greater than zero or - // NeedsVar. if len(gq.ShortestPathArgs.From.UID) > 0 { args.From = gq.ShortestPathArgs.From.UID[0] } @@ -1693,37 +1691,43 @@ func (sg *SubGraph) recursiveFillVars(doneVars map[string]varValue) error { return nil } -func (sg *SubGraph) fillVars(mp map[string]varValue) error { - if sg.Params.Alias == "shortest" { - if sg.Params.ShortestPathArgs.From != nil && len(sg.Params.ShortestPathArgs.From.NeedsVar) > 0 { - fromVar := sg.Params.ShortestPathArgs.From.NeedsVar[0].Name - uidVar, ok := mp[fromVar] - if !ok { - return errors.Errorf("value of from var(%s) should have already been populated", - fromVar) - } - if uidVar.Uids != nil { - if len(uidVar.Uids.Uids) > 1 { - return errors.Errorf("from variable(%s) should only expand to 1 uid", fromVar) - } - sg.Params.From = uidVar.Uids.Uids[0] +func (sg *SubGraph) fillShortestPathVars(mp map[string]varValue) error { + if sg.Params.ShortestPathArgs.From != nil && len(sg.Params.ShortestPathArgs.From.NeedsVar) > 0 { + fromVar := sg.Params.ShortestPathArgs.From.NeedsVar[0].Name + uidVar, ok := mp[fromVar] + if !ok { + return errors.Errorf("value of from var(%s) should have already been populated", + fromVar) + } + if uidVar.Uids != nil { + if len(uidVar.Uids.Uids) > 1 { + return errors.Errorf("from variable(%s) should only expand to 1 uid", fromVar) } + sg.Params.From = uidVar.Uids.Uids[0] } + } - if sg.Params.ShortestPathArgs.To != nil && len(sg.Params.ShortestPathArgs.To.NeedsVar) > 0 { - toVar := sg.Params.ShortestPathArgs.To.NeedsVar[0].Name - uidVar, ok := mp[toVar] - if !ok { - return errors.Errorf("value of to var(%s) should have already been populated", - toVar) - } - if uidVar.Uids != nil { - if len(uidVar.Uids.Uids) > 1 { - return errors.Errorf("to variable(%s) should only expand to 1 uid", toVar) - } - sg.Params.To = uidVar.Uids.Uids[0] + if sg.Params.ShortestPathArgs.To != nil && len(sg.Params.ShortestPathArgs.To.NeedsVar) > 0 { + toVar := sg.Params.ShortestPathArgs.To.NeedsVar[0].Name + uidVar, ok := mp[toVar] + if !ok { + return errors.Errorf("value of to var(%s) should have already been populated", + toVar) + } + if uidVar.Uids != nil { + if len(uidVar.Uids.Uids) > 1 { + return errors.Errorf("to variable(%s) should only expand to 1 uid", toVar) } - fmt.Println("to: ", sg.Params.To) + sg.Params.To = uidVar.Uids.Uids[0] + } + } + return nil +} + +func (sg *SubGraph) fillVars(mp map[string]varValue) error { + if sg.Params.Alias == "shortest" { + if err := sg.fillShortestPathVars(mp); err != nil { + return err } } diff --git a/query/query3_test.go b/query/query3_test.go index e17a5759ee0..829c1a6e55c 100644 --- a/query/query3_test.go +++ b/query/query3_test.go @@ -583,6 +583,21 @@ func TestShortestPathWithUidVariableError(t *testing.T) { require.Error(t, err) } +func TestShortestPathWithUidVariableNoMatch(t *testing.T) { + query := ` + { + a as var(func: eq(name, "blah blah")) + b as var(func: eq(name, "foo bar")) + + shortest(from: uid(a), to: uid(b)) { + password + friend + } + }` + js := processQueryNoErr(t, query) + require.JSONEq(t, `{"data":{}}`, js) +} + func TestFacetVarRetrieval(t *testing.T) { query := ` diff --git a/query/shortest.go b/query/shortest.go index b57f87b0dd8..f4c662b24b2 100644 --- a/query/shortest.go +++ b/query/shortest.go @@ -430,6 +430,9 @@ func shortestPath(ctx context.Context, sg *SubGraph) ([]*SubGraph, error) { if sg.Params.Alias != "shortest" { return nil, errors.Errorf("Invalid shortest path query") } + if sg.Params.From == 0 || sg.Params.To == 0 { + return nil, nil + } numPaths := sg.Params.numPaths if numPaths == 0 { // Return 1 path by default. From 9bbf8925418c8e7f6155d3e93abc75572fe16e8b Mon Sep 17 00:00:00 2001 From: Pawan Rawal Date: Wed, 24 Jul 2019 11:08:02 +1000 Subject: [PATCH 08/10] Address comments by Martin. --- gql/parser.go | 9 +++++++-- query/query.go | 2 ++ query/query3_test.go | 15 +++++++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/gql/parser.go b/gql/parser.go index 503e18da36b..73625f5fcf3 100644 --- a/gql/parser.go +++ b/gql/parser.go @@ -91,6 +91,7 @@ type RecurseArgs struct { AllowLoop bool } +// SHortestPathArgs stores the arguments needed to process the shortest path query. type ShortestPathArgs struct { // From, To can have a uid or a uid function as the argument. // 1. from: 0x01 @@ -2337,6 +2338,11 @@ func attrAndLang(attrData string) (attr string, langs []string) { return } +func isEmpty(gq *GraphQuery) bool { + return gq.Func == nil && len(gq.NeedsVar) == 0 && len(gq.Args) == 0 && + gq.ShortestPathArgs.From == nil && gq.ShortestPathArgs.To == nil +} + // getRoot gets the root graph query object after parsing the args. func getRoot(it *lex.ItemIterator) (gq *GraphQuery, rerr error) { gq = &GraphQuery{ @@ -2384,8 +2390,7 @@ func getRoot(it *lex.ItemIterator) (gq *GraphQuery, rerr error) { key = item.Val expectArg = false } else if item.Typ == itemRightRound { - if gq.Func == nil && len(gq.NeedsVar) == 0 && len(gq.Args) == 0 && - gq.ShortestPathArgs.From == nil && gq.ShortestPathArgs.To == nil { + if isEmpty(gq) { // Used to do aggregation at root which would be fetched in another block. gq.IsEmpty = true } diff --git a/query/query.go b/query/query.go index 3f1c7165cd3..d1c7c32c3ee 100644 --- a/query/query.go +++ b/query/query.go @@ -1692,6 +1692,8 @@ func (sg *SubGraph) recursiveFillVars(doneVars map[string]varValue) error { } func (sg *SubGraph) fillShortestPathVars(mp map[string]varValue) error { + // The uidVar.Uids can be nil if the variable didn't return any uids. This would mean + // sg.Params.From or sg.Params.To is 0 and the query would return an empty result. if sg.Params.ShortestPathArgs.From != nil && len(sg.Params.ShortestPathArgs.From.NeedsVar) > 0 { fromVar := sg.Params.ShortestPathArgs.From.NeedsVar[0].Name uidVar, ok := mp[fromVar] diff --git a/query/query3_test.go b/query/query3_test.go index 829c1a6e55c..e56b31dbf6a 100644 --- a/query/query3_test.go +++ b/query/query3_test.go @@ -598,6 +598,21 @@ func TestShortestPathWithUidVariableNoMatch(t *testing.T) { require.JSONEq(t, `{"data":{}}`, js) } +func TestShortestPathWithUidVariableNoMatchForFrom(t *testing.T) { + query := ` + { + a as var(func: eq(name, "blah blah")) + b as var(func: eq(name, "Michonne")) + + shortest(from: uid(a), to: uid(b)) { + password + friend + } + }` + js := processQueryNoErr(t, query) + require.JSONEq(t, `{"data":{}}`, js) +} + func TestFacetVarRetrieval(t *testing.T) { query := ` From f9cbe36191f224f5fb04628ee63109e17ae036d9 Mon Sep 17 00:00:00 2001 From: Pawan Rawal Date: Wed, 24 Jul 2019 11:19:33 +1000 Subject: [PATCH 09/10] Address more comments. --- gql/parser.go | 2 +- query/query.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gql/parser.go b/gql/parser.go index 73625f5fcf3..e4b910e7af7 100644 --- a/gql/parser.go +++ b/gql/parser.go @@ -2462,7 +2462,7 @@ func getRoot(it *lex.ItemIterator) (gq *GraphQuery, rerr error) { continue } - // This means its not a uid function, so it has to be an actual uid. + // This means it's not a uid function, so it has to be an actual uid. it.Next() item := it.Item() val := collectName(it, item.Val) diff --git a/query/query.go b/query/query.go index d1c7c32c3ee..656a043dfd9 100644 --- a/query/query.go +++ b/query/query.go @@ -881,7 +881,7 @@ func (args *params) fill(gq *gql.GraphQuery) error { } if gq.ShortestPathArgs.From == nil || gq.ShortestPathArgs.To == nil { - return errors.Errorf("Unexpected error: from/to can't be nil for shortest path") + return errors.Errorf("from/to can't be nil for shortest path") } if len(gq.ShortestPathArgs.From.UID) > 0 { args.From = gq.ShortestPathArgs.From.UID[0] From ff5c0e09e315b17290ecb7ea665f92643e7db877 Mon Sep 17 00:00:00 2001 From: Pawan Rawal Date: Wed, 24 Jul 2019 13:15:12 +1000 Subject: [PATCH 10/10] Address comments by mrjn. --- gql/parser.go | 6 +++--- query/query3_test.go | 8 +++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/gql/parser.go b/gql/parser.go index e4b910e7af7..075b830bcb8 100644 --- a/gql/parser.go +++ b/gql/parser.go @@ -2446,7 +2446,7 @@ func getRoot(it *lex.ItemIterator) (gq *GraphQuery, rerr error) { assignShortestPathFn := func(fn *Function, key string) { if key == "from" { gq.ShortestPathArgs.From = fn - } else { + } else if key == "to" { gq.ShortestPathArgs.To = fn } } @@ -2475,8 +2475,8 @@ func getRoot(it *lex.ItemIterator) (gq *GraphQuery, rerr error) { return nil, item.Errorf("The uid value %q is too large.", val) } return nil, - item.Errorf("from/to in shortest path can only accept uid function or an uid. Got: %s", - val) + item.Errorf("from/to in shortest path can only accept uid function or an uid."+ + " Got: %s", val) } assignShortestPathFn(fn, key) diff --git a/query/query3_test.go b/query/query3_test.go index e56b31dbf6a..617e3b6df15 100644 --- a/query/query3_test.go +++ b/query/query3_test.go @@ -778,9 +778,11 @@ func TestTwoShortestPathVariable(t *testing.T) { js := processQueryNoErr(t, query) require.JSONEq(t, `{"data": {"_path_":[ - {"uid":"0x1","_weight_":3,"path":{"uid":"0x1f","path":{"uid":"0x3e8","path":{"uid":"0x3ea"}}}}, - {"uid":"0x1","_weight_":4,"path":{"uid":"0x1f","path":{"uid":"0x3e8","path":{"uid":"0x3e9","path":{"uid":"0x3ea"}}}}}], - "me":[{"name":"Michonne"},{"name":"Andrea"},{"name":"Alice"},{"name":"Matt"}]}}`, + {"uid":"0x1","_weight_":3,"path":{"uid":"0x1f","path":{"uid":"0x3e8", + "path":{"uid":"0x3ea"}}}}, {"uid":"0x1","_weight_":4, + "path":{"uid":"0x1f","path":{"uid":"0x3e8","path":{"uid":"0x3e9", + "path":{"uid":"0x3ea"}}}}}], "me":[{"name":"Michonne"},{"name":"Andrea"} + ,{"name":"Alice"},{"name":"Matt"}]}}`, js) }