refs: Return structured errors for DeleteRefsjc-delete-ref-return-errors

In the cases where we get an error trying to update the ref, return a
structured error with the git error embedded in it so the rails side can
interpret the error and raise a GitError.

Changelog: changed

3 files changed, 166 insertions, 10 deletions

diff --git a/internal/gitaly/service/ref/delete_refs.go b/internal/gitaly/service/ref/delete_refs.go
index 54e168f3e..fd9ba90f4 100644
--- a/internal/gitaly/service/ref/delete_refs.go
+++ b/internal/gitaly/service/ref/delete_refs.go
@@ -11,6 +11,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -38,8 +39,41 @@ func (s *server) DeleteRefs(ctx context.Context, in *gitalypb.DeleteRefsRequest)
 	}
 
 	voteHash := voting.NewVoteHash()
+
+	if featureflag.DeleteRefsStructuredErrors.IsEnabled(ctx) {
+		var invalidRefnames [][]byte
+
+		for _, ref := range refnames {
+			if err := git.ValidateRevision([]byte(ref)); err != nil {
+				invalidRefnames = append(invalidRefnames, []byte(ref))
+			}
+		}
+
+		if len(invalidRefnames) > 0 {
+			detailedErr, err := helper.ErrWithDetails(
+				helper.ErrInvalidArgumentf("invalid references"),
+				&gitalypb.DeleteRefsError{
+					Error: &gitalypb.DeleteRefsError_InvalidFormat{
+						InvalidFormat: &gitalypb.InvalidRefFormatError{
+							Refs: invalidRefnames,
+						},
+					},
+				},
+			)
+			if err != nil {
+				return nil, helper.ErrInternalf("error details: %w", err)
+			}
+
+			return nil, detailedErr
+		}
+	}
+
 	for _, ref := range refnames {
 		if err := updater.Delete(ref); err != nil {
+			if featureflag.DeleteRefsStructuredErrors.IsEnabled(ctx) {
+				return nil, helper.ErrInternalf("unable to delete refs: %w", err)
+			}
+
 			return &gitalypb.DeleteRefsResponse{GitError: err.Error()}, nil
 		}
 
@@ -49,6 +83,26 @@ func (s *server) DeleteRefs(ctx context.Context, in *gitalypb.DeleteRefsRequest)
 	}
 
 	if err := updater.Prepare(); err != nil {
+		var errAlreadyLocked *updateref.ErrAlreadyLocked
+		if featureflag.DeleteRefsStructuredErrors.IsEnabled(ctx) &&
+			errors.As(err, &errAlreadyLocked) {
+			detailedErr, err := helper.ErrWithDetails(
+				helper.ErrFailedPreconditionf("cannot lock references"),
+				&gitalypb.DeleteRefsError{
+					Error: &gitalypb.DeleteRefsError_ReferencesLocked{
+						ReferencesLocked: &gitalypb.ReferencesLockedError{
+							Refs: [][]byte{errAlreadyLocked.Ref},
+						},
+					},
+				},
+			)
+			if err != nil {
+				return nil, helper.ErrInternalf("error details: %w", err)
+			}
+
+			return nil, detailedErr
+		}
+
 		return &gitalypb.DeleteRefsResponse{
 			GitError: fmt.Sprintf("unable to delete refs: %s", err.Error()),
 		}, nil
@@ -68,11 +122,11 @@ func (s *server) DeleteRefs(ctx context.Context, in *gitalypb.DeleteRefsRequest)
 	}
 
 	if err := updater.Commit(); err != nil {
-		// TODO: We should be able to easily drop this error here and return a real error as
-		// soon as we always use proper locking semantics in git-update-ref(1). All locking
-		// issues would be catched when `Prepare()`ing the changes already, so a fail
-		// afterwards would hint at a real unexpected error.
-		return &gitalypb.DeleteRefsResponse{GitError: fmt.Sprintf("unable to delete refs: %s", err.Error())}, nil
+		if featureflag.DeleteRefsStructuredErrors.IsEnabled(ctx) {
+			return nil, helper.ErrInternalf("unable to commit: %w", err)
+		}
+
+		return &gitalypb.DeleteRefsResponse{GitError: err.Error()}, nil
 	}
 
 	if err := transaction.VoteOnContext(ctx, s.txManager, vote, voting.Committed); err != nil {
diff --git a/internal/gitaly/service/ref/delete_refs_test.go b/internal/gitaly/service/ref/delete_refs_test.go
index d756ec2ff..4210bae9d 100644
--- a/internal/gitaly/service/ref/delete_refs_test.go
+++ b/internal/gitaly/service/ref/delete_refs_test.go
@@ -3,17 +3,22 @@
 package ref
 
 import (
+	"context"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service"
 	hookservice "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service/hook"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service/repository"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
@@ -25,8 +30,14 @@ import (
 
 func TestDeleteRefs_successful(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
+	testhelper.NewFeatureSets(featureflag.DeleteRefsStructuredErrors).Run(
+		t,
+		testDeleteRefSuccessful,
+	)
+}
+
+func testDeleteRefSuccessful(t *testing.T, ctx context.Context) {
 	cfg, client := setupRefServiceWithoutRepo(t)
 
 	testCases := []struct {
@@ -82,8 +93,14 @@ func TestDeleteRefs_successful(t *testing.T) {
 
 func TestDeleteRefs_transaction(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
+	testhelper.NewFeatureSets(featureflag.DeleteRefsStructuredErrors).Run(
+		t,
+		testDeleteRefsTransaction,
+	)
+}
+
+func testDeleteRefsTransaction(t *testing.T, ctx context.Context) {
 	cfg := testcfg.Build(t)
 
 	testcfg.BuildGitalyHooks(t, cfg)
@@ -158,19 +175,95 @@ func TestDeleteRefs_transaction(t *testing.T) {
 
 func TestDeleteRefs_invalidRefFormat(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
+	testhelper.NewFeatureSets(featureflag.DeleteRefsStructuredErrors).Run(
+		t,
+		testDeleteRefsInvalidRefFormat,
+	)
+}
+
+func testDeleteRefsInvalidRefFormat(t *testing.T, ctx context.Context) {
 	_, repo, _, client := setupRefService(ctx, t)
 
 	request := &gitalypb.DeleteRefsRequest{
 		Repository: repo,
-		Refs:       [][]byte{[]byte(`refs\tails\invalid-ref-format`)},
+		Refs:       [][]byte{[]byte(`refs invalid-ref-format`)},
 	}
 
 	response, err := client.DeleteRefs(ctx, request)
+
+	if featureflag.DeleteRefsStructuredErrors.IsEnabled(ctx) {
+		require.Nil(t, response)
+		detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
+			helper.ErrInvalidArgumentf("invalid references"),
+			&gitalypb.DeleteRefsError{
+				Error: &gitalypb.DeleteRefsError_InvalidFormat{
+					InvalidFormat: &gitalypb.InvalidRefFormatError{
+						Refs: request.Refs,
+					},
+				},
+			})
+		require.NoError(t, errGeneratingDetailedErr)
+		testhelper.RequireGrpcError(t, detailedErr, err)
+	} else {
+		require.NoError(t, err)
+		assert.Contains(t, response.GitError, "invalid ref format")
+	}
+}
+
+func TestDeleteRefs_refLocked(t *testing.T) {
+	t.Parallel()
+
+	testhelper.NewFeatureSets(featureflag.DeleteRefsStructuredErrors).Run(
+		t,
+		testDeleteRefsRefLocked,
+	)
+}
+
+func testDeleteRefsRefLocked(t *testing.T, ctx context.Context) {
+	cfg, repoProto, _, client := setupRefService(ctx, t)
+
+	if !gittest.GitSupportsStatusFlushing(t, ctx, cfg) {
+		t.Skip("git does not support flushing yet, which is known to be flaky")
+	}
+
+	request := &gitalypb.DeleteRefsRequest{
+		Repository: repoProto,
+		Refs:       [][]byte{[]byte("refs/heads/master")},
+	}
+
+	repo := localrepo.NewTestRepo(t, cfg, repoProto)
+	oldValue, err := repo.ResolveRevision(ctx, git.Revision("refs/heads/master"))
+	require.NoError(t, err)
+
+	updater, err := updateref.New(ctx, repo)
 	require.NoError(t, err)
+	require.NoError(t, updater.Update(
+		git.ReferenceName("refs/heads/master"),
+		"0b4bc9a49b562e85de7cc9e834518ea6828729b9",
+		oldValue.String(),
+	))
+	require.NoError(t, updater.Prepare())
 
-	assert.Contains(t, response.GitError, "unable to delete refs")
+	response, err := client.DeleteRefs(ctx, request)
+
+	if featureflag.DeleteRefsStructuredErrors.IsEnabled(ctx) {
+		require.Nil(t, response)
+		detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
+			helper.ErrFailedPreconditionf("cannot lock references"),
+			&gitalypb.DeleteRefsError{
+				Error: &gitalypb.DeleteRefsError_ReferencesLocked{
+					ReferencesLocked: &gitalypb.ReferencesLockedError{
+						Refs: [][]byte{[]byte("refs/heads/master")},
+					},
+				},
+			})
+		require.NoError(t, errGeneratingDetailedErr)
+		testhelper.RequireGrpcError(t, detailedErr, err)
+	} else {
+		require.NoError(t, err)
+		assert.Contains(t, response.GetGitError(), "reference is already locked")
+	}
 }
 
 func TestDeleteRefs_validation(t *testing.T) {
diff --git a/internal/metadata/featureflag/ff_delete_refs_structured_errors.go b/internal/metadata/featureflag/ff_delete_refs_structured_errors.go
new file mode 100644
index 000000000..eca4647c7
--- /dev/null
+++ b/internal/metadata/featureflag/ff_delete_refs_structured_errors.go
@@ -0,0 +1,9 @@
+package featureflag
+
+// DeleteRefsStructuredErrors turns on metrics for pack objects
+var DeleteRefsStructuredErrors = NewFeatureFlag(
+	"delete_refs_structured_errors",
+	"v15.3.0",
+	"https://gitlab.com/gitlab-org/gitaly/-/issues/4348",
+	false,
+)