testserver: Wire up structerr interceptors

Wire up the structerr interceptors so that we convert error metadata
into error details. This allows us to easily verify that we indeed see
the correct metadata in gRPC methods without any races. Fix tests that
now require the error detail to exist.

11 files changed, 62 insertions, 64 deletions

diff --git a/internal/gitaly/service/commit/check_objects_exist_test.go b/internal/gitaly/service/commit/check_objects_exist_test.go
index ab6d0383e..09d41d07b 100644
--- a/internal/gitaly/service/commit/check_objects_exist_test.go
+++ b/internal/gitaly/service/commit/check_objects_exist_test.go
@@ -7,12 +7,10 @@ import (
 	"io"
 	"testing"
 
-	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -22,8 +20,7 @@ func TestCheckObjectsExist(t *testing.T) {
 	t.Parallel()
 
 	ctx := testhelper.Context(t)
-	logger, hook := test.NewNullLogger()
-	cfg, client := setupCommitService(t, ctx, testserver.WithLogger(logger))
+	cfg, client := setupCommitService(t, ctx)
 
 	repo, repoPath := gittest.CreateRepository(t, ctx, cfg)
 
@@ -38,11 +35,10 @@ func TestCheckObjectsExist(t *testing.T) {
 	)
 
 	for _, tc := range []struct {
-		desc                  string
-		requests              []*gitalypb.CheckObjectsExistRequest
-		expectedResults       map[string]bool
-		expectedErr           error
-		expectedErrorMetadata any
+		desc            string
+		requests        []*gitalypb.CheckObjectsExistRequest
+		expectedResults map[string]bool
+		expectedErr     error
 	}{
 		{
 			desc:     "no repository provided",
@@ -170,10 +166,8 @@ func TestCheckObjectsExist(t *testing.T) {
 					},
 				},
 			},
-			expectedErr: structerr.NewInvalidArgument("invalid revision: revision can't start with '-'"),
-			expectedErrorMetadata: map[string]any{
-				"revision": "-not-a-rev",
-			},
+			expectedErr: structerr.NewInvalidArgument("invalid revision: revision can't start with '-'").
+				WithInterceptedMetadata("revision", "-not-a-rev"),
 		},
 		{
 			desc: "input with whitespace",
@@ -185,10 +179,8 @@ func TestCheckObjectsExist(t *testing.T) {
 					},
 				},
 			},
-			expectedErr: structerr.NewInvalidArgument("invalid revision: revision can't contain whitespace"),
-			expectedErrorMetadata: map[string]any{
-				"revision": fmt.Sprintf("%s\n%s", commitID1, commitID2),
-			},
+			expectedErr: structerr.NewInvalidArgument("invalid revision: revision can't contain whitespace").
+				WithInterceptedMetadata("revision", fmt.Sprintf("%s\n%s", commitID1, commitID2)),
 		},
 		{
 			desc: "chunked invalid input",
@@ -205,15 +197,11 @@ func TestCheckObjectsExist(t *testing.T) {
 					},
 				},
 			},
-			expectedErr: structerr.NewInvalidArgument("invalid revision: revision can't start with '-'"),
-			expectedErrorMetadata: map[string]any{
-				"revision": "-not-a-rev",
-			},
+			expectedErr: structerr.NewInvalidArgument("invalid revision: revision can't start with '-'").
+				WithInterceptedMetadata("revision", "-not-a-rev"),
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			hook.Reset()
-
 			client, err := client.CheckObjectsExist(ctx)
 			require.NoError(t, err)
 
@@ -244,23 +232,6 @@ func TestCheckObjectsExist(t *testing.T) {
 
 			testhelper.RequireGrpcError(t, tc.expectedErr, err)
 			require.Equal(t, tc.expectedResults, results)
-
-			if tc.expectedErrorMetadata == nil {
-				tc.expectedErrorMetadata = map[string]any{}
-			}
-
-			metadata := map[string]any{}
-			for _, entry := range hook.AllEntries() {
-				errorMetadata, ok := entry.Data["error_metadata"]
-				if !ok {
-					continue
-				}
-
-				for key, value := range errorMetadata.(map[string]any) {
-					metadata[key] = value
-				}
-			}
-			require.Equal(t, tc.expectedErrorMetadata, metadata)
 		})
 	}
 }
