From 70572e548c01c081a1e879d6a5dd0d37461646de Mon Sep 17 00:00:00 2001 From: Abhimanyu Singh Gaur <12651351+abhimanyusinghgaur@users.noreply.github.com> Date: Tue, 12 May 2020 17:17:34 +0530 Subject: [PATCH] graphql: Minor delete mutation msg fix (#5316) This PR changes the delete msg reported for the case when no nodes are deleted to No nodes were deleted. In addition, it corrects some tests which were wrong earlier. Fixes #GRAPHQL-423. Co-authored-by: Abhimanyu Singh Gaur <12651351+abhimanyusinghgaur@users.noreply.github.com> (cherry picked from commit 47f2f51db7d46fd17f29663257c5b26a656478e6) # Conflicts: # ee/acl/acl_test.go # graphql/e2e/auth/auth_test.go # graphql/e2e/common/mutation.go # graphql/resolve/mutation.go --- ee/acl/acl_test.go | 46 +++------ graphql/e2e/common/mutation.go | 174 ++++++++------------------------- graphql/resolve/mutation.go | 3 + 3 files changed, 58 insertions(+), 165 deletions(-) diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index b751f64cb4c..7970042cdd7 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -95,7 +95,7 @@ func checkUserCount(t *testing.T, resp []byte, expected int) { require.Equal(t, expected, len(r.Data.AddUser.User)) } -func deleteUser(t *testing.T, accessToken, username string) { +func deleteUser(t *testing.T, accessToken, username string, confirmDeletion bool) { // TODO - Verify that only one uid got deleted once numUids are returned as part of the payload. delUser := `mutation deleteUser($name: String!) { deleteUser(filter: {name: {eq: $name}}) { @@ -110,7 +110,9 @@ func deleteUser(t *testing.T, accessToken, username string) { }, } b := makeRequest(t, accessToken, params) - require.JSONEq(t, `{"data":{"deleteUser":{"msg":"Deleted"}}}`, string(b)) + if confirmDeletion { + require.JSONEq(t, `{"data":{"deleteUser":{"msg":"Deleted"}}}`, string(b)) + } } func deleteGroup(t *testing.T, accessToken, name string) { @@ -159,19 +161,13 @@ func TestPasswordReturn(t *testing.T) { } func TestGetCurrentUser(t *testing.T) { - accessJwt, _, err := testutil.HttpLogin(&testutil.LoginParams{ - Endpoint: adminEndpoint, - UserID: "groot", - Passwd: "password", - }) - require.NoError(t, err, "login failed") - + accessJwt, _ := testutil.GrootHttpLogin(adminEndpoint) require.Equal(t, string(getCurrentUser(t, accessJwt)), `{"data":{"getCurrentUser":{"name":"groot"}}}`) // clean up the user to allow repeated running of this test userid := "hamilton" - deleteUser(t, accessJwt, userid) + deleteUser(t, accessJwt, userid, false) glog.Infof("cleaned up db user state") resp := createUser(t, accessJwt, userid, userpassword) @@ -189,26 +185,15 @@ func TestGetCurrentUser(t *testing.T) { } func TestCreateAndDeleteUsers(t *testing.T) { - accessJwt, _, err := testutil.HttpLogin(&testutil.LoginParams{ - Endpoint: adminEndpoint, - UserID: "groot", - Passwd: "password", - }) - require.NoError(t, err, "login failed") - - // clean up the user to allow repeated running of this test - deleteUser(t, accessJwt, userid) - glog.Infof("cleaned up db user state") - - resp := createUser(t, accessJwt, userid, userpassword) - checkUserCount(t, resp, 1) + resetUser(t) // adding the user again should fail - resp = createUser(t, accessJwt, userid, userpassword) + accessJwt, _ := testutil.GrootHttpLogin(adminEndpoint) + resp := createUser(t, accessJwt, userid, userpassword) checkUserCount(t, resp, 0) // delete the user - deleteUser(t, accessJwt, userid) + deleteUser(t, accessJwt, userid, true) resp = createUser(t, accessJwt, userid, userpassword) // now we should be able to create the user again @@ -216,15 +201,10 @@ func TestCreateAndDeleteUsers(t *testing.T) { } func resetUser(t *testing.T) { - accessJwt, _, err := testutil.HttpLogin(&testutil.LoginParams{ - Endpoint: adminEndpoint, - UserID: "groot", - Passwd: "password", - }) - require.NoError(t, err, "login failed") + accessJwt, _ := testutil.GrootHttpLogin(adminEndpoint) // clean up the user to allow repeated running of this test - deleteUser(t, accessJwt, userid) + deleteUser(t, accessJwt, userid, false) glog.Infof("deleted user") resp := createUser(t, accessJwt, userid, userpassword) @@ -1564,7 +1544,7 @@ func TestDeleteUserShouldDeleteUserFromGroup(t *testing.T) { }) require.NoError(t, err, "login failed") - deleteUser(t, accessJwt, userid) + deleteUser(t, accessJwt, userid, true) gqlQuery := ` query { diff --git a/graphql/e2e/common/mutation.go b/graphql/e2e/common/mutation.go index fd12dd1de38..c7ddba99644 100644 --- a/graphql/e2e/common/mutation.go +++ b/graphql/e2e/common/mutation.go @@ -381,6 +381,7 @@ func deepMutationsTest(t *testing.T, executeRequest requestExecutor) { name } posts { + postID title text tags @@ -429,7 +430,7 @@ func deepMutationsTest(t *testing.T, executeRequest requestExecutor) { cleanUp(t, []*country{newCountry, anotherCountry}, []*author{newAuth}, - []*post{newAuth.Posts[0], newAuth.Posts[0], patchSet.Posts[0]}) + []*post{newAuth.Posts[0], newAuth.Posts[1], result.UpdateAuthor.Author[0].Posts[1]}) } func testMultipleMutations(t *testing.T) { @@ -1073,9 +1074,8 @@ func deleteMutationWithMultipleIds(t *testing.T) { country := addCountry(t, postExecutor) anotherCountry := addCountry(t, postExecutor) t.Run("delete Country", func(t *testing.T) { - deleteCountryExpected := `{"deleteCountry" : { "msg": "Deleted" } }` filter := map[string]interface{}{"id": []string{country.ID, anotherCountry.ID}} - deleteCountry(t, filter, deleteCountryExpected, nil) + deleteCountry(t, filter, 2, nil) }) t.Run("check Country is deleted", func(t *testing.T) { @@ -1088,9 +1088,8 @@ func deleteMutationWithSingleID(t *testing.T) { newCountry := addCountry(t, postExecutor) anotherCountry := addCountry(t, postExecutor) t.Run("delete Country", func(t *testing.T) { - deleteCountryExpected := `{"deleteCountry" : { "msg": "Deleted" } }` filter := map[string]interface{}{"id": []string{newCountry.ID}} - deleteCountry(t, filter, deleteCountryExpected, nil) + deleteCountry(t, filter, 1, nil) }) // In this case anotherCountry shouldn't be deleted. @@ -1110,14 +1109,13 @@ func deleteMutationByName(t *testing.T) { } updateCountry(t, filter, anotherCountry.Name, true) - deleteCountryExpected := `{"deleteCountry" : { "msg": "Deleted" } }` t.Run("delete Country", func(t *testing.T) { filter := map[string]interface{}{ "name": map[string]interface{}{ "regexp": "/" + newCountry.Name + "/", }, } - deleteCountry(t, filter, deleteCountryExpected, nil) + deleteCountry(t, filter, 1, nil) }) // In this case anotherCountry shouldn't be deleted. @@ -1437,7 +1435,7 @@ func deleteMutationSingleReference(t *testing.T, executeRequest requestExecutor) require.NoError(t, err) filter := map[string]interface{}{"id": []string{addResult.AddCountry.Country[0].ID}} - deleteCountry(t, filter, `{"deleteCountry" : { "msg": "Deleted" } }`, nil) + deleteCountry(t, filter, 1, nil) // the state doesn't belong to a country getCatParams := &GraphQLParams{ @@ -1484,7 +1482,7 @@ func deleteMutationMultipleReferences(t *testing.T, executeRequest requestExecut }} requireAuthor(t, newAuthor.ID, newAuthor, executeRequest) - deletePost(t, newPost.PostID, `{"deletePost" : { "msg": "Deleted" } }`, nil) + deletePost(t, newPost.PostID, 1, nil) // the post isn't in the author's list of posts newAuthor.Posts = []*post{} @@ -1511,72 +1509,27 @@ func deleteMutationMultipleReferences(t *testing.T, executeRequest requestExecut func deleteCountry( t *testing.T, filter map[string]interface{}, - deleteCountryExpected string, + expectedNumUids int, expectedErrors x.GqlErrorList) { - - deleteCountryParams := &GraphQLParams{ - Query: `mutation deleteCountry($filter: CountryFilter!) { - deleteCountry(filter: $filter) { msg } - }`, - Variables: map[string]interface{}{"filter": filter}, - } - - gqlResponse := deleteCountryParams.ExecuteAsPost(t, graphqlURL) - require.JSONEq(t, deleteCountryExpected, string(gqlResponse.Data)) - - if diff := cmp.Diff(expectedErrors, gqlResponse.Errors); diff != "" { - t.Errorf("errors mismatch (-want +got):\n%s", diff) - } + deleteGqlType(t, "Country", filter, expectedNumUids, expectedErrors) } func deleteAuthor( t *testing.T, authorID string, - deleteAuthorExpected string, + expectedNumUids int, expectedErrors x.GqlErrorList) { - - deleteAuthorParams := &GraphQLParams{ - Query: `mutation deleteAuthor($filter: AuthorFilter!) { - deleteAuthor(filter: $filter) { msg } - }`, - Variables: map[string]interface{}{ - "filter": map[string]interface{}{ - "id": []string{authorID}, - }, - }, - } - - gqlResponse := deleteAuthorParams.ExecuteAsPost(t, graphqlURL) - - require.JSONEq(t, deleteAuthorExpected, string(gqlResponse.Data)) - - if diff := cmp.Diff(expectedErrors, gqlResponse.Errors); diff != "" { - t.Errorf("errors mismatch (-want +got):\n%s", diff) - } + filter := map[string]interface{}{"id": []string{authorID}} + deleteGqlType(t, "Author", filter, expectedNumUids, expectedErrors) } func deletePost( t *testing.T, postID string, - deletePostExpected string, + expectedNumUids int, expectedErrors x.GqlErrorList) { - - deletePostParams := &GraphQLParams{ - Query: `mutation deletePost($filter: PostFilter!) { - deletePost(filter: $filter) { msg } - }`, - Variables: map[string]interface{}{"filter": map[string]interface{}{ - "postID": []string{postID}, - }}, - } - - gqlResponse := deletePostParams.ExecuteAsPost(t, graphqlURL) - - require.JSONEq(t, deletePostExpected, string(gqlResponse.Data)) - - if diff := cmp.Diff(expectedErrors, gqlResponse.Errors); diff != "" { - t.Errorf("errors mismatch (-want +got):\n%s", diff) - } + filter := map[string]interface{}{"postID": []string{postID}} + deleteGqlType(t, "Post", filter, expectedNumUids, expectedErrors) } func deleteWrongID(t *testing.T) { @@ -1584,7 +1537,7 @@ func deleteWrongID(t *testing.T) { newAuthor := addAuthor(t, newCountry.ID, postExecutor) expectedData := `{ "deleteCountry": { - "msg": "Deleted", + "msg": "No nodes were deleted", "numUids": 0 } }` @@ -1954,16 +1907,16 @@ func manyMutationsWithQueryError(t *testing.T) { func cleanUp(t *testing.T, countries []*country, authors []*author, posts []*post) { t.Run("cleaning up", func(t *testing.T) { for _, post := range posts { - deletePost(t, post.PostID, `{"deletePost" : { "msg": "Deleted" } }`, nil) + deletePost(t, post.PostID, 1, nil) } for _, author := range authors { - deleteAuthor(t, author.ID, `{"deleteAuthor" : { "msg": "Deleted" } }`, nil) + deleteAuthor(t, author.ID, 1, nil) } for _, country := range countries { filter := map[string]interface{}{"id": []string{country.ID}} - deleteCountry(t, filter, `{"deleteCountry" : { "msg": "Deleted" } }`, nil) + deleteCountry(t, filter, 1, nil) } }) } @@ -2295,55 +2248,17 @@ func queryInterfaceAfterAddMutation(t *testing.T) { func cleanupStarwars(t *testing.T, starshipID, humanID, droidID string) { // Delete everything - multiMutationParams := &GraphQLParams{ - Query: `mutation cleanup($starshipFilter: StarshipFilter!, $humanFilter: HumanFilter!, - $droidFilter: DroidFilter!) { - deleteStarship(filter: $starshipFilter) { msg } - - deleteHuman(filter: $humanFilter) { msg } - - deleteDroid(filter: $droidFilter) { msg } - }`, - Variables: map[string]interface{}{ - "starshipFilter": map[string]interface{}{ - "id": []string{starshipID}, - }, - "humanFilter": map[string]interface{}{ - "id": []string{humanID}, - }, - "droidFilter": map[string]interface{}{ - "id": []string{droidID}, - }, - }, + if starshipID != "" { + starshipFilter := map[string]interface{}{"id": []string{starshipID}} + deleteGqlType(t, "Starship", starshipFilter, 1, nil) } - multiMutationExpected := `{ - "deleteStarship": { "msg": "Deleted" }, - "deleteHuman" : { "msg": "Deleted" }, - "deleteDroid": { "msg": "Deleted" } -}` - - gqlResponse := multiMutationParams.ExecuteAsPost(t, graphqlURL) - requireNoGQLErrors(t, gqlResponse) - - var expected, result struct { - DeleteStarhip struct { - Msg string - } - DeleteHuman struct { - Msg string - } - DeleteDroid struct { - Msg string - } + if humanID != "" { + humanFilter := map[string]interface{}{"id": []string{humanID}} + deleteGqlType(t, "Human", humanFilter, 1, nil) } - - err := json.Unmarshal([]byte(multiMutationExpected), &expected) - require.NoError(t, err) - err = json.Unmarshal([]byte(gqlResponse.Data), &result) - require.NoError(t, err) - - if diff := cmp.Diff(expected, result); diff != "" { - t.Errorf("result mismatch (-want +got):\n%s", diff) + if droidID != "" { + droidFilter := map[string]interface{}{"id": []string{droidID}} + deleteGqlType(t, "Droid", droidFilter, 1, nil) } } @@ -2451,8 +2366,17 @@ func deleteGqlType( deleteField := fmt.Sprintf(`delete%s`, typeName) deleteType := result[deleteField].(map[string]interface{}) - require.Equal(t, "Deleted", deleteType["msg"]) - require.Equal(t, expectedNumUids, int(deleteType["numUids"].(float64))) + gotNumUids := int(deleteType["numUids"].(float64)) + require.Equal(t, expectedNumUids, gotNumUids, + "numUids mismatch while deleting %s (filter: %v) want: %d, got: %d", typeName, filter, + expectedNumUids, gotNumUids) + if expectedNumUids == 0 { + require.Equal(t, "No nodes were deleted", deleteType["msg"], + "while deleting %s (filter: %v)", typeName, filter) + } else { + require.Equal(t, "Deleted", deleteType["msg"], "while deleting %s (filter: %v)", + typeName, filter) + } } else { if diff := cmp.Diff(expectedErrors, gqlResponse.Errors); diff != "" { t.Errorf("errors mismatch (-want +got):\n%s", diff) @@ -2846,8 +2770,8 @@ func testNumUids(t *testing.T) { require.Equal(t, deleteResult.DeletePost.Msg, "Deleted") }) - cleanUp(t, []*country{newCountry}, result.AddAuthor.Author, - result.AddAuthor.Author[0].Posts) + // no need to delete author and posts as they would be already deleted by above test + cleanUp(t, []*country{newCountry}, nil, nil) } func checkUser(t *testing.T, userObj, expectedObj *user) { @@ -2878,21 +2802,7 @@ func checkUser(t *testing.T, userObj, expectedObj *user) { } func deleteUser(t *testing.T, userObj user) { - deletePostParams := &GraphQLParams{ - Query: `mutation deleteUser($filter: UserFilter!) { - deleteUser(filter: $filter) { msg } - }`, - Variables: map[string]interface{}{"filter": map[string]interface{}{ - "name": map[string]interface{}{ - "eq": userObj.Name, - }, - }}, - } - - gqlResponse := deletePostParams.ExecuteAsPost(t, graphqlURL) - - requireNoGQLErrors(t, gqlResponse) - require.JSONEq(t, `{"deleteUser": {"msg": "Deleted"}}`, string(gqlResponse.Data)) + deleteGqlType(t, "User", getXidFilter("name", []string{userObj.Name}), 1, nil) } func passwordTest(t *testing.T) { diff --git a/graphql/resolve/mutation.go b/graphql/resolve/mutation.go index 5082fb7094d..15c90586c56 100644 --- a/graphql/resolve/mutation.go +++ b/graphql/resolve/mutation.go @@ -289,6 +289,9 @@ func deleteCompletion() CompletionFunc { if fld, ok := resolved.Data.(map[string]interface{}); ok { if rsp, ok := fld[resolved.Field.Name()].(map[string]interface{}); ok { rsp["msg"] = "Deleted" + if rsp[schema.NumUid] == 0 { + rsp["msg"] = "No nodes were deleted" + } } } })