Enforce central entity ID in EEA and rule eval tables (#4220)

These were nullable as we were introducing them into minder. Now that we've ensured they're everywhere, we can enforce them as not null. Signed-off-by: Juan Antonio Osorio <[email protected]>
mindersec · Aug 21, 2024 · 2ab6721 · 2ab6721
1 parent 7a0da06
commit 2ab6721
Show file tree

Hide file tree

Showing 12 changed files with 101 additions and 59 deletions.
diff --git a/database/migrations/000097_entity_instance_not_null.down.sql b/database/migrations/000097_entity_instance_not_null.down.sql
@@ -0,0 +1,23 @@
+-- Copyright 2024 Stacklok, Inc
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--      http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+BEGIN;
+
+-- Make entity_instance_id nullable in entity_execution_lock, flush_cache and evaluation_rule_entities.
+
+ALTER TABLE entity_execution_lock ALTER COLUMN entity_instance_id DROP NOT NULL;
+ALTER TABLE flush_cache ALTER COLUMN entity_instance_id DROP NOT NULL;
+ALTER TABLE evaluation_rule_entities ALTER COLUMN entity_instance_id DROP NOT NULL;
+
+COMMIT;
diff --git a/database/migrations/000097_entity_instance_not_null.up.sql b/database/migrations/000097_entity_instance_not_null.up.sql
@@ -0,0 +1,24 @@
+-- Copyright 2024 Stacklok, Inc
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--      http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+BEGIN;
+
+-- Make entity_instance_id not nullable in entity_execution_lock, flush_cache and evaluation_rule_entities.
+-- Note that at this point it is verified that the entity_instance_id is not null in all these tables.
+
+ALTER TABLE entity_execution_lock ALTER COLUMN entity_instance_id SET NOT NULL;
+ALTER TABLE flush_cache ALTER COLUMN entity_instance_id SET NOT NULL;
+ALTER TABLE evaluation_rule_entities ALTER COLUMN entity_instance_id SET NOT NULL;
+
+COMMIT;
diff --git a/database/query/entity_execution_lock.sql b/database/query/entity_execution_lock.sql
@@ -22,12 +22,12 @@ INSERT INTO entity_execution_lock(
     sqlc.narg(artifact_id)::UUID,
     sqlc.narg(pull_request_id)::UUID,
     sqlc.arg(project_id)::UUID,
-    sqlc.narg(entity_instance_id)::UUID
+    sqlc.arg(entity_instance_id)::UUID
 ) ON CONFLICT(entity, COALESCE(repository_id, '00000000-0000-0000-0000-000000000000'::UUID), COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID), COALESCE(pull_request_id, '00000000-0000-0000-0000-000000000000'::UUID))
 DO UPDATE SET
     locked_by = gen_random_uuid(),
     last_lock_time = NOW(),
-    entity_instance_id = sqlc.narg(entity_instance_id)::UUID
+    entity_instance_id = sqlc.arg(entity_instance_id)::UUID
 WHERE entity_execution_lock.last_lock_time < (NOW() - (@interval::TEXT || ' seconds')::interval)
 RETURNING *;
 
@@ -65,7 +65,7 @@ INSERT INTO flush_cache(
     sqlc.narg(artifact_id)::UUID,
     sqlc.narg(pull_request_id)::UUID,
     sqlc.arg(project_id)::UUID,
-    sqlc.narg(entity_instance_id)::UUID
+    sqlc.arg(entity_instance_id)::UUID
 ) ON CONFLICT(entity, COALESCE(repository_id, '00000000-0000-0000-0000-000000000000'::UUID), COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID), COALESCE(pull_request_id, '00000000-0000-0000-0000-000000000000'::UUID))
 DO NOTHING
 RETURNING *;

