From e300cb81042764dd82c03d55bfec7a3d896585aa Mon Sep 17 00:00:00 2001 From: wang yadong Date: Thu, 7 Mar 2024 01:52:16 +0000 Subject: [PATCH 1/4] Operations: Return structured errors in UserRevert Modify the RPC UserRevert to return structured errors instead of inline errors. Modify the handling of the following four errors: 'Conflict', 'Changes Already Applied', 'Branch diverged', and 'CustomHookError'. Returns the corresponding structured error. For: https://gitlab.com/gitlab-org/gitaly/-/issues/5630 --- .../ff_return_structed_errors_in_revert.go | 13 ++ internal/gitaly/service/operations/revert.go | 78 +++++++++--- .../gitaly/service/operations/revert_test.go | 116 ++++++++++++++---- 3 files changed, 167 insertions(+), 40 deletions(-) create mode 100644 internal/featureflag/ff_return_structed_errors_in_revert.go diff --git a/internal/featureflag/ff_return_structed_errors_in_revert.go b/internal/featureflag/ff_return_structed_errors_in_revert.go new file mode 100644 index 0000000000..065b805c6d --- /dev/null +++ b/internal/featureflag/ff_return_structed_errors_in_revert.go @@ -0,0 +1,13 @@ +package featureflag + +// ReturnStructuredErrorsInUserRevert enables return structured errors in UserRevert. +// Modify the RPC UserRevert to return structured errors instead of +// inline errors. Modify the handling of the following four +// errors: 'Conflict', 'Changes Already Applied', 'Branch diverged', +// and 'CustomHookError'. Returns the corresponding structured error. +var ReturnStructuredErrorsInUserRevert = NewFeatureFlag( + "return_structured_errors_in_revert", + "v16.10.0", + "https://gitlab.com/gitlab-org/gitaly/-/issues/5752", + false, +) diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go index 98efe8e847..89567629ca 100644 --- a/internal/gitaly/service/operations/revert.go +++ b/internal/gitaly/service/operations/revert.go @@ -6,6 +6,7 @@ import ( "fmt" "time" + "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/git/remoterepo" @@ -87,22 +88,46 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest if err != nil { var conflictErr *localrepo.MergeTreeConflictError if errors.As(err, &conflictErr) { - return &gitalypb.UserRevertResponse{ - // it's better that this error matches the git2go for now - CreateTreeError: "revert: could not apply due to conflicts", - CreateTreeErrorCode: gitalypb.UserRevertResponse_CONFLICT, - }, nil + if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFileInfo)) + for _, conflictingFileInfo := range conflictErr.ConflictingFileInfo { + conflictingFiles = append(conflictingFiles, []byte(conflictingFileInfo.FileName)) + } + return nil, structerr.NewFailedPrecondition("revert: there are conflicting files").WithDetail( + &gitalypb.UserRevertError{ + Error: &gitalypb.UserRevertError_MergeConflict{ + MergeConflict: &gitalypb.MergeConflictError{ + ConflictingFiles: conflictingFiles, + }, + }, + }) + } else { + return &gitalypb.UserRevertResponse{ + // it's better that this error matches the git2go for now + CreateTreeError: "revert: could not apply due to conflicts", + CreateTreeErrorCode: gitalypb.UserRevertResponse_CONFLICT, + }, nil + } } return nil, structerr.NewInternal("merge-tree: %w", err) } if oursCommit.TreeId == treeOID.String() { - return &gitalypb.UserRevertResponse{ - // it's better that this error matches the git2go for now - CreateTreeError: "revert: could not apply because the result was empty", - CreateTreeErrorCode: gitalypb.UserRevertResponse_EMPTY, - }, nil + if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + return nil, structerr.NewFailedPrecondition("revert: could not apply because the result was empty").WithDetail( + &gitalypb.UserRevertError{ + Error: &gitalypb.UserRevertError_ChangesAlreadyApplied{ + ChangesAlreadyApplied: &gitalypb.ChangesAlreadyAppliedError{}, + }, + }) + } else { + return &gitalypb.UserRevertResponse{ + // it's better that this error matches the git2go for now + CreateTreeError: "revert: could not apply because the result was empty", + CreateTreeErrorCode: gitalypb.UserRevertResponse_EMPTY, + }, nil + } } newrev, err = quarantineRepo.WriteCommit( @@ -166,18 +191,39 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest return nil, structerr.NewInternal("checking for ancestry: %w", err) } if !ancestor { - return &gitalypb.UserRevertResponse{ - CommitError: "Branch diverged", - }, nil + if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + return nil, structerr.NewFailedPrecondition("revert: branch diverged").WithDetail( + &gitalypb.UserRevertError{ + Error: &gitalypb.UserRevertError_NotAncestor{ + NotAncestor: &gitalypb.NotAncestorError{ + ParentRevision: []byte(oldrev.Revision()), + ChildRevision: []byte(newrev.Revision()), + }, + }, + }) + } else { + return &gitalypb.UserRevertResponse{ + CommitError: "Branch diverged", + }, nil + } } } if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.User, quarantineDir, referenceName, newrev, oldrev); err != nil { var customHookErr updateref.CustomHookError if errors.As(err, &customHookErr) { - return &gitalypb.UserRevertResponse{ - PreReceiveError: customHookErr.Error(), - }, nil + if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + return nil, structerr.NewPermissionDenied("revert: custom hook error").WithDetail( + &gitalypb.UserRevertError{ + Error: &gitalypb.UserRevertError_CustomHook{ + CustomHook: customHookErr.Proto(), + }, + }) + } else { + return &gitalypb.UserRevertResponse{ + PreReceiveError: customHookErr.Error(), + }, nil + } } return nil, fmt.Errorf("update reference with hooks: %w", err) diff --git a/internal/gitaly/service/operations/revert_test.go b/internal/gitaly/service/operations/revert_test.go index 225669662a..8cd8460959 100644 --- a/internal/gitaly/service/operations/revert_test.go +++ b/internal/gitaly/service/operations/revert_test.go @@ -19,6 +19,8 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testserver" "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/timestamppb" ) @@ -427,6 +429,7 @@ func TestServer_UserRevert_quarantine(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertQuarantine) } @@ -468,9 +471,21 @@ func testServerUserRevertQuarantine(t *testing.T, ctx context.Context) { Message: []byte("Reverting " + revertedCommit.Id), Timestamp: ×tamppb.Timestamp{Seconds: 12345}, }) - require.NoError(t, err) - require.NotNil(t, response) - require.NotEmpty(t, response.PreReceiveError) + if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + expectedError := structerr.NewPermissionDenied("revert: custom hook error").WithDetail( + &gitalypb.UserRevertError{ + Error: &gitalypb.UserRevertError_CustomHook{ + CustomHook: &gitalypb.CustomHookError{ + HookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE, + }, + }, + }) + testhelper.RequireGrpcError(t, expectedError, err) + } else { + require.NoError(t, err) + require.NotNil(t, response) + require.NotEmpty(t, response.PreReceiveError) + } objectHash, err := repo.ObjectHash(ctx) require.NoError(t, err) @@ -599,6 +614,7 @@ func TestServer_UserRevert_stableID(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertStableID) } @@ -652,8 +668,10 @@ func testServerUserRevertStableID(t *testing.T, ctx context.Context) { "sha256": "28b57208e72bc2317143571997b9cfc444a51b52a43dde1c0282633a2b60de71", }), }, response.BranchUpdate) - require.Empty(t, response.CreateTreeError) - require.Empty(t, response.CreateTreeErrorCode) + if !featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + require.Empty(t, response.CreateTreeError) + require.Empty(t, response.CreateTreeErrorCode) + } // headCommit is pointed commit after revert headCommit, err := repo.ReadCommit(ctx, git.Revision(git.DefaultBranch)) @@ -695,6 +713,7 @@ func TestServer_UserRevert_successfulIntoEmptyRepo(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertSuccessfulIntoEmptyRepo) } @@ -752,8 +771,10 @@ func testServerUserRevertSuccessfulIntoEmptyRepo(t *testing.T, ctx context.Conte } require.Equal(t, expectedBranchUpdate, response.BranchUpdate) - require.Empty(t, response.CreateTreeError) - require.Empty(t, response.CreateTreeErrorCode) + if !featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + require.Empty(t, response.CreateTreeError) + require.Empty(t, response.CreateTreeErrorCode) + } require.Equal(t, request.Message, headCommit.Subject) require.Equal(t, revertedCommit.Id, headCommit.ParentIds[0]) gittest.RequireTree(t, cfg, repoPath, response.BranchUpdate.CommitId, @@ -767,6 +788,7 @@ func TestServer_UserRevert_successfulGitHooks(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertSuccessfulGitHooks) } @@ -806,7 +828,9 @@ func testServerUserRevertSuccessfulGitHooks(t *testing.T, ctx context.Context) { response, err := client.UserRevert(ctx, request) require.NoError(t, err) - require.Empty(t, response.PreReceiveError) + if !featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + require.Empty(t, response.PreReceiveError) + } headCommit, err := repo.ReadCommit(ctx, git.Revision(destinationBranch)) require.NoError(t, err) gittest.RequireTree(t, cfg, repoPath, headCommit.Id, nil) @@ -822,6 +846,7 @@ func TestServer_UserRevert_failedDueToPreReceiveError(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertFailedDueToPreReceiveError) } @@ -859,9 +884,19 @@ func testServerUserRevertFailedDueToPreReceiveError(t *testing.T, ctx context.Co t.Run(hookName, func(t *testing.T) { gittest.WriteCustomHook(t, repoPath, hookName, hookContent) - response, err := client.UserRevert(ctx, request) - require.NoError(t, err) - require.Contains(t, response.PreReceiveError, "GL_ID="+gittest.TestUser.GlId) + if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + _, err := client.UserRevert(ctx, request) + actualStatus, _ := status.FromError(err) + require.Equal(t, actualStatus.Code(), codes.PermissionDenied) + require.Equal(t, actualStatus.Message(), "revert: custom hook error") + revertError, ok := actualStatus.Details()[0].(*gitalypb.UserRevertError) + require.True(t, ok) + require.Contains(t, revertError.GetCustomHook().String(), "GL_ID="+gittest.TestUser.GlId) + } else { + response, err := client.UserRevert(ctx, request) + require.NoError(t, err) + require.Contains(t, response.PreReceiveError, "GL_ID="+gittest.TestUser.GlId) + } }) } } @@ -871,6 +906,7 @@ func TestServer_UserRevert_failedDueToCreateTreeErrorConflict(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertFailedDueToCreateTreeErrorConflict) } @@ -917,10 +953,20 @@ func testServerUserRevertFailedDueToCreateTreeErrorConflict(t *testing.T, ctx co Message: []byte("Reverting " + revertedCommit.Id), } - response, err := client.UserRevert(ctx, request) - require.NoError(t, err) - require.NotEmpty(t, response.CreateTreeError) - require.Equal(t, gitalypb.UserRevertResponse_CONFLICT, response.CreateTreeErrorCode) + if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + _, err = client.UserRevert(ctx, request) + actualStatus, _ := status.FromError(err) + require.Equal(t, actualStatus.Code(), codes.FailedPrecondition) + require.Equal(t, actualStatus.Message(), "revert: there are conflicting files") + revertError, ok := actualStatus.Details()[0].(*gitalypb.UserRevertError) + require.True(t, ok) + require.NotNil(t, revertError.GetMergeConflict()) + } else { + response, err := client.UserRevert(ctx, request) + require.NoError(t, err) + require.NotEmpty(t, response.CreateTreeError) + require.Equal(t, gitalypb.UserRevertResponse_CONFLICT, response.CreateTreeErrorCode) + } } func TestServer_UserRevert_failedDueToCreateTreeErrorEmpty(t *testing.T) { @@ -928,6 +974,7 @@ func TestServer_UserRevert_failedDueToCreateTreeErrorEmpty(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertFailedDueToCreateTreeErrorEmpty) } @@ -988,14 +1035,24 @@ func testServerUserRevertFailedDueToCreateTreeErrorEmpty(t *testing.T, ctx conte } response, err := client.UserRevert(ctx, request) - require.NoError(t, err) - require.Empty(t, response.CreateTreeError) - require.Equal(t, gitalypb.UserRevertResponse_NONE, response.CreateTreeErrorCode) + if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + _, err = client.UserRevert(ctx, request) + + expectedError := structerr.NewFailedPrecondition("revert: could not apply because the result was empty").WithDetail( + &gitalypb.UserRevertError{ + Error: &gitalypb.UserRevertError_ChangesAlreadyApplied{}, + }) + testhelper.RequireGrpcError(t, expectedError, err) + } else { + require.NoError(t, err) + require.Empty(t, response.CreateTreeError) + require.Equal(t, gitalypb.UserRevertResponse_NONE, response.CreateTreeErrorCode) - response, err = client.UserRevert(ctx, request) - require.NoError(t, err) - require.NotEmpty(t, response.CreateTreeError) - require.Equal(t, gitalypb.UserRevertResponse_EMPTY, response.CreateTreeErrorCode) + response, err = client.UserRevert(ctx, request) + require.NoError(t, err) + require.NotEmpty(t, response.CreateTreeError) + require.Equal(t, gitalypb.UserRevertResponse_EMPTY, response.CreateTreeErrorCode) + } } func TestServer_UserRevert_failedDueToCommitError(t *testing.T) { @@ -1003,6 +1060,7 @@ func TestServer_UserRevert_failedDueToCommitError(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertFailedDueToCommitError) } @@ -1047,6 +1105,16 @@ func testServerUserRevertFailedDueToCommitError(t *testing.T, ctx context.Contex } response, err := client.UserRevert(ctx, request) - require.NoError(t, err) - require.Equal(t, "Branch diverged", response.CommitError) + + if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + actualStatus, _ := status.FromError(err) + require.Equal(t, actualStatus.Code(), codes.FailedPrecondition) + require.Equal(t, actualStatus.Message(), "revert: branch diverged") + revertError, ok := actualStatus.Details()[0].(*gitalypb.UserRevertError) + require.True(t, ok) + require.NotNil(t, revertError.GetNotAncestor()) + } else { + require.NoError(t, err) + require.Equal(t, "Branch diverged", response.CommitError) + } } -- GitLab From 6c145d2a8677405c524851d27ad8d490c86fcc6b Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Fri, 8 Mar 2024 00:42:16 +0000 Subject: [PATCH 2/4] Apply 1 suggestion(s) to 1 file(s) --- internal/gitaly/service/operations/revert_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/gitaly/service/operations/revert_test.go b/internal/gitaly/service/operations/revert_test.go index 8cd8460959..e67bb10eb4 100644 --- a/internal/gitaly/service/operations/revert_test.go +++ b/internal/gitaly/service/operations/revert_test.go @@ -1036,8 +1036,6 @@ func testServerUserRevertFailedDueToCreateTreeErrorEmpty(t *testing.T, ctx conte response, err := client.UserRevert(ctx, request) if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { - _, err = client.UserRevert(ctx, request) - expectedError := structerr.NewFailedPrecondition("revert: could not apply because the result was empty").WithDetail( &gitalypb.UserRevertError{ Error: &gitalypb.UserRevertError_ChangesAlreadyApplied{}, -- GitLab From 0fcdab54ee7857df0eec9ca973236ce5e2b08f4b Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Fri, 8 Mar 2024 00:44:13 +0000 Subject: [PATCH 3/4] Apply 2 suggestion(s) to 1 file(s) --- internal/gitaly/service/operations/revert.go | 46 ++++++----- .../gitaly/service/operations/revert_test.go | 79 +++++++++---------- 2 files changed, 61 insertions(+), 64 deletions(-) diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go index 89567629ca..77108df82c 100644 --- a/internal/gitaly/service/operations/revert.go +++ b/internal/gitaly/service/operations/revert.go @@ -88,7 +88,12 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest if err != nil { var conflictErr *localrepo.MergeTreeConflictError if errors.As(err, &conflictErr) { - if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + if featureflag.ReturnStructuredErrorsInUserRevert.IsDisabled(ctx) { + return &gitalypb.UserRevertResponse{ + CreateTreeError: "revert: could not apply due to conflicts", + CreateTreeErrorCode: gitalypb.UserRevertResponse_CONFLICT, + }, nil + } else { conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFileInfo)) for _, conflictingFileInfo := range conflictErr.ConflictingFileInfo { conflictingFiles = append(conflictingFiles, []byte(conflictingFileInfo.FileName)) @@ -101,12 +106,6 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest }, }, }) - } else { - return &gitalypb.UserRevertResponse{ - // it's better that this error matches the git2go for now - CreateTreeError: "revert: could not apply due to conflicts", - CreateTreeErrorCode: gitalypb.UserRevertResponse_CONFLICT, - }, nil } } @@ -114,19 +113,18 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest } if oursCommit.TreeId == treeOID.String() { - if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + if featureflag.ReturnStructuredErrorsInUserRevert.IsDisabled(ctx) { + return &gitalypb.UserRevertResponse{ + CreateTreeError: "revert: could not apply because the result was empty", + CreateTreeErrorCode: gitalypb.UserRevertResponse_EMPTY, + }, nil + } else { return nil, structerr.NewFailedPrecondition("revert: could not apply because the result was empty").WithDetail( &gitalypb.UserRevertError{ Error: &gitalypb.UserRevertError_ChangesAlreadyApplied{ ChangesAlreadyApplied: &gitalypb.ChangesAlreadyAppliedError{}, }, }) - } else { - return &gitalypb.UserRevertResponse{ - // it's better that this error matches the git2go for now - CreateTreeError: "revert: could not apply because the result was empty", - CreateTreeErrorCode: gitalypb.UserRevertResponse_EMPTY, - }, nil } } @@ -191,7 +189,11 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest return nil, structerr.NewInternal("checking for ancestry: %w", err) } if !ancestor { - if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + if featureflag.ReturnStructuredErrorsInUserRevert.IsDisabled(ctx) { + return &gitalypb.UserRevertResponse{ + CommitError: "Branch diverged", + }, nil + } else { return nil, structerr.NewFailedPrecondition("revert: branch diverged").WithDetail( &gitalypb.UserRevertError{ Error: &gitalypb.UserRevertError_NotAncestor{ @@ -201,10 +203,6 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest }, }, }) - } else { - return &gitalypb.UserRevertResponse{ - CommitError: "Branch diverged", - }, nil } } } @@ -212,17 +210,17 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.User, quarantineDir, referenceName, newrev, oldrev); err != nil { var customHookErr updateref.CustomHookError if errors.As(err, &customHookErr) { - if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + if featureflag.ReturnStructuredErrorsInUserRevert.IsDisabled(ctx) { + return &gitalypb.UserRevertResponse{ + PreReceiveError: customHookErr.Error(), + }, nil + } else { return nil, structerr.NewPermissionDenied("revert: custom hook error").WithDetail( &gitalypb.UserRevertError{ Error: &gitalypb.UserRevertError_CustomHook{ CustomHook: customHookErr.Proto(), }, }) - } else { - return &gitalypb.UserRevertResponse{ - PreReceiveError: customHookErr.Error(), - }, nil } } diff --git a/internal/gitaly/service/operations/revert_test.go b/internal/gitaly/service/operations/revert_test.go index e67bb10eb4..b16ba309fa 100644 --- a/internal/gitaly/service/operations/revert_test.go +++ b/internal/gitaly/service/operations/revert_test.go @@ -471,7 +471,11 @@ func testServerUserRevertQuarantine(t *testing.T, ctx context.Context) { Message: []byte("Reverting " + revertedCommit.Id), Timestamp: ×tamppb.Timestamp{Seconds: 12345}, }) - if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + if featureflag.ReturnStructuredErrorsInUserRevert.IsDisabled(ctx) { + require.NoError(t, err) + require.NotNil(t, response) + require.NotEmpty(t, response.PreReceiveError) + } else { expectedError := structerr.NewPermissionDenied("revert: custom hook error").WithDetail( &gitalypb.UserRevertError{ Error: &gitalypb.UserRevertError_CustomHook{ @@ -481,10 +485,6 @@ func testServerUserRevertQuarantine(t *testing.T, ctx context.Context) { }, }) testhelper.RequireGrpcError(t, expectedError, err) - } else { - require.NoError(t, err) - require.NotNil(t, response) - require.NotEmpty(t, response.PreReceiveError) } objectHash, err := repo.ObjectHash(ctx) @@ -668,10 +668,8 @@ func testServerUserRevertStableID(t *testing.T, ctx context.Context) { "sha256": "28b57208e72bc2317143571997b9cfc444a51b52a43dde1c0282633a2b60de71", }), }, response.BranchUpdate) - if !featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { - require.Empty(t, response.CreateTreeError) - require.Empty(t, response.CreateTreeErrorCode) - } + require.Empty(t, response.CreateTreeError) + require.Empty(t, response.CreateTreeErrorCode) // headCommit is pointed commit after revert headCommit, err := repo.ReadCommit(ctx, git.Revision(git.DefaultBranch)) @@ -771,10 +769,9 @@ func testServerUserRevertSuccessfulIntoEmptyRepo(t *testing.T, ctx context.Conte } require.Equal(t, expectedBranchUpdate, response.BranchUpdate) - if !featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { - require.Empty(t, response.CreateTreeError) - require.Empty(t, response.CreateTreeErrorCode) - } + require.Empty(t, response.CreateTreeError) + require.Empty(t, response.CreateTreeErrorCode) + require.Equal(t, request.Message, headCommit.Subject) require.Equal(t, revertedCommit.Id, headCommit.ParentIds[0]) gittest.RequireTree(t, cfg, repoPath, response.BranchUpdate.CommitId, @@ -828,9 +825,7 @@ func testServerUserRevertSuccessfulGitHooks(t *testing.T, ctx context.Context) { response, err := client.UserRevert(ctx, request) require.NoError(t, err) - if !featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { - require.Empty(t, response.PreReceiveError) - } + require.Empty(t, response.PreReceiveError) headCommit, err := repo.ReadCommit(ctx, git.Revision(destinationBranch)) require.NoError(t, err) gittest.RequireTree(t, cfg, repoPath, headCommit.Id, nil) @@ -884,7 +879,11 @@ func testServerUserRevertFailedDueToPreReceiveError(t *testing.T, ctx context.Co t.Run(hookName, func(t *testing.T) { gittest.WriteCustomHook(t, repoPath, hookName, hookContent) - if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + if featureflag.ReturnStructuredErrorsInUserRevert.IsDisabled(ctx) { + response, err := client.UserRevert(ctx, request) + require.NoError(t, err) + require.Contains(t, response.PreReceiveError, "GL_ID="+gittest.TestUser.GlId) + } else { _, err := client.UserRevert(ctx, request) actualStatus, _ := status.FromError(err) require.Equal(t, actualStatus.Code(), codes.PermissionDenied) @@ -892,10 +891,6 @@ func testServerUserRevertFailedDueToPreReceiveError(t *testing.T, ctx context.Co revertError, ok := actualStatus.Details()[0].(*gitalypb.UserRevertError) require.True(t, ok) require.Contains(t, revertError.GetCustomHook().String(), "GL_ID="+gittest.TestUser.GlId) - } else { - response, err := client.UserRevert(ctx, request) - require.NoError(t, err) - require.Contains(t, response.PreReceiveError, "GL_ID="+gittest.TestUser.GlId) } }) } @@ -953,19 +948,21 @@ func testServerUserRevertFailedDueToCreateTreeErrorConflict(t *testing.T, ctx co Message: []byte("Reverting " + revertedCommit.Id), } - if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + if featureflag.ReturnStructuredErrorsInUserRevert.IsDisabled(ctx) { + response, err := client.UserRevert(ctx, request) + require.NoError(t, err) + require.NotEmpty(t, response.CreateTreeError) + require.Equal(t, gitalypb.UserRevertResponse_CONFLICT, response.CreateTreeErrorCode) + } else { _, err = client.UserRevert(ctx, request) actualStatus, _ := status.FromError(err) require.Equal(t, actualStatus.Code(), codes.FailedPrecondition) require.Equal(t, actualStatus.Message(), "revert: there are conflicting files") revertError, ok := actualStatus.Details()[0].(*gitalypb.UserRevertError) require.True(t, ok) - require.NotNil(t, revertError.GetMergeConflict()) - } else { - response, err := client.UserRevert(ctx, request) - require.NoError(t, err) - require.NotEmpty(t, response.CreateTreeError) - require.Equal(t, gitalypb.UserRevertResponse_CONFLICT, response.CreateTreeErrorCode) + testhelper.ProtoEqual(t, &gitalypb.MergeConflictError{ + ConflictingFiles: [][]byte{[]byte("blob")}, + }, revertError.GetMergeConflict()) } } @@ -1035,14 +1032,9 @@ func testServerUserRevertFailedDueToCreateTreeErrorEmpty(t *testing.T, ctx conte } response, err := client.UserRevert(ctx, request) - if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { - expectedError := structerr.NewFailedPrecondition("revert: could not apply because the result was empty").WithDetail( - &gitalypb.UserRevertError{ - Error: &gitalypb.UserRevertError_ChangesAlreadyApplied{}, - }) - testhelper.RequireGrpcError(t, expectedError, err) - } else { - require.NoError(t, err) + require.NoError(t, err) + + if featureflag.ReturnStructuredErrorsInUserRevert.IsDisabled(ctx) { require.Empty(t, response.CreateTreeError) require.Equal(t, gitalypb.UserRevertResponse_NONE, response.CreateTreeErrorCode) @@ -1050,6 +1042,13 @@ func testServerUserRevertFailedDueToCreateTreeErrorEmpty(t *testing.T, ctx conte require.NoError(t, err) require.NotEmpty(t, response.CreateTreeError) require.Equal(t, gitalypb.UserRevertResponse_EMPTY, response.CreateTreeErrorCode) + } else { + _, err = client.UserRevert(ctx, request) + expectedError := structerr.NewFailedPrecondition("revert: could not apply because the result was empty").WithDetail( + &gitalypb.UserRevertError{ + Error: &gitalypb.UserRevertError_ChangesAlreadyApplied{}, + }) + testhelper.RequireGrpcError(t, expectedError, err) } } @@ -1104,15 +1103,15 @@ func testServerUserRevertFailedDueToCommitError(t *testing.T, ctx context.Contex response, err := client.UserRevert(ctx, request) - if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) { + if featureflag.ReturnStructuredErrorsInUserRevert.IsDisabled(ctx) { + require.NoError(t, err) + require.Equal(t, "Branch diverged", response.CommitError) + } else { actualStatus, _ := status.FromError(err) require.Equal(t, actualStatus.Code(), codes.FailedPrecondition) require.Equal(t, actualStatus.Message(), "revert: branch diverged") revertError, ok := actualStatus.Details()[0].(*gitalypb.UserRevertError) require.True(t, ok) require.NotNil(t, revertError.GetNotAncestor()) - } else { - require.NoError(t, err) - require.Equal(t, "Branch diverged", response.CommitError) } } -- GitLab From ba186bef2284845622f418d43d9217d0e3ff41cb Mon Sep 17 00:00:00 2001 From: wang yadong Date: Fri, 8 Mar 2024 02:54:19 +0000 Subject: [PATCH 4/4] Apply suggestions --- internal/gitaly/service/operations/revert_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/gitaly/service/operations/revert_test.go b/internal/gitaly/service/operations/revert_test.go index b16ba309fa..6924f9caa2 100644 --- a/internal/gitaly/service/operations/revert_test.go +++ b/internal/gitaly/service/operations/revert_test.go @@ -1033,11 +1033,10 @@ func testServerUserRevertFailedDueToCreateTreeErrorEmpty(t *testing.T, ctx conte response, err := client.UserRevert(ctx, request) require.NoError(t, err) + require.Empty(t, response.CreateTreeError) + require.Equal(t, gitalypb.UserRevertResponse_NONE, response.CreateTreeErrorCode) if featureflag.ReturnStructuredErrorsInUserRevert.IsDisabled(ctx) { - require.Empty(t, response.CreateTreeError) - require.Equal(t, gitalypb.UserRevertResponse_NONE, response.CreateTreeErrorCode) - response, err = client.UserRevert(ctx, request) require.NoError(t, err) require.NotEmpty(t, response.CreateTreeError) -- GitLab