diff --git a/internal/gitaly/service/commit/tree_entry_test.go b/internal/gitaly/service/commit/tree_entry_test.go
index 781f1cd1b..0077201b5 100644
--- a/internal/gitaly/service/commit/tree_entry_test.go
+++ b/internal/gitaly/service/commit/tree_entry_test.go
@@ -214,17 +214,17 @@ func TestFailedTreeEntry(t *testing.T) {
 		{
 			name:        "Path is outside of repository",
 			req:         &gitalypb.TreeEntryRequest{Repository: repo, Revision: []byte("913c66a37b4a45b9769037c55c2d238bd0942d2e"), Path: []byte("../bar/.gitkeep")}, // Git blows up on paths like this
-			expectedErr: structerr.NewNotFound("tree entry not found"),
+			expectedErr: structerr.NewNotFound("tree entry not found").WithInterceptedMetadata("path", "../bar/.gitkeep"),
 		},
 		{
 			name:        "Missing file with space in path",
 			req:         &gitalypb.TreeEntryRequest{Repository: repo, Revision: []byte("deadfacedeadfacedeadfacedeadfacedeadface"), Path: []byte("with space/README.md")},
-			expectedErr: structerr.NewNotFound("tree entry not found"),
+			expectedErr: structerr.NewNotFound("tree entry not found").WithInterceptedMetadata("path", "with space/README.md"),
 		},
 		{
 			name:        "Missing file",
 			req:         &gitalypb.TreeEntryRequest{Repository: repo, Revision: []byte("e63f41fe459e62e1228fcef60d7189127aeba95a"), Path: []byte("missing.rb")},
-			expectedErr: structerr.NewNotFound("tree entry not found"),
+			expectedErr: structerr.NewNotFound("tree entry not found").WithInterceptedMetadata("path", "missing.rb"),
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
diff --git a/internal/gitaly/service/diff/patch_id_test.go b/internal/gitaly/service/diff/patch_id_test.go
index 35e518430..aefee8d6b 100644
--- a/internal/gitaly/service/diff/patch_id_test.go
+++ b/internal/gitaly/service/diff/patch_id_test.go
@@ -223,7 +223,8 @@ func TestGetPatchID(t *testing.T) {
 						OldRevision: []byte(fmt.Sprintf("%s:file", oldCommit)),
 						NewRevision: []byte(fmt.Sprintf("%s:file", newCommit)),
 					},
-					expectedErr: structerr.New("waiting for git-diff: exit status 128"),
+					expectedErr: structerr.New("waiting for git-diff: exit status 128").
+						WithInterceptedMetadata("stderr", fmt.Sprintf("fatal: path 'file' does not exist in '%s'\n", oldCommit)),
 				}
 			},
 		},
@@ -240,7 +241,8 @@ func TestGetPatchID(t *testing.T) {
 						OldRevision: []byte(gittest.DefaultObjectHash.ZeroOID),
 						NewRevision: []byte(newRevision),
 					},
-					expectedErr: structerr.New("waiting for git-diff: exit status 128").WithMetadata("stderr", ""),
+					expectedErr: structerr.New("waiting for git-diff: exit status 128").
+						WithInterceptedMetadata("stderr", fmt.Sprintf("fatal: bad object %s\n", gittest.DefaultObjectHash.ZeroOID)),
 				}
 			},
 		},
diff --git a/internal/gitaly/service/objectpool/create_test.go b/internal/gitaly/service/objectpool/create_test.go
index b90b5e6d2..9948de9e0 100644
--- a/internal/gitaly/service/objectpool/create_test.go
+++ b/internal/gitaly/service/objectpool/create_test.go
@@ -113,6 +113,7 @@ func testCreateUnsuccessful(t *testing.T, ctx context.Context) {
 		Origin:     repo,
 	})
 	require.NoError(t, err)
+	preexistingPoolPath := filepath.Join(cfg.Storages[0].Path, preexistingPool.Repository.RelativePath)
 
 	for _, tc := range []struct {
 		desc        string
@@ -218,7 +219,9 @@ func testCreateUnsuccessful(t *testing.T, ctx context.Context) {
 				if featureflag.AtomicCreateObjectPool.IsEnabled(ctx) {
 					return structerr.NewFailedPrecondition("creating object pool: repository exists already")
 				}
-				return structerr.NewFailedPrecondition("creating object pool: target path exists already")
+
+				return structerr.NewFailedPrecondition("creating object pool: target path exists already").
+					WithInterceptedMetadata("object_pool_path", preexistingPoolPath)
 			}(),
 		},
 	} {
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index 73cbd5667..4c93de134 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -549,8 +549,9 @@ func TestUserDeleteBranch(t *testing.T) {
 						BranchName:     []byte(branchName),
 						ExpectedOldOid: "foobar",
 					},
-					repoPath:     repoPath,
-					expectedErr:  structerr.NewInvalidArgument("invalid expected old object ID: invalid object ID: \"foobar\""),
+					repoPath: repoPath,
+					expectedErr: structerr.NewInvalidArgument("invalid expected old object ID: invalid object ID: \"foobar\"").
+						WithInterceptedMetadata("old_object_id", "foobar"),
 					expectedRefs: []string{"master", branchName},
 				}
 			},