diff --git a/internal/db/entity_execution_lock.sql.go b/internal/db/entity_execution_lock.sql.go
diff --git a/internal/db/entity_execution_lock.sql_test.go b/internal/db/entity_execution_lock.sql_test.go
@@ -50,12 +50,13 @@ func TestQueries_LockIfThresholdNotExceeded(t *testing.T) {
 		go func() {
 			defer wg.Done()
 			_, err := testQueries.LockIfThresholdNotExceeded(context.Background(), LockIfThresholdNotExceededParams{
-				Entity:        EntitiesRepository,
-				RepositoryID:  uuid.NullUUID{UUID: repo.ID, Valid: true},
-				ArtifactID:    uuid.NullUUID{},
-				PullRequestID: uuid.NullUUID{},
-				Interval:      fmt.Sprintf("%d", threshold),
-				ProjectID:     project.ID,
+				Entity:           EntitiesRepository,
+				RepositoryID:     uuid.NullUUID{UUID: repo.ID, Valid: true},
+				ArtifactID:       uuid.NullUUID{},
+				PullRequestID:    uuid.NullUUID{},
+				Interval:         fmt.Sprintf("%d", threshold),
+				ProjectID:        project.ID,
+				EntityInstanceID: repo.ID,
 			})
 
 			if err != nil && errors.Is(err, sql.ErrNoRows) {
@@ -64,11 +65,12 @@ func TestQueries_LockIfThresholdNotExceeded(t *testing.T) {
 				queueCount.Add(1)
 
 				_, err := testQueries.EnqueueFlush(context.Background(), EnqueueFlushParams{
-					Entity:        EntitiesRepository,
-					RepositoryID:  uuid.NullUUID{UUID: repo.ID, Valid: true},
-					ArtifactID:    uuid.NullUUID{},
-					PullRequestID: uuid.NullUUID{},
-					ProjectID:     project.ID,
+					Entity:           EntitiesRepository,
+					RepositoryID:     uuid.NullUUID{UUID: repo.ID, Valid: true},
+					ArtifactID:       uuid.NullUUID{},
+					PullRequestID:    uuid.NullUUID{},
+					ProjectID:        project.ID,
+					EntityInstanceID: repo.ID,
 				})
 				if err == nil {
 					effectiveFlush.Add(1)
@@ -90,11 +92,12 @@ func TestQueries_LockIfThresholdNotExceeded(t *testing.T) {
 	t.Log("Attempting to acquire lock now that threshold has passed")
 
 	_, err := testQueries.LockIfThresholdNotExceeded(context.Background(), LockIfThresholdNotExceededParams{
-		Entity:        EntitiesRepository,
-		RepositoryID:  uuid.NullUUID{UUID: repo.ID, Valid: true},
-		ArtifactID:    uuid.NullUUID{},
-		PullRequestID: uuid.NullUUID{},
-		Interval:      fmt.Sprintf("%d", threshold),
+		Entity:           EntitiesRepository,
+		RepositoryID:     uuid.NullUUID{UUID: repo.ID, Valid: true},
+		ArtifactID:       uuid.NullUUID{},
+		PullRequestID:    uuid.NullUUID{},
+		Interval:         fmt.Sprintf("%d", threshold),
+		EntityInstanceID: repo.ID,
 	})
 
 	assert.NoError(t, err, "expected no error")

diff --git a/internal/db/eval_history.sql.go b/internal/db/eval_history.sql.go
diff --git a/internal/db/eval_history_test.go b/internal/db/eval_history_test.go
@@ -718,11 +718,8 @@ func createRandomEvaluationRuleEntity(
 				UUID:  entityID,
 				Valid: true,
 			},
-			EntityType: EntitiesRepository,
-			EntityInstanceID: uuid.NullUUID{
-				UUID:  entityID,
-				Valid: true,
-			},
+			EntityType:       EntitiesRepository,
+			EntityInstanceID: entityID,
 		},
 	)
 	require.NoError(t, err)

diff --git a/internal/db/models.go b/internal/db/models.go
diff --git a/internal/db/profiles_test.go b/internal/db/profiles_test.go
@@ -162,13 +162,10 @@ func createRuleEntity(
 				UUID:  repoID,
 				Valid: true,
 			},
-			PullRequestID: uuid.NullUUID{},
-			ArtifactID:    uuid.NullUUID{},
-			EntityType:    EntitiesRepository,
-			EntityInstanceID: uuid.NullUUID{
-				UUID:  repoID,
-				Valid: true,
-			},
+			PullRequestID:    uuid.NullUUID{},
+			ArtifactID:       uuid.NullUUID{},
+			EntityType:       EntitiesRepository,
+			EntityInstanceID: repoID,
 		},
 	)
 	require.NoError(t, err)

diff --git a/internal/eea/eea.go b/internal/eea/eea.go
@@ -94,15 +94,9 @@ func (e *EEA) aggregate(msg *message.Message) (*message.Message, error) {
 		Str("entity", inf.Type.ToString()).
 		Logger()
 
-	var entityID uuid.NullUUID
-	eID, err := inf.GetID()
+	entityID, err := inf.GetID()
 	if err != nil {
 		logger.Debug().AnErr("error getting entity ID", err).Msgf("Entity ID was not set for event %s", inf.Type)
-	} else {
-		entityID = uuid.NullUUID{
-			UUID:  eID,
-			Valid: true,
-		}
 	}
 
 	// We need to check that the resources still exist before attempting to lock them.

diff --git a/internal/eea/eea_test.go b/internal/eea/eea_test.go
@@ -210,10 +210,11 @@ func TestFlushAll(t *testing.T) {
 				mockStore.EXPECT().ListFlushCache(ctx).
 					Return([]db.FlushCache{
 						{
-							ID:           uuid.New(),
-							Entity:       db.EntitiesRepository,
-							RepositoryID: uuid.NullUUID{UUID: repoID, Valid: true},
-							QueuedAt:     time.Now(),
+							ID:               uuid.New(),
+							Entity:           db.EntitiesRepository,
+							RepositoryID:     uuid.NullUUID{UUID: repoID, Valid: true},
+							QueuedAt:         time.Now(),
+							EntityInstanceID: repoID,
 						},
 					}, nil)
 
@@ -259,7 +260,8 @@ func TestFlushAll(t *testing.T) {
 								UUID:  artID,
 								Valid: true,
 							},
-							QueuedAt: time.Now(),
+							EntityInstanceID: artID,
+							QueuedAt:         time.Now(),
 						},
 					}, nil)
 
@@ -318,6 +320,7 @@ func TestFlushAll(t *testing.T) {
 								UUID:  projectID,
 								Valid: true,
 							},
+							EntityInstanceID: artID,
 						},
 					}, nil)
 
@@ -367,6 +370,7 @@ func TestFlushAll(t *testing.T) {
 								UUID:  projectID,
 								Valid: true,
 							},
+							EntityInstanceID: artID,
 						},
 					}, nil)
 

diff --git a/internal/history/service.go b/internal/history/service.go
@@ -169,7 +169,7 @@ func paramsFromEntity(
 		Valid: true,
 	}
 
-	params.EntityID = nullableEntityID
+	params.EntityID = entityID
 
 	switch entityType {
 	case db.EntitiesRepository:
@@ -193,7 +193,7 @@ type ruleEntityParams struct {
 	PullRequestID uuid.NullUUID
 	// Is the target entity ID. We'll be replacing the single-entity IDs
 	// with this one.
-	EntityID uuid.NullUUID
+	EntityID uuid.UUID
 }
 
 func (_ *evaluationHistoryService) ListEvaluationHistory(