Skip to content

fix(GraphQL): This PR addd input coercion from single object to list and fix panic when we pass single ID in filter as a string. #7133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Dec 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ require (
github.com/dgraph-io/badger/v2 v2.0.1-rc1.0.20201214114056-bcfae6104545
github.com/dgraph-io/dgo/v200 v200.0.0-20200805103119-a3544c464dd6
github.com/dgraph-io/gqlgen v0.13.2
github.com/dgraph-io/gqlparser/v2 v2.1.1
github.com/dgraph-io/gqlparser/v2 v2.1.2
github.com/dgraph-io/graphql-transport-ws v0.0.0-20200916064635-48589439591b
github.com/dgraph-io/ristretto v0.0.4-0.20201207174236-c72a155bcf05
github.com/dgrijalva/jwt-go v3.2.0+incompatible
Expand Down
3 changes: 2 additions & 1 deletion go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,9 @@ github.com/dgraph-io/dgo/v200 v200.0.0-20200805103119-a3544c464dd6 h1:toHzMCdCUg
github.com/dgraph-io/dgo/v200 v200.0.0-20200805103119-a3544c464dd6/go.mod h1:rHa+h3kI4M8ASOirxyIyNeXBfHFgeskVUum2OrDMN3U=
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 h1:OBGI6VR+WcegjDB3JdNPMpKpEtITHWzgmiCXtELWolI=
github.com/dgraph-io/gqlparser/v2 v2.1.1/go.mod h1:MYS4jppjyx8b9tuUtjV7jU1UFZK6P9fvO8TsIsQtRKU=
github.com/dgraph-io/gqlparser/v2 v2.1.2 h1:N7dyP6Y2ScsyQjqw5+wjPJOsAmZE1q8TcMvsPClaFV4=
github.com/dgraph-io/gqlparser/v2 v2.1.2/go.mod h1:MYS4jppjyx8b9tuUtjV7jU1UFZK6P9fvO8TsIsQtRKU=
github.com/dgraph-io/graphql-transport-ws v0.0.0-20200916064635-48589439591b h1:PDEhlwHpkEQ5WBfOOKZCNZTXFDGyCEWTYDhxGQbyIpk=
github.com/dgraph-io/graphql-transport-ws v0.0.0-20200916064635-48589439591b/go.mod h1:7z3c/5w0sMYYZF5bHsrh8IH4fKwG5O5Y70cPH1ZLLRQ=
github.com/dgraph-io/ristretto v0.0.4-0.20201205013540-bafef7527542/go.mod h1:tv2ec8nA7vRpSYX7/MbP52ihrUMXIHit54CQMq8npXQ=
Expand Down
2 changes: 1 addition & 1 deletion graphql/e2e/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,7 @@ func RunAll(t *testing.T) {
t.Run("query id directive with int", idDirectiveWithInt)
t.Run("query id directive with int64", idDirectiveWithInt64)
t.Run("query id directive with float", idDirectiveWithFloat)

t.Run("query using single ID in a filter that will be coerced to list", queryFilterSingleIDListCoercion)
// mutation tests
t.Run("add mutation", addMutation)
t.Run("update mutation by ids", updateMutationByIds)
Expand Down
66 changes: 66 additions & 0 deletions graphql/e2e/common/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -3284,6 +3284,72 @@ func passwordTest(t *testing.T) {
deleteUser(t, *newUser)
}

func queryFilterSingleIDListCoercion(t *testing.T) {
authors := addMultipleAuthorFromRef(t, []*author{
{
Name: "George",
Reputation: 4.5,
Qualification: "Phd in CSE",
Posts: []*post{{Title: "A show about nothing", Text: "Got ya!", Tags: []string{}}},
}, {
Name: "Jerry",
Reputation: 4.6,
Country: &country{Name: "outer Galaxy2"},
Posts: []*post{{Title: "Outside", Tags: []string{}}},
},
}, postExecutor)
authorIds := []string{authors[0].ID, authors[1].ID}
postIds := []string{authors[0].Posts[0].PostID, authors[1].Posts[0].PostID}
countryIds := []string{authors[1].Country.ID}
tcase := struct {
name string
query string
variables map[string]interface{}
respData string
}{

name: "Query using single ID in a filter",
query: `query($filter:AuthorFilter){
queryAuthor(filter:$filter){
name
reputation
posts {
text
}
}
}`,
variables: map[string]interface{}{"filter": map[string]interface{}{"id": authors[0].ID}},
respData: `{
"queryAuthor": [
{
"name": "George",
"reputation": 4.5,
"posts": [
{
"text": "Got ya!"
}
]
}
]
}`,
}

t.Run(tcase.name, func(t *testing.T) {
params := &GraphQLParams{
Query: tcase.query,
Variables: tcase.variables,
}
resp := params.ExecuteAsPost(t, GraphqlURL)
RequireNoGQLErrors(t, resp)
testutil.CompareJSON(t, tcase.respData, string(resp.Data))
})

// cleanup
deleteAuthors(t, authorIds, nil)
deleteCountry(t, map[string]interface{}{"id": countryIds}, len(countryIds), nil)
DeleteGqlType(t, "Post", map[string]interface{}{"postID": postIds}, len(postIds), nil)
}

func idDirectiveWithInt64(t *testing.T) {
query := &GraphQLParams{
Query: `query {
Expand Down
10 changes: 1 addition & 9 deletions graphql/resolve/mutation_rewriter.go
Original file line number Diff line number Diff line change
Expand Up @@ -263,15 +263,7 @@ func (xidMetadata *xidMetadata) isDuplicateXid(atTopLevel bool, xidVar string,
// }
func (mrw *AddRewriter) Rewrite(ctx context.Context, m schema.Mutation) ([]*UpsertMutation, error) {
mutatedType := m.MutatedType()
var val []interface{}

switch v := m.ArgValue(schema.InputArgName).(type) {
case []interface{}:
val = v
case interface{}:
val = append(val, v)
}

val, _ := m.ArgValue(schema.InputArgName).([]interface{})
varGen := NewVariableGenerator()
xidMd := newXidMetadata()
var errs error
Expand Down
15 changes: 0 additions & 15 deletions graphql/resolve/query_rewriter.go
Original file line number Diff line number Diff line change
Expand Up @@ -1563,11 +1563,6 @@ func buildFilter(typ schema.Type, filter map[string]interface{}) *gql.FilterTree
ft := buildFilter(typ, obj.(map[string]interface{}))
ands = append(ands, ft)
}
case []map[string]interface{}:
for _, obj := range v {
ft := buildFilter(typ, obj)
ands = append(ands, ft)
}
}
case "or":
// title: { anyofterms: "GraphQL" }, or: { ... }
Expand All @@ -1592,16 +1587,6 @@ func buildFilter(typ schema.Type, filter map[string]interface{}) *gql.FilterTree
Child: ors,
Op: "or",
}
case []map[string]interface{}:
ors := make([]*gql.FilterTree, 0, len(v))
for _, obj := range v {
ft := buildFilter(typ, obj)
ors = append(ands, ft)
}
or = &gql.FilterTree{
Child: ors,
Op: "or",
}
}
case "not":
// title: { anyofterms: "GraphQL" }, not: { isPublished: true}
Expand Down
40 changes: 28 additions & 12 deletions graphql/resolve/query_test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -916,7 +916,7 @@
}
dgquery: |-
query {
queryAuthor(func: type(Author)) @filter((eq(Author.name, "A. N. Author") OR le(Author.dob, "2001-01-01"))) {
queryAuthor(func: type(Author)) @filter((eq(Author.name, "A. N. Author") OR (le(Author.dob, "2001-01-01")))) {
name : Author.name
dgraph.uid : uid
}
Expand Down Expand Up @@ -948,7 +948,7 @@
}
dgquery: |-
query {
queryAuthor(func: type(Author)) @filter(eq(Author.name, "A. N. Author")) {
queryAuthor(func: type(Author)) @filter((eq(Author.name, "A. N. Author"))) {
name : Author.name
dgraph.uid : uid
}
Expand All @@ -965,7 +965,7 @@
}
dgquery: |-
query {
queryAuthor(func: type(Author)) @filter(((eq(Author.name, "A. N. Author") AND gt(Author.reputation, "2.5")) OR le(Author.dob, "2001-01-01"))) {
queryAuthor(func: type(Author)) @filter(((eq(Author.name, "A. N. Author") AND gt(Author.reputation, "2.5")) OR (le(Author.dob, "2001-01-01")))) {
name : Author.name
dgraph.uid : uid
}
Expand All @@ -981,7 +981,7 @@
}
dgquery: |-
query {
queryAuthor(func: type(Author)) @filter((eq(Author.name, "A. N. Author") OR (le(Author.dob, "2001-01-01") AND gt(Author.reputation, "2.5")))) {
queryAuthor(func: type(Author)) @filter((eq(Author.name, "A. N. Author") OR ((le(Author.dob, "2001-01-01") AND gt(Author.reputation, "2.5"))))) {
name : Author.name
dgraph.uid : uid
}
Expand All @@ -997,7 +997,7 @@
}
dgquery: |-
query {
queryAuthor(func: type(Author)) @filter((eq(Author.name, "A. N. Author") OR (gt(Author.reputation, "2.5") OR le(Author.dob, "2001-01-01")))) {
queryAuthor(func: type(Author)) @filter((eq(Author.name, "A. N. Author") OR ((gt(Author.reputation, "2.5") OR (le(Author.dob, "2001-01-01")))))) {
name : Author.name
dgraph.uid : uid
}
Expand Down Expand Up @@ -1257,7 +1257,7 @@
}
dgquery: |-
query {
queryAuthor(func: type(Author)) @filter((gt(Author.reputation, "1.1") OR (ge(Author.reputation, "1.1") OR (lt(Author.reputation, "1.1") OR (le(Author.reputation, "1.1") OR eq(Author.reputation, "1.1")))))) {
queryAuthor(func: type(Author)) @filter((gt(Author.reputation, "1.1") OR ((ge(Author.reputation, "1.1") OR ((lt(Author.reputation, "1.1") OR ((le(Author.reputation, "1.1") OR (eq(Author.reputation, "1.1")))))))))) {
name : Author.name
dgraph.uid : uid
}
Expand All @@ -1273,7 +1273,7 @@
}
dgquery: |-
query {
queryAuthor(func: type(Author)) @filter((gt(Author.dob, "2000-01-01") OR (ge(Author.dob, "2000-01-01") OR (lt(Author.dob, "2000-01-01") OR (le(Author.dob, "2000-01-01") OR eq(Author.dob, "2000-01-01")))))) {
queryAuthor(func: type(Author)) @filter((gt(Author.dob, "2000-01-01") OR ((ge(Author.dob, "2000-01-01") OR ((lt(Author.dob, "2000-01-01") OR ((le(Author.dob, "2000-01-01") OR (eq(Author.dob, "2000-01-01")))))))))) {
name : Author.name
dgraph.uid : uid
}
Expand All @@ -1289,7 +1289,7 @@
}
dgquery: |-
query {
queryPost(func: type(Post)) @filter((gt(Post.numLikes, 10) OR (ge(Post.numLikes, 10) OR (lt(Post.numLikes, 10) OR (le(Post.numLikes, 10) OR eq(Post.numLikes, 10)))))) {
queryPost(func: type(Post)) @filter((gt(Post.numLikes, 10) OR ((ge(Post.numLikes, 10) OR ((lt(Post.numLikes, 10) OR ((le(Post.numLikes, 10) OR (eq(Post.numLikes, 10)))))))))) {
title : Post.title
dgraph.uid : uid
}
Expand Down Expand Up @@ -1321,7 +1321,7 @@
}
dgquery: |-
query {
queryCountry(func: type(Country)) @filter((gt(Country.name, "AAA") OR (ge(Country.name, "AAA") OR (lt(Country.name, "AAA") OR (le(Country.name, "AAA") OR eq(Country.name, "AAA")))))) {
queryCountry(func: type(Country)) @filter((gt(Country.name, "AAA") OR ((ge(Country.name, "AAA") OR ((lt(Country.name, "AAA") OR ((le(Country.name, "AAA") OR (eq(Country.name, "AAA")))))))))) {
name : Country.name
dgraph.uid : uid
}
Expand Down Expand Up @@ -1370,7 +1370,7 @@
}
dgquery: |-
query {
queryCountry(func: type(Country)) @filter(((gt(Country.name, "AAA") OR lt(Country.name, "XXX")) AND (gt(Country.name, "CCC") OR lt(Country.name, "MMM")))) {
queryCountry(func: type(Country)) @filter(((gt(Country.name, "AAA") OR (lt(Country.name, "XXX"))) AND (gt(Country.name, "CCC") OR (lt(Country.name, "MMM"))))) {
name : Country.name
dgraph.uid : uid
}
Expand All @@ -1386,7 +1386,7 @@
}
dgquery: |-
query {
queryPost(func: type(Post)) @filter((anyofterms(Post.title, "GraphQL") OR allofterms(Post.title, "GraphQL"))) {
queryPost(func: type(Post)) @filter((anyofterms(Post.title, "GraphQL") OR (allofterms(Post.title, "GraphQL")))) {
title : Post.title
dgraph.uid : uid
}
Expand All @@ -1403,7 +1403,7 @@
}
dgquery: |-
query {
queryPost(func: type(Post)) @filter((anyoftext(Post.text, "GraphQL") OR alloftext(Post.text, "GraphQL"))) {
queryPost(func: type(Post)) @filter((anyoftext(Post.text, "GraphQL") OR (alloftext(Post.text, "GraphQL")))) {
title : Post.title
dgraph.uid : uid
}
Expand Down Expand Up @@ -3129,3 +3129,19 @@
scoreVar as Tweets.score
}
}

-
name: "query using single ID in filter"
gqlquery: |
query {
queryAuthor(filter:{id: "0x1"}) {
name
}
}
dgquery: |-
query {
queryAuthor(func: uid(0x1)) @filter(type(Author)) {
name : Author.name
dgraph.uid : uid
}
}
1 change: 1 addition & 0 deletions graphql/schema/rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func init() {
validator.AddRule("Check variable type is correct", variableTypeCheck)
validator.AddRule("Check arguments of cascade directive", directiveArgumentsCheck)
validator.AddRule("Check range for Int type", intRangeCheck)
validator.AddRule("Input Coercion to List", listInputCoercion)

}

Expand Down
22 changes: 22 additions & 0 deletions graphql/schema/validation_rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,28 @@ import (
"github.com/dgraph-io/gqlparser/v2/validator"
)

func listInputCoercion(observers *validator.Events, addError validator.AddErrFunc) {
observers.OnValue(func(walker *validator.Walker, value *ast.Value) {
if value.Definition == nil || value.ExpectedType == nil {
return
}

if value.Kind == ast.Variable {
return
}
// If the expected value is a list (ExpectedType.Elem != nil) && the value is not of list type,
// then we need to coerce the value to a list, otherwise, we can return here as we do below.

if !(value.ExpectedType.Elem != nil && value.Kind != ast.ListValue) {
return
}
val := *value
child := &ast.ChildValue{Value: &val}
valueNew := ast.Value{Children: []*ast.ChildValue{child}, Kind: ast.ListValue, Position: val.Position, Definition: val.Definition}
*value = valueNew
})
}

func variableTypeCheck(observers *validator.Events, addError validator.AddErrFunc) {
observers.OnValue(func(walker *validator.Walker, value *ast.Value) {
if value.Definition == nil || value.ExpectedType == nil ||
Expand Down