Remove deduplication logic in evaluation history

I have run into a number of issues involving pq and sqlc's ability to
deal with postgres timestamp arrays. At this point, rather than invest
further effort, I have decided to remove the deduplication feature for
now. Since this will only be useful at the point where we use
reconciliation more extensively, I would prefer to reintroduce this
functionality once we have more data to back up its utility.

database/migrations/000075_evaluation_history_no_dedupe.down.sql

@@ -0,0 +1,20 @@
+-- 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;
+
+ALTER TABLE evaluation_statuses ADD COLUMN evaluation_times TIMESTAMPZ[] NOT NULL DEFAULT ARRAY[NOW()]::TIMESTAMP[];
+ALTER TABLE evaluation_statuses RENAME COLUMN evaluation_time TO most_recent_evaluation;
+
+COMMIT;

database/migrations/000075_evaluation_history_no_dedupe.up.sql

@@ -0,0 +1,20 @@
+-- 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;
+
+ALTER TABLE evaluation_statuses DROP COLUMN evaluation_times;
+ALTER TABLE evaluation_statuses RENAME COLUMN most_recent_evaluation TO evaluation_time;
+
+COMMIT;

database/query/eval_history.sql

@@ -50,13 +50,6 @@ INSERT INTO evaluation_statuses(
 )
 RETURNING id;
 
