From fd3083277b008f067a64bf0cb33d859e036050cf Mon Sep 17 00:00:00 2001 From: minhaj-shakeel Date: Mon, 5 Oct 2020 18:23:15 +0530 Subject: [PATCH] Fix(GraphQL): fix object Linking with `hasInverse` (#6557) (#6648) This PR fixes incorrect linking of objects with `hasInverse`. For example For this schema, `type Country` has field `state` with `hasInverse` predicate. ``` type Country { id: ID! name: String! @search(by: [trigram, hash]) states: [State] @hasInverse(field: country) } type State { id: ID! xcode: String! @id @search(by: [regexp]) name: String! capital: String country: Country } ``` And for the following mutation ``` mutation addCountry($input: [AddCountryInput!]!) { addCountry(input: $input) { country { name states{ xcode name country{ name } } } } } ``` with `input`: ``` { "input": { "name": "A Country", "states": [ { "xcode": "abc", "name": "Alphabet" }, { "xcode": "def", "name": "Vowel", "country": { "name": "B country" } } ] } } ``` should result in ``` { "addCountry": { "country": [ { "name": "A country", "states": [ { "country": { "name": "A country" }, "name": "Alphabet", "xcode": "abc" }, { "country": { "name": "A country" }, "name": "Vowel", "xcode": "def" } ] } ] } }` ``` whereas the incorrect output was ``` { "addCountry": { "country": [ { "name": "A country", "states": [ { "country": { "name": "A country" }, "name": "Alphabet", "xcode": "abc" }, { "country": { "name": "B country" }, "name": "Vowel", "xcode": "def" } ] } ] } }` ``` This PR fixes this issue. (cherry picked from commit e496467f2f733bdd800d4ea0cec2dca7567ff485) --- graphql/e2e/common/common.go | 1 + graphql/e2e/common/mutation.go | 76 ++++++++++++++++++++++++++ graphql/resolve/add_mutation_test.yaml | 42 +++++++++++++- graphql/resolve/mutation_rewriter.go | 55 +++++++++++++++++++ 4 files changed, 171 insertions(+), 3 deletions(-) diff --git a/graphql/e2e/common/common.go b/graphql/e2e/common/common.go index 2abfaebd7d3..73ce7017848 100644 --- a/graphql/e2e/common/common.go +++ b/graphql/e2e/common/common.go @@ -358,6 +358,7 @@ func RunAll(t *testing.T) { t.Run("deep XID mutations", deepXIDMutations) t.Run("three level xid", testThreeLevelXID) t.Run("nested add mutation with @hasInverse", nestedAddMutationWithHasInverse) + t.Run("add mutation with @hasInverse overrides correctly", addMutationWithHasInverseOverridesCorrectly) t.Run("error in multiple mutations", addMultipleMutationWithOneError) t.Run("dgraph directive with reverse edge adds data correctly", addMutationWithReverseDgraphEdge) diff --git a/graphql/e2e/common/mutation.go b/graphql/e2e/common/mutation.go index 771fcb1d2e9..924ecfcc766 100644 --- a/graphql/e2e/common/mutation.go +++ b/graphql/e2e/common/mutation.go @@ -3607,3 +3607,79 @@ func nestedAddMutationWithHasInverse(t *testing.T) { // cleanup deleteGqlType(t, "Person1", map[string]interface{}{}, 3, nil) } + +func addMutationWithHasInverseOverridesCorrectly(t *testing.T) { + params := &GraphQLParams{ + Query: `mutation addCountry($input: [AddCountryInput!]!) { + addCountry(input: $input) { + country { + name + states{ + xcode + name + country{ + name + } + } + } + } + }`, + + Variables: map[string]interface{}{ + "input": []interface{}{ + map[string]interface{}{ + "name": "A country", + "states": []interface{}{ + map[string]interface{}{ + "xcode": "abc", + "name": "Alphabet", + }, + map[string]interface{}{ + "xcode": "def", + "name": "Vowel", + "country": map[string]interface{}{ + "name": "B country", + }, + }, + }, + }, + }, + }, + } + + gqlResponse := postExecutor(t, GraphqlURL, params) + RequireNoGQLErrors(t, gqlResponse) + + expected := `{ + "addCountry": { + "country": [ + { + "name": "A country", + "states": [ + { + "country": { + "name": "A country" + }, + "name": "Alphabet", + "xcode": "abc" + }, + { + "country": { + "name": "A country" + }, + "name": "Vowel", + "xcode": "def" + } + ] + } + ] + } + }` + testutil.CompareJSON(t, expected, string(gqlResponse.Data)) + filter := map[string]interface{}{"name": map[string]interface{}{"eq": "A country"}} + deleteCountry(t, filter, 1, nil) + filter = map[string]interface{}{"xcode": map[string]interface{}{"eq": "abc"}} + deleteState(t, filter, 1, nil) + filter = map[string]interface{}{"xcode": map[string]interface{}{"eq": "def"}} + deleteState(t, filter, 1, nil) +} diff --git a/graphql/resolve/add_mutation_test.yaml b/graphql/resolve/add_mutation_test.yaml index 91a80162e4f..7bbc516257d 100644 --- a/graphql/resolve/add_mutation_test.yaml +++ b/graphql/resolve/add_mutation_test.yaml @@ -1600,7 +1600,7 @@ { "auth": { "name": "A.N. Author", - "posts": [ { "postID": "0x456" } ] + "posts": [ { "postID": "0x456" }, {"title": "New Post", "author": {"name": "Abhimanyu"}} ] } } dgmutations: @@ -1613,6 +1613,12 @@ { "uid": "0x456", "Post.author": { "uid": "_:Author1" } + }, + { + "uid": "_:Post4", + "dgraph.type": ["Post"], + "Post.title": "New Post", + "Post.author": { "uid": "_:Author1" } } ] } @@ -1647,7 +1653,10 @@ { "inp": { "name": "A Country", - "states": [ { "code": "abc", "name": "Alphabet" } ] + "states": [ + { "code": "abc", "name": "Alphabet" }, + { "code": "def", "name": "Vowel", "country": { "name": "B country" } } + ] } } @@ -1656,6 +1665,9 @@ State3 as State3(func: eq(State.code, "abc")) @filter(type(State)) { uid } + State6 as State6(func: eq(State.code, "def")) @filter(type(State)) { + uid + } } dgmutations: - setjson: | @@ -1668,6 +1680,16 @@ "uid": "_:State3" } cond: "@if(eq(len(State3), 0))" + - setjson: | + { + "State.code": "def", + "State.name": "Vowel", + "dgraph.type": [ + "State" + ], + "uid": "_:State6" + } + cond: "@if(eq(len(State6), 0))" dgquerysec: |- query { @@ -1677,6 +1699,12 @@ var(func: uid(State3)) { Country4 as State.country } + State6 as State6(func: eq(State.code, "def")) @filter(type(State)) { + uid + } + var(func: uid(State6)) { + Country7 as State.country + } } dgmutationssec: - setjson: | @@ -1688,6 +1716,10 @@ { "uid": "uid(State3)", "State.country": { "uid": "_:Country1" } + }, + { + "uid": "uid(State6)", + "State.country": { "uid": "_:Country1" } } ] } @@ -1696,9 +1728,13 @@ { "uid": "uid(Country4)", "Country.states": [ { "uid": "uid(State3)" } ] + }, + { + "uid": "uid(Country7)", + "Country.states": [ { "uid": "uid(State6)" } ] } ] - cond: "@if(eq(len(State3), 1))" + cond: "@if(eq(len(State3), 1) AND eq(len(State6), 1))" - name: "Deep XID 4 level deep" gqlmutation: | diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index 90050d51ae7..6cee9a6c684 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -996,6 +996,30 @@ func rewriteObject( if xid != nil && xidString != "" { xidFrag = asXIDReference(ctx, srcField, srcUID, typ, xid.Name(), xidString, variable, withAdditionalDeletes, varGen, xidMetadata) + + // Inverse Link is added as a Part of asXIDReference so we delete any provided + // Link to the object. + // Example: for this mutation + // mutation addCountry($inp: AddCountryInput!) { + // addCountry(input: [$inp]) { + // country { + // id + // } + // } + // } + // with the input: + //{ + // "inp": { + // "name": "A Country", + // "states": [ + // { "code": "abc", "name": "Alphabet" }, + // { "code": "def", "name": "Vowel", "country": { "name": "B country" } } + // ] + // } + // } + // we delete the link of Second state to "B Country" + deleteInverseObject(obj, srcField) + if deepXID > 2 { // Here we link the already existing node with an xid to the parent whose id is // passed in srcUID. We do this linking only if there is a hasInverse relationship @@ -1059,11 +1083,33 @@ func rewriteObject( myUID = fmt.Sprintf("_:%s", variable) if xid == nil || deepXID > 2 { + // If this object had an overwritten value for the inverse field, then we don't want to + // use that value as we will add the link to the inverse field in the below + // function call with the parent of this object + // for example, for this mutation: + // mutation addAuthor($auth: AddAuthorInput!) { + // addAuthor(input: [$auth]) { + // author { + // id + // } + // } + // } + // with the following input + // { + // "auth": { + // "name": "A.N. Author", + // "posts": [ { "postID": "0x456" }, {"title": "New Post", "author": {"name": "Abhimanyu"}} ] + // } + // } + // We delete the link of second input post with Author "name" : "Abhimanyu". + deleteInverseObject(obj, srcField) + // Lets link the new node that we are creating with the parent if a @hasInverse // exists between the two. // So for example if we had the addAuthor mutation which is also adding nested // posts, then we add the link _:Post Post.author AuthorUID(srcUID) here. addInverseLink(newObj, srcField, srcUID) + } } else { myUID = srcUID @@ -1636,6 +1682,15 @@ func attachChild(res map[string]interface{}, parent schema.Type, child schema.Fi } } +func deleteInverseObject(obj map[string]interface{}, srcField schema.FieldDefinition) { + if srcField != nil { + invField := srcField.Inverse() + if invField != nil && invField.Type().ListType() == nil { + delete(obj, invField.Name()) + } + } +} + func addInverseLink(obj map[string]interface{}, srcField schema.FieldDefinition, srcUID string) { if srcField != nil { invField := srcField.Inverse()