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.

Sign up for GitHub

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

Remove unused RetrieveProperty function #5014

Merged
merged 4 commits into from
Nov 20, 2024
Merged

Remove unused RetrieveProperty function #5014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
67bf2c6
Remove unused RetrieveProperty function
navnitms Nov 20, 2024
58e9392
Merge branch 'main' into remove-unused-RetrieveProperty
navnitms Nov 20, 2024
4e87be8
Remove unused withUpstreamRepoProperty function
navnitms Nov 20, 2024
d44adee
Remove unused 'other' field from fetchParams struct
navnitms Nov 20, 2024
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
15 changes: 0 additions & 15 deletions internal/entities/properties/service/mock/service.go
View file
Open in desktop

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

57 changes: 0 additions & 57 deletions internal/entities/properties/service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,6 @@ type PropertiesService interface {
RetrieveAllPropertiesForEntity(ctx context.Context, efp *models.EntityWithProperties,
provMan manager.ProviderManager, opts *ReadOptions,
) error
// RetrieveProperty fetches a single property for the given entity given
// a project, provider, and identifying properties.
RetrieveProperty(
ctx context.Context, provider provifv1.Provider, projectId uuid.UUID,
providerID uuid.UUID,
lookupProperties *properties.Properties, entType minderv1.Entity, key string,
opts *ReadOptions,
) (*properties.Property, error)
// ReplaceAllProperties saves all properties for the given entity
ReplaceAllProperties(
ctx context.Context, entityID uuid.UUID, props *properties.Properties, opts *CallOptions,
Expand Down Expand Up @@ -181,55 +173,6 @@ func (ps *propertiesService) RetrieveAllPropertiesForEntity(
return nil
}

// RetrieveProperty fetches a single property for the given entity
func (ps *propertiesService) RetrieveProperty(
ctx context.Context, provider provifv1.Provider, projectId uuid.UUID,
providerID uuid.UUID,
lookupProperties *properties.Properties, entType minderv1.Entity, key string,
opts *ReadOptions,
) (*properties.Property, error) {
l := zerolog.Ctx(ctx).With().
Str("projectID", projectId.String()).
Str("providerID", providerID.String()).
Str("entityType", entType.String()).
Logger()

// fetch the entity first. If there's no entity, there's no properties, go straight to provider
entID, err := getEntityIdByProperties(ctx, projectId, providerID, lookupProperties, entType, ps.store)
if err != nil && !errors.Is(err, ErrEntityNotFound) {
return nil, err
}

// fetch properties from db
var dbProp db.Property
if entID != uuid.Nil {
dbProp, err = ps.store.GetProperty(ctx, db.GetPropertyParams{
EntityID: entID,
Key: key,
})
if err != nil && !errors.Is(err, ErrPropertyNotFound) {
return nil, err
}
} else {
l.Info().Msg("no entity found, skipping properties fetch")
}

// if exists, turn into our model
if ps.isDatabasePropertyValid(dbProp, opts) {
return models.DbPropToModel(dbProp)
}

// if not, fetch from provider
prop, err := provider.FetchProperty(ctx, lookupProperties, entType, key)
if errors.Is(err, provifv1.ErrEntityNotFound) {
return nil, fmt.Errorf("failed to fetch upstream property: %w", ErrEntityNotFound)
} else if err != nil {
return nil, err
}

return prop, nil
}

func (ps *propertiesService) ReplaceAllProperties(
ctx context.Context, entityID uuid.UUID, props *properties.Properties,
opts *CallOptions,
Expand Down
204 changes: 0 additions & 204 deletions internal/entities/properties/service/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,6 @@ func withUpstreamRepoProperties(repoProperties map[string]any, entType minderv1.
}
}

func withUpstreamRepoProperty(key string, val any, entType minderv1.Entity) func(mock *mock_github.MockGitHub) {
return func(mock *mock_github.MockGitHub) {
prop, err := properties.NewProperty(val)
if err != nil {
panic(err)
}
mock.EXPECT().
FetchProperty(gomock.Any(), gomock.Any(), entType, key).
Return(prop, nil)
}
}

func insertProperties(ctx context.Context, t *testing.T, store db.Store, entID uuid.UUID, props *properties.Properties) {
t.Helper()

Expand Down Expand Up @@ -95,8 +83,6 @@ type fetchParams struct {

providerID uuid.UUID
projectID uuid.UUID

other map[string]any
}

type testCtx struct {
Expand Down Expand Up @@ -415,196 +401,6 @@ func TestPropertiesService_SaveAllProperties(t *testing.T) {
}
}

func TestPropertiesService_RetrieveProperty(t *testing.T) {
t.Parallel()

seed := time.Now().UnixNano()

scenarios := []struct {
name string
propName string
dbSetup func(t *testing.T, store db.Store, params fetchParams)
githubSetup func(params fetchParams) githubMockBuilder
params fetchParams
expectErr string
checkResult func(t *testing.T, props *properties.Property)
opts []propertiesServiceOption
}{
{
name: "No cache, fetch from provider",
propName: properties.RepoPropertyIsPrivate,
dbSetup: func(_ *testing.T, _ db.Store, _ fetchParams) {
},
githubSetup: func(params fetchParams) githubMockBuilder {
return newGithubMock(
withUpstreamRepoProperty(properties.RepoPropertyIsPrivate, true, params.entType),
)
},
params: fetchParams{
entType: minderv1.Entity_ENTITY_REPOSITORIES,
entName: rand.RandomName(seed),
},
checkResult: func(t *testing.T, prop *properties.Property) {
t.Helper()
require.Equal(t, prop.GetBool(), true)
},
},
{
name: "Cache miss, fetch from provider",
propName: ghprop.RepoPropertyId,
dbSetup: func(t *testing.T, store db.Store, params fetchParams) {
t.Helper()
ent, err := store.CreateEntity(context.TODO(), db.CreateEntityParams{
EntityType: entities.EntityTypeToDB(params.entType),
Name: params.entName,
ProjectID: params.projectID,
ProviderID: params.providerID,
})
require.NoError(t, err)

// these are different than tt.params.properties
oldPropMap := map[string]any{
ghprop.RepoPropertyId: int64(1234),
}
insertPropertiesFromMap(context.TODO(), t, store, ent.ID, oldPropMap)
},
githubSetup: func(params fetchParams) githubMockBuilder {
t.Helper()
return newGithubMock(
withUpstreamRepoProperty(ghprop.RepoPropertyId, int64(123), params.entType),
)
},
params: fetchParams{
entType: minderv1.Entity_ENTITY_REPOSITORIES,
entName: rand.RandomName(seed),
},
checkResult: func(t *testing.T, prop *properties.Property) {
t.Helper()
require.Equal(t, prop.GetInt64(), int64(123))
},
opts: []propertiesServiceOption{
WithEntityTimeout(bypassCacheTimeout),
},
},
{
name: "Cache hit by name, fetch from cache",
propName: ghprop.RepoPropertyId,
dbSetup: func(t *testing.T, store db.Store, params fetchParams) {
t.Helper()

ent, err := store.CreateEntity(context.TODO(), db.CreateEntityParams{
EntityType: entities.EntityTypeToDB(params.entType),
Name: params.entName,
ProjectID: params.projectID,
ProviderID: params.providerID,
})
require.NoError(t, err)

propMap := map[string]any{
ghprop.RepoPropertyId: int64(123),
}
props, err := properties.NewProperties(propMap)
require.NoError(t, err)
insertProperties(context.TODO(), t, store, ent.ID, props)
},
githubSetup: func(_ fetchParams) githubMockBuilder {
return newGithubMock()
},
params: fetchParams{
entType: minderv1.Entity_ENTITY_REPOSITORIES,
entName: rand.RandomName(seed),
},
checkResult: func(t *testing.T, prop *properties.Property) {
t.Helper()

require.Equal(t, prop.GetInt64(), int64(123))
},
},
{
name: "Cache hit by upstream ID, fetch from cache",
propName: properties.RepoPropertyIsArchived,
dbSetup: func(t *testing.T, store db.Store, params fetchParams) {
t.Helper()

ent, err := store.CreateEntity(context.TODO(), db.CreateEntityParams{
EntityType: entities.EntityTypeToDB(params.entType),
Name: params.entName,
ProjectID: params.projectID,
ProviderID: params.providerID,
})
require.NoError(t, err)

propMap := map[string]any{
properties.PropertyUpstreamID: "this is an upstream ID",
properties.RepoPropertyIsArchived: true,
}
props, err := properties.NewProperties(propMap)
require.NoError(t, err)
insertProperties(context.TODO(), t, store, ent.ID, props)
},
githubSetup: func(_ fetchParams) githubMockBuilder {
return newGithubMock()
},
params: fetchParams{
entType: minderv1.Entity_ENTITY_REPOSITORIES,
other: map[string]any{
properties.PropertyUpstreamID: "this is an upstream ID",
},
},
checkResult: func(t *testing.T, prop *properties.Property) {
t.Helper()

// This is checking for IsArchived
require.Equal(t, prop.GetBool(), true)
},
},
}

ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)

tctx := createTestCtx(ctx, t)

for _, tt := range scenarios {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

ctrl := gomock.NewController(t)
t.Cleanup(ctrl.Finish)

tt.params.providerID = tctx.ghAppProvider.ID
tt.params.projectID = tctx.dbProj.ID

githubSetup := tt.githubSetup(tt.params)
githubMock := githubSetup(ctrl)

tt.dbSetup(t, tctx.testQueries, tt.params)

propSvc := NewPropertiesService(tctx.testQueries, tt.opts...)

propSearch := map[string]any{}
if tt.params.entName == "" {
propSearch[properties.PropertyUpstreamID] = tt.params.other[properties.PropertyUpstreamID]
} else {
propSearch[properties.PropertyName] = tt.params.entName
}
getByProps, err := properties.NewProperties(propSearch)
require.NoError(t, err)

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

if tt.expectErr != "" {
require.Contains(t, err.Error(), tt.expectErr)
return
}

require.NoError(t, err)
tt.checkResult(t, gotProps)
})
}
}

func TestPropertiesService_EntityWithPropertiesByUpstreamHint(t *testing.T) {
t.Parallel()

Expand Down