From a531fe5c1e488f4b998d8f8f815f52d33655865e Mon Sep 17 00:00:00 2001
From: Juan Antonio Osorio <ozz@stacklok.com>
Date: Mon, 7 Oct 2024 12:43:38 +0300
Subject: [PATCH] origination: First get properties from provider before trying
 to persist (#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 <ozz@stacklok.com>

* Use provider directly

Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>

* github: Return `nil` when filtering operational properties if there's no cache

Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>

* Update tests

---------

Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
Co-authored-by: Jakub Hrozek <jakub@stacklok.com>
---
 internal/entities/handlers/handler_test.go    | 22 ++++++++++++++++++-
 .../entity/add_originating_entity.go          | 20 +++++++++++------
 internal/providers/github/properties.go       |  5 +++++
 3 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/internal/entities/handlers/handler_test.go b/internal/entities/handlers/handler_test.go
index 7102375a57..8d9a07ccae 100644
--- 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
index 60238908f2..19ce70b5db 100644
--- 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
index 7985f8ce32..2c623319f0 100644
--- 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