Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass Provider ID to the GetEntity calls from the provider service #4343

Merged
merged 2 commits into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion database/query/entities.sql
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,11 @@ LIMIT 1;
-- GetEntityByName retrieves an entity by its name for a project or hierarchy of projects.
-- name: GetEntityByName :one
SELECT * FROM entity_instances
WHERE entity_instances.name = sqlc.arg(name) AND entity_instances.project_id = $1 AND entity_instances.entity_type = $2
WHERE
entity_instances.name = sqlc.arg(name)
AND entity_instances.project_id = $1
AND entity_instances.entity_type = $2
AND entity_instances.provider_id = sqlc.arg(provider_id)
LIMIT 1;

-- GetEntitiesByType retrieves all entities of a given type for a project or hierarchy of projects.
Expand Down
4 changes: 3 additions & 1 deletion internal/controlplane/handlers_githubwebhooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -1628,7 +1628,9 @@ func (s *Server) updatePullRequestInfoFromProvider(
return fmt.Errorf("error creating properties: %w", err)
}

_, err = s.props.RetrieveAllProperties(ctx, provider, repoEnt.Entity.ProjectID, lookupProperties, pb.Entity_ENTITY_PULL_REQUESTS)
_, err = s.props.RetrieveAllProperties(ctx, provider,
repoEnt.Entity.ProjectID, repoEnt.Entity.ProviderID,
lookupProperties, pb.Entity_ENTITY_PULL_REQUESTS)
if err != nil {
return fmt.Errorf("error retrieving properties: %w", err)
}
Expand Down
2 changes: 1 addition & 1 deletion internal/controlplane/handlers_githubwebhooks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func newPropSvcMock(opts ...func(mck propSvcMock)) propSvcMockBuilder {
func withSuccessRetrieveAllProperties(entity v1.Entity, retProps *properties.Properties) func(mck propSvcMock) {
return func(mock propSvcMock) {
mock.EXPECT().
RetrieveAllProperties(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), entity).
RetrieveAllProperties(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), entity).
Return(retProps, nil)
}
}
Expand Down
14 changes: 12 additions & 2 deletions internal/db/entities.sql.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions internal/db/entities_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ func Test_EntityCrud(t *testing.T) {
ProjectID: proj.ID,
Name: "garbage/nosuchentity",
EntityType: EntitiesRepository,
ProviderID: prov.ID,
})
require.ErrorIs(t, err, sql.ErrNoRows)
require.Empty(t, ent)
Expand Down Expand Up @@ -127,6 +128,7 @@ func Test_EntityCrud(t *testing.T) {
ProjectID: proj.ID,
Name: testEntName,
EntityType: EntitiesRepository,
ProviderID: prov.ID,
})
require.NoError(t, err)
require.NotEmpty(t, getRepo)
Expand All @@ -136,6 +138,7 @@ func Test_EntityCrud(t *testing.T) {
ProjectID: proj.ID,
Name: testEntName,
EntityType: EntitiesRepository,
ProviderID: prov.ID,
})
require.NoError(t, err)
require.NotEmpty(t, getRepo)
Expand Down
16 changes: 8 additions & 8 deletions internal/entities/properties/service/mock/service.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 10 additions & 3 deletions internal/entities/properties/service/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,13 @@ type PropertiesService interface {
// RetrieveAllProperties fetches all properties for the given entity
RetrieveAllProperties(
ctx context.Context, provider v1.Provider, projectId uuid.UUID,
providerID uuid.UUID,
lookupProperties *properties.Properties, entType minderv1.Entity,
) (*properties.Properties, error)
// RetrieveProperty fetches a single property for the given entity
RetrieveProperty(
ctx context.Context, provider v1.Provider, projectId uuid.UUID,
providerID uuid.UUID,
lookupProperties *properties.Properties, entType minderv1.Entity, key string,
) (*properties.Property, error)
// ReplaceAllProperties saves all properties for the given entity
Expand Down Expand Up @@ -97,10 +99,11 @@ func NewPropertiesService(
// RetrieveAllProperties fetches a single property for the given entity
func (ps *propertiesService) RetrieveAllProperties(
ctx context.Context, provider v1.Provider, projectId uuid.UUID,
providerID uuid.UUID,
lookupProperties *properties.Properties, entType minderv1.Entity,
) (*properties.Properties, error) {
// fetch the entity first. If there's no entity, there's no properties, go straight to provider
entID, err := ps.getEntityIdByProperties(ctx, projectId, lookupProperties, entType)
entID, err := ps.getEntityIdByProperties(ctx, projectId, providerID, lookupProperties, entType)
if err != nil && !errors.Is(err, sql.ErrNoRows) {
return nil, fmt.Errorf("failed to get entity ID: %w", err)
}
Expand Down Expand Up @@ -156,10 +159,11 @@ func (ps *propertiesService) RetrieveAllProperties(
// RetrieveProperty fetches a single property for the given entity
func (ps *propertiesService) RetrieveProperty(
ctx context.Context, provider v1.Provider, projectId uuid.UUID,
providerID uuid.UUID,
lookupProperties *properties.Properties, entType minderv1.Entity, key string,
) (*properties.Property, error) {
// fetch the entity first. If there's no entity, there's no properties, go straight to provider
entID, err := ps.getEntityIdByProperties(ctx, projectId, lookupProperties, entType)
entID, err := ps.getEntityIdByProperties(ctx, projectId, providerID, lookupProperties, entType)
if err != nil && !errors.Is(err, sql.ErrNoRows) {
return nil, err
}
Expand Down Expand Up @@ -192,12 +196,13 @@ func (ps *propertiesService) RetrieveProperty(

func (ps *propertiesService) getEntityIdByProperties(
ctx context.Context, projectId uuid.UUID,
providerID uuid.UUID,
props *properties.Properties, entType minderv1.Entity,
) (uuid.UUID, error) {
// TODO: Add more ways to look up a property, e.g. by the upstream ID
name := props.GetProperty(properties.PropertyName)
if name != nil {
return ps.getEntityIdByName(ctx, projectId, name.GetString(), entType)
return ps.getEntityIdByName(ctx, projectId, providerID, name.GetString(), entType)
}

// returning nil ID and nil error would make us just go to the provider. Slow, but we'd continue.
Expand All @@ -206,12 +211,14 @@ func (ps *propertiesService) getEntityIdByProperties(

func (ps *propertiesService) getEntityIdByName(
ctx context.Context, projectId uuid.UUID,
providerID uuid.UUID,
name string, entType minderv1.Entity,
) (uuid.UUID, error) {
ent, err := ps.store.GetEntityByName(ctx, db.GetEntityByNameParams{
ProjectID: projectId,
Name: name,
EntityType: entities.EntityTypeToDB(entType),
ProviderID: providerID,
})
if err != nil {
return uuid.Nil, fmt.Errorf("failed to get entity by name: %w", err)
Expand Down
4 changes: 2 additions & 2 deletions internal/entities/properties/service/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,7 @@ func TestPropertiesService_RetrieveProperty(t *testing.T) {
})
require.NoError(t, err)

gotProps, err := propSvc.RetrieveProperty(ctx, githubMock, tctx.dbProj.ID, getByProps, tt.params.entType, tt.propName)
gotProps, err := propSvc.RetrieveProperty(ctx, githubMock, tctx.dbProj.ID, tctx.ghAppProvider.ID, getByProps, tt.params.entType, tt.propName)

if tt.expectErr != "" {
require.Contains(t, err.Error(), tt.expectErr)
Expand Down Expand Up @@ -706,7 +706,7 @@ func TestPropertiesService_RetrieveAllProperties(t *testing.T) {
})
require.NoError(t, err)

gotProps, err := propSvc.RetrieveAllProperties(ctx, githubMock, tctx.dbProj.ID, getByProps, tt.params.entType)
gotProps, err := propSvc.RetrieveAllProperties(ctx, githubMock, tctx.dbProj.ID, tctx.ghAppProvider.ID, getByProps, tt.params.entType)

if tt.expectErr != "" {
require.Contains(t, err.Error(), tt.expectErr)
Expand Down
2 changes: 2 additions & 0 deletions internal/repositories/github/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ func (r *repositoryService) CreateRepository(
ctx,
propClient,
projectID,
provider.ID,
fetchByProps,
pb.Entity_ENTITY_REPOSITORIES)
if err != nil {
Expand Down Expand Up @@ -370,6 +371,7 @@ func (r *repositoryService) RefreshRepositoryByUpstreamID(
ctx,
prov,
entRepo.ProjectID,
entRepo.ProviderID,
fetchByProps,
pb.Entity_ENTITY_REPOSITORIES)
if errors.Is(err, provifv1.ErrEntityNotFound) {
Expand Down
4 changes: 2 additions & 2 deletions internal/repositories/github/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -644,14 +644,14 @@ func withSuccessfulReplaceProps(mock propSvcMock) {

func withFailingGet(mock propSvcMock) {
mock.EXPECT().
RetrieveAllProperties(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
RetrieveAllProperties(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
Return(nil, errDefault)
}

func withSuccessfulPropFetch(prop *properties.Properties) func(svcMock propSvcMock) {
return func(mock propSvcMock) {
mock.EXPECT().
RetrieveAllProperties(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
RetrieveAllProperties(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
Return(prop, nil)
}
}