From dd806c27ea879db77bfe4a038756f3c3ff725e05 Mon Sep 17 00:00:00 2001 From: shivaji-dgraph Date: Wed, 17 Apr 2024 11:03:24 +0530 Subject: [PATCH] resolved review comments --- graphql/admin/endpoints_ee.go | 4 - graphql/e2e/common/mutation.go | 320 +++++++++++++++++++++++++ graphql/resolve/add_mutation_test.yaml | 313 ++++++++++++++++++++++++ graphql/schema/rules.go | 4 +- graphql/schema/wrappers.go | 1 - 5 files changed, 635 insertions(+), 7 deletions(-) diff --git a/graphql/admin/endpoints_ee.go b/graphql/admin/endpoints_ee.go index 27d1e913a73..3a3f806d9be 100644 --- a/graphql/admin/endpoints_ee.go +++ b/graphql/admin/endpoints_ee.go @@ -9,10 +9,6 @@ * may obtain a copy of the License at * * https://github.com/dgraph-io/dgraph/blob/main/licenses/DCL.txt - * - * Oct 2023: added Embedding related types and functions - * underlying DQL schema for embeddings is defined in schema.go - * resolvers are declared in admin.go */ package admin diff --git a/graphql/e2e/common/mutation.go b/graphql/e2e/common/mutation.go index 07c1675519d..cb9c8320b72 100644 --- a/graphql/e2e/common/mutation.go +++ b/graphql/e2e/common/mutation.go @@ -6273,6 +6273,63 @@ func addMutationWithIDFieldHavingInterfaceArg(t *testing.T) { error: "couldn't rewrite mutation addLibraryManager because failed to rewrite mutation" + " payload because id 102 already exists for field refID in some other implementing" + " type of interface Member", + }, { + name: "updating inherited @id with interface argument true," + + "returns error if given value for id already exist in a node of " + + "some other implementing type", + query: `mutation update($patch: UpdateLibraryMemberInput!) { + updateLibraryMember(input: $patch) { + libraryMember { + refID + } + } + }`, + variables: `{ + "patch": { + "filter": { + "refID": { + "in": "101" + } + }, + "set": { + "refID": "102", + "name": "Miles", + "readHours": "5d2hr" + } + } + }`, + error: "couldn't rewrite mutation updateLibraryMember because failed to rewrite" + + " mutation payload because id 102 already exists for field refID in some other" + + " implementing type of interface Member", + }, + { + name: "updating link to a type that have inherited @id field with interface" + + " argument true, returns error if given value for id field already exist" + + " in a node of some other implementing type", + query: `mutation update($patch: UpdateLibraryManagerInput!) { + updateLibraryManager(input: $patch) { + libraryManager { + name + } + } + }`, + variables: `{ + "patch": { + "filter": { + "name": { + "in": "Juliet" + } + }, + "set": { + "manages": { + "refID": "102" + } + } + } + }`, + error: "couldn't rewrite mutation updateLibraryManager because failed to rewrite mutation" + + " payload because id 102 already exists for field refID in some other" + + " implementing type of interface Member", }, } @@ -6303,3 +6360,266 @@ func addMutationWithIDFieldHavingInterfaceArg(t *testing.T) { DeleteGqlType(t, "CricketTeam", map[string]interface{}{}, 1, nil) DeleteGqlType(t, "LibraryManager", map[string]interface{}{}, 1, nil) } + +func xidUpdateAndNullableTests(t *testing.T) { + tcases := []struct { + name string + query string + variables string + error string + }{ + { + name: "2-level add mutation without nullable @id fields", + query: `mutation addEmployer($input: [AddEmployerInput!]!) { + addEmployer(input: $input, upsert: false) { + employer { + company + } + } + }`, + variables: `{ + "input": [ + { + "company": "ABC tech", + "name": "XYZ", + "worker": { + "name": "Alice", + "regNo": 101, + "empId": "E01" + } + }, + { + "company": "XYZ industry", + "name": "ABC", + "worker": { + "name": "Bob", + "regNo": 102, + "empId": "E02" + } + } + ] + }`, + }, { + name: "2-level add mutation with upserts without nullable @id fields", + query: `mutation addEmployer($input: [AddEmployerInput!]!) { + addEmployer(input: $input, upsert: true) { + employer { + company + } + } + }`, + variables: `{ + "input": { + "company": "ABC tech", + "worker": { + "name": "Juliet", + "regNo": 103, + "empId": "E03" + } + } + }`, + }, { + name: "upsert mutation gives error when multiple nodes are found with given @id fields", + query: `mutation addEmployer($input: [AddEmployerInput!]!) { + addEmployer(input: $input, upsert: true) { + employer { + company + } + } + }`, + variables: `{ + "input": { + "company": "ABC tech", + "name": "ABC" + } + }`, + error: "couldn't rewrite mutation addEmployer because failed to rewrite mutation" + + " payload because multiple nodes found for given xid values, updation not possible", + }, { + name: "upsert mutation gives error when multiple nodes are found with" + + " given @id fields at nested level", + query: `mutation addEmployer($input: [AddEmployerInput!]!) { + addEmployer(input: $input, upsert: true) { + employer { + company + } + } + }`, + variables: `{ + "input": { + "company": "ABC tech", + "worker": { + "empId": "E02", + "regNo": 103, + "name": "William" + } + } + }`, + error: "couldn't rewrite mutation addEmployer because failed to rewrite mutation" + + " payload because multiple nodes found for given xid values, updation not possible", + }, + { + name: "Non-nullable id should be present while creating new node at nested level" + + " using upsert", + query: `mutation addEmployer($input: [AddEmployerInput!]!) { + addEmployer(input: $input, upsert: true) { + employer { + company + } + } + }`, + variables: `{ + "input": { + "company": "ABC tech1", + "worker": { + "regNo": 104, + "name": "John" + } + } + }`, + error: "couldn't rewrite mutation addEmployer because failed to rewrite" + + " mutation payload because type Worker requires a value for" + + " field empId, but no value present", + }, + { + name: "update mutation fails when @id field is being updated" + + " and multiple nodes are selected in filter", + query: `mutation update($patch: UpdateEmployerInput!) { + updateEmployer(input: $patch) { + employer { + company + } + } + }`, + variables: `{ + "patch": { + "filter": { + "name": { + "in": [ + "XYZ", + "ABC" + ] + } + }, + "set": { + "company": "JKL" + } + } + }`, + error: "mutation updateEmployer failed because only one node is allowed in the filter" + + " while updating fields with @id directive", + }, { + name: "successfully updating @id field of a node ", + query: `mutation update($patch: UpdateEmployerInput!) { + updateEmployer(input: $patch) { + employer { + company + } + } + }`, + variables: `{ + "patch": { + "filter": { + "name": { + "in": [ + "XYZ" + ] + } + }, + "set": { + "name": "JKL", + "company": "JKL tech" + } + } + }`, + }, + { + name: "updating @id field returns error because given value in update mutation already exists", + query: `mutation update($patch: UpdateEmployerInput!) { + updateEmployer(input: $patch) { + employer { + company + } + } + }`, + variables: `{ + "patch": { + "filter": { + "name": { + "in": [ + "JKL" + ] + } + }, + "set": { + "name": "ABC", + "company": "ABC tech" + } + } + }`, + error: "couldn't rewrite mutation updateEmployer because failed to rewrite mutation" + + " payload because id ABC already exists for field name inside type Employer", + }, + { + name: "updating root @id fields and also create a nested link to nested object", + query: `mutation update($patch: UpdateEmployerInput!) { + updateEmployer(input: $patch) { + employer { + company + } + } + }`, + variables: `{ + "patch": { + "filter": { + "name": { + "in": [ + "JKL" + ] + } + }, + "set": { + "name": "MNO", + "company": "MNO tech", + "worker": { + "name": "Miles", + "empId": "E05", + "regNo": 105 + } + } + } + }`, + }, + } + + for _, tcase := range tcases { + t.Run(tcase.name, func(t *testing.T) { + var vars map[string]interface{} + if tcase.variables != "" { + err := json.Unmarshal([]byte(tcase.variables), &vars) + require.NoError(t, err) + } + params := &GraphQLParams{ + Query: tcase.query, + Variables: vars, + } + resp := params.ExecuteAsPost(t, GraphqlURL) + if tcase.error != "" { + require.Equal(t, tcase.error, resp.Errors[0].Error()) + } else { + RequireNoGQLErrors(t, resp) + } + + }) + } + + // Cleanup + filterEmployer := + map[string]interface{}{ + "name": map[string]interface{}{"in": []string{"ABC", "MNO"}}} + filterWorker := + map[string]interface{}{ + "regNo": map[string]interface{}{"in": []int{101, 102, 103, 105}}} + DeleteGqlType(t, "Employer", filterEmployer, 2, nil) + DeleteGqlType(t, "Worker", filterWorker, 4, nil) +} diff --git a/graphql/resolve/add_mutation_test.yaml b/graphql/resolve/add_mutation_test.yaml index 1f85a553e98..562d026a058 100644 --- a/graphql/resolve/add_mutation_test.yaml +++ b/graphql/resolve/add_mutation_test.yaml @@ -5204,3 +5204,316 @@ "dgraph.type": ["Person"], "uid": "_:Person_1" } +- + name: "2-level add mutation with nullable @id fields " + explaination: "bookId in Book and PenName in author are @id and nullable field, + we can skip them while doing add mutation. Nested object author doesn't exist, so we + add it and link it to book" + gqlmutation: | + mutation addBook($input: [AddBookInput!]!) { + addBook(input: $input, upsert: false) { + book { + title + } + } + } + gqlvariables: | + { "input": + [ + { + "title": "Sapiens", + "ISBN": "B02", + "publisher": "penguin", + "author": { + "name": "Alice", + "authorId": "A02" + } + } + ] + } + dgquery: |- + query { + Book_1(func: eq(Book.ISBN, "B02")) { + uid + dgraph.type + } + Book_2(func: eq(Book.title, "Sapiens")) { + uid + dgraph.type + } + author_3(func: eq(author.authorId, "A02")) { + uid + dgraph.type + } + } + dgmutations: + - setjson: | + { + "Book.title": "Sapiens", + "Book.ISBN": "B02", + "Book.publisher": "penguin", + "dgraph.type": [ + "Book" + ], + "Book.author": { + "author.authorId":"A02", + "author.book": [ + { + "uid": "_:Book_2" + } + ], + "author.name": "Alice", + "dgraph.type": [ + "author" + ], + "uid": "_:author_3" + }, + "uid": "_:Book_2" + } +- + name: "2- level add mutation with upsert and nullable @id fields " + explaination: "bookId in @id,penName in author are nullable @id fields and we can skip them. + title,ISBN in Book are @id fields,so also added in set Json, because @id fields will also be updated by upserts. + Both book and author already exist so we just link new author to book and delete old reference from book to author, + if there is any" + gqlmutation: | + mutation addBook($input: [AddBookInput!]!) { + addBook(input: $input, upsert: true) { + book { + title + } + } + } + gqlvariables: | + { "input": + [ + { + "title": "Sapiens", + "ISBN": "B01", + "publisher": "penguin", + "author": { + "name": "Alice", + "authorId": "A01" + } + } + ] + } + dgquery: |- + query { + Book_1(func: eq(Book.ISBN, "B01")) { + uid + dgraph.type + } + Book_2(func: eq(Book.title, "Sapiens")) { + uid + dgraph.type + } + author_3(func: eq(author.authorId, "A01")) { + uid + dgraph.type + } + } + qnametouid: | + { + "Book_2":"0x11", + "author_3": "0x12" + } + dgquerysec: |- + query { + Book_2 as Book_2(func: uid(0x11)) @filter(type(Book)) { + uid + } + var(func: uid(Book_2)) { + author_4 as Book.author @filter(NOT (uid(0x12))) + } + } + dgmutations: + - setjson: | + { + "Book.ISBN": "B01", + "Book.author": { + "author.book": [ + { + "uid": "uid(Book_2)" + } + ], + "uid": "0x12" + }, + "Book.publisher": "penguin", + "Book.title": "Sapiens", + "uid": "uid(Book_2)" + } + deletejson: | + [{ + "author.book": [ + { + "uid": "uid(Book_2)" + } + ], + "uid": "uid(author_4)" + }] + cond: "@if(gt(len(Book_2), 0))" +- + name: "add mutation with upsert gives error when multiple nodes are found for existence queries" + explaination: "Two different books exist for title and Sapiens @id fields, We can't do upsert mutation " + gqlmutation: | + mutation addBook($input: [AddBookInput!]!) { + addBook(input: $input, upsert: true) { + book { + title + } + } + } + gqlvariables: | + { "input": + [ + { + "title": "Sapiens", + "ISBN": "B01", + "publisher": "penguin" + } + ] + } + dgquery: |- + query { + Book_1(func: eq(Book.ISBN, "B01")) { + uid + dgraph.type + } + Book_2(func: eq(Book.title, "Sapiens")) { + uid + dgraph.type + } + } + qnametouid: | + { + "Book_1":"0x11", + "Book_2": "0x12" + } + error2: + { + "message": "failed to rewrite mutation payload because multiple nodes found + for given xid values, updation not possible" + } + +- + name: "add mutation with upsert at nested level gives error when multiple nodes are found + for existence queries" + explaination: "Two different author exist for penName and authorId @id fields inside author, + We can't link author to both books " + gqlmutation: | + mutation addBook($input: [AddBookInput!]!) { + addBook(input: $input, upsert: true) { + book { + title + } + } + } + gqlvariables: | + { "input": + [ + { + "title": "Sapiens", + "ISBN": "B01", + "publisher": "penguin", + "author": { + "penName": "Alice", + "authorId": "A01" + } + } + ] + } + dgquery: |- + query { + Book_1(func: eq(Book.ISBN, "B01")) { + uid + dgraph.type + } + Book_2(func: eq(Book.title, "Sapiens")) { + uid + dgraph.type + } + author_3(func: eq(author.authorId, "A01")) { + uid + dgraph.type + } + author_4(func: eq(author.penName, "Alice")) { + uid + dgraph.type + } + } + qnametouid: | + { + "author_3":"0x11", + "author_4": "0x12" + } + error2: + { + "message": "failed to rewrite mutation payload because multiple nodes + found for given xid values, updation not possible" + } + +- + name: "No xid present for add mutation with upsert" + explaination: "If none of the xid field is given in upsert mutation then there will be no existence queries, + and it will behave as simple add mutation,i.e. create new node with all the given fields" + gqlmutation: | + mutation addBook($input: [AddBookInput!]!) { + addBook(input: $input, upsert: true) { + book { + title + } + } + } + gqlvariables: | + { "input": + [ + { + "publisher": "penguin" + } + ] + } + dgmutations: + - setjson: | + { + "Book.publisher": "penguin", + "dgraph.type": [ + "Book" + ], + "uid":"_:Book_1" + } +- + name: "Non-nullable xid should be present in add Mutation for nested field" + explaination: "non-nullable @id field id in comment1 type not provided. As no reference is + provided for comment, we treat it as new node, and return error for missing xid." + gqlmutation: | + mutation addPost1($input: [AddPost1Input!]!) { + addPost1(input: $input, upsert: false) { + post1 { + content + } + } + } + gqlvariables: | + { "input": + [ + { + "id": "P01", + "content":"Intro to GraphQL", + "comments":[{ + "message":"Nice Intro! Love GraphQl" + }] + } + ] + } + dgquery: |- + query { + Post1_1(func: eq(Post1.id, "P01")) { + uid + dgraph.type + } + } + error2: + { + "message": "failed to rewrite mutation payload because field id cannot be empty" + } \ No newline at end of file diff --git a/graphql/schema/rules.go b/graphql/schema/rules.go index c2660ae5246..14de5f5229d 100644 --- a/graphql/schema/rules.go +++ b/graphql/schema/rules.go @@ -33,8 +33,8 @@ import ( ) const ( - baseRules = 0 - listCoercionRules = 1000 + baseRules = -2 + listCoercionRules = -1 ) func init() { diff --git a/graphql/schema/wrappers.go b/graphql/schema/wrappers.go index 2f27ed54b0b..91f5ff88801 100644 --- a/graphql/schema/wrappers.go +++ b/graphql/schema/wrappers.go @@ -113,7 +113,6 @@ const ( SimilarByIdQuerySuffix = "ById" SimilarByEmbeddingQuerySuffix = "ByEmbedding" SimilarQueryResultTypeSuffix = "WithDistance" - SimilarQueryDistanceFieldName = "hm_distance" SimilarSearchMetricEuclidian = "euclidian" SimilarSearchMetricDotProduct = "dotproduct" SimilarSearchMetricCosine = "cosine"