--- name: UpdateEvaluationTimes :exec
-UPDATE evaluation_statuses
-SET
-    evaluation_times = $1,
-    most_recent_evaluation = NOW()
-WHERE id = $2;
-
 -- name: UpsertLatestEvaluationStatus :exec
 INSERT INTO latest_evaluation_statuses(
     rule_entity_id,
@@ -96,7 +89,7 @@ INSERT INTO alert_events(
 
 -- name: ListEvaluationHistory :many
 SELECT s.id::uuid AS evaluation_id,
-       s.most_recent_evaluation as evaluated_at,
+       s.evaluation_time as evaluated_at,
        -- entity type
        CASE WHEN ere.repository_id IS NOT NULL THEN 'repository'::entities
             WHEN ere.pull_request_id IS NOT NULL THEN 'pull_request'::entities
@@ -137,8 +130,8 @@ SELECT s.id::uuid AS evaluation_id,
   LEFT JOIN remediation_events re ON re.evaluation_id = s.id
   LEFT JOIN alert_events ae ON ae.evaluation_id = s.id
   LEFT JOIN projects j ON r.project_id = j.id
- WHERE (sqlc.narg(next)::timestamp without time zone IS NULL OR sqlc.narg(next) > s.most_recent_evaluation)
-   AND (sqlc.narg(prev)::timestamp without time zone IS NULL OR sqlc.narg(prev) < s.most_recent_evaluation)
+ WHERE (sqlc.narg(next)::timestamp without time zone IS NULL OR sqlc.narg(next) > s.evaluation_time)
+   AND (sqlc.narg(prev)::timestamp without time zone IS NULL OR sqlc.narg(prev) < s.evaluation_time)
    -- inclusion filters
    AND (sqlc.slice(entityTypes)::entities[] IS NULL OR entity_type::entities = ANY(sqlc.slice(entityTypes)::entities[]))
    AND (sqlc.slice(entityNames)::text[] IS NULL OR ere.repository_id IS NULL OR CONCAT(r.repo_owner, '/', r.repo_name) = ANY(sqlc.slice(entityNames)::text[]))
@@ -160,8 +153,8 @@ SELECT s.id::uuid AS evaluation_id,
    -- time range filter
    AND (sqlc.narg(fromts)::timestamp without time zone IS NULL
         OR sqlc.narg(tots)::timestamp without time zone IS NULL
-        OR s.most_recent_evaluation BETWEEN sqlc.narg(fromts) AND sqlc.narg(tots))
+        OR s.evaluation_time BETWEEN sqlc.narg(fromts) AND sqlc.narg(tots))
    -- implicit filter by project id
    AND j.id = sqlc.arg(projectId)
- ORDER BY s.most_recent_evaluation DESC
+ ORDER BY s.evaluation_time DESC
  LIMIT sqlc.arg(size)::integer;

internal/history/service.go

@@ -20,7 +20,6 @@ import (
 	"database/sql"
 	"errors"
 	"fmt"
-	"time"
 
 	"github.com/google/uuid"
 
@@ -68,7 +67,7 @@ func (e *evaluationHistoryService) StoreEvaluationStatus(
 	entityID uuid.UUID,
 	evalError error,
 ) (uuid.UUID, error) {
-	var ruleEntityID, evaluationID uuid.UUID
+	var ruleEntityID uuid.UUID
 	status := evalerrors.ErrorAsEvalStatus(evalError)
 	details := evalerrors.ErrorAsEvalDetails(evalError)
 
@@ -105,22 +104,11 @@ func (e *evaluationHistoryService) StoreEvaluationStatus(
 		}
 	} else {
 		ruleEntityID = latestRecord.RuleEntityID
-		evaluationID = latestRecord.ID
 	}
 
-	previousDetails := latestRecord.Details
-	previousStatus := latestRecord.Status
-
-	if evaluationID == uuid.Nil || previousDetails != details || previousStatus != status {
-		// if there is no prior state for this rule/entity, or the previous state
-		// differs from the current one, create a new status record.
-		if evaluationID, err = e.createNewStatus(ctx, qtx, ruleEntityID, status, details); err != nil {
-			return uuid.Nil, fmt.Errorf("error while creating new evaluation status for rule/entity %s: %w", ruleEntityID, err)
-		}
-	} else {
-		if err = e.updateExistingStatus(ctx, qtx, entityID, latestRecord.EvaluationTimes); err != nil {
-			return uuid.Nil, fmt.Errorf("error while updating existing evaluation status for rule/entity %s: %w", ruleEntityID, err)
-		}
+	evaluationID, err := e.createNewStatus(ctx, qtx, ruleEntityID, status, details)
+	if err != nil {
+		return uuid.Nil, fmt.Errorf("error while creating new evaluation status for rule/entity %s: %w", ruleEntityID, err)
 	}
 
 	return evaluationID, nil
@@ -158,20 +146,6 @@ func (_ *evaluationHistoryService) createNewStatus(
 	return newEvaluationID, err
 }
 
-func (_ *evaluationHistoryService) updateExistingStatus(
-	ctx context.Context,
-	qtx db.Querier,
-	evaluationID uuid.UUID,
-	times db.PgTimeArray,
-) error {
-	// if the status is repeated, then just append the current timestamp to it
-	times = append(times, db.PgTime{Time: time.Now()})
-	return qtx.UpdateEvaluationTimes(ctx, db.UpdateEvaluationTimesParams{
-		EvaluationTimes: times,
-		ID:              evaluationID,
-	})
-}
-
 func paramsFromEntity(
 	ruleID uuid.UUID,
 	entityID uuid.UUID,

internal/history/service_test.go

@@ -27,7 +27,6 @@ import (
 
 	"github.com/stacklok/minder/internal/db"
 	dbf "github.com/stacklok/minder/internal/db/fixtures"
-	engerr "github.com/stacklok/minder/internal/engine/errors"
 )
 
 func TestStoreEvaluationStatus(t *testing.T) {
@@ -80,15 +79,6 @@ func TestStoreEvaluationStatus(t *testing.T) {
 			),
 			ExpectedError: "error while creating new evaluation status for rule/entity",
 		},
-		{
-			Name:       "StoreEvaluationStatus returns error when unable to update status with timestamp",
-			EntityType: db.EntitiesRepository,
-			DBSetup: dbf.NewDBMock(
-				withGetLatestEval(sameState, nil),
-				withUpdateEvaluationTimes(errTest),
-			),
-			ExpectedError: "error while updating existing evaluation status for rule/entity",
-		},
 		{
 			Name:       "StoreEvaluationStatus creates new status for new rule/entity",
 			EntityType: db.EntitiesRepository,
@@ -103,28 +93,11 @@ func TestStoreEvaluationStatus(t *testing.T) {
 			Name:       "StoreEvaluationStatus creates new status for state change",
 			EntityType: db.EntitiesRepository,
 			DBSetup: dbf.NewDBMock(
-				withGetLatestEval(differentState, nil),
+				withGetLatestEval(existingState, nil),
 				withInsertEvaluationStatus(evaluationID, nil),
 				withUpsertLatestEvaluationStatus(nil),
 			),
 		},
-		{
-			Name:       "StoreEvaluationStatus creates new status when status is the same, but details differ",
-			EntityType: db.EntitiesRepository,
-			DBSetup: dbf.NewDBMock(
-				withGetLatestEval(differentDetails, nil),
-				withInsertEvaluationStatus(evaluationID, nil),
-				withUpsertLatestEvaluationStatus(nil),
-			),
-		},
-		{
-			Name:       "StoreEvaluationStatus adds timestamp when state does not change",
-			EntityType: db.EntitiesRepository,
-			DBSetup: dbf.NewDBMock(
-				withGetLatestEval(sameState, nil),
-				withUpdateEvaluationTimes(nil),
-			),
-		},
 	}
 
 	for _, scenario := range scenarios {
@@ -742,26 +715,11 @@ var (
 	evaluationID = uuid.New()
 
 	emptyLatestResult = db.EvaluationStatus{}
-	sameState         = db.EvaluationStatus{
-		ID:              evaluationID,
-		RuleEntityID:    ruleEntityID,
-		Status:          db.EvalStatusTypesError,
-		Details:         errTest.Error(),
-		EvaluationTimes: db.PgTimeArray{db.PgTime{Time: time.Now()}},
-	}
-	differentDetails = db.EvaluationStatus{
-		ID:              evaluationID,
-		RuleEntityID:    ruleEntityID,
-		Status:          db.EvalStatusTypesError,
-		Details:         "something went wrong",
-		EvaluationTimes: db.PgTimeArray{db.PgTime{Time: time.Now()}},
-	}
-	differentState = db.EvaluationStatus{
-		ID:              evaluationID,
-		RuleEntityID:    ruleEntityID,
-		Status:          db.EvalStatusTypesSkipped,
-		Details:         engerr.ErrEvaluationSkipped.Error(),
-		EvaluationTimes: db.PgTimeArray{db.PgTime{Time: time.Now()}},
+	existingState     = db.EvaluationStatus{
+		ID:           evaluationID,
+		RuleEntityID: ruleEntityID,
+		Status:       db.EvalStatusTypesError,
+		Details:      errTest.Error(),
 	}
 	errTest = errors.New("oh no")
 )
@@ -790,14 +748,6 @@ func withInsertEvaluationStatus(id uuid.UUID, err error) func(dbf.DBMock) {
 	}
 }
 
-func withUpdateEvaluationTimes(err error) func(dbf.DBMock) {
-	return func(mock dbf.DBMock) {
-		mock.EXPECT().
-			UpdateEvaluationTimes(gomock.Any(), gomock.Any()).
-			Return(err)
-	}
-}
-
 func withUpsertLatestEvaluationStatus(err error) func(dbf.DBMock) {
 	return func(mock dbf.DBMock) {
 		mock.EXPECT().