@@ -574,8 +575,9 @@ func TestUserDeleteBranch(t *testing.T) {
 						BranchName:     []byte(branchName),
 						ExpectedOldOid: gittest.DefaultObjectHash.ZeroOID.String(),
 					},
-					repoPath:     repoPath,
-					expectedErr:  structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found"),
+					repoPath: repoPath,
+					expectedErr: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found").
+						WithInterceptedMetadata("old_object_id", gittest.DefaultObjectHash.ZeroOID),
 					expectedRefs: []string{"master", branchName},
 				}
 			},
diff --git a/internal/gitaly/service/operations/cherry_pick_test.go b/internal/gitaly/service/operations/cherry_pick_test.go
index 8d5b6b183..71cae333f 100644
--- a/internal/gitaly/service/operations/cherry_pick_test.go
+++ b/internal/gitaly/service/operations/cherry_pick_test.go
@@ -205,7 +205,8 @@ func TestUserCherryPick(t *testing.T) {
 					ExpectedOldOid: "foobar",
 				}, &gitalypb.UserCherryPickResponse{}
 			},
-			expectedErr: structerr.NewInvalidArgument("invalid expected old object ID: invalid object ID: \"foobar\""),
+			expectedErr: structerr.NewInvalidArgument("invalid expected old object ID: invalid object ID: \"foobar\"").
+				WithInterceptedMetadata("old_object_id", "foobar"),
 		},
 		{
 			desc: "valid but non-existent expectedOldOID",
@@ -221,7 +222,8 @@ func TestUserCherryPick(t *testing.T) {
 					ExpectedOldOid: gittest.DefaultObjectHash.ZeroOID.String(),
 				}, &gitalypb.UserCherryPickResponse{}
 			},
-			expectedErr: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found"),
+			expectedErr: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found").
+				WithInterceptedMetadata("old_object_id", gittest.DefaultObjectHash.ZeroOID),
 		},
 		{
 			desc: "incorrect expectedOldOID",
diff --git a/internal/gitaly/service/operations/commit_files_test.go b/internal/gitaly/service/operations/commit_files_test.go
index 7c764ae77..2bff1f4db 100644
--- a/internal/gitaly/service/operations/commit_files_test.go
+++ b/internal/gitaly/service/operations/commit_files_test.go
@@ -1154,7 +1154,8 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) {
 			repoPath:       repoPath,
 			branchName:     "few-commits",
 			expectedOldOID: "foobar",
-			expectedError:  structerr.NewInvalidArgument(`invalid expected old object ID: invalid object ID: "foobar"`),
+			expectedError: structerr.NewInvalidArgument(`invalid expected old object ID: invalid object ID: "foobar"`).
+				WithInterceptedMetadata("old_object_id", "foobar"),
 		},
 		{
 			desc:           "existing repo and branch + valid expectedOldOID but invalid object",
@@ -1162,7 +1163,8 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) {
 			repoPath:       repoPath,
 			branchName:     "few-commits",
 			expectedOldOID: gittest.DefaultObjectHash.ZeroOID.String(),
-			expectedError:  structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found"),
+			expectedError: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found").
+				WithInterceptedMetadata("old_object_id", gittest.DefaultObjectHash.ZeroOID),
 		},
 		{
 			desc:           "existing repo and branch + incorrect expectedOldOID",
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index 80a4b8f65..f799d0b11 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -135,7 +135,8 @@ func TestUserMergeBranch(t *testing.T) {
 						Message:        []byte(data.message),
 						ExpectedOldOid: "foobar",
 					},
-					firstExpectedErr: structerr.NewInvalidArgument("invalid expected old object ID: invalid object ID: \"foobar\""),
+					firstExpectedErr: structerr.NewInvalidArgument("invalid expected old object ID: invalid object ID: \"foobar\"").
+						WithInterceptedMetadata("old_object_id", "foobar"),
 				}
 			},
 		},
@@ -152,7 +153,8 @@ func TestUserMergeBranch(t *testing.T) {
 						Message:        []byte(data.message),
 						ExpectedOldOid: gittest.DefaultObjectHash.ZeroOID.String(),
 					},
-					firstExpectedErr: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found"),
+					firstExpectedErr: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found").
+						WithInterceptedMetadata("old_object_id", gittest.DefaultObjectHash.ZeroOID),
 				}
 			},
 		},
@@ -1254,7 +1256,8 @@ func TestUserFFBranch(t *testing.T) {
 					},
 				}
 			},
-			expectedErr: structerr.NewInvalidArgument(`invalid expected old object ID: invalid object ID: "foobar"`),
+			expectedErr: structerr.NewInvalidArgument(`invalid expected old object ID: invalid object ID: "foobar"`).
+				WithInterceptedMetadata("old_object_id", "foobar"),
 		},
 		{
 			desc: "valid SHA, but not existing expectedOldOID",
@@ -1275,7 +1278,8 @@ func TestUserFFBranch(t *testing.T) {
 					},
 				}
 			},
