origination: First get properties from provider before trying to pers…

…ist (#4660) * origination: First get properties from provider before trying to persist The incoming origination message might not have all properties set. This finds the entity from the provider before trying to persist. Signed-off-by: Juan Antonio Osorio <[email protected]> * Use provider directly Signed-off-by: Juan Antonio Osorio <[email protected]> * github: Return `nil` when filtering operational properties if there's no cache Signed-off-by: Juan Antonio Osorio <[email protected]> * Update tests --------- Signed-off-by: Juan Antonio Osorio <[email protected]> Co-authored-by: Jakub Hrozek <[email protected]>
mindersec · Oct 7, 2024 · a531fe5 · a531fe5
1 parent 3b0f330
commit a531fe5
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 8 deletions.
diff --git a/internal/entities/handlers/handler_test.go b/internal/entities/handlers/handler_test.go
@@ -98,6 +98,15 @@ type (
 	providerMockBuilder = func(controller *gomock.Controller) providerMock
 )
 
+func getPullRequestProperties() *properties.Properties {
+	props, err := properties.NewProperties(pullRequestPropMap)
+	if err != nil {
+		panic(err)
+	}
+
+	return props
+}
+
 func newProviderMock(opts ...func(providerMock)) providerMockBuilder {
 	return func(ctrl *gomock.Controller) providerMock {
 		mock := mockgithub.NewMockGitHub(ctrl)
@@ -116,6 +125,14 @@ func withSuccessfulGetEntityName(name string) func(providerMock) {
 	}
 }
 
+func withSuccessfulFetchAllProperties(props *properties.Properties) func(mock providerMock) {
+	return func(mock providerMock) {
+		mock.EXPECT().
+			FetchAllProperties(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+			Return(props, nil)
+	}
+}
+
 func buildEwp(t *testing.T, ewp models.EntityWithProperties, propMap map[string]any) *models.EntityWithProperties {
 	t.Helper()
 
@@ -415,7 +432,10 @@ func TestRefreshEntityAndDoHandler_HandleRefreshEntityAndEval(t *testing.T) {
 						EntityType: db.EntitiesRepository,
 					}),
 			),
-			providerSetup: newProviderMock(withSuccessfulGetEntityName(pullName)),
+			providerSetup: newProviderMock(
+				withSuccessfulGetEntityName(pullName),
+				withSuccessfulFetchAllProperties(getPullRequestProperties()),
+			),
 			providerManagerSetup: func(prov provifv1.Provider) provManFixtures.ProviderManagerMockBuilder {
 				return provManFixtures.NewProviderManagerMock(
 					provManFixtures.WithSuccessfulInstantiateFromID(prov),

diff --git a/internal/entities/handlers/strategies/entity/add_originating_entity.go b/internal/entities/handlers/strategies/entity/add_originating_entity.go
@@ -72,17 +72,22 @@ func (a *addOriginatingEntityStrategy) GetEntity(
 			return nil, fmt.Errorf("error getting parent entity: %w", err)
 		}
 
-		legacyId, err := a.upsertLegacyEntity(ctx, entMsg.Entity.Type, parentEwp, childProps, t)
+		prov, err := a.provMgr.InstantiateFromID(ctx, parentEwp.Entity.ProviderID)
 		if err != nil {
-			return nil, fmt.Errorf("error upserting legacy entity: %w", err)
+			return nil, fmt.Errorf("error getting provider: %w", err)
 		}
 
-		prov, err := a.provMgr.InstantiateFromID(ctx, parentEwp.Entity.ProviderID)
+		upstreamProps, err := prov.FetchAllProperties(ctx, childProps, entMsg.Entity.Type, nil)
 		if err != nil {
-			return nil, fmt.Errorf("error getting provider: %w", err)
+			return nil, fmt.Errorf("error retrieving properties: %w", err)
 		}
 
-		childEntName, err := prov.GetEntityName(entMsg.Entity.Type, childProps)
+		legacyId, err := a.upsertLegacyEntity(ctx, entMsg.Entity.Type, parentEwp, upstreamProps, t)
+		if err != nil {
+			return nil, fmt.Errorf("error upserting legacy entity: %w", err)
+		}
+
+		childEntName, err := prov.GetEntityName(entMsg.Entity.Type, upstreamProps)
 		if err != nil {
 			return nil, fmt.Errorf("error getting child entity name: %w", err)
 		}
@@ -102,13 +107,14 @@ func (a *addOriginatingEntityStrategy) GetEntity(
 			return nil, err
 		}
 
-		upstreamProps, err := a.propSvc.RetrieveAllProperties(ctx, prov,
+		// Persist the properties
+		upstreamProps, err = a.propSvc.RetrieveAllProperties(ctx, prov,
 			parentEwp.Entity.ProjectID, parentEwp.Entity.ProviderID,
 			childProps, entMsg.Entity.Type,
 			propertyService.ReadBuilder().WithStoreOrTransaction(t),
 		)
 		if err != nil {
-			return nil, fmt.Errorf("error retrieving properties: %w", err)
+			return nil, fmt.Errorf("error persisting properties: %w", err)
 		}
 
 		return models.NewEntityWithProperties(childEnt, upstreamProps), nil

diff --git a/internal/providers/github/properties.go b/internal/providers/github/properties.go
@@ -93,6 +93,11 @@ func (c *GitHub) FetchAllProperties(
 }
 
 func filterOperational(cachedProperties *properties.Properties, fetcher properties2.GhPropertyFetcher) *properties.Properties {
+	if cachedProperties == nil {
+		// Nothing to filter
+		return nil
+	}
+
 	operational := fetcher.OperationalProperties()
 	if len(operational) == 0 {
 		return cachedProperties