From 97f280fa3b88bcdc2ba1aac6371fc9481c8a2b30 Mon Sep 17 00:00:00 2001 From: Jatin Dev <64803093+JatinDevDG@users.noreply.github.com> Date: Fri, 26 Feb 2021 15:56:59 +0530 Subject: [PATCH] fix(GraphQL): Added support for parameterized cascade with variables. (#7477) 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. --- go.mod | 2 +- go.sum | 4 +- graphql/e2e/common/error_test.yaml | 96 +++++++++++++++++++++--- graphql/e2e/common/query.go | 36 +++++++++ graphql/resolve/mutation_query_test.yaml | 8 +- graphql/resolve/mutation_test.go | 7 +- graphql/resolve/query_test.go | 15 ++-- graphql/resolve/query_test.yaml | 61 +++++++++++++-- graphql/schema/auth.go | 2 +- graphql/schema/request.go | 2 +- graphql/schema/validation_rules.go | 40 ++++++++-- graphql/schema/wrappers.go | 16 ++-- 12 files changed, 243 insertions(+), 46 deletions(-) diff --git a/go.mod b/go.mod index e4858e04467..107af39dcbc 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,7 @@ require ( github.com/dgraph-io/badger/v3 v3.0.0-20210209201111-6c35ad6c28e0 github.com/dgraph-io/dgo/v200 v200.0.0-20210212152539-e0a5bde40ba2 github.com/dgraph-io/gqlgen v0.13.2 - github.com/dgraph-io/gqlparser/v2 v2.1.5 + github.com/dgraph-io/gqlparser/v2 v2.1.8 github.com/dgraph-io/graphql-transport-ws v0.0.0-20210223074046-e5b8b80bb4ed github.com/dgraph-io/ristretto v0.0.4-0.20210223002318-8ec1dc18f880 github.com/dgraph-io/simdjson-go v0.3.0 diff --git a/go.sum b/go.sum index 584273441c7..79bf1aabbfa 100644 --- a/go.sum +++ b/go.sum @@ -125,8 +125,8 @@ github.com/dgraph-io/dgo/v200 v200.0.0-20210212152539-e0a5bde40ba2/go.mod h1:zCf github.com/dgraph-io/gqlgen v0.13.2 h1:TNhndk+eHKj5qE7BenKKSYdSIdOGhLqxR1rCiMso9KM= github.com/dgraph-io/gqlgen v0.13.2/go.mod h1:iCOrOv9lngN7KAo+jMgvUPVDlYHdf7qDwsTkQby2Sis= github.com/dgraph-io/gqlparser/v2 v2.1.1/go.mod h1:MYS4jppjyx8b9tuUtjV7jU1UFZK6P9fvO8TsIsQtRKU= -github.com/dgraph-io/gqlparser/v2 v2.1.5 h1:FNFSzyOEZ8gs97SLBtn2Pez9f12emvsQ6Vv6oNtg5rc= -github.com/dgraph-io/gqlparser/v2 v2.1.5/go.mod h1:MYS4jppjyx8b9tuUtjV7jU1UFZK6P9fvO8TsIsQtRKU= +github.com/dgraph-io/gqlparser/v2 v2.1.8 h1:d4CprjlDyMNGvnZG/pKqe6Oj6qQd4V0TVsuOIjCKXWQ= +github.com/dgraph-io/gqlparser/v2 v2.1.8/go.mod h1:MYS4jppjyx8b9tuUtjV7jU1UFZK6P9fvO8TsIsQtRKU= github.com/dgraph-io/graphql-transport-ws v0.0.0-20210223074046-e5b8b80bb4ed h1:pgGMBoTtFhR+xkyzINaToLYRurHn+6pxMYffIGmmEPc= github.com/dgraph-io/graphql-transport-ws v0.0.0-20210223074046-e5b8b80bb4ed/go.mod h1:7z3c/5w0sMYYZF5bHsrh8IH4fKwG5O5Y70cPH1ZLLRQ= github.com/dgraph-io/ristretto v0.0.4-0.20210205182321-f8e4908e34d1/go.mod h1:tv2ec8nA7vRpSYX7/MbP52ihrUMXIHit54CQMq8npXQ= diff --git a/graphql/e2e/common/error_test.yaml b/graphql/e2e/common/error_test.yaml index af96676a181..fa1d8362ce4 100644 --- a/graphql/e2e/common/error_test.yaml +++ b/graphql/e2e/common/error_test.yaml @@ -10,14 +10,14 @@ [ { "message": "Cannot query field \"getAuthorszzz\" on type \"Query\". Did you mean \"getAuthor\" or \"getauthor1\"?", "locations": [ { "line": 2, "column": 3 } ] } ] - + - name: "Unknown field" gqlrequest: | query { getAuthor(id: "0x1") { namezzz } } - gqlvariables: | + gqlvariables: | { } errors: [ { "message": "Cannot query field \"namezzz\" on type \"Author\". Did you mean \"name\"?", @@ -29,7 +29,7 @@ query { getAuthor(id: $theID) { name } } - gqlvariables: | + gqlvariables: | { } errors: [ { "message": "Variable \"$theID\" is not defined.", @@ -41,7 +41,7 @@ query { queryAuthor(filter: { reputation: { le: "hi there" } }) { name } } - gqlvariables: | + gqlvariables: | { } errors: [ { "message": "Expected type Float, found \"hi there\".", @@ -53,7 +53,7 @@ query queryAuthor($filter: AuthorFiltarzzz!) { queryAuthor(filter: $filter) { name } } - gqlvariables: | + gqlvariables: | { "filter": "type was wrong" } errors: [ { "message": "Variable type provided AuthorFiltarzzz! is incompatible with expected @@ -71,7 +71,7 @@ query queryAuthor($filter: AuthorFilter!) { queryAuthor(filter: $filter) { name } } - gqlvariables: | + gqlvariables: | { "filter": 57 } errors: [ { "message": "must be a AuthorFilter", @@ -101,7 +101,7 @@ "locations": [ { "line": 2, "column": 3 } ] } ] - - name: "@cascade only accepts those fields as a argument, which are present in given type " + name: "@cascade only accepts those fields as a argument, which are present in given type" gqlrequest: | query { queryAuthor @cascade(fields:["title"]){ @@ -112,7 +112,8 @@ gqlvariables: | { } errors: - [ { "message": "Field `title` is not present in type `Author`. You can only use fields which are in type `Author`", + [ { "message": "Field `title` is not present in type `Author`. You can only use fields in cascade which are in type `Author`", + "locations": [{ "line": 2, "column": 16}] } ] - @@ -168,7 +169,8 @@ gqlvariables: | { } errors: - [ { "message": "Field `name` is not present in type `AddAuthorPayload`. You can only use fields which are in type `AddAuthorPayload`", + [ { "message": "Field `name` is not present in type `AddAuthorPayload`. You can only use fields in cascade which are in type `AddAuthorPayload`", + "locations": [{ "line": 2, "column": 38}] } ] - @@ -362,4 +364,78 @@ { } errors: [ { "message": "Int64Filter filter expects only one filter function, got: 2", - "locations": [ { "line": 2, "column": 29 } ] } ] \ No newline at end of file + "locations": [ { "line": 2, "column": 29 } ] } ] + +- + name: "@cascade only accepts those fields as a argument, which are present in given type at both root and deep levels" + gqlrequest: | + query { + queryAuthor @cascade(fields: ["dob","reputation"]) { + dob + reputation + posts @cascade(fields: ["text1"]) { + text + title + } + } + } + errors: + [ { "message": "Field `text1` is not present in type `Post`. You can only use fields in cascade which are in type `Post`", + "locations": [{ "line": 5, "column": 10}] + } ] + +- + name: "@cascade only accepts those fields as a argument, which are present in given type at deep level using variables" + gqlrequest: | + query($fieldsRoot: [String], $fieldsDeep: [String]) { + queryAuthor @cascade(fields: $fieldsRoot) { + dob + reputation + posts @cascade(fields: $fieldsDeep) { + text + title + } + } + } + gqlvariables: | + { + "fieldsRoot": [ + "dob", + "reputation" + ], + "fieldsDeep": [ + "text1" + ] + } + errors: + [ { "message": "input: variables.fieldsDeep.text1 Field `text1` is not present in type `Post`. You can only use fields in cascade which are in type `Post`", + "locations": [{ "line": 5, "column": 10}] + } ] + +- + name: "@cascade only accepts those fields as a argument, which are present in given type at root level using variables" + gqlrequest: | + query($fieldsRoot: [String], $fieldsDeep: [String]) { + queryAuthor @cascade(fields: $fieldsRoot) { + dob + reputation + posts @cascade(fields: $fieldsDeep) { + text + title + } + } + } + gqlvariables: | + { + "fieldsRoot": [ + "dob", + "reputation1" + ], + "fieldsDeep": [ + "text" + ] + } + errors: + [ { "message": "input: variables.fieldsRoot.reputation1 Field `reputation1` is not present in type `Author`. You can only use fields in cascade which are in type `Author`", + "locations": [{ "line": 2, "column": 15}] + } ] diff --git a/graphql/e2e/common/query.go b/graphql/e2e/common/query.go index 2d3038e8435..a6a3892790e 100644 --- a/graphql/e2e/common/query.go +++ b/graphql/e2e/common/query.go @@ -2460,6 +2460,42 @@ func queryWithCascade(t *testing.T) { ] }`, }, + { + name: "parameterized cascade at all levels using variables", + query: `query ($ids: [ID!],$fieldsRoot: [String], $fieldsDeep: [String]) { + queryAuthor(filter: {id: $ids}) @cascade(fields: $fieldsRoot) { + reputation + name + dob + posts @cascade(fields: $fieldsDeep) { + title + text + } + } + }`, + variables: map[string]interface{}{"ids": authorIds, "fieldsRoot": []string{"reputation", "name"}, "fieldsDeep": []string{"text"}}, + respData: `{ + "queryAuthor": [ + { + "reputation": 4.5, + "name": "George", + "dob": null, + "posts": [ + { + "title": "A show about nothing", + "text": "Got ya!" + } + ] + }, + { + "dob": null, + "name": "Jerry", + "posts": [], + "reputation": 4.6 + } + ] + }`, + }, { name: "parameterized cascade on ID type ", query: `query ($ids: [ID!]) { diff --git a/graphql/resolve/mutation_query_test.yaml b/graphql/resolve/mutation_query_test.yaml index 43a76a43e67..8ced23604f4 100644 --- a/graphql/resolve/mutation_query_test.yaml +++ b/graphql/resolve/mutation_query_test.yaml @@ -124,9 +124,6 @@ ADD_UPDATE_MUTATION: - name: "can work with skip and filter" - variables: - skip: true - include: false gqlquery: | mutation ($skip: Boolean!, $include: Boolean!) { ADD_UPDATE_MUTATION { @@ -142,6 +139,11 @@ ADD_UPDATE_MUTATION: } } } + gqlvariables: | + { + "skip": true, + "include": false + } dgquery: |- query { PAYLOAD_TYPE.post(func: uid(0x4)) { diff --git a/graphql/resolve/mutation_test.go b/graphql/resolve/mutation_test.go index 7119e142a1c..06166f51c0c 100644 --- a/graphql/resolve/mutation_test.go +++ b/graphql/resolve/mutation_test.go @@ -368,10 +368,15 @@ func TestMutationQueryRewriting(t *testing.T) { gqlMutationStr := strings.Replace(tcase.GQLQuery, testType, tt.mut, 1) tcase.DGQuery = strings.Replace(tcase.DGQuery, "PAYLOAD_TYPE", tt.payloadType, 1) + var vars map[string]interface{} + if tcase.GQLVariables != "" { + err := json.Unmarshal([]byte(tcase.GQLVariables), &vars) + require.NoError(t, err) + } op, err := gqlSchema.Operation( &schema.Request{ Query: gqlMutationStr, - Variables: tcase.Variables, + Variables: vars, }) require.NoError(t, err) gqlMutation := test.GetMutation(t, op) diff --git a/graphql/resolve/query_test.go b/graphql/resolve/query_test.go index b9355318360..7b723373c68 100644 --- a/graphql/resolve/query_test.go +++ b/graphql/resolve/query_test.go @@ -36,10 +36,10 @@ import ( // Tests showing that the query rewriter produces the expected Dgraph queries type QueryRewritingCase struct { - Name string - GQLQuery string - Variables map[string]interface{} - DGQuery string + Name string + GQLQuery string + GQLVariables string + DGQuery string } func TestQueryRewriting(t *testing.T) { @@ -56,10 +56,15 @@ func TestQueryRewriting(t *testing.T) { for _, tcase := range tests { t.Run(tcase.Name, func(t *testing.T) { + var vars map[string]interface{} + if tcase.GQLVariables != "" { + err := json.Unmarshal([]byte(tcase.GQLVariables), &vars) + require.NoError(t, err) + } op, err := gqlSchema.Operation( &schema.Request{ Query: tcase.GQLQuery, - Variables: tcase.Variables, + Variables: vars, }) require.NoError(t, err) gqlQuery := test.GetQuery(t, op) diff --git a/graphql/resolve/query_test.yaml b/graphql/resolve/query_test.yaml index 0ddb7ba8dd2..6eb80324bf1 100644 --- a/graphql/resolve/query_test.yaml +++ b/graphql/resolve/query_test.yaml @@ -1574,9 +1574,6 @@ - name: "Skip directive" - variables: - skipTrue: true - skipFalse: false gqlquery: | query ($skipTrue: Boolean!, $skipFalse: Boolean!) { getAuthor(id: "0x1") { @@ -1587,6 +1584,11 @@ } } } + gqlvariables: | + { + "skipTrue": true, + "skipFalse": false + } dgquery: |- query { getAuthor(func: uid(0x1)) @filter(type(Author)) { @@ -1597,9 +1599,6 @@ - name: "Include directive" - variables: - includeTrue: true - includeFalse: false gqlquery: | query ($includeTrue: Boolean!, $includeFalse: Boolean!) { queryAuthor { @@ -1609,6 +1608,12 @@ } } } + + gqlvariables: | + { + "includeTrue": true, + "includeFalse": false + } dgquery: |- query { queryAuthor(func: type(Author)) { @@ -1638,6 +1643,13 @@ } } } + gqlvariables: | + { + "includeTrue": true, + "includeFalse": false, + "skipTrue": true, + "skipFalse": false + } dgquery: |- query { queryAuthor(func: type(Author)) { @@ -1935,6 +1947,43 @@ } } +- + name: "Parameterized Cascade directive on root and nested field using variables" + gqlquery: | + query($fieldsRoot:[String],$fieldsDeep:[String]) { + queryAuthor @cascade(fields: $fieldsRoot) { + dob + reputation + posts @cascade(fields: $fieldsDeep) { + text + title + } + } + } + gqlvariables: | + { + "fieldsRoot": [ + "dob", + "reputation" + ], + "fieldsDeep": [ + "text" + ] + } + dgquery: |- + query { + queryAuthor(func: type(Author)) @cascade(Author.dob, Author.reputation) { + Author.dob : Author.dob + Author.reputation : Author.reputation + Author.posts : Author.posts @cascade(Post.text) { + Post.text : Post.text + Post.title : Post.title + dgraph.uid : uid + } + dgraph.uid : uid + } + } + - name: "getHuman which implements an interface" gqlquery: | diff --git a/graphql/schema/auth.go b/graphql/schema/auth.go index c2006f5c426..5655bb27c8a 100644 --- a/graphql/schema/auth.go +++ b/graphql/schema/auth.go @@ -549,7 +549,7 @@ func gqlValidateRule(sch *schema, typ *ast.Definition, rule string, node *RuleNo " one query, found an %s", typ.Name, op.Name) } - listErr := validator.Validate(sch.schema, doc) + listErr := validator.Validate(sch.schema, doc, nil) if len(listErr) != 0 { var errs error for _, err := range listErr { diff --git a/graphql/schema/request.go b/graphql/schema/request.go index f6698367f1a..8faa0d6bbd9 100644 --- a/graphql/schema/request.go +++ b/graphql/schema/request.go @@ -60,7 +60,7 @@ func (s *schema) Operation(req *Request) (Operation, error) { return nil, gqlErr } - listErr := validator.Validate(s.schema, doc) + listErr := validator.Validate(s.schema, doc, req.Variables) if len(listErr) != 0 { return nil, listErr } diff --git a/graphql/schema/validation_rules.go b/graphql/schema/validation_rules.go index 15a1328cfc0..05557af0d81 100644 --- a/graphql/schema/validation_rules.go +++ b/graphql/schema/validation_rules.go @@ -18,11 +18,12 @@ package schema import ( "errors" + "fmt" "github.com/dgraph-io/dgraph/x" - "strconv" - "github.com/dgraph-io/gqlparser/v2/ast" + "github.com/dgraph-io/gqlparser/v2/gqlerror" "github.com/dgraph-io/gqlparser/v2/validator" + "strconv" ) var allowedFilters = []string{"StringHashFilter", "StringExactFilter", "StringFullTextFilter", @@ -90,20 +91,43 @@ func variableTypeCheck(observers *validator.Events, addError validator.AddErrFun func directiveArgumentsCheck(observers *validator.Events, addError validator.AddErrFunc) { observers.OnDirective(func(walker *validator.Walker, directive *ast.Directive) { - if directive.Name == cascadeDirective && len(directive.Arguments) == 1 { if directive.ParentDefinition == nil { addError(validator.Message("Schema is not set yet. Please try after sometime.")) return } - if directive.Arguments.ForName(cascadeArg) == nil { + fieldArg := directive.Arguments.ForName(cascadeArg) + if fieldArg == nil { + return + } + isVariable := fieldArg.Value.Kind == ast.Variable + fieldsVal, _ := directive.ArgumentMap(walker.Variables)[cascadeArg].([]interface{}) + if len(fieldsVal) == 0 { return } + var validatorPath ast.Path + if isVariable { + validatorPath = ast.Path{ast.PathName("variables")} + validatorPath = append(validatorPath, ast.PathName(fieldArg.Value.Raw)) + + } + typFields := directive.ParentDefinition.Fields - for _, child := range directive.Arguments.ForName(cascadeArg).Value.Children { - if typFields.ForName(child.Value.Raw) == nil { - addError(validator.Message("Field `%s` is not present in type `%s`. You can only use fields which are in type `%s`", - child.Value.Raw, directive.ParentDefinition.Name, directive.ParentDefinition.Name)) + typName := directive.ParentDefinition.Name + + for _, value := range fieldsVal { + v, ok := value.(string) + if !ok { + continue + } + if typFields.ForName(v) == nil { + err := fmt.Sprintf("Field `%s` is not present in type `%s`."+ + " You can only use fields in cascade which are in type `%s`", value, typName, typName) + if isVariable { + validatorPath = append(validatorPath, ast.PathName(v)) + err = gqlerror.ErrorPathf(validatorPath, err).Error() + } + addError(validator.Message(err), validator.At(directive.Position)) return } diff --git a/graphql/schema/wrappers.go b/graphql/schema/wrappers.go index e70aded690e..e4e27867166 100644 --- a/graphql/schema/wrappers.go +++ b/graphql/schema/wrappers.go @@ -27,7 +27,6 @@ import ( "strings" "github.com/dgraph-io/dgraph/graphql/authorization" - "github.com/dgraph-io/gqlparser/v2/parser" "github.com/dgraph-io/dgraph/x" @@ -1135,19 +1134,20 @@ func (f *field) Cascade() []string { if dir == nil { return nil } - arg := dir.Arguments.ForName(cascadeArg) - if arg == nil || arg.Value == nil || len(arg.Value.Children) == 0 { + fieldsVal, _ := dir.ArgumentMap(f.op.vars)[cascadeArg].([]interface{}) + if len(fieldsVal) == 0 { return []string{"__all__"} } - fields := make([]string, 0, len(arg.Value.Children)) + + fields := make([]string, 0) typ := f.Type() idField := typ.IDField() - for _, child := range arg.Value.Children { - if idField != nil && idField.Name() == child.Value.Raw { + for _, value := range fieldsVal { + if idField != nil && idField.Name() == value { fields = append(fields, "uid") } else { - fields = append(fields, typ.DgraphPredicate(child.Value.Raw)) + fields = append(fields, typ.DgraphPredicate(value.(string))) } } @@ -2013,7 +2013,7 @@ func (m *mutation) QueryField() Field { // gets executed to fetch the results of that mutation, so propagating it to the QueryField. if len(m.Cascade()) != 0 && len(f.Cascade()) == 0 { field := f.(*field).field - field.Directives = append(field.Directives, &ast.Directive{Name: cascadeDirective}) + field.Directives = append(field.Directives, &ast.Directive{Name: cascadeDirective, Definition: m.op.inSchema.schema.Directives[cascadeDirective]}) } return f }