Skip to content
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.

Sign up for GitHub

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

fix(GRAPHQL): fix empty remove in update mutation patch, that remove all the data for nodes in filter. #7563

Merged
merged 4 commits into from
Mar 15, 2021
Merged

fix(GRAPHQL): fix empty remove in update mutation patch, that remove all the data for nodes in filter. #7563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
605ad18
added fix and test for empty remove case
JatinDev543 Mar 15, 2021
0faff24
modified tests
JatinDev543 Mar 15, 2021
941c49e
added test for empty set
JatinDev543 Mar 15, 2021
61f479c
addressed rajas comments
JatinDev543 Mar 15, 2021
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 graphql/e2e/common/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,7 @@ func RunAll(t *testing.T) {
t.Run("mutations have extensions", mutationsHaveExtensions)
t.Run("alias works for mutations", mutationsWithAlias)
t.Run("three level deep", threeLevelDeepMutation)
t.Run("update mutation without set & remove", updateMutationWithoutSetRemove)
t.Run("update mutation without set & remove", updateMutationTestsWithDifferentSetRemoveCases)
t.Run("Input coercing for int64 type", int64BoundaryTesting)
t.Run("List of integers", intWithList)
t.Run("Check cascade with mutation without ID field", checkCascadeWithMutationWithoutIDField)
Expand Down
98 changes: 83 additions & 15 deletions graphql/e2e/common/mutation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3918,11 +3918,16 @@ func mutationsWithAlias(t *testing.T) {
require.JSONEq(t, multiMutationExpected, string(gqlResponse.Data))
}

func updateMutationWithoutSetRemove(t *testing.T) {
func updateMutationTestsWithDifferentSetRemoveCases(t *testing.T) {
country := addCountry(t, postExecutor)

updateCountryParams := &GraphQLParams{
Query: `mutation updateCountry($id: ID!){
tcases := []struct {
name string
query string
variables map[string]interface{}
expected string
}{{
name: "update mutation without set and Remove",
query: `mutation updateCountry($id: ID!){
updateCountry(input: {filter: {id: [$id]}}) {
numUids
country {
Expand All @@ -3931,19 +3936,82 @@ func updateMutationWithoutSetRemove(t *testing.T) {
}
}
}`,
Variables: map[string]interface{}{"id": country.ID},
variables: map[string]interface{}{"id": country.ID},
expected: `{
"updateCountry": {
"numUids": 0,
"country": []
}
}`,
}, {
name: "update mutation with empty remove",
query: `mutation updateCountry($id: ID!){
updateCountry(input: {filter: {id: [$id]}, remove:{} }) {
numUids
country {
id
name
}
}
}`,
variables: map[string]interface{}{"id": country.ID},
expected: `{
"updateCountry": {
"numUids": 0,
"country": []
}
}`,
}, {
name: "update mutation with empty set and remove",
query: `mutation updateCountry($id: ID!){
updateCountry(input: {filter: {id: [$id]}, remove:{}, set: {} }) {
numUids
country {
id
name
}
}
}`,
variables: map[string]interface{}{"id": country.ID},
expected: `{
"updateCountry": {
"numUids": 0,
"country": []
}
}`,
}, {
name: "update mutation with empty set",
query: `mutation updateCountry($id: ID!){
updateCountry(input: {filter: {id: [$id]}, set:{} }) {
numUids
country {
id
name
}
}
}`,
variables: map[string]interface{}{"id": country.ID},
expected: `{
"updateCountry": {
"numUids": 0,
"country": []
}
}`,
},
}
for _, tcase := range tcases {
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.expected, string(resp.Data))
})
}
gqlResponse := updateCountryParams.ExecuteAsPost(t, GraphqlURL)
RequireNoGQLErrors(t, gqlResponse)

require.JSONEq(t, `{
"updateCountry": {
"numUids": 0,
"country": []
}
}`, string(gqlResponse.Data))

// cleanup
// expectedNumUids:1 will ensures that no node has been deleted because of remove {}
deleteCountry(t, map[string]interface{}{"id": []string{country.ID}}, 1, nil)
}

Expand Down
130 changes: 70 additions & 60 deletions graphql/resolve/mutation_rewriter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,31 +328,35 @@ func (urw *UpdateRewriter) RewriteQueries(
// Write existence queries for set
if setArg != nil {
obj := setArg.(map[string]interface{})
queries, errs := existenceQueries(ctx, mutatedType, nil, urw.VarGen, obj, urw.XidMetadata)
if len(errs) > 0 {
var gqlErrors x.GqlErrorList
for _, err := range errs {
gqlErrors = append(gqlErrors, schema.AsGQLErrors(err)...)
if len(obj) != 0 {
queries, errs := existenceQueries(ctx, mutatedType, nil, urw.VarGen, obj, urw.XidMetadata)
if len(errs) > 0 {
var gqlErrors x.GqlErrorList
for _, err := range errs {
gqlErrors = append(gqlErrors, schema.AsGQLErrors(err)...)
}
retErrors = schema.AppendGQLErrs(retErrors, schema.GQLWrapf(gqlErrors,
"failed to rewrite mutation payload"))
}
retErrors = schema.AppendGQLErrs(retErrors, schema.GQLWrapf(gqlErrors,
"failed to rewrite mutation payload"))
ret = append(ret, queries...)
}
ret = append(ret, queries...)
}

// Write existence queries for remove
if delArg != nil {
obj := delArg.(map[string]interface{})
queries, errs := existenceQueries(ctx, mutatedType, nil, urw.VarGen, obj, urw.XidMetadata)
if len(errs) > 0 {
var gqlErrors x.GqlErrorList
for _, err := range errs {
gqlErrors = append(gqlErrors, schema.AsGQLErrors(err)...)
if len(obj) != 0 {
JatinDev543 marked this conversation as resolved.
Show resolved Hide resolved
queries, errs := existenceQueries(ctx, mutatedType, nil, urw.VarGen, obj, urw.XidMetadata)
if len(errs) > 0 {
var gqlErrors x.GqlErrorList
for _, err := range errs {
gqlErrors = append(gqlErrors, schema.AsGQLErrors(err)...)
}
retErrors = schema.AppendGQLErrs(retErrors, schema.GQLWrapf(gqlErrors,
"failed to rewrite mutation payload"))
}
retErrors = schema.AppendGQLErrs(retErrors, schema.GQLWrapf(gqlErrors,
"failed to rewrite mutation payload"))
ret = append(ret, queries...)
}
ret = append(ret, queries...)
}
return ret, retErrors
}
Expand Down Expand Up @@ -600,46 +604,50 @@ func (urw *UpdateRewriter) Rewrite(

upsertQuery := RewriteUpsertQueryFromMutation(m, authRw, MutationQueryVar, m.Name(), "")
srcUID := MutationQueryVarUID

if setArg == nil && delArg == nil {
objDel, okDelArg := delArg.(map[string]interface{})
JatinDev543 marked this conversation as resolved.
Show resolved Hide resolved
objSet, okSetArg := setArg.(map[string]interface{})
// if set and remove arguments in update patch are not present or they are empty
// then we return from here
if (setArg == nil || (len(objSet) == 0 && okSetArg)) && (delArg == nil || (len(objDel) == 0 && okDelArg)) {
JatinDev543 marked this conversation as resolved.
Show resolved Hide resolved
return ret, nil
}

if setArg != nil {
obj := setArg.(map[string]interface{})
fragment, _, errs := rewriteObject(ctx, mutatedType, nil, srcUID, varGen, obj, xidMetadata, idExistence, UpdateWithSet)
if len(errs) > 0 {
var gqlErrors x.GqlErrorList
for _, err := range errs {
gqlErrors = append(gqlErrors, schema.AsGQLErrors(err)...)
if len(objSet) != 0 {
fragment, _, errs := rewriteObject(ctx, mutatedType, nil, srcUID, varGen, objSet, xidMetadata, idExistence, UpdateWithSet)
if len(errs) > 0 {
var gqlErrors x.GqlErrorList
for _, err := range errs {
gqlErrors = append(gqlErrors, schema.AsGQLErrors(err)...)
}
retErrors = schema.AppendGQLErrs(retErrors, schema.GQLWrapf(gqlErrors,
"failed to rewrite mutation payload"))
}
if fragment != nil {
setFrag = append(setFrag, fragment)
urw.setFrags = append(urw.setFrags, fragment)
}
retErrors = schema.AppendGQLErrs(retErrors, schema.GQLWrapf(gqlErrors,
"failed to rewrite mutation payload"))
}
if fragment != nil {
setFrag = append(setFrag, fragment)
urw.setFrags = append(urw.setFrags, fragment)
}
}

if delArg != nil {
obj := delArg.(map[string]interface{})
// Set additional deletes to false
fragment, _, errs := rewriteObject(ctx, mutatedType, nil, srcUID, varGen, obj, xidMetadata, idExistence, UpdateWithRemove)
if len(errs) > 0 {
var gqlErrors x.GqlErrorList
for _, err := range errs {
gqlErrors = append(gqlErrors, schema.AsGQLErrors(err)...)
if len(objDel) != 0 {
// Set additional deletes to false
fragment, _, errs := rewriteObject(ctx, mutatedType, nil, srcUID, varGen, objDel, xidMetadata, idExistence, UpdateWithRemove)
if len(errs) > 0 {
var gqlErrors x.GqlErrorList
for _, err := range errs {
gqlErrors = append(gqlErrors, schema.AsGQLErrors(err)...)
}
retErrors = schema.AppendGQLErrs(retErrors, schema.GQLWrapf(gqlErrors,
"failed to rewrite mutation payload"))
}
if fragment != nil {
delFrag = append(delFrag, fragment)
urw.delFrags = append(urw.delFrags, fragment)
}
retErrors = schema.AppendGQLErrs(retErrors, schema.GQLWrapf(gqlErrors,
"failed to rewrite mutation payload"))
}
if fragment != nil {
delFrag = append(delFrag, fragment)
urw.delFrags = append(urw.delFrags, fragment)
}
}

buildMutation := func(setFrag, delFrag []*mutationFragment) *UpsertMutation {
var mutSet, mutDel []*dgoapi.Mutation
queries := upsertQuery
Expand Down Expand Up @@ -668,22 +676,24 @@ func (urw *UpdateRewriter) Rewrite(
}

if delArg != nil {
addUpdateCondition(delFrag)
var errDel error
mutDel, errDel = mutationsFromFragments(
delFrag,
func(frag *mutationFragment) ([]byte, error) {
return nil, nil
},
func(frag *mutationFragment) ([]byte, error) {
return json.Marshal(frag.fragment)
})

retErrors = schema.AppendGQLErrs(retErrors, errDel)

q2 := queryFromFragments(delFrag)
if q2 != nil {
queries = append(queries, q2.Children...)
if len(objDel) != 0 {
addUpdateCondition(delFrag)
var errDel error
mutDel, errDel = mutationsFromFragments(
delFrag,
func(frag *mutationFragment) ([]byte, error) {
return nil, nil
},
func(frag *mutationFragment) ([]byte, error) {
return json.Marshal(frag.fragment)
})

retErrors = schema.AppendGQLErrs(retErrors, errDel)

q2 := queryFromFragments(delFrag)
if q2 != nil {
queries = append(queries, q2.Children...)
}
}
}

Expand Down
68 changes: 68 additions & 0 deletions graphql/resolve/update_mutation_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1891,3 +1891,71 @@
"Book.publisher": "penguin"
}
cond: "@if(gt(len(x), 0))"

-
name: "delete json shouldn't be generated for empty remove"
gqlmutation: |
mutation updateAuthor($patch: UpdateAuthorInput!) {
updateAuthor(input: $patch) {
author {
id
}
}
}
gqlvariables: |
{
"patch": {
"filter": {
"id": ["0x123"]
},
"set": { "name": "Alice" },
"remove": {}
}
}

dgquerysec: |-
query {
x as updateAuthor(func: uid(0x123)) @filter(type(Author)) {
uid
}
}
dgmutations:
- setjson: |
{ "uid" : "uid(x)",
"Author.name": "Alice"
}
cond: "@if(gt(len(x), 0))"

-
name: "set json shouldn't be generated for empty set"
gqlmutation: |
mutation updateAuthor($patch: UpdateAuthorInput!) {
updateAuthor(input: $patch) {
author {
id
}
}
}
gqlvariables: |
{
"patch": {
"filter": {
"id": ["0x123"]
},
"set": {},
"remove": {"name": "Alice"}
}
}

dgquerysec: |-
query {
x as updateAuthor(func: uid(0x123)) @filter(type(Author)) {
uid
}
}
dgmutations:
- deletejson: |
{ "uid": "uid(x)",
"Author.name": "Alice"
}
cond: "@if(gt(len(x), 0))"