Refactor repo deletion to move db/provider logic behind interface (#3200

)

Relates to #2845

Change the interface of the repo service so that it takes care of DB
queries and provider instantiation. This moves various details about
repo cleanup out of the controlplane.

As part of this, change the provider cleanup process to delete webhooks
directly instead of deleting repos. This avoided a circular dependency,
and more closely aligns with the idea that webhook management is a
provider-specific detail.

database/query/repositories.sql

@@ -51,3 +51,9 @@ WHERE id = $1;
 
 -- name: CountRepositories :one
 SELECT COUNT(*) FROM repositories;
+
+-- get a list of repos with webhooks belonging to a provider
+-- is used for webhook cleanup during provider deletion
+-- name: GetProviderWebhooks :many
+SELECT repo_owner, repo_name, webhook_id FROM repositories
+WHERE webhook_id IS NOT NULL AND provider_id = $1;

internal/controlplane/handlers_providers_test.go

@@ -16,6 +16,7 @@ package controlplane
 
 import (
 	"context"
+	"database/sql"
 	"encoding/json"
 	"testing"
 
@@ -39,7 +40,7 @@ import (
 	mockprovsvc "github.com/stacklok/minder/internal/providers/github/service/mock"
 	"github.com/stacklok/minder/internal/providers/manager"
 	"github.com/stacklok/minder/internal/providers/ratecache"
-	mockghrepo "github.com/stacklok/minder/internal/repositories/github/mock"
+	mockwebhooks "github.com/stacklok/minder/internal/repositories/github/webhooks/mock"
 	minder "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
 	provinfv1 "github.com/stacklok/minder/pkg/providers/v1"
 )
@@ -92,9 +93,17 @@ func TestDeleteProvider(t *testing.T) {
 		ID:        providerID,
 		ProjectID: projectID,
 	}).Return(nil)
+	mockStore.EXPECT().
+		GetProviderWebhooks(gomock.Any(), gomock.Eq(providerID)).
+		Return([]db.GetProviderWebhooksRow{
+			{
+				RepoOwner: "test",
+				RepoName:  "repo",
+				WebhookID: sql.NullInt64{Valid: true, Int64: 12345},
+			}}, nil)
 
