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 0000000000000000000000000000000000000000..065b805c6d9cedd48f3c2b62ccb4bcd82f9bb1d2 --- /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 98efe8e847b25901cf7010bfda7fd94b49b6e209..77108df82c26e5b3e5f7b2dcd669f31a5abe925e 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,44 @@ 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.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)) + } + return nil, structerr.NewFailedPrecondition("revert: there are conflicting files").WithDetail( + &gitalypb.UserRevertError{ + Error: &gitalypb.UserRevertError_MergeConflict{ + MergeConflict: &gitalypb.MergeConflictError{ + ConflictingFiles: conflictingFiles, + }, + }, + }) + } } 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.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{}, + }, + }) + } } newrev, err = quarantineRepo.WriteCommit( @@ -166,18 +189,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.IsDisabled(ctx) { + return &gitalypb.UserRevertResponse{ + CommitError: "Branch diverged", + }, nil + } else { + 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()), + }, + }, + }) + } } } 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.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(), + }, + }) + } } 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 225669662a9b75ba631c39fee3a34539f1d7caa9..6924f9caa2e16d8b28d2801f9bcbe8d68c22aaf4 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.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{ + CustomHook: &gitalypb.CustomHookError{ + HookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE, + }, + }, + }) + testhelper.RequireGrpcError(t, expectedError, err) + } 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) } @@ -695,6 +711,7 @@ func TestServer_UserRevert_successfulIntoEmptyRepo(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertSuccessfulIntoEmptyRepo) } @@ -754,6 +771,7 @@ func testServerUserRevertSuccessfulIntoEmptyRepo(t *testing.T, ctx context.Conte require.Equal(t, expectedBranchUpdate, response.BranchUpdate) 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 +785,7 @@ func TestServer_UserRevert_successfulGitHooks(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertSuccessfulGitHooks) } @@ -822,6 +841,7 @@ func TestServer_UserRevert_failedDueToPreReceiveError(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertFailedDueToPreReceiveError) } @@ -859,9 +879,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.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) + 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) + } }) } } @@ -871,6 +901,7 @@ func TestServer_UserRevert_failedDueToCreateTreeErrorConflict(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertFailedDueToCreateTreeErrorConflict) } @@ -917,10 +948,22 @@ 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.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) + testhelper.ProtoEqual(t, &gitalypb.MergeConflictError{ + ConflictingFiles: [][]byte{[]byte("blob")}, + }, revertError.GetMergeConflict()) + } } func TestServer_UserRevert_failedDueToCreateTreeErrorEmpty(t *testing.T) { @@ -928,6 +971,7 @@ func TestServer_UserRevert_failedDueToCreateTreeErrorEmpty(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertFailedDueToCreateTreeErrorEmpty) } @@ -992,10 +1036,19 @@ func testServerUserRevertFailedDueToCreateTreeErrorEmpty(t *testing.T, ctx conte 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) + 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_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) + } } func TestServer_UserRevert_failedDueToCommitError(t *testing.T) { @@ -1003,6 +1056,7 @@ func TestServer_UserRevert_failedDueToCommitError(t *testing.T) { testhelper.NewFeatureSets( featureflag.GPGSigning, + featureflag.ReturnStructuredErrorsInUserRevert, ).Run(t, testServerUserRevertFailedDueToCommitError) } @@ -1047,6 +1101,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.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()) + } }