-			expectedErr: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found"),
+			expectedErr: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found").
+				WithInterceptedMetadata("old_object_id", gittest.DefaultObjectHash.ZeroOID),
 		},
 		{
 			desc: "expectedOldOID pointing to old commit",
diff --git a/internal/gitaly/service/operations/revert_test.go b/internal/gitaly/service/operations/revert_test.go
index 088739913..78e100b59 100644
--- a/internal/gitaly/service/operations/revert_test.go
+++ b/internal/gitaly/service/operations/revert_test.go
@@ -320,7 +320,8 @@ func TestUserRevert(t *testing.T) {
 					},
 				}
 			},
-			expectedErr: structerr.NewInvalidArgument(`invalid expected old object ID: invalid object ID: "foobar"`),
+			expectedErr: structerr.NewInvalidArgument(`invalid expected old object ID: invalid object ID: "foobar"`).
+				WithInterceptedMetadata("old_object_id", "foobar"),
 		},
 		{
 			desc: "expectedOldOID with valid SHA, but not present in repo",
@@ -342,7 +343,8 @@ func TestUserRevert(t *testing.T) {
 					},
 				}
 			},
-			expectedErr: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found"),
+			expectedErr: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found").
+				WithInterceptedMetadata("old_object_id", gittest.DefaultObjectHash.ZeroOID),
 		},
 		{
 			desc: "expectedOldOID pointing to old commit",
diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go
index b2aac01dd..7dee225ed 100644
--- a/internal/gitaly/service/operations/tags_test.go
+++ b/internal/gitaly/service/operations/tags_test.go
@@ -190,7 +190,8 @@ func TestUserDeleteTag(t *testing.T) {
 					ExpectedOldOid: "io",
 				}
 			},
-			expectedErr:  structerr.NewInvalidArgument(`invalid expected old object ID: invalid object ID: "io"`),
+			expectedErr: structerr.NewInvalidArgument(`invalid expected old object ID: invalid object ID: "io"`).
+				WithInterceptedMetadata("old_object_id", "io"),
 			expectedTags: []string{"europa"},
 		},
 		{
@@ -207,7 +208,8 @@ func TestUserDeleteTag(t *testing.T) {
 					ExpectedOldOid: gittest.DefaultObjectHash.ZeroOID.String(),
 				}
 			},
-			expectedErr:  structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found"),
+			expectedErr: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found").
+				WithInterceptedMetadata("old_object_id", gittest.DefaultObjectHash.ZeroOID),
 			expectedTags: []string{"europa"},
 		},
 		{
diff --git a/internal/testhelper/testserver/gitaly.go b/internal/testhelper/testserver/gitaly.go
index 9ec09d357..d2a377710 100644
--- a/internal/testhelper/testserver/gitaly.go
+++ b/internal/testhelper/testserver/gitaly.go
@@ -33,6 +33,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/middleware/limithandler"
 	praefectconfig "gitlab.com/gitlab-org/gitaly/v15/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/streamcache"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testdb"
 	"google.golang.org/grpc"
@@ -153,6 +154,13 @@ func runGitaly(tb testing.TB, cfg config.Cfg, rubyServer *rubyserver.Server, reg
 		gsd = opt(gsd)
 	}
 
+	// We set up the structerr interceptors so that any error metadata that gets set via
+	// `structerr.WithMetadata()` is not only logged, but also present in the error details.
+	serverOpts := []server.Option{
+		server.WithUnaryInterceptor(structerr.UnaryInterceptor),
+		server.WithStreamInterceptor(structerr.StreamInterceptor),
+	}
+
 	deps := gsd.createDependencies(tb, cfg, rubyServer)
 	tb.Cleanup(func() { gsd.conns.Close() })
 
@@ -165,7 +173,7 @@ func runGitaly(tb testing.TB, cfg config.Cfg, rubyServer *rubyserver.Server, reg
 	)
 
 	if cfg.RuntimeDir != "" {
-		internalServer, err := serverFactory.CreateInternal()
+		internalServer, err := serverFactory.CreateInternal(serverOpts...)
 		require.NoError(tb, err)
 		tb.Cleanup(internalServer.Stop)
 
@@ -187,7 +195,7 @@ func runGitaly(tb testing.TB, cfg config.Cfg, rubyServer *rubyserver.Server, reg
 
 	secure := cfg.TLS.CertPath != "" && cfg.TLS.KeyPath != ""
 
-	externalServer, err := serverFactory.CreateExternal(secure)
+	externalServer, err := serverFactory.CreateExternal(secure, serverOpts...)
 	require.NoError(tb, err)
 	tb.Cleanup(externalServer.Stop)