-	mockRepoSvc := mockghrepo.NewMockRepositoryService(ctrl)
-	mockRepoSvc.EXPECT().DeleteRepositoriesByProvider(gomock.Any(), gomock.Any(), providerName, projectID).Return(nil)
+	whManager := mockwebhooks.NewMockWebhookManager(ctrl)
+	whManager.EXPECT().DeleteWebhook(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
 
 	cancelable, cancel := context.WithCancel(context.Background())
 	clientCache := ratecache.NewRestClientCache(cancelable)
@@ -113,7 +122,7 @@ func TestDeleteProvider(t *testing.T) {
 		&serverconfig.ProviderConfig{},
 		nil,
 		mockCryptoEngine,
-		mockRepoSvc,
+		whManager,
 		mockStore,
 		mockProvidersSvc,
 	)
@@ -124,7 +133,6 @@ func TestDeleteProvider(t *testing.T) {
 		cryptoEngine:    mockCryptoEngine,
 		store:           mockStore,
 		ghProviders:     mockProvidersSvc,
-		repos:           mockRepoSvc,
 		authzClient:     authzClient,
 		providerStore:   providerStore,
 		providerManager: providerManager,
@@ -201,9 +209,17 @@ func TestDeleteProviderByID(t *testing.T) {
 		Return(db.ProviderAccessToken{
 			EncryptedToken: "encryptedToken",
 		}, nil).AnyTimes()
+	mockStore.EXPECT().
+		GetProviderWebhooks(gomock.Any(), gomock.Eq(providerID)).
+		Return([]db.GetProviderWebhooksRow{
+			{
+				RepoOwner: "test",
+				RepoName:  "repo",
+				WebhookID: sql.NullInt64{Valid: true, Int64: 12345},
+			}}, nil)
 
-	mockRepoSvc := mockghrepo.NewMockRepositoryService(ctrl)
-	mockRepoSvc.EXPECT().DeleteRepositoriesByProvider(gomock.Any(), gomock.Any(), providerName, projectID).Return(nil)
+	whManager := mockwebhooks.NewMockWebhookManager(ctrl)
+	whManager.EXPECT().DeleteWebhook(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
 
 	cancelable, cancel := context.WithCancel(context.Background())
 	clientCache := ratecache.NewRestClientCache(cancelable)
@@ -220,7 +236,7 @@ func TestDeleteProviderByID(t *testing.T) {
 		&serverconfig.ProviderConfig{},
 		nil,
 		mockCryptoEngine,
-		mockRepoSvc,
+		whManager,
 		mockStore,
 		mockProvidersSvc,
 	)
@@ -231,7 +247,6 @@ func TestDeleteProviderByID(t *testing.T) {
 		cryptoEngine:    mockCryptoEngine,
 		store:           mockStore,
 		ghProviders:     mockProvidersSvc,
-		repos:           mockRepoSvc,
 		authzClient:     authzClient,
 		providerStore:   providerStore,
 		providerManager: providerManager,

internal/controlplane/handlers_repositories.go

@@ -272,11 +272,11 @@ func (s *Server) DeleteRepositoryById(
 
 	projectID := getProjectID(ctx)
 
-	err = s.deleteRepository(ctx, func() (db.Repository, error) {
-		return s.repos.GetRepositoryById(ctx, parsedRepositoryID, projectID)
-	})
-	if err != nil {
-		return nil, err
+	err = s.repos.DeleteByID(ctx, parsedRepositoryID, projectID)
+	if errors.Is(err, sql.ErrNoRows) {
+		return nil, status.Errorf(codes.NotFound, "repository not found")
+	} else if err != nil {
+		return nil, status.Errorf(codes.Internal, "unexpected error deleting repo: %v", err)
 	}
 
 	// return the response with the id of the deleted repository
@@ -299,11 +299,11 @@ func (s *Server) DeleteRepositoryByName(
 	projectID := getProjectID(ctx)
 	providerName := getProviderName(ctx)
 
-	err := s.deleteRepository(ctx, func() (db.Repository, error) {
-		return s.repos.GetRepositoryByName(ctx, fragments[0], fragments[1], projectID, providerName)
-	})
-	if err != nil {
-		return nil, err
+	err := s.repos.DeleteByName(ctx, fragments[0], fragments[1], projectID, providerName)
+	if errors.Is(err, sql.ErrNoRows) {
+		return nil, status.Errorf(codes.NotFound, "repository not found")
+	} else if err != nil {
+		return nil, status.Errorf(codes.Internal, "unexpected error deleting repo: %v", err)
 	}
 
 	// return the response with the name of the deleted repository
@@ -429,35 +429,6 @@ func (s *Server) listRemoteRepositoriesForProvider(
 	return results, nil
 }
 
-// covers the common logic for the two varieties of repo deletion
-func (s *Server) deleteRepository(
-	ctx context.Context,
-	repoQueryMethod func() (db.Repository, error),
-) error {
-	repo, err := repoQueryMethod()
-	if errors.Is(err, sql.ErrNoRows) {
-		return status.Errorf(codes.NotFound, "repository not found")
-	} else if err != nil {
-		return status.Errorf(codes.Internal, "unexpected error fetching repo: %v", err)
-	}
-
-	provider, err := s.providerStore.GetByID(ctx, repo.ProviderID)
-	if err != nil {
-		return status.Errorf(codes.Internal, "cannot get provider: %v", err)
-	}
-
-	client, err := s.getClientForProvider(ctx, *provider)
-	if err != nil {
-		return status.Errorf(codes.Internal, "cannot get client for provider: %v", err)
-	}
-
-	err = s.repos.DeleteRepository(ctx, client, &repo)
-	if err != nil {
-		return status.Errorf(codes.Internal, "unexpected error deleting repo: %v", err)
-	}
-	return nil
-}
-
 // TODO: move out of controlplane
 // inferProviderByOwner returns the provider to use for a given repo owner
 func (s *Server) inferProviderByOwner(ctx context.Context, owner string, projectID uuid.UUID, providerName string,

internal/controlplane/handlers_repositories_test.go

@@ -217,13 +217,6 @@ func TestServer_DeleteRepository(t *testing.T) {
 		ProviderFails    bool
 		ExpectedError    string
 	}{
-		{
-			Name:             "deletion fails when provider cannot be found",
-			RepoName:         repoOwnerAndName,
-			RepoServiceSetup: newRepoService(withSuccessfulGetRepoByName),
-			ProviderFails:    true,
-			ExpectedError:    "cannot get provider",
-		},
 		{
 			Name:          "delete by name fails when name is malformed",
 			RepoName:      "I am not a repo name",
@@ -234,33 +227,39 @@ func TestServer_DeleteRepository(t *testing.T) {
 			RepoID:        "I am not a UUID",
 			ExpectedError: "invalid repository ID",
 		},
-		{
-			Name:             "deletion fails when repo is not found",
-			RepoName:         repoOwnerAndName,
-			RepoServiceSetup: newRepoService(withFailedGetRepoByName(sql.ErrNoRows)),
-			ExpectedError:    "repository not found",
-		},
 		{
 			Name:             "deletion fails when repo service returns error",
 			RepoName:         repoOwnerAndName,
-			RepoServiceSetup: newRepoService(withSuccessfulGetRepoByName, withFailedDelete(errDefault)),
+			RepoServiceSetup: newRepoService(withFailedDeleteByName(errDefault)),
 			ExpectedError:    "unexpected error deleting repo",
 		},
 		{
 			Name:             "delete by ID fails when repo service returns error",
 			RepoID:           repoID,
-			RepoServiceSetup: newRepoService(withSuccessfulGetRepoById, withFailedDelete(errDefault)),
+			RepoServiceSetup: newRepoService(withFailedDeleteByID(errDefault)),
 			ExpectedError:    "unexpected error deleting repo",
 		},
+		{
+			Name:             "deletion fails when repo is not found",
+			RepoName:         repoOwnerAndName,
+			RepoServiceSetup: newRepoService(withFailedDeleteByName(sql.ErrNoRows)),
+			ExpectedError:    "repository not found",
+		},
+		{
+			Name:             "delete by ID fails when repo is not found",
+			RepoID:           repoID,
+			RepoServiceSetup: newRepoService(withFailedDeleteByID(sql.ErrNoRows)),
+			ExpectedError:    "repository not found",
+		},
 		{
 			Name:             "delete by name succeeds",
 			RepoName:         repoOwnerAndName,
-			RepoServiceSetup: newRepoService(withSuccessfulGetRepoByName, withSuccessfulDelete),
+			RepoServiceSetup: newRepoService(withSuccessfulDeleteByName),
 		},
 		{
 			Name:             "delete by ID succeeds",
 			RepoID:           repoID,
-			RepoServiceSetup: newRepoService(withSuccessfulGetRepoById, withSuccessfulDelete),
+			RepoServiceSetup: newRepoService(withSuccessfulDeleteByID),
 		},
 	}
 
@@ -382,35 +381,27 @@ func withFailedCreate(err error) func(repoServiceMock) {
 	}
 }
 
-func withSuccessfulDelete(mock repoServiceMock) {
-	withFailedDelete(nil)(mock)
+func withSuccessfulDeleteByID(mock repoServiceMock) {
+	withFailedDeleteByID(nil)(mock)
 }
 
-func withFailedDelete(err error) func(repoServiceMock) {
+func withFailedDeleteByID(err error) func(repoServiceMock) {
 	return func(mock repoServiceMock) {
 		mock.EXPECT().
-			DeleteRepository(gomock.Any(), gomock.Any(), gomock.Any()).
+			DeleteByID(gomock.Any(), gomock.Any(), gomock.Any()).
 			Return(err)
 	}
 }
 
-func withSuccessfulGetRepoById(mock repoServiceMock) {
-	mock.EXPECT().
-		GetRepositoryById(gomock.Any(), gomock.Any(), gomock.Any()).
-		Return(db.Repository{}, nil)
+func withSuccessfulDeleteByName(mock repoServiceMock) {
+	withFailedDeleteByName(nil)(mock)
 }
 
-func withSuccessfulGetRepoByName(mock repoServiceMock) {
-	mock.EXPECT().
-		GetRepositoryByName(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
-		Return(db.Repository{}, nil)
-}
-
-func withFailedGetRepoByName(err error) func(repoServiceMock) {
+func withFailedDeleteByName(err error) func(repoServiceMock) {
 	return func(mock repoServiceMock) {
 		mock.EXPECT().
-			GetRepositoryByName(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
-			Return(db.Repository{}, err)
+			DeleteByName(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+			Return(err)
 	}
 }
 

internal/controlplane/server.go

@@ -181,7 +181,6 @@ func NewServer(
 		return nil, fmt.Errorf("failed to create marketplace: %w", err)
 	}
 	fallbackTokenClient := ghprov.NewFallbackTokenClient(cfg.Provider)
-	repoSvc := github.NewRepositoryService(whManager, store, evt)
 
 	s := &Server{
 		store:               store,
@@ -194,7 +193,6 @@ func NewServer(
 		provMt:              provMt,
 		profiles:            profileSvc,
 		ruleTypes:           ruleSvc,
-		repos:               repoSvc,
 		marketplace:         marketplace,
 		providerStore:       providerStore,
 		featureFlags:        openfeature.NewClient(cfg.Flags.AppName),
@@ -223,7 +221,7 @@ func NewServer(
 		&cfg.Provider,
 		fallbackTokenClient,
 		eng,
-		repoSvc,
+		whManager,
 		store,
 		s.ghProviders,
 	)
@@ -232,6 +230,7 @@ func NewServer(
 		return nil, fmt.Errorf("failed to create provider manager: %w", err)
 	}
 	s.providerManager = providerManager
+	s.repos = github.NewRepositoryService(whManager, store, evt, providerManager)
 
 	return s, nil
 }