From 49be52446858e6465c318d13f39c2bbb926025a6 Mon Sep 17 00:00:00 2001 From: DharunMR Date: Sat, 11 Apr 2026 00:25:37 +0530 Subject: [PATCH 1/2] Fixed Repo delete by name Signed-off-by: DharunMR --- internal/controlplane/handlers_repositories.go | 5 +++++ .../controlplane/handlers_repositories_test.go | 15 ++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/internal/controlplane/handlers_repositories.go b/internal/controlplane/handlers_repositories.go index 5fd823e857..4578b26354 100644 --- a/internal/controlplane/handlers_repositories.go +++ b/internal/controlplane/handlers_repositories.go @@ -298,6 +298,11 @@ func (s *Server) DeleteRepositoryByName( } projectID := GetProjectID(ctx) + + if in.GetContext() == nil || in.GetContext().GetProvider() == "" { + return nil, util.UserVisibleError(codes.InvalidArgument, "provider name must be specified when deleting a repository by name") + } + providerName := GetProviderName(ctx) err := s.repos.DeleteByName(ctx, fragments[0], fragments[1], projectID, providerName) diff --git a/internal/controlplane/handlers_repositories_test.go b/internal/controlplane/handlers_repositories_test.go index ce7f31e8ae..7a1a07eb62 100644 --- a/internal/controlplane/handlers_repositories_test.go +++ b/internal/controlplane/handlers_repositories_test.go @@ -415,6 +415,7 @@ func TestServer_DeleteRepository(t *testing.T) { RepoID string RepoServiceSetup repoMockBuilder ProviderFails bool + EmptyProvider bool ExpectedError string }{ { @@ -427,6 +428,12 @@ func TestServer_DeleteRepository(t *testing.T) { RepoID: "I am not a UUID", ExpectedError: "invalid repository ID", }, + { + Name: "delete by name fails when provider is empty", + RepoName: repoOwnerAndName, + EmptyProvider: true, + ExpectedError: "provider name must be specified when deleting a repository by name", + }, { Name: "deletion fails when repo service returns error", RepoName: repoOwnerAndName, @@ -485,8 +492,14 @@ func TestServer_DeleteRepository(t *testing.T) { var expectation string if scenario.RepoName != "" { req := &pb.DeleteRepositoryByNameRequest{ - Name: scenario.RepoName, + Name: scenario.RepoName, + Context: &pb.Context{}, } + + if !scenario.EmptyProvider { + req.Context.Provider = ptr.Ptr(ghprovider.Github) + } + res, err := server.DeleteRepositoryByName(ctx, req) if res != nil { result = res.Name From e61893c9f2df47cd7983ba2e402a394741acd6f1 Mon Sep 17 00:00:00 2001 From: DharunMR Date: Sat, 11 Apr 2026 01:19:18 +0530 Subject: [PATCH 2/2] added missing get repo testcases Signed-off-by: DharunMR --- .../controlplane/handlers_repositories.go | 23 ++- .../handlers_repositories_test.go | 169 +++++++++++++++++- 2 files changed, 186 insertions(+), 6 deletions(-) diff --git a/internal/controlplane/handlers_repositories.go b/internal/controlplane/handlers_repositories.go index 4578b26354..94dc08d3d2 100644 --- a/internal/controlplane/handlers_repositories.go +++ b/internal/controlplane/handlers_repositories.go @@ -40,7 +40,11 @@ func (s *Server) RegisterRepository( in *pb.RegisterRepositoryRequest, ) (*pb.RegisterRepositoryResponse, error) { projectID := GetProjectID(ctx) - providerName := GetProviderName(ctx) + + if in.GetContext() == nil || in.GetContext().GetProvider() == "" { + return nil, util.UserVisibleError(codes.InvalidArgument, "provider name must be specified when registering a repository") + } + providerName := in.GetContext().GetProvider() var fetchByProps *properties.Properties var provider *db.Provider @@ -137,10 +141,15 @@ func (s *Server) repoCreateInfoFromUpstreamEntityRef( // This function will typically be called by the client to get a list of // repositories that are registered present in the minder database func (s *Server) ListRepositories(ctx context.Context, - _ *pb.ListRepositoriesRequest) (*pb.ListRepositoriesResponse, error) { + in *pb.ListRepositoriesRequest) (*pb.ListRepositoriesResponse, error) { + + if in.GetContext() == nil || in.GetContext().GetProvider() == "" { + return nil, util.UserVisibleError(codes.InvalidArgument, "provider name must be specified when listing repositories") + } + entityCtx := engcontext.EntityFromContext(ctx) projectID := entityCtx.Project.ID - providerName := entityCtx.Provider.Name + providerName := in.GetContext().GetProvider() logger.BusinessRecord(ctx).Provider = providerName logger.BusinessRecord(ctx).Project = projectID @@ -239,9 +248,13 @@ func (s *Server) GetRepositoryByName(ctx context.Context, return nil, util.UserVisibleError(codes.InvalidArgument, "invalid repository name, needs to have the format: owner/name") } + if in.GetContext() == nil || in.GetContext().GetProvider() == "" { + return nil, util.UserVisibleError(codes.InvalidArgument, "provider name must be specified when getting a repository by name") + } + entityCtx := engcontext.EntityFromContext(ctx) projectID := entityCtx.Project.ID - providerName := entityCtx.Provider.Name + providerName := in.GetContext().GetProvider() // Use the service to fetch the repository repo, err := s.repos.GetRepositoryByName(ctx, fragments[0], fragments[1], projectID, providerName) @@ -303,7 +316,7 @@ func (s *Server) DeleteRepositoryByName( return nil, util.UserVisibleError(codes.InvalidArgument, "provider name must be specified when deleting a repository by name") } - providerName := GetProviderName(ctx) + providerName := in.GetContext().GetProvider() err := s.repos.DeleteByName(ctx, fragments[0], fragments[1], projectID, providerName) if errors.Is(err, sql.ErrNoRows) { diff --git a/internal/controlplane/handlers_repositories_test.go b/internal/controlplane/handlers_repositories_test.go index 7a1a07eb62..6c3dbdf436 100644 --- a/internal/controlplane/handlers_repositories_test.go +++ b/internal/controlplane/handlers_repositories_test.go @@ -50,8 +50,16 @@ func TestServer_RegisterRepository(t *testing.T) { RepoName string RepoServiceSetup repoMockBuilder ProviderFails bool + EmptyProvider bool ExpectedError string }{ + { + Name: "Repo registration fails when provider is empty", + RepoOwner: repoOwner, + RepoName: repoName, + EmptyProvider: true, + ExpectedError: "provider name must be specified when registering a repository", + }, { Name: "Repo creation fails when provider cannot be found", RepoOwner: repoOwner, @@ -136,11 +144,17 @@ func TestServer_RegisterRepository(t *testing.T) { ) req := &pb.RegisterRepositoryRequest{ + Context: &pb.Context{}, Repository: &pb.UpstreamRepositoryRef{ Owner: scenario.RepoOwner, Name: scenario.RepoName, }, } + + if !scenario.EmptyProvider { + req.Context.Provider = ptr.Ptr(ghprovider.Github) + } + res, err := server.RegisterRepository(ctx, req) if scenario.ExpectedError == "" { expectation := &pb.RegisterRepositoryResponse{ @@ -169,9 +183,15 @@ func TestServer_ListRepositories(t *testing.T) { RepoServiceSetup repoMockBuilder ProviderSetup func(ctrl *gomock.Controller, mgr *mockmanager.MockProviderManager) ProviderFails bool + EmptyProvider bool ExpectedResults []*pb.Repository ExpectedError string }{ + { + Name: "List repositories fails when provider is empty", + EmptyProvider: true, + ExpectedError: "provider name must be specified when listing repositories", + }, { Name: "List repositories succeeds with multiple results", RepoServiceSetup: rf.NewRepoService( @@ -282,7 +302,14 @@ func TestServer_ListRepositories(t *testing.T) { mgr, ) - req := &pb.ListRepositoriesRequest{} + req := &pb.ListRepositoriesRequest{ + Context: &pb.Context{}, + } + + if !scenario.EmptyProvider { + req.Context.Provider = ptr.Ptr(ghprovider.Github) + } + res, err := server.ListRepositories(ctx, req) if scenario.ExpectedError == "" { require.NoError(t, err) @@ -405,6 +432,146 @@ func TestServer_ListRemoteRepositoriesFromProvider(t *testing.T) { } } +func mockGetRepoByName(res *pb.Repository, err error) repoMockBuilder { + return func(ctrl *gomock.Controller) repoServiceMock { + mock := mockrepo.NewMockRepositoryService(ctrl) + mock.EXPECT().GetRepositoryByName(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(res, err).AnyTimes() + return mock + } +} + +func mockGetRepoByID(res *pb.Repository, err error) repoMockBuilder { + return func(ctrl *gomock.Controller) repoServiceMock { + mock := mockrepo.NewMockRepositoryService(ctrl) + mock.EXPECT().GetRepositoryById(gomock.Any(), gomock.Any(), gomock.Any()).Return(res, err).AnyTimes() + return mock + } +} + +func TestServer_GetRepository(t *testing.T) { + t.Parallel() + + scenarios := []struct { + Name string + RepoName string + RepoID string + RepoServiceSetup repoMockBuilder + ProviderFails bool + EmptyProvider bool + ExpectedError string + }{ + { + Name: "get by name fails when name is malformed", + RepoName: "I am not a repo name", + ExpectedError: "invalid repository name", + }, + { + Name: "get by ID fails when ID is malformed", + RepoID: "I am not a UUID", + ExpectedError: "invalid repository ID", + }, + { + Name: "get by name fails when provider is empty", + RepoName: repoOwnerAndName, + EmptyProvider: true, + ExpectedError: "provider name must be specified when getting a repository by name", + }, + { + Name: "get by name fails when repo service returns error", + RepoName: repoOwnerAndName, + RepoServiceSetup: mockGetRepoByName(nil, errDefault), + ExpectedError: errDefault.Error(), + }, + { + Name: "get by ID fails when repo service returns error", + RepoID: repoID, + RepoServiceSetup: mockGetRepoByID(nil, errDefault), + ExpectedError: "cannot read repository", + }, + { + Name: "get by name fails when repo is not found", + RepoName: repoOwnerAndName, + RepoServiceSetup: mockGetRepoByName(nil, sql.ErrNoRows), + ExpectedError: "repository not found", + }, + { + Name: "get by ID fails when repo is not found", + RepoID: repoID, + RepoServiceSetup: mockGetRepoByID(nil, sql.ErrNoRows), + ExpectedError: "repository not found", + }, + { + Name: "get by name succeeds", + RepoName: repoOwnerAndName, + RepoServiceSetup: mockGetRepoByName(&existingRepo, nil), + }, + { + Name: "get by ID succeeds", + RepoID: repoID, + RepoServiceSetup: mockGetRepoByID(&existingRepo, nil), + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.Name, func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + ctx := engcontext.WithEntityContext(context.Background(), &engcontext.EntityContext{ + Provider: engcontext.Provider{Name: ghprovider.Github}, + Project: engcontext.Project{ID: projectID}, + }) + + server := createServer( + ctrl, + scenario.RepoServiceSetup, + scenario.ProviderFails, + nil, + ) + + var result *pb.Repository + var resultError error + + if scenario.RepoName != "" { + req := &pb.GetRepositoryByNameRequest{ + Name: scenario.RepoName, + Context: &pb.Context{}, + } + + if !scenario.EmptyProvider { + req.Context.Provider = ptr.Ptr(ghprovider.Github) + } + + res, err := server.GetRepositoryByName(ctx, req) + if res != nil { + result = res.Repository + } + resultError = err + } else { + req := &pb.GetRepositoryByIdRequest{ + RepositoryId: scenario.RepoID, + } + res, err := server.GetRepositoryById(ctx, req) + if res != nil { + result = res.Repository + } + resultError = err + } + + if scenario.ExpectedError == "" { + require.NoError(t, resultError) + require.NotNil(t, result) + if scenario.RepoName != "" { + require.Equal(t, scenario.RepoName, fmt.Sprintf("%s/%s", result.Owner, result.Name)) + } + } else { + require.Nil(t, result) + require.ErrorContains(t, resultError, scenario.ExpectedError) + } + }) + } +} + // lump both deletion endpoints together since they are so similar func TestServer_DeleteRepository(t *testing.T) { t.Parallel()