diff --git a/database/migrations/000075_evaluation_history_no_dedupe.down.sql b/database/migrations/000075_evaluation_history_no_dedupe.down.sql new file mode 100644 index 0000000000..81bb1e5ba5 --- /dev/null +++ b/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; \ No newline at end of file diff --git a/database/migrations/000075_evaluation_history_no_dedupe.up.sql b/database/migrations/000075_evaluation_history_no_dedupe.up.sql new file mode 100644 index 0000000000..4e2d54dfb1 --- /dev/null +++ b/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; \ No newline at end of file diff --git a/database/mock/store.go b/database/mock/store.go index 861eab39e0..18f76b9c87 100644 --- a/database/mock/store.go +++ b/database/mock/store.go @@ -1895,20 +1895,6 @@ func (mr *MockStoreMockRecorder) UpdateEncryptedSecret(arg0, arg1 any) *gomock.C return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateEncryptedSecret", reflect.TypeOf((*MockStore)(nil).UpdateEncryptedSecret), arg0, arg1) } -// UpdateEvaluationTimes mocks base method. -func (m *MockStore) UpdateEvaluationTimes(arg0 context.Context, arg1 db.UpdateEvaluationTimesParams) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "UpdateEvaluationTimes", arg0, arg1) - ret0, _ := ret[0].(error) - return ret0 -} - -// UpdateEvaluationTimes indicates an expected call of UpdateEvaluationTimes. -func (mr *MockStoreMockRecorder) UpdateEvaluationTimes(arg0, arg1 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateEvaluationTimes", reflect.TypeOf((*MockStore)(nil).UpdateEvaluationTimes), arg0, arg1) -} - // UpdateInvitationRole mocks base method. func (m *MockStore) UpdateInvitationRole(arg0 context.Context, arg1 db.UpdateInvitationRoleParams) (db.UserInvite, error) { m.ctrl.T.Helper() diff --git a/database/query/eval_history.sql b/database/query/eval_history.sql index 382300d2f1..4de34efec8 100644 --- a/database/query/eval_history.sql +++ b/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; diff --git a/internal/db/eval_history.sql.go b/internal/db/eval_history.sql.go index 78ed55ea01..1461a1d6c9 100644 --- a/internal/db/eval_history.sql.go +++ b/internal/db/eval_history.sql.go @@ -17,7 +17,7 @@ import ( const getLatestEvalStateForRuleEntity = `-- name: GetLatestEvalStateForRuleEntity :one -SELECT eh.id, eh.rule_entity_id, eh.status, eh.details, eh.evaluation_times, eh.most_recent_evaluation FROM evaluation_rule_entities AS re +SELECT eh.id, eh.rule_entity_id, eh.status, eh.details, eh.evaluation_time FROM evaluation_rule_entities AS re JOIN latest_evaluation_statuses AS les ON les.rule_entity_id = re.id JOIN evaluation_statuses AS eh ON les.evaluation_history_id = eh.id WHERE re.rule_id = $1 @@ -62,8 +62,7 @@ func (q *Queries) GetLatestEvalStateForRuleEntity(ctx context.Context, arg GetLa &i.RuleEntityID, &i.Status, &i.Details, - &i.EvaluationTimes, - &i.MostRecentEvaluation, + &i.EvaluationTime, ) return i, err } @@ -192,7 +191,7 @@ func (q *Queries) InsertRemediationEvent(ctx context.Context, arg InsertRemediat const listEvaluationHistory = `-- 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 @@ -233,8 +232,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 ($1::timestamp without time zone IS NULL OR $1 > s.most_recent_evaluation) - AND ($2::timestamp without time zone IS NULL OR $2 < s.most_recent_evaluation) + WHERE ($1::timestamp without time zone IS NULL OR $1 > s.evaluation_time) + AND ($2::timestamp without time zone IS NULL OR $2 < s.evaluation_time) -- inclusion filters AND ($3::entities[] IS NULL OR entity_type::entities = ANY($3::entities[])) AND ($4::text[] IS NULL OR ere.repository_id IS NULL OR CONCAT(r.repo_owner, '/', r.repo_name) = ANY($4::text[])) @@ -256,10 +255,10 @@ SELECT s.id::uuid AS evaluation_id, -- time range filter AND ($15::timestamp without time zone IS NULL OR $16::timestamp without time zone IS NULL - OR s.most_recent_evaluation BETWEEN $15 AND $16) + OR s.evaluation_time BETWEEN $15 AND $16) -- implicit filter by project id AND j.id = $17 - ORDER BY s.most_recent_evaluation DESC + ORDER BY s.evaluation_time DESC LIMIT $18::integer ` @@ -366,24 +365,6 @@ func (q *Queries) ListEvaluationHistory(ctx context.Context, arg ListEvaluationH return items, nil } -const updateEvaluationTimes = `-- name: UpdateEvaluationTimes :exec -UPDATE evaluation_statuses -SET - evaluation_times = $1, - most_recent_evaluation = NOW() -WHERE id = $2 -` - -type UpdateEvaluationTimesParams struct { - EvaluationTimes PgTimeArray `json:"evaluation_times"` - ID uuid.UUID `json:"id"` -} - -func (q *Queries) UpdateEvaluationTimes(ctx context.Context, arg UpdateEvaluationTimesParams) error { - _, err := q.db.ExecContext(ctx, updateEvaluationTimes, arg.EvaluationTimes, arg.ID) - return err -} - const upsertLatestEvaluationStatus = `-- name: UpsertLatestEvaluationStatus :exec INSERT INTO latest_evaluation_statuses( rule_entity_id, diff --git a/internal/db/models.go b/internal/db/models.go index a91ae7fcca..5d13b950ed 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -494,12 +494,11 @@ type EvaluationRuleEntity struct { } type EvaluationStatus struct { - ID uuid.UUID `json:"id"` - RuleEntityID uuid.UUID `json:"rule_entity_id"` - Status EvalStatusTypes `json:"status"` - Details string `json:"details"` - EvaluationTimes PgTimeArray `json:"evaluation_times"` - MostRecentEvaluation time.Time `json:"most_recent_evaluation"` + ID uuid.UUID `json:"id"` + RuleEntityID uuid.UUID `json:"rule_entity_id"` + Status EvalStatusTypes `json:"status"` + Details string `json:"details"` + EvaluationTime time.Time `json:"evaluation_time"` } type Feature struct { diff --git a/internal/db/querier.go b/internal/db/querier.go index 516254064b..0b288d73a4 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -217,7 +217,6 @@ type Querier interface { RepositoryExistsAfterID(ctx context.Context, id uuid.UUID) (bool, error) SetCurrentVersion(ctx context.Context, arg SetCurrentVersionParams) error UpdateEncryptedSecret(ctx context.Context, arg UpdateEncryptedSecretParams) error - UpdateEvaluationTimes(ctx context.Context, arg UpdateEvaluationTimesParams) error // UpdateInvitationRole updates an invitation by its code. This is intended to be // called by a user who has issued an invitation and then decided to change the // role of the invitee. diff --git a/internal/history/service.go b/internal/history/service.go index e90c2d10b6..bd920c3d67 100644 --- a/internal/history/service.go +++ b/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, diff --git a/internal/history/service_test.go b/internal/history/service_test.go index 60fc9909bb..bf3611db38 100644 --- a/internal/history/service_test.go +++ b/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().