Skip to content

Commit

Permalink
fix(GraphQL): This PR addd input coercion from single object to list …
Browse files Browse the repository at this point in the history 
…and fix panic when we pass single ID in filter as a string. #7133 (#7306)

* 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)

Fixes GRAPHQL-745
Filter with id as a string was giving panic because we were expecting it to be of ["0x1"] type.

 query {
        queryAuthor(filter:{id: "0x1"}) {
          name
          }
        }
So, we added input coercion such that if we require an object of type list and the input object is scalar then we coerce it to list.

(cherry picked from commit 5e5f5ca)
  • Loading branch information
@JatinDev543
JatinDev543 authored Jan 15, 2021
1 parent 23e45d9 commit fa85ff9
Show file tree
Hide file tree
Showing 9 changed files with 122 additions and 39 deletions.
2 changes: 1 addition & 1 deletion go.mod
View file
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.20210108140656-b1486d8516f2
github.com/dgrijalva/jwt-go v3.2.0+incompatible
Expand Down
3 changes: 2 additions & 1 deletion go.sum
View file
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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,7 @@ func RunAll(t *testing.T) {
t.Run("query aggregate and other fields at child level", queryAggregateAndOtherFieldsAtChildLevel)
t.Run("query at child level with multiple alias on scalar field", queryChildLevelWithMultipleAliasOnScalarField)
t.Run("checkUserPassword query", passwordTest)

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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3283,3 +3283,69 @@ 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)
}
10 changes: 1 addition & 9 deletions graphql/resolve/mutation_rewriter.go
View file
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
View file
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
View file
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 @@ -1242,7 +1242,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 @@ -1258,7 +1258,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 @@ -1274,7 +1274,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 @@ -1306,7 +1306,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 @@ -1355,7 +1355,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 @@ -1371,7 +1371,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 @@ -1388,7 +1388,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 @@ -3114,3 +3114,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
View file
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
View file
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

0 comments on commit fa85ff9

Please sign in to comment.