feat(graphql+-): Allow parameterized @cascade directive. #5607

parasssh · 2020-06-09T07:43:53Z

Fixes DGRAPH-1279

This change is

pawanrawal

mostly looking good, some minor comments

Reviewed 4 of 6 files at r1, 1 of 2 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @parasssh, and @vvbalaji-dgraph)

gql/parser.go, line 70 at r3 (raw file):

	RecurseArgs      RecurseArgs
	ShortestPathArgs ShortestPathArgs
	Cascade          []string

Please add some docs about how does this look like under different scenarios.

gql/parser.go, line 2117 at r3 (raw file):

	if items[0].Typ == itemLeftCurl || items[0].Typ == itemRightCurl || items[0].
		Typ == itemAt || items[0].Typ == itemName {
		gq.Cascade = append(gq.Cascade, "__all__") // __all__ implies @cascade, the old directive.

all implies values for all the children are mandatory.

gql/parser.go, line 2125 at r3 (raw file):

	it.Next()
	item = it.Item()
	//alias := ""

can be removed?

gql/parser.go, line 2133 at r3 (raw file):

	for it.Next() {
		item := it.Item()
		switch item.Typ {

should this have a default error case? What if someone gives a query like

friends @cascade(n: name)

graphql/dgraph/graphquery.go, line 72 at r3 (raw file):

	}

	if len(query.Cascade) != 0 {

In GraphQL we don't support @cascade with parameters for now. Supporting it would take some more work. That is supported only for GraphQL+-. So we just need the first part of the code here and don't need writeCascade as it won't be used.

if len(query.Cascade) != 0 {
  x.Check2(b.WriteString(" @cascade"))
}

graphql/dgraph/graphquery.go, line 187 at r3 (raw file):

}

func writeCascade(b *strings.Builder, params []string) {

don't need it

query/query.go, line 159 at r3 (raw file):

	// RecurseArgs stores the arguments passed to the @recurse directive.
	RecurseArgs gql.RecurseArgs
	// Cascade is the list of predicates to apply @cascade to. __all__ is special to mean @cascade.

New folks coming to this code might not know about the old meaning of @cascade. We should make it explicit to say __all__ means that all the children of this subgraph are mandatory and should have values otherwise the node will be excluded.

query/query.go, line 553 at r3 (raw file):

		// If parent has @cascade (with or without params), inherit @cascade (with no params)
		if len(sg.Params.Cascade) > 0 {
			args.Cascade = append(args.Cascade, "__all__")

Can we make this __all__ value a constant and use it everywhere? Also, I am assuming @cascade(__all__) also has the same effect as just @cascade? Another question is that do we allow __all__ as a predicate name? If we do, then that might cause some confusion.

query/query.go, line 1354 at r3 (raw file):

			// If the length of child UID list is zero and it has no valid value, then the
			// current UID should be removed from this level.
			if (cascadeAllPreds || cascadeArgMap[child.Attr]) &&

looks good

systest/queries_test.go, line 751 at r3 (raw file):

	}{
		{
			in: `schema(pred: [name]) {}`,

doesn't need to be checked for the purpose of this test

systest/queries_test.go, line 803 at r3 (raw file):

				q(func: anyoftext(name, "Alice")) @cascade(name, age, friend)   {
				  name
				  age

something is up with the formatting, fix if its easy to do so.

systest/queries_test.go, line 868 at r3 (raw file):

				  name
				  age
					  friend {

formatting is a bit messed up

systest/queries_test.go, line 1099 at r3 (raw file):

					  friend @facets(eq(close, true) OR eq(close, false)) {
					name
					# friend {

can be removed if unused

systest/queries_test.go, line 1141 at r3 (raw file):

					name
					age
					# friend {

can be removed

systest/queries_test.go, line 1167 at r3 (raw file):

		},

		// {

Can be removed

systest/queries_test.go, line 1176 at r3 (raw file):

	for _, tc := range tests {
		resp, err := c.NewTxn().Query(ctx, tc.in)

nice comprehensive set of tests

parasssh

Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

gql/parser.go, line 70 at r3 (raw file):