Use central entity tables to list remote repos (#4429)

This is part of generalizing entity registration, as other providers will need to work with this mechanism. As part of the migration, this moved the logic to use our new entity model. Related-To: #4331 Signed-off-by: Juan Antonio Osorio <[email protected]>
mindersec · Sep 12, 2024 · ab5af43 · ab5af43
1 parent 50e6060
commit ab5af43
Show file tree

Hide file tree

Showing 12 changed files with 169 additions and 80 deletions.
diff --git a/database/query/entities.sql b/database/query/entities.sql
@@ -74,7 +74,9 @@ LIMIT 1;
 
 -- name: GetEntitiesByType :many
 SELECT * FROM entity_instances
-WHERE entity_instances.entity_type = $1 AND entity_instances.project_id = ANY(sqlc.arg(projects)::uuid[]);
+WHERE entity_instances.entity_type = $1
+    AND entity_instances.provider_id = sqlc.arg(provider_id)
+    AND entity_instances.project_id = ANY(sqlc.arg(projects)::uuid[]);
 
 -- GetEntitiesByProvider retrieves all entities of a given provider.
 -- this is how one would get all repositories, artifacts, etc. for a given provider.

diff --git a/internal/controlplane/handlers_entities.go b/internal/controlplane/handlers_entities.go
@@ -55,8 +55,9 @@ func (s *Server) ReconcileEntityRegistration(
 		return nil, util.UserVisibleError(codes.InvalidArgument, "entity type %s not supported", entityType)
 	}
 
-	providerName := in.GetContext().GetProvider()
-	provs, errorProvs, err := s.providerManager.BulkInstantiateByTrait(ctx, projectID, db.ProviderTypeRepoLister, providerName)
+	providerNameParam := in.GetContext().GetProvider()
+	provs, errorProvs, err := s.providerManager.BulkInstantiateByTrait(
+		ctx, projectID, db.ProviderTypeRepoLister, providerNameParam)
 	if err != nil {
 		pErr := providers.ErrProviderNotFoundBy{}
 		if errors.As(err, &pErr) {
@@ -65,22 +66,15 @@ func (s *Server) ReconcileEntityRegistration(
 		return nil, providerError(err)
 	}
 
-	for providerName, provider := range provs {
-		// Explicitly fetch the provider here as we need its ID for posting the event.
-		pvr, err := s.providerStore.GetByName(ctx, projectID, providerName)
-		if err != nil {
-			errorProvs = append(errorProvs, providerName)
-			continue
-		}
-
-		repos, err := s.fetchRepositoriesForProvider(ctx, projectID, providerName, provider)
+	for providerID, providerT := range provs {
+		repos, err := s.fetchRepositoriesForProvider(ctx, projectID, providerID, providerT.Name, providerT.Provider)
 		if err != nil {
 			l.Error().
-				Str("providerName", providerName).
+				Str("providerName", providerT.Name).
 				Str("projectID", projectID.String()).
 				Err(err).
 				Msg("error fetching repositories for provider")
-			errorProvs = append(errorProvs, providerName)
+			errorProvs = append(errorProvs, providerT.Name)
 			continue
 		}
 
@@ -89,11 +83,11 @@ func (s *Server) ReconcileEntityRegistration(
 				continue
 			}
 
-			msg, err := createEntityMessage(ctx, &l, projectID, pvr.ID, repo.GetName(), repo.GetOwner())
+			msg, err := createEntityMessage(ctx, &l, projectID, providerID, repo.GetName(), repo.GetOwner())
 			if err != nil {
 				l.Error().Err(err).
 					Int64("repoID", repo.RepoId).
-					Str("providerName", providerName).
+					Str("providerName", providerT.Name).
 					Msg("error creating registration entity message")
 				// This message will not be sent, but we can continue with the rest.
 				continue

diff --git a/internal/controlplane/handlers_entities_test.go b/internal/controlplane/handlers_entities_test.go
@@ -18,17 +18,18 @@ import (
 	"context"
 	"testing"
 
+	"github.com/google/uuid"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/mock/gomock"
 
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/engine/engcontext"
 	mockevents "github.com/stacklok/minder/internal/events/mock"
 	mockgh "github.com/stacklok/minder/internal/providers/github/mock"
+	"github.com/stacklok/minder/internal/providers/manager"
 	mockmanager "github.com/stacklok/minder/internal/providers/manager/mock"
 	rf "github.com/stacklok/minder/internal/repositories/mock/fixtures"
 	pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
-	provinfv1 "github.com/stacklok/minder/pkg/providers/v1"
 )
 
 func TestServer_ReconcileEntityRegistration(t *testing.T) {
@@ -58,7 +59,12 @@ func TestServer_ReconcileEntityRegistration(t *testing.T) {
 				providerManager := mockmanager.NewMockProviderManager(ctrl)
 				providerManager.EXPECT().BulkInstantiateByTrait(
 					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(),
-				).Return(map[string]provinfv1.Provider{provider.Name: mockgh.NewMockGitHub(ctrl)}, []string{}, nil).Times(1)
+				).Return(map[uuid.UUID]manager.NameProviderTuple{
+					uuid.New(): {
+						Name:     provider.Name,
+						Provider: mockgh.NewMockGitHub(ctrl),
+					},
+				}, []string{}, nil).Times(1)
 				return providerManager
 			},
 			EventerSetup: func(ctrl *gomock.Controller) *mockevents.MockInterface {
@@ -80,7 +86,12 @@ func TestServer_ReconcileEntityRegistration(t *testing.T) {
 				providerManager := mockmanager.NewMockProviderManager(ctrl)
 				providerManager.EXPECT().BulkInstantiateByTrait(
 					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(),
-				).Return(map[string]provinfv1.Provider{provider.Name: mockgh.NewMockGitHub(ctrl)}, []string{}, nil).Times(1)
+				).Return(map[uuid.UUID]manager.NameProviderTuple{
+					uuid.New(): {
+						Name:     provider.Name,
+						Provider: mockgh.NewMockGitHub(ctrl),
+					},
+				}, []string{}, nil).Times(1)
 				return providerManager
 			},
 			ExpectedError: "cannot register entities for providers: [github]",
@@ -101,22 +112,27 @@ func TestServer_ReconcileEntityRegistration(t *testing.T) {
 				Project: engcontext.Project{ID: projectID},
 			})
 
-			manager := mockmanager.NewMockProviderManager(ctrl)
+			mgr := mockmanager.NewMockProviderManager(ctrl)
 			if scenario.ProviderSetup != nil && scenario.GitHubSetup != nil {
 				prov := scenario.GitHubSetup(ctrl)
-				manager.EXPECT().BulkInstantiateByTrait(
+				mgr.EXPECT().BulkInstantiateByTrait(
 					gomock.Any(),
 					gomock.Eq(projectID),
 					gomock.Eq(db.ProviderTypeRepoLister),
 					gomock.Eq(""),
-				).Return(map[string]provinfv1.Provider{provider.Name: prov}, []string{}, nil)
+				).Return(map[uuid.UUID]manager.NameProviderTuple{
+					uuid.New(): {
+						Name:     provider.Name,
+						Provider: prov,
+					},
+				}, []string{}, nil)
 			}
 
 			server := createServer(
 				ctrl,
 				scenario.RepoServiceSetup,
 				scenario.ProviderFails,
-				manager,
+				mgr,
 			)
 
 			if scenario.EventerSetup != nil {

diff --git a/internal/controlplane/handlers_repositories.go b/internal/controlplane/handlers_repositories.go
@@ -29,6 +29,7 @@ import (
 
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/engine/engcontext"
+	"github.com/stacklok/minder/internal/entities/properties"
 	"github.com/stacklok/minder/internal/logger"
 	"github.com/stacklok/minder/internal/projects/features"
 	"github.com/stacklok/minder/internal/providers"
@@ -318,7 +319,8 @@ func (s *Server) ListRemoteRepositoriesFromProvider(
 	logger.BusinessRecord(ctx).Project = projectID
 
 	providerName := in.GetContext().GetProvider()
-	provs, errorProvs, err := s.providerManager.BulkInstantiateByTrait(ctx, projectID, db.ProviderTypeRepoLister, providerName)
+	provs, errorProvs, err := s.providerManager.BulkInstantiateByTrait(
+		ctx, projectID, db.ProviderTypeRepoLister, providerName)
 	if err != nil {
 		pErr := providers.ErrProviderNotFoundBy{}
 		if errors.As(err, &pErr) {
@@ -331,11 +333,13 @@ func (s *Server) ListRemoteRepositoriesFromProvider(
 		Results: []*pb.UpstreamRepositoryRef{},
 	}
 
-	for providerName, provider := range provs {
-		results, err := s.fetchRepositoriesForProvider(ctx, projectID, providerName, provider)
+	for providerID, providerT := range provs {
+		results, err := s.fetchRepositoriesForProvider(
+			ctx, projectID, providerID, providerT.Name, providerT.Provider)
 		if err != nil {
-			zerolog.Ctx(ctx).Error().Err(err).Msgf("error listing repositories for provider %s in project %s", providerName, projectID)
-			errorProvs = append(errorProvs, providerName)
+			zerolog.Ctx(ctx).Error().Err(err).
+				Msgf("error listing repositories for provider %s in project %s", providerT.Name, projectID)
+			errorProvs = append(errorProvs, providerT.Name)
 			continue
 		}
 		out.Results = append(out.Results, results...)
@@ -355,11 +359,12 @@ func (s *Server) ListRemoteRepositoriesFromProvider(
 func (s *Server) fetchRepositoriesForProvider(
 	ctx context.Context,
 	projectID uuid.UUID,
+	providerID uuid.UUID,
 	providerName string,
 	provider v1.Provider,
 ) ([]*pb.UpstreamRepositoryRef, error) {
 	zerolog.Ctx(ctx).Trace().
-		Str("provider", providerName).
+		Str("provider_id", providerID.String()).
 		Str("project_id", projectID.String()).
 		Msg("listing repositories")
 
@@ -378,26 +383,39 @@ func (s *Server) fetchRepositoriesForProvider(
 	registeredRepos, err := s.repos.ListRepositories(
 		ctx,
 		projectID,
-		providerName,
+		providerID,
 	)
 	if err != nil {
 		zerolog.Ctx(ctx).Error().
-			Str("projectID", projectID.String()).
-			Str("providerName", providerName).
+			Str("project_id", projectID.String()).
+			Str("provider_id", providerID.String()).
 			Err(err).Msg("cannot list registered repositories")
 		return nil, util.UserVisibleError(
 			codes.Internal,
 			"cannot list registered repositories",
 		)
 	}
 
-	registered := make(map[int64]bool)
+	registered := make(map[string]bool)
 	for _, repo := range registeredRepos {
-		registered[repo.RepoID] = true
+		uidP := repo.Properties.GetProperty(properties.PropertyUpstreamID)
+		if uidP == nil {
+			zerolog.Ctx(ctx).Warn().
+				Str("entity_id", repo.Entity.ID.String()).
+				Str("entity_name", repo.Entity.Name).
+				Str("provider_id", providerID.String()).
+				Str("project_id", projectID.String()).
+				Msg("repository has no upstream ID")
+			continue
+		}
+		registered[uidP.GetString()] = true
 	}
 
 	for _, result := range results {
-		result.Registered = registered[result.RepoId]
+		// TEMPORARY: This will be changed to use properties.
+		// for now, we transform the repo ID to a string
+		uid := fmt.Sprintf("%d", result.RepoId)
+		result.Registered = registered[uid]
 	}
 
 	return results, nil

diff --git a/internal/controlplane/handlers_repositories_test.go b/internal/controlplane/handlers_repositories_test.go
@@ -19,6 +19,7 @@ import (
 	"database/sql"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"testing"
 
 	"github.com/google/uuid"
@@ -30,6 +31,8 @@ import (
 	"github.com/stacklok/minder/internal/config/server"
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/engine/engcontext"
+	"github.com/stacklok/minder/internal/entities/models"
+	"github.com/stacklok/minder/internal/entities/properties"
 	"github.com/stacklok/minder/internal/providers"
 	ghprovider "github.com/stacklok/minder/internal/providers/github/clients"
 	mockgh "github.com/stacklok/minder/internal/providers/github/mock"
@@ -223,19 +226,24 @@ func TestServer_ListRemoteRepositoriesFromProvider(t *testing.T) {
 			})
 
 			prov := scenario.GitHubSetup(ctrl)
-			manager := mockmanager.NewMockProviderManager(ctrl)
-			manager.EXPECT().BulkInstantiateByTrait(
+			mgr := mockmanager.NewMockProviderManager(ctrl)
+			mgr.EXPECT().BulkInstantiateByTrait(
 				gomock.Any(),
 				gomock.Eq(projectID),
 				gomock.Eq(db.ProviderTypeRepoLister),
 				gomock.Eq(""),
-			).Return(map[string]provinfv1.Provider{provider.Name: prov}, []string{}, nil)
+			).Return(map[uuid.UUID]manager.NameProviderTuple{
+				provider.ID: {
+					Name:     provider.Name,
+					Provider: prov,
+				},
+			}, []string{}, nil)
 
 			server := createServer(
 				ctrl,
 				scenario.RepoServiceSetup,
 				scenario.ProviderFails,
-				manager,
+				mgr,
 			)
 
 			projectIDStr := projectID.String()
@@ -420,14 +428,16 @@ var (
 	}
 )
 
-func simpleDbRepository(name string, id int64) db.Repository {
-	return db.Repository{
-		ID:        uuid.UUID{},
-		Provider:  ghprovider.Github,
-		RepoOwner: repoOwner,
-		RepoName:  name,
-		RepoID:    id,
-	}
+func simpleDbRepository(name string, id int64) *models.EntityWithProperties {
+	//nolint:errcheck // this shouldn't fail
+	props, _ := properties.NewProperties(map[string]any{
+		"repo_id":                     id,
+		properties.PropertyUpstreamID: fmt.Sprintf("%d", id),
+	})
+	return models.NewEntityWithPropertiesFromInstance(models.EntityInstance{
+		ID:   uuid.UUID{},
+		Name: name,
+	}, props)
 }
 
 func simpleUpstreamRepositoryRef(name string, id int64, registered bool) *pb.UpstreamRepositoryRef {

diff --git a/internal/db/entities.sql.go b/internal/db/entities.sql.go
diff --git a/internal/providers/manager/manager.go b/internal/providers/manager/manager.go
@@ -33,6 +33,12 @@ import (
 
 //go:generate go run go.uber.org/mock/mockgen -package mock_$GOPACKAGE -destination=./mock/$GOFILE -source=./$GOFILE
 
+// NameProviderTuple is a tuple of a provider name and the provider instance
+type NameProviderTuple struct {
+	Name     string
+	Provider v1.Provider
+}
+
 // ProviderManager encapsulates operations for manipulating Provider instances
 type ProviderManager interface {
 	// CreateFromConfig creates a new Provider instance in the database with a given configuration or the provider default
@@ -55,7 +61,7 @@ type ProviderManager interface {
 		projectID uuid.UUID,
 		trait db.ProviderType,
 		name string,
-	) (map[string]v1.Provider, []string, error)
+	) (map[uuid.UUID]NameProviderTuple, []string, error)
 	// DeleteByID deletes the specified instance of the Provider, and
 	// carries out any cleanup needed.
 	DeleteByID(ctx context.Context, providerID uuid.UUID, projectID uuid.UUID) error
@@ -217,13 +223,13 @@ func (p *providerManager) BulkInstantiateByTrait(
 	projectID uuid.UUID,
 	trait db.ProviderType,
 	name string,
-) (map[string]v1.Provider, []string, error) {
+) (map[uuid.UUID]NameProviderTuple, []string, error) {
 	providerConfigs, err := p.store.GetByTraitInHierarchy(ctx, projectID, name, trait)
 	if err != nil {
 		return nil, nil, fmt.Errorf("error retrieving db records: %w", err)
 	}
 
-	result := make(map[string]v1.Provider, len(providerConfigs))
+	result := make(map[uuid.UUID]NameProviderTuple, len(providerConfigs))
 	failedProviders := []string{}
 	for _, config := range providerConfigs {
 		provider, err := p.buildFromDBRecord(ctx, &config)
@@ -232,7 +238,10 @@ func (p *providerManager) BulkInstantiateByTrait(
 			failedProviders = append(failedProviders, config.Name)
 			continue
 		}
-		result[config.Name] = provider
+		result[config.ID] = NameProviderTuple{
+			Name:     config.Name,
+			Provider: provider,
+		}
 	}
 
 	return result, failedProviders, nil

diff --git a/internal/providers/manager/manager_test.go b/internal/providers/manager/manager_test.go
@@ -379,7 +379,10 @@ func TestProviderManager_BulkInstantiateByTrait(t *testing.T) {
 			} else {
 				require.Len(t, success, 1)
 				require.Empty(t, fail)
-				require.Equal(t, provider, success[scenario.Provider.Name])
+				require.Equal(t, manager.NameProviderTuple{
+					Name:     scenario.Provider.Name,
+					Provider: provider,
+				}, success[scenario.Provider.ID])
 			}
 		})
 	}