diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-08-01 13:59:55 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-08-01 14:32:10 +0300 |
commit | 453ac57fc19b14da7f579df8d5c3a36adaa2ef3a (patch) | |
tree | 8505cee68e46f8467f4329545ea0cc7c7e0546f6 | |
parent | 2667f87af97aaf7aaa5c54bbc7d91472b26dfc33 (diff) |
ref: Fix `Internal` errors in `FindTag()` when tag doesn't existpks-find-tag-structured-errors
The `FindTag()` RPC is currently returning an `Internal` error in case
the requested tag doesn't exist. This is bad due to two reasons:
- It impacts our SLOs as `Internal` errors are typically unexpected
error cases that shouldn't happen during normal operations. A tag
that doesn't exist is an entirely valid outcome though and should
thus have a proper error code.
- The Rails callsite needs to always handle `Internal` errors as if
the tag doesn't exist, even though the RPC call could have due to
an entirely different reason.
Fix both issues by returning a structured `FindTagError` error which
allows us to indicate that the tag indeed wasn't found. Like this,
callers can special-case this scenario and don't have to parse error
messages and we can start to return a proper gRPC error code, which is
`NotFound` in our case here.
Changelog: fixed
-rw-r--r-- | internal/gitaly/service/ref/find_tag.go | 21 | ||||
-rw-r--r-- | internal/gitaly/service/ref/find_tag_test.go | 37 | ||||
-rw-r--r-- | internal/metadata/featureflag/ff_find_tag_structured_error.go | 10 |
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 6461e9e77..215a88065 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" @@ -284,6 +286,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, +) |