From 9361c735d6baaee5dd68c41fde78316dab5e28da Mon Sep 17 00:00:00 2001
From: Juan Antonio Osorio <ozz@stacklok.com>
Date: Fri, 4 Oct 2024 13:56:41 +0300
Subject: [PATCH] properties service: Don't fail if multiple entries are found
 (#4642)

* properties service: Don't fail if multiple entries are found

the `matchEntityWithHint` attempts to look for an entity using
identifying properties. For PRs, the upstream ID is not sufficient since
it's an integer that will clash with other PRs. In this case, we should
instead continue the search and search by name which is more unique.

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

* Adjust unit tests

---------

Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
Co-authored-by: Jakub Hrozek <jakub@stacklok.com>
---
 .../entities/properties/service/helpers.go    |  2 +-
 .../properties/service/service_test.go        | 80 ++++++++++++++++++-
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/internal/entities/properties/service/helpers.go b/internal/entities/properties/service/helpers.go
index 658f341e61..32ada80cc3 100644
--- a/internal/entities/properties/service/helpers.go
+++ b/internal/entities/properties/service/helpers.go
@@ -232,7 +232,7 @@ func matchEntityWithHint(
 				continue
 			} else if errors.Is(err, ErrMultipleEntities) {
 				l.Error().Msg("multiple entities matched")
-				return nil, ErrMultipleEntities
+				continue
 			}
 			return nil, fmt.Errorf("failed to match entity by hint: %w", err)
 		}
diff --git a/internal/entities/properties/service/service_test.go b/internal/entities/properties/service/service_test.go
index e095db189b..fbde620a7d 100644
--- a/internal/entities/properties/service/service_test.go
+++ b/internal/entities/properties/service/service_test.go
@@ -675,7 +675,85 @@ func TestPropertiesService_EntityWithPropertiesByUpstreamHint(t *testing.T) {
 					insertPropertiesFromMap(ctx, t, store, ent.ID, propMap)
 				}
 			},
-			expectedError: ErrMultipleEntities,
+			expectedError: ErrEntityNotFound,
+		},
+		{
+			name:    "Multiple pull requests with the same ID matched by name",
+			entType: minderv1.Entity_ENTITY_PULL_REQUESTS,
+			byPropMap: map[string]any{
+				properties.PropertyUpstreamID: "1",
+				properties.PropertyName:       "test-repo-2-with-pr/1",
+			},
+			hint: ByUpstreamHint{
+				ProviderImplements: db.NullProviderType{
+					ProviderType: db.ProviderTypeGithub,
+					Valid:        true,
+				},
+			},
+			dbSetup: func(t *testing.T, store db.Store) {
+				t.Helper()
+
+				repo1, err := store.CreateEntity(ctx, db.CreateEntityParams{
+					EntityType: entities.EntityTypeToDB(minderv1.Entity_ENTITY_REPOSITORIES),
+					Name:       "test-repo-1-with-pr",
+					ProjectID:  tctx.dbProj.ID,
+					ProviderID: tctx.ghAppProvider.ID,
+				})
+				require.NoError(t, err)
+
+				repo2, err := store.CreateEntity(ctx, db.CreateEntityParams{
+					EntityType: entities.EntityTypeToDB(minderv1.Entity_ENTITY_REPOSITORIES),
+					Name:       "test-repo-2-with-pr",
+					ProjectID:  tctx.dbProj.ID,
+					ProviderID: tctx.ghAppProvider.ID,
+				})
+				require.NoError(t, err)
+
+				repo1Pr, err := store.CreateEntity(ctx, db.CreateEntityParams{
+					EntityType: entities.EntityTypeToDB(minderv1.Entity_ENTITY_PULL_REQUESTS),
+					Name:       "test-repo-1-with-pr/1",
+					ProjectID:  tctx.dbProj.ID,
+					ProviderID: tctx.ghAppProvider.ID,
+					OriginatedFrom: uuid.NullUUID{
+						UUID: repo1.ID,
+					},
+				})
+				require.NoError(t, err)
+
+				insertPropertiesFromMap(ctx, t, store,
+					repo1Pr.ID,
+					map[string]any{
+						properties.PropertyUpstreamID: "1",
+						properties.PropertyName:       "test-repo-1-with-pr/1",
+					},
+				)
+
+				repo2Pr, err := store.CreateEntity(ctx, db.CreateEntityParams{
+					EntityType: entities.EntityTypeToDB(minderv1.Entity_ENTITY_PULL_REQUESTS),
+					Name:       "test-repo-2-with-pr/1",
+					ProjectID:  tctx.dbProj.ID,
+					ProviderID: tctx.ghAppProvider.ID,
+					OriginatedFrom: uuid.NullUUID{
+						UUID: repo2.ID,
+					},
+				})
+				require.NoError(t, err)
+
+				insertPropertiesFromMap(ctx, t, store,
+					repo2Pr.ID,
+					map[string]any{
+						properties.PropertyUpstreamID: "1",
+						properties.PropertyName:       "test-repo-2-with-pr/1",
+					},
+				)
+			},
+			checkResult: func(t *testing.T, result *models.EntityWithProperties) {
+				t.Helper()
+				require.NotNil(t, result)
+				require.Equal(t, "test-repo-2-with-pr/1", result.Entity.Name)
+				require.Equal(t, "1", result.Properties.GetProperty(properties.PropertyUpstreamID).GetString())
+				require.Equal(t, tctx.ghAppProvider.ID, result.Entity.ProviderID)
+			},
 		},
 		{
 			name:      "One of multiple entities matched by provider hint",