Merge branch 'pks-find-tag-structured-errors' into 'master'

ref: Fix `Internal` errors in `FindTag()` when tag doesn't exist

See merge request gitlab-org/gitaly!4771

3 files changed, 66 insertions, 2 deletions

diff --git a/internal/gitaly/service/ref/find_tag.go b/internal/gitaly/service/ref/find_tag.go
index 5c92d5cbf..b23b50b3e 100644
--- a/internal/gitaly/service/ref/find_tag.go
+++ b/internal/gitaly/service/ref/find_tag.go
@@ -10,6 +10,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
 
@@ -94,8 +95,24 @@ func (s *server) findTag(ctx context.Context, repo git.RepositoryExecutor, tagNa
 			return nil, err
 		}
 	} else {
-		// TODO: this callsite will eventually be converted to return a FindTagError with
-		// `TagNotFound` set.
+		if featureflag.FindTagStructuredError.IsEnabled(ctx) {
+			detailedErr, err := helper.ErrWithDetails(
+				helper.ErrNotFoundf("tag does not exist"),
+				&gitalypb.FindTagError{
+					Error: &gitalypb.FindTagError_TagNotFound{
+						TagNotFound: &gitalypb.ReferenceNotFoundError{
+							ReferenceName: []byte(fmt.Sprintf("refs/tags/%s", tagName)),
+						},
+					},
+				},
+			)
+			if err != nil {
+				return nil, helper.ErrInternalf("generating detailed error: %w", err)
+			}
+
+			return nil, detailedErr
+		}
+
 		return nil, errors.New("no tag found")
 	}
 
diff --git a/internal/gitaly/service/ref/find_tag_test.go b/internal/gitaly/service/ref/find_tag_test.go
index d9359a31b..aeeaf9c56 100644
--- a/internal/gitaly/service/ref/find_tag_test.go
+++ b/internal/gitaly/service/ref/find_tag_test.go
@@ -3,6 +3,7 @@
 package ref
 
 import (
+	"context"
 	"fmt"
 	"strings"
 	"testing"
@@ -14,6 +15,7 @@ import (
 	"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/helper"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -259,6 +261,41 @@ func TestFindTag_nestedTag(t *testing.T) {
 	}
 }
 
+func TestFindTag_notFound(t *testing.T) {
+	t.Parallel()
+	testhelper.NewFeatureSets(featureflag.FindTagStructuredError).Run(t, testFindTagNotFound)
+}
+
+func testFindTagNotFound(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
+	cfg, client := setupRefServiceWithoutRepo(t)
+	repoProto, _ := gittest.CreateRepository(ctx, t, cfg)
+
+	response, err := client.FindTag(ctx, &gitalypb.FindTagRequest{
+		Repository: repoProto,
+		TagName:    []byte("does-not-exist"),
+	})
+	require.Nil(t, response)
+	if featureflag.FindTagStructuredError.IsEnabled(ctx) {
+		expectedErr, errGeneratingDetails := helper.ErrWithDetails(
+			helper.ErrNotFoundf("tag does not exist"),
+			&gitalypb.FindTagError{
+				Error: &gitalypb.FindTagError_TagNotFound{
+					TagNotFound: &gitalypb.ReferenceNotFoundError{
+						ReferenceName: []byte("refs/tags/does-not-exist"),
+					},
+				},
+			},
+		)
+		require.NoError(t, errGeneratingDetails)
+
+		testhelper.RequireGrpcError(t, expectedErr, err)
+	} else {
+		testhelper.RequireGrpcError(t, helper.ErrInternalf("no tag found"), err)
+	}
+}
+
 func TestFindTag_invalidRequest(t *testing.T) {
 	t.Parallel()
 
diff --git a/internal/metadata/featureflag/ff_find_tag_structured_error.go b/internal/metadata/featureflag/ff_find_tag_structured_error.go
new file mode 100644
index 000000000..900f1eefc
--- /dev/null
+++ b/internal/metadata/featureflag/ff_find_tag_structured_error.go
@@ -0,0 +1,10 @@
+package featureflag
+
+// FindTagStructuredError enables the use of structured errors for the FindTag RPC in case the tag
+// could not be found.
+var FindTagStructuredError = NewFeatureFlag(
+	"find_tag_structured_error",
+	"v15.3.0",
+	"https://gitlab.com/gitlab-org/gitaly/-/issues/4398",
+	false,
+)