From 1a5c09ec1677fa8382cc3c355300026a4c4053d5 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Wed, 4 Sep 2024 14:18:03 +0300 Subject: [PATCH] Decouple legacy entity tables from results queries (#4342) * Decouple legacy entity tables from results queries This is another step into moving away from the central entity tables and into the new per-entity tables. Signed-off-by: Juan Antonio Osorio * Remove EntityForProperties model This is not needed and redundant. Instead the logic is moved towards the properties service Signed-off-by: Juan Antonio Osorio * Use pointers for entity with properties, not values Signed-off-by: Juan Antonio Osorio --------- Signed-off-by: Juan Antonio Osorio --- cmd/server/app/history_purge_test.go | 20 +- ...00104_rm_one_entity_id_constraint.down.sql | 18 + .../000104_rm_one_entity_id_constraint.up.sql | 19 + database/mock/store.go | 38 +- database/query/eval_history.sql | 82 +--- database/query/profile_status.sql | 36 +- database/query/repositories.sql | 9 - internal/controlplane/handlers_evalstatus.go | 187 ++++----- .../controlplane/handlers_evalstatus_test.go | 387 ++++++------------ internal/controlplane/handlers_profile.go | 129 +++--- internal/db/eval_history.sql.go | 143 ++----- internal/db/eval_history_test.go | 34 +- internal/db/profile_status.sql.go | 48 +-- internal/db/profiles_test.go | 50 ++- internal/db/querier.go | 2 - internal/db/repositories.sql.go | 36 -- internal/db/repositories_test.go | 3 +- internal/db/store.go | 4 +- internal/engine/eval_status.go | 6 - internal/engine/executor_test.go | 1 - internal/entities/models/models.go | 17 +- .../properties/service/mock/service.go | 31 ++ .../entities/properties/service/service.go | 80 +++- internal/history/mock/service.go | 2 +- internal/history/models.go | 10 +- internal/history/service.go | 101 +++-- internal/history/service_test.go | 196 ++++++--- internal/repositories/github/service.go | 6 +- internal/service/service.go | 4 +- 29 files changed, 777 insertions(+), 922 deletions(-) create mode 100644 database/migrations/000104_rm_one_entity_id_constraint.down.sql create mode 100644 database/migrations/000104_rm_one_entity_id_constraint.up.sql diff --git a/cmd/server/app/history_purge_test.go b/cmd/server/app/history_purge_test.go index d0990adde3..15fcd670d7 100644 --- a/cmd/server/app/history_purge_test.go +++ b/cmd/server/app/history_purge_test.go @@ -41,13 +41,13 @@ func TestRecordSize(t *testing.T) { db.ListEvaluationHistoryStaleRecordsRow{ ID: uuid.Nil, EvaluationTime: time.Now(), - EntityType: int32(1), + EntityType: db.EntitiesArtifact, EntityID: uuid.Nil, RuleID: uuid.Nil, }, ) - require.Equal(t, uintptr(96), size) + require.Equal(t, uintptr(88), size) } func TestPurgeLoop(t *testing.T) { @@ -76,7 +76,7 @@ func TestPurgeLoop(t *testing.T) { EvaluationTime: time.Now(), ID: uuid1, RuleID: ruleID1, - EntityType: int32(1), + EntityType: db.EntitiesArtifact, EntityID: entityID1, }, ), @@ -104,7 +104,7 @@ func TestPurgeLoop(t *testing.T) { EvaluationTime: time.Now(), ID: uuid1, RuleID: ruleID1, - EntityType: int32(1), + EntityType: db.EntitiesArtifact, EntityID: entityID1, }, ), @@ -126,21 +126,21 @@ func TestPurgeLoop(t *testing.T) { EvaluationTime: time.Now(), ID: uuid1, RuleID: ruleID1, - EntityType: int32(1), + EntityType: db.EntitiesArtifact, EntityID: entityID1, }, db.ListEvaluationHistoryStaleRecordsRow{ EvaluationTime: time.Now(), ID: uuid2, RuleID: ruleID2, - EntityType: int32(1), + EntityType: db.EntitiesArtifact, EntityID: entityID2, }, db.ListEvaluationHistoryStaleRecordsRow{ EvaluationTime: time.Now(), ID: uuid3, RuleID: ruleID3, - EntityType: int32(1), + EntityType: db.EntitiesArtifact, EntityID: entityID3, }, ), @@ -201,7 +201,7 @@ func TestPurgeLoop(t *testing.T) { EvaluationTime: time.Now(), ID: uuid1, RuleID: ruleID1, - EntityType: int32(1), + EntityType: db.EntitiesArtifact, EntityID: entityID1, }, ), @@ -434,14 +434,14 @@ var ( ruleID3 = uuid.MustParse("00000000-0000-0000-0000-000000000333") evaluatedAt1 = time.Now() evaluatedAt2 = evaluatedAt1.Add(-1 * time.Hour) - entityType = int32(1) + entityType = db.EntitiesRepository ) //nolint:unparam func makeHistoryRow( id uuid.UUID, evaluatedAt time.Time, - entityType int32, + entityType db.Entities, entityID uuid.UUID, ruleID uuid.UUID, ) db.ListEvaluationHistoryStaleRecordsRow { diff --git a/database/migrations/000104_rm_one_entity_id_constraint.down.sql b/database/migrations/000104_rm_one_entity_id_constraint.down.sql new file mode 100644 index 0000000000..e4bef160bc --- /dev/null +++ b/database/migrations/000104_rm_one_entity_id_constraint.down.sql @@ -0,0 +1,18 @@ +-- 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; + +-- recreate `one_entity_id` constraint +ALTER TABLE evaluation_rule_entities ADD CONSTRAINT one_entity_id CHECK (num_nonnulls(repository_id, artifact_id, pull_request_id) = 1); \ No newline at end of file diff --git a/database/migrations/000104_rm_one_entity_id_constraint.up.sql b/database/migrations/000104_rm_one_entity_id_constraint.up.sql new file mode 100644 index 0000000000..4214e3b44a --- /dev/null +++ b/database/migrations/000104_rm_one_entity_id_constraint.up.sql @@ -0,0 +1,19 @@ +-- 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_rule_entities DROP CONSTRAINT one_entity_id; + +COMMIT; \ No newline at end of file diff --git a/database/mock/store.go b/database/mock/store.go index 31d0afd46c..e3ce86d592 100644 --- a/database/mock/store.go +++ b/database/mock/store.go @@ -1463,36 +1463,6 @@ func (mr *MockStoreMockRecorder) GetQuerierWithTransaction(arg0 any) *gomock.Cal return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetQuerierWithTransaction", reflect.TypeOf((*MockStore)(nil).GetQuerierWithTransaction), arg0) } -// GetRepoPathFromArtifactID mocks base method. -func (m *MockStore) GetRepoPathFromArtifactID(arg0 context.Context, arg1 uuid.UUID) (db.GetRepoPathFromArtifactIDRow, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetRepoPathFromArtifactID", arg0, arg1) - ret0, _ := ret[0].(db.GetRepoPathFromArtifactIDRow) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetRepoPathFromArtifactID indicates an expected call of GetRepoPathFromArtifactID. -func (mr *MockStoreMockRecorder) GetRepoPathFromArtifactID(arg0, arg1 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRepoPathFromArtifactID", reflect.TypeOf((*MockStore)(nil).GetRepoPathFromArtifactID), arg0, arg1) -} - -// GetRepoPathFromPullRequestID mocks base method. -func (m *MockStore) GetRepoPathFromPullRequestID(arg0 context.Context, arg1 uuid.UUID) (db.GetRepoPathFromPullRequestIDRow, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetRepoPathFromPullRequestID", arg0, arg1) - ret0, _ := ret[0].(db.GetRepoPathFromPullRequestIDRow) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetRepoPathFromPullRequestID indicates an expected call of GetRepoPathFromPullRequestID. -func (mr *MockStoreMockRecorder) GetRepoPathFromPullRequestID(arg0, arg1 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRepoPathFromPullRequestID", reflect.TypeOf((*MockStore)(nil).GetRepoPathFromPullRequestID), arg0, arg1) -} - // GetRepositoryByID mocks base method. func (m *MockStore) GetRepositoryByID(arg0 context.Context, arg1 uuid.UUID) (db.Repository, error) { m.ctrl.T.Helper() @@ -1554,18 +1524,18 @@ func (mr *MockStoreMockRecorder) GetRepositoryByRepoName(arg0, arg1 any) *gomock } // GetRuleEvaluationByProfileIdAndRuleType mocks base method. -func (m *MockStore) GetRuleEvaluationByProfileIdAndRuleType(arg0 context.Context, arg1 uuid.UUID, arg2 db.NullEntities, arg3 sql.NullString, arg4 uuid.NullUUID, arg5 sql.NullString) (*db.ListRuleEvaluationsByProfileIdRow, error) { +func (m *MockStore) GetRuleEvaluationByProfileIdAndRuleType(arg0 context.Context, arg1 uuid.UUID, arg2 sql.NullString, arg3 uuid.NullUUID, arg4 sql.NullString) (*db.ListRuleEvaluationsByProfileIdRow, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetRuleEvaluationByProfileIdAndRuleType", arg0, arg1, arg2, arg3, arg4, arg5) + ret := m.ctrl.Call(m, "GetRuleEvaluationByProfileIdAndRuleType", arg0, arg1, arg2, arg3, arg4) ret0, _ := ret[0].(*db.ListRuleEvaluationsByProfileIdRow) ret1, _ := ret[1].(error) return ret0, ret1 } // GetRuleEvaluationByProfileIdAndRuleType indicates an expected call of GetRuleEvaluationByProfileIdAndRuleType. -func (mr *MockStoreMockRecorder) GetRuleEvaluationByProfileIdAndRuleType(arg0, arg1, arg2, arg3, arg4, arg5 any) *gomock.Call { +func (mr *MockStoreMockRecorder) GetRuleEvaluationByProfileIdAndRuleType(arg0, arg1, arg2, arg3, arg4 any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRuleEvaluationByProfileIdAndRuleType", reflect.TypeOf((*MockStore)(nil).GetRuleEvaluationByProfileIdAndRuleType), arg0, arg1, arg2, arg3, arg4, arg5) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRuleEvaluationByProfileIdAndRuleType", reflect.TypeOf((*MockStore)(nil).GetRuleEvaluationByProfileIdAndRuleType), arg0, arg1, arg2, arg3, arg4) } // GetRuleInstancesEntityInProjects mocks base method. diff --git a/database/query/eval_history.sql b/database/query/eval_history.sql index 68879e5f1f..73c06882d0 100644 --- a/database/query/eval_history.sql +++ b/database/query/eval_history.sql @@ -16,29 +16,18 @@ SELECT eh.* 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 -AND ( - re.repository_id = $2 - OR re.pull_request_id = $3 - OR re.artifact_id = $4 -) +WHERE re.rule_id = $1 AND re.entity_instance_id = $2 FOR UPDATE; -- name: InsertEvaluationRuleEntity :one INSERT INTO evaluation_rule_entities( rule_id, - repository_id, - pull_request_id, - artifact_id, entity_type, entity_instance_id ) VALUES ( $1, $2, - $3, - $4, - $5, - $6 + $3 ) RETURNING id; @@ -100,19 +89,9 @@ SELECT s.id::uuid AS evaluation_id, s.evaluation_time as evaluated_at, ere.entity_type, -- entity id - CAST( - CASE - WHEN ere.repository_id IS NOT NULL THEN r.id - WHEN ere.pull_request_id IS NOT NULL THEN pr.id - WHEN ere.artifact_id IS NOT NULL THEN a.id - END AS uuid - ) AS entity_id, - ere.entity_instance_id as new_entity_id, - -- raw fields for entity names - r.repo_owner, - r.repo_name, - pr.pr_number, - a.artifact_name, + ere.entity_instance_id as entity_id, + -- entity name + ei.name as entity_name, j.id as project_id, -- rule type, name, and profile rt.name AS rule_type, @@ -133,12 +112,10 @@ FROM evaluation_statuses s JOIN rule_instances ri ON ere.rule_id = ri.id JOIN rule_type rt ON ri.rule_type_id = rt.id JOIN profiles p ON ri.profile_id = p.id - LEFT JOIN repositories r ON ere.repository_id = r.id - LEFT JOIN pull_requests pr ON ere.pull_request_id = pr.id - LEFT JOIN artifacts a ON ere.artifact_id = a.id + JOIN projects j ON ei.project_id = j.id + JOIN entity_instances ei ON ere.entity_instance_id = ei.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 s.id = sqlc.arg(evaluation_id) AND j.id = sqlc.arg(project_id); -- name: ListEvaluationHistory :many @@ -146,19 +123,7 @@ SELECT s.id::uuid AS evaluation_id, s.evaluation_time as evaluated_at, ere.entity_type, -- entity id - CAST( - CASE - WHEN ere.repository_id IS NOT NULL THEN r.id - WHEN ere.pull_request_id IS NOT NULL THEN pr.id - WHEN ere.artifact_id IS NOT NULL THEN a.id - END AS uuid - ) AS entity_id, - ere.entity_instance_id as new_entity_id, - -- raw fields for entity names - r.repo_owner, - r.repo_name, - pr.pr_number, - a.artifact_name, + ere.entity_instance_id as entity_id, j.id as project_id, -- rule type, name, and profile rt.name AS rule_type, @@ -179,28 +144,22 @@ SELECT s.id::uuid AS evaluation_id, JOIN rule_instances ri ON ere.rule_id = ri.id JOIN rule_type rt ON ri.rule_type_id = rt.id JOIN profiles p ON ri.profile_id = p.id - LEFT JOIN repositories r ON ere.repository_id = r.id - LEFT JOIN pull_requests pr ON ere.pull_request_id = pr.id - LEFT JOIN artifacts a ON ere.artifact_id = a.id + JOIN entity_instances ei ON ere.entity_instance_id = ei.id + JOIN projects j ON ei.project_id = j.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.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 ere.entity_type = 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[])) - AND (sqlc.slice(entityNames)::text[] IS NULL OR ere.pull_request_id IS NULL OR pr.pr_number::text = ANY(sqlc.slice(entityNames)::text[])) - AND (sqlc.slice(entityNames)::text[] IS NULL OR ere.artifact_id IS NULL OR a.artifact_name = ANY(sqlc.slice(entityNames)::text[])) + AND (sqlc.slice(entityNames)::text[] IS NULL OR ei.name = ANY(sqlc.slice(entityNames)::text[])) AND (sqlc.slice(profileNames)::text[] IS NULL OR p.name = ANY(sqlc.slice(profileNames)::text[])) AND (sqlc.slice(remediations)::remediation_status_types[] IS NULL OR re.status = ANY(sqlc.slice(remediations)::remediation_status_types[])) AND (sqlc.slice(alerts)::alert_status_types[] IS NULL OR ae.status = ANY(sqlc.slice(alerts)::alert_status_types[])) AND (sqlc.slice(statuses)::eval_status_types[] IS NULL OR s.status = ANY(sqlc.slice(statuses)::eval_status_types[])) -- exclusion filters AND (sqlc.slice(notEntityTypes)::entities[] IS NULL OR ere.entity_type != ALL(sqlc.slice(notEntityTypes)::entities[])) - AND (sqlc.slice(notEntityNames)::text[] IS NULL OR ere.repository_id IS NULL OR CONCAT(r.repo_owner, '/', r.repo_name) != ALL(sqlc.slice(notEntityNames)::text[])) - AND (sqlc.slice(notEntityNames)::text[] IS NULL OR ere.pull_request_id IS NULL OR pr.pr_number::text != ALL(sqlc.slice(notEntityNames)::text[])) - AND (sqlc.slice(notEntityNames)::text[] IS NULL OR ere.artifact_id IS NULL OR a.artifact_name != ALL(sqlc.slice(notEntityNames)::text[])) + AND (sqlc.slice(notEntityNames)::text[] IS NULL OR ei.name != ALL(sqlc.slice(notEntityNames)::text[])) AND (sqlc.slice(notProfileNames)::text[] IS NULL OR p.name != ALL(sqlc.slice(notProfileNames)::text[])) AND (sqlc.slice(notRemediations)::remediation_status_types[] IS NULL OR re.status != ALL(sqlc.slice(notRemediations)::remediation_status_types[])) AND (sqlc.slice(notAlerts)::alert_status_types[] IS NULL OR ae.status != ALL(sqlc.slice(notAlerts)::alert_status_types[])) @@ -220,22 +179,9 @@ SELECT s.evaluation_time, s.id, ere.rule_id, -- entity type - CAST( - CASE - WHEN ere.repository_id IS NOT NULL THEN 1 - WHEN ere.pull_request_id IS NOT NULL THEN 2 - WHEN ere.artifact_id IS NOT NULL THEN 3 - END AS integer - ) AS entity_type, + ere.entity_type, -- entity id - CAST( - CASE - WHEN ere.repository_id IS NOT NULL THEN ere.repository_id - WHEN ere.pull_request_id IS NOT NULL THEN ere.pull_request_id - WHEN ere.artifact_id IS NOT NULL THEN ere.artifact_id - END AS uuid - ) AS entity_id, - ere.entity_instance_id as new_entity_id + ere.entity_instance_id as entity_id FROM evaluation_statuses s JOIN evaluation_rule_entities ere ON s.rule_entity_id = ere.id LEFT JOIN latest_evaluation_statuses l diff --git a/database/query/profile_status.sql b/database/query/profile_status.sql index 5075f96cc1..aaf69e5a6d 100644 --- a/database/query/profile_status.sql +++ b/database/query/profile_status.sql @@ -17,12 +17,13 @@ WHERE p.project_id = $1; -- cast after MIN is required due to a known bug in sqlc: https://github.com/sqlc-dev/sqlc/issues/1965 -- name: ListOldestRuleEvaluationsByRepositoryId :many -SELECT ere.repository_id::uuid AS repository_id, MIN(es.evaluation_time)::timestamp AS oldest_last_updated +SELECT ere.entity_instance_id::uuid AS repository_id, MIN(es.evaluation_time)::timestamp AS oldest_last_updated FROM evaluation_rule_entities AS ere INNER JOIN latest_evaluation_statuses AS les ON ere.id = les.rule_entity_id INNER JOIN evaluation_statuses AS es ON les.evaluation_history_id = es.id -WHERE ere.repository_id = ANY (sqlc.arg('repository_ids')::uuid[]) -GROUP BY ere.repository_id; +WHERE ere.entity_instance_id = ANY (sqlc.arg('repository_ids')::uuid[]) +AND ere.entity_type = 'repository' +GROUP BY ere.entity_instance_id; -- name: ListRuleEvaluationsByProfileId :many WITH @@ -66,23 +67,16 @@ SELECT ad.alert_metadata, ad.alert_last_updated, ed.id AS rule_evaluation_id, - ere.repository_id, ere.entity_type, ri.name AS rule_name, - repo.repo_name, - repo.repo_owner, - repo.provider, + prov.name AS provider, rt.name AS rule_type_name, rt.severity_value as rule_type_severity_value, rt.id AS rule_type_id, rt.guidance as rule_type_guidance, rt.display_name as rule_type_display_name, - -- TODO: store entity ID directly in evaluation_rule_entities - CASE - WHEN ere.entity_type = 'artifact'::entities THEN ere.artifact_id - WHEN ere.entity_type = 'repository'::entities THEN ere.repository_id - WHEN ere.entity_type = 'pull_request'::entities THEN ere.pull_request_id - END::uuid as entity_id, + ere.entity_instance_id as entity_id, + ei.project_id as project_id, rt.release_phase as rule_type_release_phase FROM latest_evaluation_statuses les INNER JOIN evaluation_rule_entities ere ON ere.id = les.rule_entity_id @@ -91,16 +85,10 @@ FROM latest_evaluation_statuses les INNER JOIN alert_details ad ON ad.evaluation_id = les.evaluation_history_id INNER JOIN rule_instances AS ri ON ri.id = ere.rule_id INNER JOIN rule_type rt ON rt.id = ri.rule_type_id - LEFT JOIN repositories repo ON repo.id = ere.repository_id + INNER JOIN entity_instances ei ON ei.id = ere.entity_instance_id + INNER JOIN providers prov ON prov.id = ei.provider_id WHERE les.profile_id = $1 AND - ( - CASE - WHEN sqlc.narg(entity_type)::entities = 'repository' AND ere.repository_id = sqlc.narg(entity_id)::UUID THEN true - WHEN sqlc.narg(entity_type)::entities = 'artifact' AND ere.artifact_id = sqlc.narg(entity_id)::UUID THEN true - WHEN sqlc.narg(entity_type)::entities = 'pull_request' AND ere.pull_request_id = sqlc.narg(entity_id)::UUID THEN true - WHEN sqlc.narg(entity_id)::UUID IS NULL THEN true - ELSE false - END - ) AND (rt.name = sqlc.narg(rule_type_name) OR sqlc.narg(rule_type_name) IS NULL) - AND (lower(ri.name) = lower(sqlc.narg(rule_name)) OR sqlc.narg(rule_name) IS NULL) + (ere.entity_instance_id = sqlc.narg(entity_id)::UUID OR sqlc.narg(entity_id)::UUID IS NULL) + AND (rt.name = sqlc.narg(rule_type_name) OR sqlc.narg(rule_type_name) IS NULL) + AND (lower(ri.name) = lower(sqlc.narg(rule_name)) OR sqlc.narg(rule_name) IS NULL) ; diff --git a/database/query/repositories.sql b/database/query/repositories.sql index 770d36555a..e0a1a6cfe3 100644 --- a/database/query/repositories.sql +++ b/database/query/repositories.sql @@ -82,12 +82,3 @@ WHERE project_id = $1; SELECT repo_owner, repo_name, webhook_id FROM repositories WHERE webhook_id IS NOT NULL AND provider_id = $1; --- name: GetRepoPathFromArtifactID :one -SELECT r.repo_owner AS owner , r.repo_name AS name FROM repositories AS r -JOIN artifacts AS a ON a.repository_id = r.id -WHERE a.id = $1; - --- name: GetRepoPathFromPullRequestID :one -SELECT r.repo_owner AS owner , r.repo_name AS name FROM repositories AS r -JOIN pull_requests AS p ON p.repository_id = r.id -WHERE p.id = $1; \ No newline at end of file diff --git a/internal/controlplane/handlers_evalstatus.go b/internal/controlplane/handlers_evalstatus.go index ec80a6c881..a8c91fbf9b 100644 --- a/internal/controlplane/handlers_evalstatus.go +++ b/internal/controlplane/handlers_evalstatus.go @@ -31,10 +31,14 @@ import ( "github.com/stacklok/minder/internal/db" "github.com/stacklok/minder/internal/engine/engcontext" + entmodels "github.com/stacklok/minder/internal/entities/models" + "github.com/stacklok/minder/internal/entities/properties" "github.com/stacklok/minder/internal/history" + ghprop "github.com/stacklok/minder/internal/providers/github/properties" "github.com/stacklok/minder/internal/ruletypes" "github.com/stacklok/minder/internal/util" minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" + provifv1 "github.com/stacklok/minder/pkg/providers/v1" ) const ( @@ -77,25 +81,13 @@ func (s *Server) GetEvaluationHistory( return nil, status.Error(codes.Internal, evalErrMsg) } - entityName, err := getEntityName( - eval.EntityType, - eval.RepoOwner, - eval.RepoName, - eval.PrNumber, - eval.ArtifactName, - ) - if err != nil { - zerolog.Ctx(ctx).Error().Err(err).Msg(evalErrMsg) - return nil, status.Error(codes.Internal, evalErrMsg) - } - pbEval := &minderv1.EvaluationHistory{ Id: eval.EvaluationID.String(), EvaluatedAt: timestamppb.New(eval.EvaluatedAt), Entity: &minderv1.EvaluationHistoryEntity{ Id: eval.EntityID.String(), Type: dbEntityToEntity(eval.EntityType), - Name: entityName, + Name: eval.EntityName, }, Rule: &minderv1.EvaluationHistoryRule{ Name: eval.RuleName, @@ -220,48 +212,39 @@ func (s *Server) ListEvaluationHistory( } func fromEvaluationHistoryRows( - rows []db.ListEvaluationHistoryRow, + rows []*history.OneEvalHistoryAndEntity, ) ([]*minderv1.EvaluationHistory, error) { res := make([]*minderv1.EvaluationHistory, len(rows)) for i, row := range rows { - entityType := dbEntityToEntity(row.EntityType) - entityName, err := getEntityName( - row.EntityType, - row.RepoOwner, - row.RepoName, - row.PrNumber, - row.ArtifactName, - ) - if err != nil { - return nil, err - } + entityType := row.Entity.Type + entityName := row.Entity.Name - ruleSeverity, err := dbSeverityToSeverity(row.RuleSeverity) + ruleSeverity, err := dbSeverityToSeverity(row.EvalHistoryRow.RuleSeverity) if err != nil { return nil, err } res[i] = &minderv1.EvaluationHistory{ - Id: row.EvaluationID.String(), - EvaluatedAt: timestamppb.New(row.EvaluatedAt), + Id: row.EvalHistoryRow.EvaluationID.String(), + EvaluatedAt: timestamppb.New(row.EvalHistoryRow.EvaluatedAt), Entity: &minderv1.EvaluationHistoryEntity{ - Id: row.EntityID.String(), + Id: row.Entity.ID.String(), Type: entityType, Name: entityName, }, Rule: &minderv1.EvaluationHistoryRule{ - Name: row.RuleName, - RuleType: row.RuleType, + Name: row.EvalHistoryRow.RuleName, + RuleType: row.EvalHistoryRow.RuleType, Severity: ruleSeverity, - Profile: row.ProfileName, + Profile: row.EvalHistoryRow.ProfileName, }, Status: &minderv1.EvaluationHistoryStatus{ - Status: string(row.EvaluationStatus), - Details: row.EvaluationDetails, + Status: string(row.EvalHistoryRow.EvaluationStatus), + Details: row.EvalHistoryRow.EvaluationDetails, }, - Alert: getAlert(row.AlertStatus, row.AlertDetails.String), - Remediation: getRemediation(row.RemediationStatus, row.RemediationDetails.String), + Alert: getAlert(row.EvalHistoryRow.AlertStatus, row.EvalHistoryRow.AlertDetails.String), + Remediation: getRemediation(row.EvalHistoryRow.RemediationStatus, row.EvalHistoryRow.RemediationDetails.String), } } @@ -354,7 +337,7 @@ func (s *Server) ListEvaluationResults( profileList, profileStatusList = filterProfileLists(in, profileList, profileStatusList) // Do the final sort of all the data - entities, profileStatuses, statusByEntity, err := sortEntitiesEvaluationStatus( + entities, profileStatuses, statusByEntity, err := s.sortEntitiesEvaluationStatus( ctx, s.store, profileList, profileStatusList, rtIndex, entIdIndex, entTypeIndex, ) if err != nil { @@ -368,7 +351,9 @@ func (s *Server) ListEvaluationResults( // and compiles the unified entities map, the map of profile statuses and the // status by entity map that will be assembled into the evaluation status // response. -func sortEntitiesEvaluationStatus( +// +//nolint:gocyclo // This function is complex by nature +func (s *Server) sortEntitiesEvaluationStatus( ctx context.Context, store db.Store, profileList []db.ListProfilesByProjectIDAndLabelRow, profileStatusList map[uuid.UUID]db.GetProfileStatusByProjectRow, @@ -388,6 +373,10 @@ func sortEntitiesEvaluationStatus( ctx, db.ListRuleEvaluationsByProfileIdParams{ProfileID: p.Profile.ID}, ) if err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, nil, nil, + util.UserVisibleError(codes.NotFound, "profile not found") + } return nil, nil, nil, status.Errorf(codes.Internal, "error reading evaluations from profile %q: %v", p.Profile.ID.String(), err) @@ -399,7 +388,23 @@ func sortEntitiesEvaluationStatus( continue } - ent := buildEntityFromEvaluation(e) + efp, err := s.props.EntityWithProperties(ctx, e.EntityID, e.ProjectID, nil) + if err != nil { + if errors.Is(err, sql.ErrNoRows) || errors.Is(err, provifv1.ErrEntityNotFound) { + // If the entity is not found, log and skip + zerolog.Ctx(ctx).Error(). + Str("entity_id", e.EntityID.String()). + Err(err).Msg("Entity not found while building rule evaluation status") + continue + } + + zerolog.Ctx(ctx).Error(). + Str("entity_id", e.EntityID.String()). + Err(err).Msg("error building entity for rule evaluation status") + continue + } + + ent := buildEntityFromEvaluation(efp) entString := fmt.Sprintf("%s/%s", ent.Type, ent.Id) /// If we're constrained to a single entity type, ignore others @@ -418,7 +423,7 @@ func sortEntitiesEvaluationStatus( profileStatuses[p.Profile.ID] = buildProfileStatus(&p, profileStatusList) } - stat, err := buildRuleEvaluationStatusFromDBEvaluation(ctx, &p, e) + stat, err := s.buildRuleEvaluationStatusFromDBEvaluation(ctx, &p, e, efp) if err != nil { // A failure parsing the PR metadata points to a corrupt record. Log but don't err. zerolog.Ctx(ctx).Error().Err(err).Msg("error building rule evaluation status") @@ -564,9 +569,10 @@ func filterProfileLists( // buildRuleEvaluationStatusFromDBEvaluation converts from an evaluation and a // profile from the database to a minder RuleEvaluationStatus -func buildRuleEvaluationStatusFromDBEvaluation( +func (s *Server) buildRuleEvaluationStatusFromDBEvaluation( ctx context.Context, profile *db.ListProfilesByProjectIDAndLabelRow, eval db.ListRuleEvaluationsByProfileIdRow, + efp *entmodels.EntityWithProperties, ) (*minderv1.RuleEvaluationStatus, error) { guidance := "" // Only return the rule type guidance text when there is a problem @@ -586,17 +592,36 @@ func buildRuleEvaluationStatusFromDBEvaluation( Msg("error converting severity will use defaults") } + err = s.props.RetrieveAllPropertiesForEntity(ctx, efp, s.providerManager) + if err != nil { + return nil, fmt.Errorf("error fetching properties for entity: %w", err) + } + entityInfo := map[string]string{} + entityInfo["entity_id"] = eval.EntityID.String() + + if uid := efp.Properties.GetProperty(properties.PropertyUpstreamID); uid != nil { + entityInfo["upstream_id"] = uid.GetString() + } + remediationURL := "" if eval.EntityType == db.EntitiesRepository { // If any fields are missing, just leave them empty in the response - entityInfo["provider"] = eval.Provider.String - entityInfo["repo_owner"] = eval.RepoOwner.String - entityInfo["repo_name"] = eval.RepoName.String - entityInfo["repository_id"] = eval.RepositoryID.UUID.String() + entityInfo["provider"] = eval.Provider + // TODO: We'll probably remove these fields in the future as we + // introduce more providers + if owner := efp.Properties.GetProperty(ghprop.RepoPropertyOwner); owner != nil { + entityInfo["repo_owner"] = owner.GetString() + } + if name := efp.Properties.GetProperty(ghprop.RepoPropertyName); name != nil { + entityInfo["repo_name"] = name.GetString() + } + + // TODO: This will be removed in favor of entity_id + entityInfo["repository_id"] = efp.Entity.ID.String() remediationURL, err = getRemediationURLFromMetadata( - eval.RemMetadata, fmt.Sprintf("%s/%s", eval.RepoOwner.String, eval.RepoName.String), + eval.RemMetadata, efp.Entity.Name, ) if err != nil { return nil, fmt.Errorf("parsing remediation pull request data: %w", err) @@ -631,19 +656,19 @@ func buildRuleEvaluationStatusFromDBEvaluation( RemediationUrl: remediationURL, RuleDisplayName: nString, RuleTypeName: eval.RuleTypeName, - Alert: buildEvalResultAlertFromLRERow(&eval), + Alert: buildEvalResultAlertFromLRERow(&eval, efp), Severity: sev, ReleasePhase: rp, }, nil } -func buildEntityFromEvaluation(eval db.ListRuleEvaluationsByProfileIdRow) *minderv1.EntityTypedId { +func buildEntityFromEvaluation(efp *entmodels.EntityWithProperties) *minderv1.EntityTypedId { ent := &minderv1.EntityTypedId{ - Type: dbEntityToEntity(eval.EntityType), + Type: efp.Entity.Type, } - if ent.Type == minderv1.Entity_ENTITY_REPOSITORIES && eval.RepoOwner.String != "" && eval.RepoName.String != "" { - ent.Id = eval.RepositoryID.UUID.String() + if ent.Type == minderv1.Entity_ENTITY_REPOSITORIES { + ent.Id = efp.Entity.ID.String() } return ent } @@ -674,7 +699,9 @@ func buildProfileStatus( // buildEvalResultAlertFromLRERow build the evaluation result alert from a // database row. -func buildEvalResultAlertFromLRERow(eval *db.ListRuleEvaluationsByProfileIdRow) *minderv1.EvalResultAlert { +func buildEvalResultAlertFromLRERow( + eval *db.ListRuleEvaluationsByProfileIdRow, ent *entmodels.EntityWithProperties, +) *minderv1.EvalResultAlert { era := &minderv1.EvalResultAlert{ Status: string(eval.AlertStatus), LastUpdated: timestamppb.New(eval.AlertLastUpdated), @@ -682,12 +709,8 @@ func buildEvalResultAlertFromLRERow(eval *db.ListRuleEvaluationsByProfileIdRow) } if eval.AlertStatus == db.AlertStatusTypesOn { - repoSlug := "" - if eval.RepoOwner.String != "" && eval.RepoName.String != "" { - repoSlug = fmt.Sprintf("%s/%s", eval.RepoOwner.String, eval.RepoName.String) - } urlString, err := getAlertURLFromMetadata( - eval.AlertMetadata, repoSlug, + eval.AlertMetadata, ent.Entity.Name, ) if err == nil { era.Url = urlString @@ -732,53 +755,3 @@ func dbSeverityToSeverity(dbSev db.Severity) (*minderv1.Severity, error) { return severity, nil } - -// ugly function signature needed to handle the fact that we have two different -// row types with the same fields, but no mechanism in the language to treat -// them as the same thing. -func getEntityName( - entityType db.Entities, - repoOwner sql.NullString, - repoName sql.NullString, - prNumber sql.NullInt64, - artifactName sql.NullString, -) (string, error) { - switch entityType { - case db.EntitiesPullRequest: - if !repoOwner.Valid { - return "", errors.New("repo_owner is missing") - } - if !repoName.Valid { - return "", errors.New("repo_name is missing") - } - if !prNumber.Valid { - return "", errors.New("pr_number is missing") - } - return fmt.Sprintf("%s/%s#%d", - repoOwner.String, - repoName.String, - prNumber.Int64, - ), nil - case db.EntitiesArtifact: - if !artifactName.Valid { - return "", errors.New("artifact_name is missing") - } - return artifactName.String, nil - case db.EntitiesRepository: - if !repoOwner.Valid { - return "", errors.New("repo_owner is missing") - } - if !repoName.Valid { - return "", errors.New("repo_name is missing") - } - return fmt.Sprintf("%s/%s", - repoOwner.String, - repoName.String, - ), nil - case db.EntitiesBuildEnvironment, db.EntitiesRelease, - db.EntitiesPipelineRun, db.EntitiesTaskRun, db.EntitiesBuild: - return "", errors.New("invalid entity type") - default: - return "", errors.New("invalid entity type") - } -} diff --git a/internal/controlplane/handlers_evalstatus_test.go b/internal/controlplane/handlers_evalstatus_test.go index 6fdff006ee..c9e018eb6c 100644 --- a/internal/controlplane/handlers_evalstatus_test.go +++ b/internal/controlplane/handlers_evalstatus_test.go @@ -25,6 +25,8 @@ import ( "google.golang.org/protobuf/types/known/timestamppb" "github.com/stacklok/minder/internal/db" + entmodels "github.com/stacklok/minder/internal/entities/models" + "github.com/stacklok/minder/internal/history" minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" ) @@ -34,6 +36,7 @@ func TestBuildEvalResultAlertFromLRERow(t *testing.T) { for _, tc := range []struct { name string sut *db.ListRuleEvaluationsByProfileIdRow + efp *entmodels.EntityWithProperties expect *minderv1.EvalResultAlert }{ { @@ -43,15 +46,12 @@ func TestBuildEvalResultAlertFromLRERow(t *testing.T) { AlertLastUpdated: d, AlertDetails: "details go here", AlertMetadata: []byte(`{"ghsa_id": "GHAS-advisory_ID_here"}`), - RepoOwner: sql.NullString{ - String: "example", - Valid: true, - }, - RepoName: sql.NullString{ - String: "test", - Valid: true, - }, }, + efp: entmodels.NewEntityWithPropertiesFromInstance(entmodels.EntityInstance{ + ID: uuid.New(), + Type: minderv1.Entity_ENTITY_REPOSITORIES, + Name: "example/test", + }, nil), expect: &minderv1.EvalResultAlert{ Status: string(db.AlertStatusTypesOn), LastUpdated: timestamppb.New(d), @@ -66,6 +66,11 @@ func TestBuildEvalResultAlertFromLRERow(t *testing.T) { AlertLastUpdated: d, AlertDetails: "details go here", }, + efp: entmodels.NewEntityWithPropertiesFromInstance(entmodels.EntityInstance{ + ID: uuid.New(), + Type: minderv1.Entity_ENTITY_REPOSITORIES, + Name: "example/test", + }, nil), expect: &minderv1.EvalResultAlert{ Status: string(db.AlertStatusTypesOn), LastUpdated: timestamppb.New(d), @@ -80,15 +85,12 @@ func TestBuildEvalResultAlertFromLRERow(t *testing.T) { AlertLastUpdated: d, AlertDetails: "details go here", AlertMetadata: []byte(`{"ghsa_id": "GHAS-advisory_ID_here"}`), - RepoOwner: sql.NullString{ - String: "", - Valid: true, - }, - RepoName: sql.NullString{ - String: "test", - Valid: true, - }, }, + efp: entmodels.NewEntityWithPropertiesFromInstance(entmodels.EntityInstance{ + ID: uuid.New(), + Type: minderv1.Entity_ENTITY_REPOSITORIES, + Name: "test", + }, nil), expect: &minderv1.EvalResultAlert{ Status: string(db.AlertStatusTypesOn), LastUpdated: timestamppb.New(d), @@ -100,7 +102,7 @@ func TestBuildEvalResultAlertFromLRERow(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() - res := buildEvalResultAlertFromLRERow(tc.sut) + res := buildEvalResultAlertFromLRERow(tc.sut, tc.efp) require.Equal(t, tc.expect.Details, res.Details) require.Equal(t, tc.expect.LastUpdated, res.LastUpdated) @@ -174,165 +176,6 @@ func TestDBEntityToEntity(t *testing.T) { } } -func TestGetEntityName(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - dbEnt db.Entities - row db.ListEvaluationHistoryRow - output string - err bool - }{ - { - name: "pull request", - dbEnt: db.EntitiesPullRequest, - row: db.ListEvaluationHistoryRow{ - RepoOwner: sql.NullString{ - Valid: true, - String: "stacklok", - }, - RepoName: sql.NullString{ - Valid: true, - String: "minder", - }, - PrNumber: sql.NullInt64{ - Valid: true, - Int64: 12345, - }, - }, - output: "stacklok/minder#12345", - }, - { - name: "pull request no repo owner", - dbEnt: db.EntitiesPullRequest, - row: db.ListEvaluationHistoryRow{ - RepoName: sql.NullString{ - Valid: true, - String: "minder", - }, - PrNumber: sql.NullInt64{ - Valid: true, - Int64: 12345, - }, - }, - err: true, - }, - { - name: "pull request no repo name", - dbEnt: db.EntitiesPullRequest, - row: db.ListEvaluationHistoryRow{ - RepoOwner: sql.NullString{ - Valid: true, - String: "stacklok", - }, - PrNumber: sql.NullInt64{ - Valid: true, - Int64: 12345, - }, - }, - err: true, - }, - { - name: "pull request no pr number", - dbEnt: db.EntitiesPullRequest, - row: db.ListEvaluationHistoryRow{ - RepoOwner: sql.NullString{ - Valid: true, - String: "stacklok", - }, - RepoName: sql.NullString{ - Valid: true, - String: "minder", - }, - }, - err: true, - }, - { - name: "artifact", - dbEnt: db.EntitiesArtifact, - row: db.ListEvaluationHistoryRow{ - ArtifactName: sql.NullString{ - Valid: true, - String: "artifact name", - }, - }, - output: "artifact name", - }, - { - name: "repository", - dbEnt: db.EntitiesRepository, - row: db.ListEvaluationHistoryRow{ - RepoOwner: sql.NullString{ - Valid: true, - String: "stacklok", - }, - RepoName: sql.NullString{ - Valid: true, - String: "minder", - }, - }, - output: "stacklok/minder", - }, - { - name: "repository no repo owner", - dbEnt: db.EntitiesRepository, - row: db.ListEvaluationHistoryRow{ - RepoName: sql.NullString{ - Valid: true, - String: "minder", - }, - }, - err: true, - }, - { - name: "repository no repo name", - dbEnt: db.EntitiesRepository, - row: db.ListEvaluationHistoryRow{ - RepoOwner: sql.NullString{ - Valid: true, - String: "stacklok", - }, - }, - err: true, - }, - { - name: "build environments", - dbEnt: db.EntitiesBuildEnvironment, - row: db.ListEvaluationHistoryRow{}, - err: true, - }, - { - name: "default", - dbEnt: db.Entities("whatever"), - row: db.ListEvaluationHistoryRow{}, - err: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - res, err := getEntityName( - tt.dbEnt, - tt.row.RepoOwner, - tt.row.RepoName, - tt.row.PrNumber, - tt.row.ArtifactName, - ) - - if tt.err { - require.Error(t, err) - require.Equal(t, "", res) - return - } - - require.NoError(t, err) - require.Equal(t, tt.output, res) - }) - } -} - func TestFromEvaluationHistoryRows(t *testing.T) { t.Parallel() @@ -344,100 +187,130 @@ func TestFromEvaluationHistoryRows(t *testing.T) { tests := []struct { name string - rows []db.ListEvaluationHistoryRow + rows []*history.OneEvalHistoryAndEntity checkf func(*testing.T, db.ListEvaluationHistoryRow, *minderv1.EvaluationHistory) err bool }{ { name: "empty", - rows: []db.ListEvaluationHistoryRow{}, + rows: []*history.OneEvalHistoryAndEntity{}, }, { name: "happy path", - rows: []db.ListEvaluationHistoryRow{ + rows: []*history.OneEvalHistoryAndEntity{ { - EvaluationID: uuid1, - EvaluatedAt: now, - EntityType: db.EntitiesRepository, - EntityID: entityid1, - RepoOwner: nullStr("stacklok"), - RepoName: nullStr("minder"), - ProjectID: uuid.NullUUID{}, - RuleType: "rule_type", - RuleName: "rule_name", - RuleSeverity: "unknown", - ProfileName: "profile_name", + EntityWithProperties: entmodels.NewEntityWithPropertiesFromInstance( + entmodels.EntityInstance{ + ID: entityid1, + Type: minderv1.Entity_ENTITY_REPOSITORIES, + Name: "stacklok/minder", + }, nil), + EvalHistoryRow: db.ListEvaluationHistoryRow{ + EvaluationID: uuid1, + EvaluatedAt: now, + EntityType: db.EntitiesRepository, + EntityID: entityid1, + ProjectID: uuid.New(), + RuleType: "rule_type", + RuleName: "rule_name", + RuleSeverity: "unknown", + ProfileName: "profile_name", + }, }, }, }, { name: "order preserved", - rows: []db.ListEvaluationHistoryRow{ + rows: []*history.OneEvalHistoryAndEntity{ { - EvaluationID: uuid1, - EvaluatedAt: now, - EntityType: db.EntitiesRepository, - EntityID: entityid1, - RepoOwner: nullStr("stacklok"), - RepoName: nullStr("minder"), - ProjectID: uuid.NullUUID{}, - RuleType: "rule_type", - RuleName: "rule_name", - RuleSeverity: "unknown", - ProfileName: "profile_name", + EntityWithProperties: entmodels.NewEntityWithPropertiesFromInstance( + entmodels.EntityInstance{ + ID: entityid1, + Type: minderv1.Entity_ENTITY_REPOSITORIES, + Name: "stacklok/minder", + }, nil), + EvalHistoryRow: db.ListEvaluationHistoryRow{ + EvaluationID: uuid1, + EvaluatedAt: now, + EntityType: db.EntitiesRepository, + EntityID: entityid1, + ProjectID: uuid.New(), + RuleType: "rule_type", + RuleName: "rule_name", + RuleSeverity: "unknown", + ProfileName: "profile_name", + }, }, { - EvaluationID: uuid2, - EvaluatedAt: now, - EntityType: db.EntitiesRepository, - EntityID: entityid2, - RepoOwner: nullStr("stacklok"), - RepoName: nullStr("frizbee"), - ProjectID: uuid.NullUUID{}, - RuleType: "rule_type", - RuleName: "rule_name", - RuleSeverity: "unknown", - ProfileName: "profile_name", + EntityWithProperties: entmodels.NewEntityWithPropertiesFromInstance( + entmodels.EntityInstance{ + ID: entityid2, + Type: minderv1.Entity_ENTITY_REPOSITORIES, + Name: "stacklok/frizbee", + }, nil), + EvalHistoryRow: db.ListEvaluationHistoryRow{ + EvaluationID: uuid2, + EvaluatedAt: now, + EntityType: db.EntitiesRepository, + EntityID: entityid2, + ProjectID: uuid.New(), + RuleType: "rule_type", + RuleName: "rule_name", + RuleSeverity: "unknown", + ProfileName: "profile_name", + }, }, }, }, { name: "optional alert", - rows: []db.ListEvaluationHistoryRow{ + rows: []*history.OneEvalHistoryAndEntity{ { - EvaluationID: uuid1, - EvaluatedAt: now, - EntityType: db.EntitiesRepository, - EntityID: entityid1, - RepoOwner: nullStr("stacklok"), - RepoName: nullStr("minder"), - ProjectID: uuid.NullUUID{}, - RuleType: "rule_type", - RuleName: "rule_name", - RuleSeverity: "unknown", - ProfileName: "profile_name", - AlertStatus: nullAlertStatusOK(), - AlertDetails: nullStr("alert details"), + EntityWithProperties: entmodels.NewEntityWithPropertiesFromInstance( + entmodels.EntityInstance{ + ID: entityid1, + Type: minderv1.Entity_ENTITY_REPOSITORIES, + Name: "stacklok/minder", + }, nil), + EvalHistoryRow: db.ListEvaluationHistoryRow{ + EvaluationID: uuid1, + EvaluatedAt: now, + EntityType: db.EntitiesRepository, + EntityID: entityid1, + ProjectID: uuid.New(), + RuleType: "rule_type", + RuleName: "rule_name", + RuleSeverity: "unknown", + ProfileName: "profile_name", + AlertStatus: nullAlertStatusOK(), + AlertDetails: nullStr("alert details"), + }, }, }, }, { name: "optional remediation", - rows: []db.ListEvaluationHistoryRow{ + rows: []*history.OneEvalHistoryAndEntity{ { - EvaluationID: uuid1, - EvaluatedAt: now, - EntityType: db.EntitiesRepository, - EntityID: entityid1, - RepoOwner: nullStr("stacklok"), - RepoName: nullStr("minder"), - ProjectID: uuid.NullUUID{}, - RuleType: "rule_type", - RuleName: "rule_name", - RuleSeverity: "unknown", - ProfileName: "profile_name", - RemediationStatus: nullRemediationStatusTypesSuccess(), - RemediationDetails: nullStr("remediation details"), + EntityWithProperties: entmodels.NewEntityWithPropertiesFromInstance( + entmodels.EntityInstance{ + ID: entityid1, + Type: minderv1.Entity_ENTITY_REPOSITORIES, + Name: "stacklok/minder", + }, nil), + EvalHistoryRow: db.ListEvaluationHistoryRow{ + EvaluationID: uuid1, + EvaluatedAt: now, + EntityType: db.EntitiesRepository, + EntityID: entityid1, + ProjectID: uuid.New(), + RuleType: "rule_type", + RuleName: "rule_name", + RuleSeverity: "unknown", + ProfileName: "profile_name", + RemediationStatus: nullRemediationStatusTypesSuccess(), + RemediationDetails: nullStr("remediation details"), + }, }, }, }, @@ -459,37 +332,37 @@ func TestFromEvaluationHistoryRows(t *testing.T) { for i := 0; i < len(tt.rows); i++ { row := tt.rows[i] item := res[i] - require.Equal(t, row.EvaluationID.String(), item.Id) - require.Equal(t, row.EvaluatedAt, item.EvaluatedAt.AsTime()) - require.Equal(t, row.EntityID.String(), item.Entity.Id) - require.Equal(t, dbEntityToEntity(row.EntityType), item.Entity.Type) - require.Equal(t, row.RuleType, item.Rule.RuleType) - require.Equal(t, row.RuleName, item.Rule.Name) - sev, err := dbSeverityToSeverity(row.RuleSeverity) + require.Equal(t, row.EvalHistoryRow.EvaluationID.String(), item.Id) + require.Equal(t, row.EvalHistoryRow.EvaluatedAt, item.EvaluatedAt.AsTime()) + require.Equal(t, row.Entity.ID.String(), item.Entity.Id) + require.Equal(t, row.Entity.Type, item.Entity.Type) + require.Equal(t, row.EvalHistoryRow.RuleType, item.Rule.RuleType) + require.Equal(t, row.EvalHistoryRow.RuleName, item.Rule.Name) + sev, err := dbSeverityToSeverity(row.EvalHistoryRow.RuleSeverity) require.NoError(t, err) require.Equal(t, sev, item.Rule.Severity) - require.Equal(t, row.ProfileName, item.Rule.Profile) + require.Equal(t, row.EvalHistoryRow.ProfileName, item.Rule.Profile) - require.Equal(t, row.AlertStatus.Valid, item.Alert != nil) - if row.AlertStatus.Valid { + require.Equal(t, row.EvalHistoryRow.AlertStatus.Valid, item.Alert != nil) + if row.EvalHistoryRow.AlertStatus.Valid { require.Equal(t, - string(row.AlertStatus.AlertStatusTypes), + string(row.EvalHistoryRow.AlertStatus.AlertStatusTypes), item.Alert.Status, ) require.Equal(t, - row.AlertDetails.String, + row.EvalHistoryRow.AlertDetails.String, item.Alert.Details, ) } - require.Equal(t, row.RemediationStatus.Valid, item.Remediation != nil) - if row.RemediationStatus.Valid { + require.Equal(t, row.EvalHistoryRow.RemediationStatus.Valid, item.Remediation != nil) + if row.EvalHistoryRow.RemediationStatus.Valid { require.Equal(t, - string(row.RemediationStatus.RemediationStatusTypes), + string(row.EvalHistoryRow.RemediationStatus.RemediationStatusTypes), item.Remediation.Status, ) require.Equal(t, - string(row.RemediationDetails.String), + string(row.EvalHistoryRow.RemediationDetails.String), item.Remediation.Details, ) } diff --git a/internal/controlplane/handlers_profile.go b/internal/controlplane/handlers_profile.go index 878c933757..057fe02716 100644 --- a/internal/controlplane/handlers_profile.go +++ b/internal/controlplane/handlers_profile.go @@ -31,11 +31,14 @@ import ( "github.com/stacklok/minder/internal/db" "github.com/stacklok/minder/internal/engine/engcontext" "github.com/stacklok/minder/internal/engine/entities" + entmodels "github.com/stacklok/minder/internal/entities/models" "github.com/stacklok/minder/internal/logger" prof "github.com/stacklok/minder/internal/profiles" + ghprop "github.com/stacklok/minder/internal/providers/github/properties" "github.com/stacklok/minder/internal/ruletypes" "github.com/stacklok/minder/internal/util" minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" + provifv1 "github.com/stacklok/minder/pkg/providers/v1" ) // CreateProfile creates a profile for a project @@ -273,41 +276,38 @@ func getProfilePBFromDB( return nil, fmt.Errorf("profile not found") } -func (s *Server) getRuleEvalEntityInfo( - ctx context.Context, - entityType *db.NullEntities, - selector *uuid.NullUUID, +// TODO: We need to replace this with a more generic method that can be used for all entities +// probably coming from the properties. +func getRuleEvalEntityInfo( rs db.ListRuleEvaluationsByProfileIdRow, - projectID uuid.UUID, + efp *entmodels.EntityWithProperties, ) map[string]string { - l := zerolog.Ctx(ctx) entityInfo := map[string]string{} - if rs.RepositoryID.Valid { - // this is always true now but might not be when we support entities not tied to a repo - entityInfo["repo_name"] = rs.RepoName.String - entityInfo["repo_owner"] = rs.RepoOwner.String - entityInfo["provider"] = rs.Provider.String - entityInfo["repository_id"] = rs.RepositoryID.UUID.String() + if owner := efp.Properties.GetProperty(ghprop.RepoPropertyOwner); owner != nil { + entityInfo["repo_owner"] = owner.GetString() + } + if name := efp.Properties.GetProperty(ghprop.RepoPropertyName); name != nil { + entityInfo["repo_name"] = name.GetString() } - if !selector.Valid || !entityType.Valid { - return entityInfo + if artName := efp.Properties.GetProperty(ghprop.ArtifactPropertyName); artName != nil { + entityInfo["artifact_name"] = artName.GetString() } - if entityType.Entities == db.EntitiesArtifact { - artifact, err := s.store.GetArtifactByID(ctx, db.GetArtifactByIDParams{ - ID: selector.UUID, - ProjectID: projectID, - }) - if err != nil { - l.Err(err).Msg("error getting artifact by ID") - return entityInfo - } - entityInfo["artifact_id"] = artifact.ID.String() - entityInfo["artifact_name"] = artifact.ArtifactName - entityInfo["artifact_type"] = artifact.ArtifactType - entityInfo["provider"] = artifact.ProviderName + if artType := efp.Properties.GetProperty(ghprop.ArtifactPropertyType); artType != nil { + entityInfo["artifact_type"] = artType.GetString() + } + + entityInfo["provider"] = rs.Provider + entityInfo["entity_type"] = efp.Entity.Type.ToString() + entityInfo["entity_id"] = rs.EntityID.String() + + // temporary: These will be replaced by entity_id + if rs.EntityType == db.EntitiesRepository { + entityInfo["repository_id"] = efp.Entity.ID.String() + } else if rs.EntityType == db.EntitiesArtifact { + entityInfo["artifact_id"] = efp.Entity.ID.String() } return entityInfo @@ -319,7 +319,6 @@ func (s *Server) GetProfileStatusByName(ctx context.Context, in *minderv1.GetProfileStatusByNameRequest) (*minderv1.GetProfileStatusByNameResponse, error) { entityCtx := engcontext.EntityFromContext(ctx) - projectID := entityCtx.Project.ID err := entityCtx.ValidateProject(ctx, s.store) if err != nil { @@ -339,13 +338,11 @@ func (s *Server) GetProfileStatusByName(ctx context.Context, var ruleEvaluationStatuses []*minderv1.RuleEvaluationStatus var selector *uuid.NullUUID - var dbEntity *db.NullEntities var ruleType *sql.NullString var ruleName *sql.NullString if in.GetAll() { selector = &uuid.NullUUID{} - dbEntity = &db.NullEntities{} } else if e := in.GetEntity(); e != nil { if !e.GetType().IsValid() { return nil, util.UserVisibleError(codes.InvalidArgument, @@ -356,7 +353,6 @@ func (s *Server) GetProfileStatusByName(ctx context.Context, if err := selector.Scan(e.GetId()); err != nil { return nil, util.UserVisibleError(codes.InvalidArgument, "invalid entity ID in selector") } - dbEntity = &db.NullEntities{Entities: entities.EntityTypeToDB(e.GetType()), Valid: true} } ruleType = &sql.NullString{ @@ -383,7 +379,6 @@ func (s *Server) GetProfileStatusByName(ctx context.Context, dbRuleEvaluationStatuses, err := s.store.ListRuleEvaluationsByProfileId(ctx, db.ListRuleEvaluationsByProfileIdParams{ ProfileID: dbProfileStatus.ID, EntityID: *selector, - EntityType: *dbEntity, RuleTypeName: *ruleType, RuleName: *ruleName, }) @@ -391,11 +386,9 @@ func (s *Server) GetProfileStatusByName(ctx context.Context, return nil, status.Errorf(codes.Unknown, "failed to list rule evaluation status: %s", err) } - ruleEvaluationStatuses = s. - getRuleEvaluationStatuses( - ctx, dbRuleEvaluationStatuses, dbProfileStatus.ID.String(), - dbEntity, selector, projectID, - ) + ruleEvaluationStatuses = s.getRuleEvaluationStatuses( + ctx, dbRuleEvaluationStatuses, dbProfileStatus.ID.String(), + ) // TODO: Add other entities once we have database entries for them } @@ -418,9 +411,6 @@ func (s *Server) getRuleEvaluationStatuses( ctx context.Context, dbRuleEvaluationStatuses []db.ListRuleEvaluationsByProfileIdRow, profileId string, - dbEntity *db.NullEntities, - selector *uuid.NullUUID, - projectID uuid.UUID, ) []*minderv1.RuleEvaluationStatus { ruleEvaluationStatuses := make( []*minderv1.RuleEvaluationStatus, 0, len(dbRuleEvaluationStatuses), @@ -428,7 +418,24 @@ func (s *Server) getRuleEvaluationStatuses( // Loop through the rule evaluation statuses and convert them to protobuf for _, dbRuleEvalStat := range dbRuleEvaluationStatuses { // Get the rule evaluation status - st := s.getRuleEvalStatus(ctx, profileId, dbEntity, selector, dbRuleEvalStat, projectID) + st, err := s.getRuleEvalStatus(ctx, profileId, dbRuleEvalStat) + if err != nil { + if errors.Is(err, sql.ErrNoRows) || errors.Is(err, provifv1.ErrEntityNotFound) { + zerolog.Ctx(ctx).Error(). + Str("profile_id", profileId). + Str("rule_id", dbRuleEvalStat.RuleTypeID.String()). + Str("entity_id", dbRuleEvalStat.EntityID.String()). + Err(err).Msg("entity not found. error getting rule evaluation status") + continue + } + + zerolog.Ctx(ctx).Error(). + Str("profile_id", profileId). + Str("rule_id", dbRuleEvalStat.RuleTypeID.String()). + Str("entity_id", dbRuleEvalStat.EntityID.String()). + Err(err).Msg("error getting rule evaluation status") + continue + } // Append the rule evaluation status to the list ruleEvaluationStatuses = append(ruleEvaluationStatuses, st) } @@ -441,15 +448,23 @@ func (s *Server) getRuleEvaluationStatuses( func (s *Server) getRuleEvalStatus( ctx context.Context, profileID string, - dbEntity *db.NullEntities, - selector *uuid.NullUUID, dbRuleEvalStat db.ListRuleEvaluationsByProfileIdRow, - projectID uuid.UUID, -) *minderv1.RuleEvaluationStatus { +) (*minderv1.RuleEvaluationStatus, error) { l := zerolog.Ctx(ctx) var guidance string var err error + efp, err := s.props.EntityWithProperties( + ctx, dbRuleEvalStat.EntityID, dbRuleEvalStat.ProjectID, nil) + if err != nil { + return nil, fmt.Errorf("error fetching entity for properties: %w", err) + } + + err = s.props.RetrieveAllPropertiesForEntity(ctx, efp, s.providerManager) + if err != nil { + return nil, fmt.Errorf("error fetching properties for entity: %w", err) + } + if dbRuleEvalStat.EvalStatus == db.EvalStatusTypesFailure || dbRuleEvalStat.EvalStatus == db.EvalStatusTypesError { ruleTypeInfo, err := s.store.GetRuleTypeByID(ctx, dbRuleEvalStat.RuleTypeID) @@ -463,7 +478,7 @@ func (s *Server) getRuleEvalStatus( if dbRuleEvalStat.EntityType == db.EntitiesRepository { remediationURL, err = getRemediationURLFromMetadata( dbRuleEvalStat.RemMetadata, - fmt.Sprintf("%s/%s", dbRuleEvalStat.RepoOwner.String, dbRuleEvalStat.RepoName.String), + efp.Entity.Name, ) if err != nil { // A failure parsing the alert metadata points to a corrupt record. Log but don't err. @@ -486,7 +501,7 @@ func (s *Server) getRuleEvalStatus( Entity: string(dbRuleEvalStat.EntityType), Status: string(dbRuleEvalStat.EvalStatus), Details: dbRuleEvalStat.EvalDetails, - EntityInfo: s.getRuleEvalEntityInfo(ctx, dbEntity, selector, dbRuleEvalStat, projectID), + EntityInfo: getRuleEvalEntityInfo(dbRuleEvalStat, efp), Guidance: guidance, LastUpdated: timestamppb.New(dbRuleEvalStat.EvalLastUpdated), RemediationStatus: string(dbRuleEvalStat.RemStatus), @@ -513,19 +528,17 @@ func (s *Server) getRuleEvalStatus( if dbRuleEvalStat.EntityType == db.EntitiesRepository { repoPath = fmt.Sprintf("%s/%s", st.EntityInfo["repo_owner"], st.EntityInfo["repo_name"]) } else if dbRuleEvalStat.EntityType == db.EntitiesArtifact { - repoDetails, err := s.store.GetRepoPathFromArtifactID(ctx, dbRuleEvalStat.EntityID) - if err != nil { - l.Err(err).Msg("error getting repo details from db") - return st + artRepoOwner := efp.Properties.GetProperty(ghprop.ArtifactPropertyRepoOwner).GetString() + artRepoName := efp.Properties.GetProperty(ghprop.ArtifactPropertyRepoName).GetString() + if artRepoOwner != "" && artRepoName != "" { + repoPath = fmt.Sprintf("%s/%s", artRepoOwner, artRepoName) } - repoPath = fmt.Sprintf("%s/%s", repoDetails.Owner, repoDetails.Name) } else if dbRuleEvalStat.EntityType == db.EntitiesPullRequest { - repoDetails, err := s.store.GetRepoPathFromPullRequestID(ctx, dbRuleEvalStat.EntityID) - if err != nil { - l.Err(err).Msg("error getting repo details from db") - return st + prRepoOwner := efp.Properties.GetProperty(ghprop.PullPropertyRepoOwner).GetString() + prRepoName := efp.Properties.GetProperty(ghprop.PullPropertyRepoName).GetString() + if prRepoOwner != "" && prRepoName != "" { + repoPath = fmt.Sprintf("%s/%s", prRepoOwner, prRepoName) } - repoPath = fmt.Sprintf("%s/%s", repoDetails.Owner, repoDetails.Name) } alertURL, err := getAlertURLFromMetadata( dbRuleEvalStat.AlertMetadata, @@ -537,7 +550,7 @@ func (s *Server) getRuleEvalStatus( st.Alert.Url = alertURL } } - return st + return st, nil } // GetProfileStatusByProject is a method to get profile status for a project diff --git a/internal/db/eval_history.sql.go b/internal/db/eval_history.sql.go index 71c4a58641..a9f1fa5c86 100644 --- a/internal/db/eval_history.sql.go +++ b/internal/db/eval_history.sql.go @@ -44,19 +44,9 @@ SELECT s.id::uuid AS evaluation_id, s.evaluation_time as evaluated_at, ere.entity_type, -- entity id - CAST( - CASE - WHEN ere.repository_id IS NOT NULL THEN r.id - WHEN ere.pull_request_id IS NOT NULL THEN pr.id - WHEN ere.artifact_id IS NOT NULL THEN a.id - END AS uuid - ) AS entity_id, - ere.entity_instance_id as new_entity_id, - -- raw fields for entity names - r.repo_owner, - r.repo_name, - pr.pr_number, - a.artifact_name, + ere.entity_instance_id as entity_id, + -- entity name + ei.name as entity_name, j.id as project_id, -- rule type, name, and profile rt.name AS rule_type, @@ -77,12 +67,10 @@ FROM evaluation_statuses s JOIN rule_instances ri ON ere.rule_id = ri.id JOIN rule_type rt ON ri.rule_type_id = rt.id JOIN profiles p ON ri.profile_id = p.id - LEFT JOIN repositories r ON ere.repository_id = r.id - LEFT JOIN pull_requests pr ON ere.pull_request_id = pr.id - LEFT JOIN artifacts a ON ere.artifact_id = a.id + JOIN projects j ON ei.project_id = j.id + JOIN entity_instances ei ON ere.entity_instance_id = ei.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 s.id = $1 AND j.id = $2 ` @@ -96,12 +84,8 @@ type GetEvaluationHistoryRow struct { EvaluatedAt time.Time `json:"evaluated_at"` EntityType Entities `json:"entity_type"` EntityID uuid.UUID `json:"entity_id"` - NewEntityID uuid.UUID `json:"new_entity_id"` - RepoOwner sql.NullString `json:"repo_owner"` - RepoName sql.NullString `json:"repo_name"` - PrNumber sql.NullInt64 `json:"pr_number"` - ArtifactName sql.NullString `json:"artifact_name"` - ProjectID uuid.NullUUID `json:"project_id"` + EntityName string `json:"entity_name"` + ProjectID uuid.UUID `json:"project_id"` RuleType string `json:"rule_type"` RuleName string `json:"rule_name"` RuleSeverity Severity `json:"rule_severity"` @@ -122,11 +106,7 @@ func (q *Queries) GetEvaluationHistory(ctx context.Context, arg GetEvaluationHis &i.EvaluatedAt, &i.EntityType, &i.EntityID, - &i.NewEntityID, - &i.RepoOwner, - &i.RepoName, - &i.PrNumber, - &i.ArtifactName, + &i.EntityName, &i.ProjectID, &i.RuleType, &i.RuleName, @@ -147,20 +127,13 @@ const getLatestEvalStateForRuleEntity = `-- name: GetLatestEvalStateForRuleEntit SELECT eh.id, eh.rule_entity_id, eh.status, eh.details, eh.evaluation_time, eh.checkpoint 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 -AND ( - re.repository_id = $2 - OR re.pull_request_id = $3 - OR re.artifact_id = $4 -) +WHERE re.rule_id = $1 AND re.entity_instance_id = $2 FOR UPDATE ` type GetLatestEvalStateForRuleEntityParams struct { - RuleID uuid.UUID `json:"rule_id"` - RepositoryID uuid.NullUUID `json:"repository_id"` - PullRequestID uuid.NullUUID `json:"pull_request_id"` - ArtifactID uuid.NullUUID `json:"artifact_id"` + RuleID uuid.UUID `json:"rule_id"` + EntityInstanceID uuid.UUID `json:"entity_instance_id"` } // Copyright 2024 Stacklok, Inc @@ -177,12 +150,7 @@ type GetLatestEvalStateForRuleEntityParams struct { // See the License for the specific language governing permissions and // limitations under the License. func (q *Queries) GetLatestEvalStateForRuleEntity(ctx context.Context, arg GetLatestEvalStateForRuleEntityParams) (EvaluationStatus, error) { - row := q.db.QueryRowContext(ctx, getLatestEvalStateForRuleEntity, - arg.RuleID, - arg.RepositoryID, - arg.PullRequestID, - arg.ArtifactID, - ) + row := q.db.QueryRowContext(ctx, getLatestEvalStateForRuleEntity, arg.RuleID, arg.EntityInstanceID) var i EvaluationStatus err := row.Scan( &i.ID, @@ -229,40 +197,24 @@ func (q *Queries) InsertAlertEvent(ctx context.Context, arg InsertAlertEventPara const insertEvaluationRuleEntity = `-- name: InsertEvaluationRuleEntity :one INSERT INTO evaluation_rule_entities( rule_id, - repository_id, - pull_request_id, - artifact_id, entity_type, entity_instance_id ) VALUES ( $1, $2, - $3, - $4, - $5, - $6 + $3 ) RETURNING id ` type InsertEvaluationRuleEntityParams struct { - RuleID uuid.UUID `json:"rule_id"` - RepositoryID uuid.NullUUID `json:"repository_id"` - PullRequestID uuid.NullUUID `json:"pull_request_id"` - ArtifactID uuid.NullUUID `json:"artifact_id"` - EntityType Entities `json:"entity_type"` - EntityInstanceID uuid.UUID `json:"entity_instance_id"` + RuleID uuid.UUID `json:"rule_id"` + EntityType Entities `json:"entity_type"` + EntityInstanceID uuid.UUID `json:"entity_instance_id"` } func (q *Queries) InsertEvaluationRuleEntity(ctx context.Context, arg InsertEvaluationRuleEntityParams) (uuid.UUID, error) { - row := q.db.QueryRowContext(ctx, insertEvaluationRuleEntity, - arg.RuleID, - arg.RepositoryID, - arg.PullRequestID, - arg.ArtifactID, - arg.EntityType, - arg.EntityInstanceID, - ) + row := q.db.QueryRowContext(ctx, insertEvaluationRuleEntity, arg.RuleID, arg.EntityType, arg.EntityInstanceID) var id uuid.UUID err := row.Scan(&id) return id, err @@ -338,19 +290,7 @@ SELECT s.id::uuid AS evaluation_id, s.evaluation_time as evaluated_at, ere.entity_type, -- entity id - CAST( - CASE - WHEN ere.repository_id IS NOT NULL THEN r.id - WHEN ere.pull_request_id IS NOT NULL THEN pr.id - WHEN ere.artifact_id IS NOT NULL THEN a.id - END AS uuid - ) AS entity_id, - ere.entity_instance_id as new_entity_id, - -- raw fields for entity names - r.repo_owner, - r.repo_name, - pr.pr_number, - a.artifact_name, + ere.entity_instance_id as entity_id, j.id as project_id, -- rule type, name, and profile rt.name AS rule_type, @@ -371,28 +311,22 @@ SELECT s.id::uuid AS evaluation_id, JOIN rule_instances ri ON ere.rule_id = ri.id JOIN rule_type rt ON ri.rule_type_id = rt.id JOIN profiles p ON ri.profile_id = p.id - LEFT JOIN repositories r ON ere.repository_id = r.id - LEFT JOIN pull_requests pr ON ere.pull_request_id = pr.id - LEFT JOIN artifacts a ON ere.artifact_id = a.id + JOIN entity_instances ei ON ere.entity_instance_id = ei.id + JOIN projects j ON ei.project_id = j.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.evaluation_time) AND ($2::timestamp without time zone IS NULL OR $2 < s.evaluation_time) -- inclusion filters AND ($3::entities[] IS NULL OR ere.entity_type = 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[])) - AND ($4::text[] IS NULL OR ere.pull_request_id IS NULL OR pr.pr_number::text = ANY($4::text[])) - AND ($4::text[] IS NULL OR ere.artifact_id IS NULL OR a.artifact_name = ANY($4::text[])) + AND ($4::text[] IS NULL OR ei.name = ANY($4::text[])) AND ($5::text[] IS NULL OR p.name = ANY($5::text[])) AND ($6::remediation_status_types[] IS NULL OR re.status = ANY($6::remediation_status_types[])) AND ($7::alert_status_types[] IS NULL OR ae.status = ANY($7::alert_status_types[])) AND ($8::eval_status_types[] IS NULL OR s.status = ANY($8::eval_status_types[])) -- exclusion filters AND ($9::entities[] IS NULL OR ere.entity_type != ALL($9::entities[])) - AND ($10::text[] IS NULL OR ere.repository_id IS NULL OR CONCAT(r.repo_owner, '/', r.repo_name) != ALL($10::text[])) - AND ($10::text[] IS NULL OR ere.pull_request_id IS NULL OR pr.pr_number::text != ALL($10::text[])) - AND ($10::text[] IS NULL OR ere.artifact_id IS NULL OR a.artifact_name != ALL($10::text[])) + AND ($10::text[] IS NULL OR ei.name != ALL($10::text[])) AND ($11::text[] IS NULL OR p.name != ALL($11::text[])) AND ($12::remediation_status_types[] IS NULL OR re.status != ALL($12::remediation_status_types[])) AND ($13::alert_status_types[] IS NULL OR ae.status != ALL($13::alert_status_types[])) @@ -434,12 +368,7 @@ type ListEvaluationHistoryRow struct { EvaluatedAt time.Time `json:"evaluated_at"` EntityType Entities `json:"entity_type"` EntityID uuid.UUID `json:"entity_id"` - NewEntityID uuid.UUID `json:"new_entity_id"` - RepoOwner sql.NullString `json:"repo_owner"` - RepoName sql.NullString `json:"repo_name"` - PrNumber sql.NullInt64 `json:"pr_number"` - ArtifactName sql.NullString `json:"artifact_name"` - ProjectID uuid.NullUUID `json:"project_id"` + ProjectID uuid.UUID `json:"project_id"` RuleType string `json:"rule_type"` RuleName string `json:"rule_name"` RuleSeverity Severity `json:"rule_severity"` @@ -485,11 +414,6 @@ func (q *Queries) ListEvaluationHistory(ctx context.Context, arg ListEvaluationH &i.EvaluatedAt, &i.EntityType, &i.EntityID, - &i.NewEntityID, - &i.RepoOwner, - &i.RepoName, - &i.PrNumber, - &i.ArtifactName, &i.ProjectID, &i.RuleType, &i.RuleName, @@ -520,22 +444,9 @@ SELECT s.evaluation_time, s.id, ere.rule_id, -- entity type - CAST( - CASE - WHEN ere.repository_id IS NOT NULL THEN 1 - WHEN ere.pull_request_id IS NOT NULL THEN 2 - WHEN ere.artifact_id IS NOT NULL THEN 3 - END AS integer - ) AS entity_type, + ere.entity_type, -- entity id - CAST( - CASE - WHEN ere.repository_id IS NOT NULL THEN ere.repository_id - WHEN ere.pull_request_id IS NOT NULL THEN ere.pull_request_id - WHEN ere.artifact_id IS NOT NULL THEN ere.artifact_id - END AS uuid - ) AS entity_id, - ere.entity_instance_id as new_entity_id + ere.entity_instance_id as entity_id FROM evaluation_statuses s JOIN evaluation_rule_entities ere ON s.rule_entity_id = ere.id LEFT JOIN latest_evaluation_statuses l @@ -558,9 +469,8 @@ type ListEvaluationHistoryStaleRecordsRow struct { EvaluationTime time.Time `json:"evaluation_time"` ID uuid.UUID `json:"id"` RuleID uuid.UUID `json:"rule_id"` - EntityType int32 `json:"entity_type"` + EntityType Entities `json:"entity_type"` EntityID uuid.UUID `json:"entity_id"` - NewEntityID uuid.UUID `json:"new_entity_id"` } func (q *Queries) ListEvaluationHistoryStaleRecords(ctx context.Context, arg ListEvaluationHistoryStaleRecordsParams) ([]ListEvaluationHistoryStaleRecordsRow, error) { @@ -578,7 +488,6 @@ func (q *Queries) ListEvaluationHistoryStaleRecords(ctx context.Context, arg Lis &i.RuleID, &i.EntityType, &i.EntityID, - &i.NewEntityID, ); err != nil { return nil, err } diff --git a/internal/db/eval_history_test.go b/internal/db/eval_history_test.go index 42b1d5dfd7..2cdabbbebb 100644 --- a/internal/db/eval_history_test.go +++ b/internal/db/eval_history_test.go @@ -70,8 +70,6 @@ func TestListEvaluationHistoryFilters(t *testing.T) { require.Equal(t, es1, row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repo1.ID, row.EntityID) - require.Equal(t, repo1.RepoOwner, row.RepoOwner.String) - require.Equal(t, repo1.RepoName, row.RepoName.String) }, }, @@ -94,8 +92,6 @@ func TestListEvaluationHistoryFilters(t *testing.T) { require.Equal(t, es1, row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repo1.ID, row.EntityID) - require.Equal(t, repo1.RepoOwner, row.RepoOwner.String) - require.Equal(t, repo1.RepoName, row.RepoName.String) }, }, { @@ -148,8 +144,6 @@ func TestListEvaluationHistoryFilters(t *testing.T) { require.Equal(t, es1, row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repo1.ID, row.EntityID) - require.Equal(t, repo1.RepoOwner, row.RepoOwner.String) - require.Equal(t, repo1.RepoName, row.RepoName.String) }, }, { @@ -170,8 +164,6 @@ func TestListEvaluationHistoryFilters(t *testing.T) { require.Equal(t, es1, row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repo1.ID, row.EntityID) - require.Equal(t, repo1.RepoOwner, row.RepoOwner.String) - require.Equal(t, repo1.RepoName, row.RepoName.String) }, }, { @@ -210,8 +202,6 @@ func TestListEvaluationHistoryFilters(t *testing.T) { require.Equal(t, es1, row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repo1.ID, row.EntityID) - require.Equal(t, repo1.RepoOwner, row.RepoOwner.String) - require.Equal(t, repo1.RepoName, row.RepoName.String) }, }, { @@ -264,8 +254,6 @@ func TestListEvaluationHistoryFilters(t *testing.T) { require.Equal(t, es1, row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repo1.ID, row.EntityID) - require.Equal(t, repo1.RepoOwner, row.RepoOwner.String) - require.Equal(t, repo1.RepoName, row.RepoName.String) }, }, { @@ -286,8 +274,6 @@ func TestListEvaluationHistoryFilters(t *testing.T) { require.Equal(t, es1, row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repo1.ID, row.EntityID) - require.Equal(t, repo1.RepoOwner, row.RepoOwner.String) - require.Equal(t, repo1.RepoName, row.RepoName.String) }, }, { @@ -326,8 +312,6 @@ func TestListEvaluationHistoryFilters(t *testing.T) { require.Equal(t, es1, row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repo1.ID, row.EntityID) - require.Equal(t, repo1.RepoOwner, row.RepoOwner.String) - require.Equal(t, repo1.RepoName, row.RepoName.String) }, }, { @@ -380,8 +364,6 @@ func TestListEvaluationHistoryFilters(t *testing.T) { require.Equal(t, es1, row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repo1.ID, row.EntityID) - require.Equal(t, repo1.RepoOwner, row.RepoOwner.String) - require.Equal(t, repo1.RepoName, row.RepoName.String) }, }, { @@ -402,8 +384,6 @@ func TestListEvaluationHistoryFilters(t *testing.T) { require.Equal(t, es1, row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repo1.ID, row.EntityID) - require.Equal(t, repo1.RepoOwner, row.RepoOwner.String) - require.Equal(t, repo1.RepoName, row.RepoName.String) }, }, { @@ -464,8 +444,6 @@ func TestListEvaluationHistoryFilters(t *testing.T) { require.Equal(t, es1, row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repo1.ID, row.EntityID) - require.Equal(t, repo1.RepoOwner, row.RepoOwner.String) - require.Equal(t, repo1.RepoName, row.RepoName.String) }, }, { @@ -489,8 +467,6 @@ func TestListEvaluationHistoryFilters(t *testing.T) { require.Equal(t, es1, row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repo1.ID, row.EntityID) - require.Equal(t, repo1.RepoOwner, row.RepoOwner.String) - require.Equal(t, repo1.RepoName, row.RepoName.String) }, }, { @@ -601,8 +577,6 @@ func TestListEvaluationHistoryPagination(t *testing.T) { require.Equal(t, ess[9], row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repos[9].ID, row.EntityID) - require.Equal(t, repos[9].RepoOwner, row.RepoOwner.String) - require.Equal(t, repos[9].RepoName, row.RepoName.String) }, }, { @@ -646,8 +620,6 @@ func TestListEvaluationHistoryPagination(t *testing.T) { require.Equal(t, ess[0], row.EvaluationID) require.Equal(t, EntitiesRepository, row.EntityType) require.Equal(t, repos[0].ID, row.EntityID) - require.Equal(t, repos[0].RepoOwner, row.RepoOwner.String) - require.Equal(t, repos[0].RepoName, row.RepoName.String) }, }, { @@ -713,11 +685,7 @@ func createRandomEvaluationRuleEntity( ereID, err := testQueries.InsertEvaluationRuleEntity( context.Background(), InsertEvaluationRuleEntityParams{ - RuleID: ruleID, - RepositoryID: uuid.NullUUID{ - UUID: entityID, - Valid: true, - }, + RuleID: ruleID, EntityType: EntitiesRepository, EntityInstanceID: entityID, }, diff --git a/internal/db/profile_status.sql.go b/internal/db/profile_status.sql.go index f4d08e2714..343623dd47 100644 --- a/internal/db/profile_status.sql.go +++ b/internal/db/profile_status.sql.go @@ -118,12 +118,13 @@ func (q *Queries) GetProfileStatusByProject(ctx context.Context, projectID uuid. const listOldestRuleEvaluationsByRepositoryId = `-- name: ListOldestRuleEvaluationsByRepositoryId :many -SELECT ere.repository_id::uuid AS repository_id, MIN(es.evaluation_time)::timestamp AS oldest_last_updated +SELECT ere.entity_instance_id::uuid AS repository_id, MIN(es.evaluation_time)::timestamp AS oldest_last_updated FROM evaluation_rule_entities AS ere INNER JOIN latest_evaluation_statuses AS les ON ere.id = les.rule_entity_id INNER JOIN evaluation_statuses AS es ON les.evaluation_history_id = es.id -WHERE ere.repository_id = ANY ($1::uuid[]) -GROUP BY ere.repository_id +WHERE ere.entity_instance_id = ANY ($1::uuid[]) +AND ere.entity_type = 'repository' +GROUP BY ere.entity_instance_id ` type ListOldestRuleEvaluationsByRepositoryIdRow struct { @@ -198,23 +199,16 @@ SELECT ad.alert_metadata, ad.alert_last_updated, ed.id AS rule_evaluation_id, - ere.repository_id, ere.entity_type, ri.name AS rule_name, - repo.repo_name, - repo.repo_owner, - repo.provider, + prov.name AS provider, rt.name AS rule_type_name, rt.severity_value as rule_type_severity_value, rt.id AS rule_type_id, rt.guidance as rule_type_guidance, rt.display_name as rule_type_display_name, - -- TODO: store entity ID directly in evaluation_rule_entities - CASE - WHEN ere.entity_type = 'artifact'::entities THEN ere.artifact_id - WHEN ere.entity_type = 'repository'::entities THEN ere.repository_id - WHEN ere.entity_type = 'pull_request'::entities THEN ere.pull_request_id - END::uuid as entity_id, + ere.entity_instance_id as entity_id, + ei.project_id as project_id, rt.release_phase as rule_type_release_phase FROM latest_evaluation_statuses les INNER JOIN evaluation_rule_entities ere ON ere.id = les.rule_entity_id @@ -223,23 +217,16 @@ FROM latest_evaluation_statuses les INNER JOIN alert_details ad ON ad.evaluation_id = les.evaluation_history_id INNER JOIN rule_instances AS ri ON ri.id = ere.rule_id INNER JOIN rule_type rt ON rt.id = ri.rule_type_id - LEFT JOIN repositories repo ON repo.id = ere.repository_id + INNER JOIN entity_instances ei ON ei.id = ere.entity_instance_id + INNER JOIN providers prov ON prov.id = ei.provider_id WHERE les.profile_id = $1 AND - ( - CASE - WHEN $2::entities = 'repository' AND ere.repository_id = $3::UUID THEN true - WHEN $2::entities = 'artifact' AND ere.artifact_id = $3::UUID THEN true - WHEN $2::entities = 'pull_request' AND ere.pull_request_id = $3::UUID THEN true - WHEN $3::UUID IS NULL THEN true - ELSE false - END - ) AND (rt.name = $4 OR $4 IS NULL) - AND (lower(ri.name) = lower($5) OR $5 IS NULL) + (ere.entity_instance_id = $2::UUID OR $2::UUID IS NULL) + AND (rt.name = $3 OR $3 IS NULL) + AND (lower(ri.name) = lower($4) OR $4 IS NULL) ` type ListRuleEvaluationsByProfileIdParams struct { ProfileID uuid.UUID `json:"profile_id"` - EntityType NullEntities `json:"entity_type"` EntityID uuid.NullUUID `json:"entity_id"` RuleTypeName sql.NullString `json:"rule_type_name"` RuleName sql.NullString `json:"rule_name"` @@ -258,25 +245,22 @@ type ListRuleEvaluationsByProfileIdRow struct { AlertMetadata json.RawMessage `json:"alert_metadata"` AlertLastUpdated time.Time `json:"alert_last_updated"` RuleEvaluationID uuid.UUID `json:"rule_evaluation_id"` - RepositoryID uuid.NullUUID `json:"repository_id"` EntityType Entities `json:"entity_type"` RuleName string `json:"rule_name"` - RepoName sql.NullString `json:"repo_name"` - RepoOwner sql.NullString `json:"repo_owner"` - Provider sql.NullString `json:"provider"` + Provider string `json:"provider"` RuleTypeName string `json:"rule_type_name"` RuleTypeSeverityValue Severity `json:"rule_type_severity_value"` RuleTypeID uuid.UUID `json:"rule_type_id"` RuleTypeGuidance string `json:"rule_type_guidance"` RuleTypeDisplayName string `json:"rule_type_display_name"` EntityID uuid.UUID `json:"entity_id"` + ProjectID uuid.UUID `json:"project_id"` RuleTypeReleasePhase ReleaseStatus `json:"rule_type_release_phase"` } func (q *Queries) ListRuleEvaluationsByProfileId(ctx context.Context, arg ListRuleEvaluationsByProfileIdParams) ([]ListRuleEvaluationsByProfileIdRow, error) { rows, err := q.db.QueryContext(ctx, listRuleEvaluationsByProfileId, arg.ProfileID, - arg.EntityType, arg.EntityID, arg.RuleTypeName, arg.RuleName, @@ -301,11 +285,8 @@ func (q *Queries) ListRuleEvaluationsByProfileId(ctx context.Context, arg ListRu &i.AlertMetadata, &i.AlertLastUpdated, &i.RuleEvaluationID, - &i.RepositoryID, &i.EntityType, &i.RuleName, - &i.RepoName, - &i.RepoOwner, &i.Provider, &i.RuleTypeName, &i.RuleTypeSeverityValue, @@ -313,6 +294,7 @@ func (q *Queries) ListRuleEvaluationsByProfileId(ctx context.Context, arg ListRu &i.RuleTypeGuidance, &i.RuleTypeDisplayName, &i.EntityID, + &i.ProjectID, &i.RuleTypeReleasePhase, ); err != nil { return nil, err diff --git a/internal/db/profiles_test.go b/internal/db/profiles_test.go index fbe8dcd5f2..42bffce164 100644 --- a/internal/db/profiles_test.go +++ b/internal/db/profiles_test.go @@ -158,13 +158,7 @@ func createRuleEntity( id, err := testQueries.InsertEvaluationRuleEntity(ctx, InsertEvaluationRuleEntityParams{ - RuleID: ruleID, - RepositoryID: uuid.NullUUID{ - UUID: repoID, - Valid: true, - }, - PullRequestID: uuid.NullUUID{}, - ArtifactID: uuid.NullUUID{}, + RuleID: ruleID, EntityType: EntitiesRepository, EntityInstanceID: repoID, }, @@ -3427,8 +3421,16 @@ func TestCreateProfileStatusStoredDeleteProcedure(t *testing.T) { }, expectedStatusAfterSetup: EvalStatusTypesFailure, ruleStatusDeleteFn: func(delRepo *Repository) { + // TODO: This will be removed once we fully move + // to using the entity_instances table. err := testQueries.DeleteRepository(context.Background(), delRepo.ID) require.NoError(t, err) + + err = testQueries.DeleteEntity(context.Background(), DeleteEntityParams{ + ID: delRepo.ID, + ProjectID: delRepo.ProjectID, + }) + require.NoError(t, err) }, expectedStatusAfterModify: EvalStatusTypesSuccess, }, @@ -3477,8 +3479,16 @@ func TestCreateProfileStatusStoredDeleteProcedure(t *testing.T) { expectedStatusAfterSetup: EvalStatusTypesError, ruleStatusDeleteFn: func(delRepo *Repository) { + // TODO: This will be removed once we fully move + // to using the entity_instances table. err := testQueries.DeleteRepository(context.Background(), delRepo.ID) require.NoError(t, err) + + err = testQueries.DeleteEntity(context.Background(), DeleteEntityParams{ + ID: delRepo.ID, + ProjectID: delRepo.ProjectID, + }) + require.NoError(t, err) }, expectedStatusAfterModify: EvalStatusTypesFailure, }, @@ -3527,8 +3537,16 @@ func TestCreateProfileStatusStoredDeleteProcedure(t *testing.T) { expectedStatusAfterSetup: EvalStatusTypesError, ruleStatusDeleteFn: func(delRepo *Repository) { + // TODO: This will be removed once we fully move + // to using the entity_instances table. err := testQueries.DeleteRepository(context.Background(), delRepo.ID) require.NoError(t, err) + + err = testQueries.DeleteEntity(context.Background(), DeleteEntityParams{ + ID: delRepo.ID, + ProjectID: delRepo.ProjectID, + }) + require.NoError(t, err) }, expectedStatusAfterModify: EvalStatusTypesError, }, @@ -3558,8 +3576,16 @@ func TestCreateProfileStatusStoredDeleteProcedure(t *testing.T) { expectedStatusAfterSetup: EvalStatusTypesFailure, ruleStatusDeleteFn: func(delRepo *Repository) { + // TODO: This will be removed once we fully move + // to using the entity_instances table. err := testQueries.DeleteRepository(context.Background(), delRepo.ID) require.NoError(t, err) + + err = testQueries.DeleteEntity(context.Background(), DeleteEntityParams{ + ID: delRepo.ID, + ProjectID: delRepo.ProjectID, + }) + require.NoError(t, err) }, expectedStatusAfterModify: EvalStatusTypesSkipped, }, @@ -3640,7 +3666,6 @@ func verifyRow( expectedRow *ListRuleEvaluationsByProfileIdRow, fetchedRows []ListRuleEvaluationsByProfileIdRow, rt RuleType, - randomEntities *testRandomEntities, ) { t.Helper() @@ -3652,11 +3677,6 @@ func verifyRow( require.Equal(t, rt.ID, row.RuleTypeID) require.Equal(t, rt.Name, row.RuleTypeName) - - require.Equal(t, randomEntities.repo.RepoName, row.RepoName.String) - require.Equal(t, randomEntities.repo.RepoOwner, row.RepoOwner.String) - - require.Equal(t, randomEntities.prov.Name, row.Provider.String) } func TestListRuleEvaluations(t *testing.T) { @@ -3864,8 +3884,8 @@ func TestListRuleEvaluations(t *testing.T) { require.Len(t, evalStatusRows, tt.totalRows) require.Equal(t, getStatusCount(t, evalStatusRows), tt.sc) - verifyRow(t, tt.rule1Expected, evalStatusRows, randomEntities.ruleType1, randomEntities) - verifyRow(t, tt.rule2Expected, evalStatusRows, randomEntities.ruleType2, randomEntities) + verifyRow(t, tt.rule1Expected, evalStatusRows, randomEntities.ruleType1) + verifyRow(t, tt.rule2Expected, evalStatusRows, randomEntities.ruleType2) }) } } diff --git a/internal/db/querier.go b/internal/db/querier.go index d94078034f..985c95066e 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -158,8 +158,6 @@ type Querier interface { GetProviderWebhooks(ctx context.Context, providerID uuid.UUID) ([]GetProviderWebhooksRow, error) GetPullRequest(ctx context.Context, arg GetPullRequestParams) (PullRequest, error) GetPullRequestByID(ctx context.Context, id uuid.UUID) (PullRequest, error) - GetRepoPathFromArtifactID(ctx context.Context, id uuid.UUID) (GetRepoPathFromArtifactIDRow, error) - GetRepoPathFromPullRequestID(ctx context.Context, id uuid.UUID) (GetRepoPathFromPullRequestIDRow, error) // avoid using this, where possible use GetRepositoryByIDAndProject instead GetRepositoryByID(ctx context.Context, id uuid.UUID) (Repository, error) GetRepositoryByIDAndProject(ctx context.Context, arg GetRepositoryByIDAndProjectParams) (Repository, error) diff --git a/internal/db/repositories.sql.go b/internal/db/repositories.sql.go index 1f443b2f91..4e0271fa11 100644 --- a/internal/db/repositories.sql.go +++ b/internal/db/repositories.sql.go @@ -160,42 +160,6 @@ func (q *Queries) GetProviderWebhooks(ctx context.Context, providerID uuid.UUID) return items, nil } -const getRepoPathFromArtifactID = `-- name: GetRepoPathFromArtifactID :one -SELECT r.repo_owner AS owner , r.repo_name AS name FROM repositories AS r -JOIN artifacts AS a ON a.repository_id = r.id -WHERE a.id = $1 -` - -type GetRepoPathFromArtifactIDRow struct { - Owner string `json:"owner"` - Name string `json:"name"` -} - -func (q *Queries) GetRepoPathFromArtifactID(ctx context.Context, id uuid.UUID) (GetRepoPathFromArtifactIDRow, error) { - row := q.db.QueryRowContext(ctx, getRepoPathFromArtifactID, id) - var i GetRepoPathFromArtifactIDRow - err := row.Scan(&i.Owner, &i.Name) - return i, err -} - -const getRepoPathFromPullRequestID = `-- name: GetRepoPathFromPullRequestID :one -SELECT r.repo_owner AS owner , r.repo_name AS name FROM repositories AS r -JOIN pull_requests AS p ON p.repository_id = r.id -WHERE p.id = $1 -` - -type GetRepoPathFromPullRequestIDRow struct { - Owner string `json:"owner"` - Name string `json:"name"` -} - -func (q *Queries) GetRepoPathFromPullRequestID(ctx context.Context, id uuid.UUID) (GetRepoPathFromPullRequestIDRow, error) { - row := q.db.QueryRowContext(ctx, getRepoPathFromPullRequestID, id) - var i GetRepoPathFromPullRequestIDRow - err := row.Scan(&i.Owner, &i.Name) - return i, err -} - const getRepositoryByID = `-- name: GetRepositoryByID :one SELECT id, provider, project_id, repo_owner, repo_name, repo_id, is_private, is_fork, webhook_id, webhook_url, deploy_url, clone_url, created_at, updated_at, default_branch, license, provider_id, reminder_last_sent FROM repositories WHERE id = $1 ` diff --git a/internal/db/repositories_test.go b/internal/db/repositories_test.go index dbb0263ec7..65be3da728 100644 --- a/internal/db/repositories_test.go +++ b/internal/db/repositories_test.go @@ -18,6 +18,7 @@ package db import ( "context" "database/sql" + "fmt" "testing" "time" @@ -69,7 +70,7 @@ func createRandomRepository(t *testing.T, project uuid.UUID, prov Provider, opts ei, err := testQueries.CreateEntityWithID(context.Background(), CreateEntityWithIDParams{ ID: repo.ID, - Name: arg.RepoName, + Name: fmt.Sprintf("%s/%s", arg.RepoOwner, arg.RepoName), ProjectID: project, ProviderID: prov.ID, EntityType: EntitiesRepository, diff --git a/internal/db/store.go b/internal/db/store.go index 1a7d37cd2f..80712d2fdf 100644 --- a/internal/db/store.go +++ b/internal/db/store.go @@ -26,7 +26,7 @@ import ( // ExtendQuerier extends the Querier interface with custom queries type ExtendQuerier interface { Querier - GetRuleEvaluationByProfileIdAndRuleType(ctx context.Context, profileID uuid.UUID, entityType NullEntities, + GetRuleEvaluationByProfileIdAndRuleType(ctx context.Context, profileID uuid.UUID, ruleName sql.NullString, entityID uuid.NullUUID, ruleTypeName sql.NullString) (*ListRuleEvaluationsByProfileIdRow, error) UpsertPropertyValueV1(ctx context.Context, params UpsertPropertyValueV1Params) (Property, error) GetPropertyValueV1(ctx context.Context, entityID uuid.UUID, key string) (PropertyValueV1, error) @@ -112,14 +112,12 @@ func NewStore(db *sql.DB) Store { func (q *Queries) GetRuleEvaluationByProfileIdAndRuleType( ctx context.Context, profileID uuid.UUID, - entityType NullEntities, ruleName sql.NullString, entityID uuid.NullUUID, ruleTypeName sql.NullString, ) (*ListRuleEvaluationsByProfileIdRow, error) { params := ListRuleEvaluationsByProfileIdParams{ ProfileID: profileID, - EntityType: entityType, EntityID: entityID, RuleName: ruleName, RuleTypeName: ruleTypeName, diff --git a/internal/engine/eval_status.go b/internal/engine/eval_status.go index a0fd201854..500e0cd88b 100644 --- a/internal/engine/eval_status.go +++ b/internal/engine/eval_status.go @@ -56,11 +56,6 @@ func (e *executor) createEvalStatusParams( ExecutionID: *inf.ExecutionID, // Execution ID is required in the executor. } - // Prepare params for fetching the current rule evaluation from the database - entityType := db.NullEntities{ - Entities: params.EntityType, - Valid: true, - } entityID := uuid.NullUUID{} switch params.EntityType { case db.EntitiesArtifact: @@ -94,7 +89,6 @@ func (e *executor) createEvalStatusParams( // Get the current rule evaluation from the database evalStatus, err := e.querier.GetRuleEvaluationByProfileIdAndRuleType(ctx, params.Profile.ID, - entityType, ruleName, entityID, nullableRuleTypeName, diff --git a/internal/engine/executor_test.go b/internal/engine/executor_test.go index 2410f2bb9f..3f3850709a 100644 --- a/internal/engine/executor_test.go +++ b/internal/engine/executor_test.go @@ -107,7 +107,6 @@ func TestExecutor_handleEntityEvent(t *testing.T) { gomock.Any(), gomock.Any(), gomock.Any(), - gomock.Any(), ).Return(nil, nil) mockStore.EXPECT(). diff --git a/internal/entities/models/models.go b/internal/entities/models/models.go index 7d8cc361b8..e771d7b107 100644 --- a/internal/entities/models/models.go +++ b/internal/entities/models/models.go @@ -40,8 +40,8 @@ type EntityWithProperties struct { } // NewEntityWithProperties creates a new EntityWithProperties instance -func NewEntityWithProperties(dbEntity db.EntityInstance, props *properties.Properties) EntityWithProperties { - return EntityWithProperties{ +func NewEntityWithProperties(dbEntity db.EntityInstance, props *properties.Properties) *EntityWithProperties { + return &EntityWithProperties{ Entity: EntityInstance{ ID: dbEntity.ID, Type: entities.EntityTypeFromDB(dbEntity.EntityType), @@ -52,3 +52,16 @@ func NewEntityWithProperties(dbEntity db.EntityInstance, props *properties.Prope Properties: props, } } + +// NewEntityWithPropertiesFromInstance creates a new EntityWithProperties instance from an existing entity instance +func NewEntityWithPropertiesFromInstance(entity EntityInstance, props *properties.Properties) *EntityWithProperties { + return &EntityWithProperties{ + Entity: entity, + Properties: props, + } +} + +// UpdateProperties updates the properties for the "entity for properties" instance +func (e *EntityWithProperties) UpdateProperties(props *properties.Properties) { + e.Properties = props +} diff --git a/internal/entities/properties/service/mock/service.go b/internal/entities/properties/service/mock/service.go index 69506d8a9f..dbdf9128d1 100644 --- a/internal/entities/properties/service/mock/service.go +++ b/internal/entities/properties/service/mock/service.go @@ -15,7 +15,9 @@ import ( uuid "github.com/google/uuid" db "github.com/stacklok/minder/internal/db" + models "github.com/stacklok/minder/internal/entities/models" properties "github.com/stacklok/minder/internal/entities/properties" + manager "github.com/stacklok/minder/internal/providers/manager" v1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" v10 "github.com/stacklok/minder/pkg/providers/v1" gomock "go.uber.org/mock/gomock" @@ -44,6 +46,21 @@ func (m *MockPropertiesService) EXPECT() *MockPropertiesServiceMockRecorder { return m.recorder } +// EntityWithProperties mocks base method. +func (m *MockPropertiesService) EntityWithProperties(ctx context.Context, entityID, projectId uuid.UUID, qtx db.ExtendQuerier) (*models.EntityWithProperties, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "EntityWithProperties", ctx, entityID, projectId, qtx) + ret0, _ := ret[0].(*models.EntityWithProperties) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// EntityWithProperties indicates an expected call of EntityWithProperties. +func (mr *MockPropertiesServiceMockRecorder) EntityWithProperties(ctx, entityID, projectId, qtx any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EntityWithProperties", reflect.TypeOf((*MockPropertiesService)(nil).EntityWithProperties), ctx, entityID, projectId, qtx) +} + // ReplaceAllProperties mocks base method. func (m *MockPropertiesService) ReplaceAllProperties(ctx context.Context, entityID uuid.UUID, props *properties.Properties, qtx db.ExtendQuerier) error { m.ctrl.T.Helper() @@ -87,6 +104,20 @@ func (mr *MockPropertiesServiceMockRecorder) RetrieveAllProperties(ctx, provider return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RetrieveAllProperties", reflect.TypeOf((*MockPropertiesService)(nil).RetrieveAllProperties), ctx, provider, projectId, providerID, lookupProperties, entType) } +// RetrieveAllPropertiesForEntity mocks base method. +func (m *MockPropertiesService) RetrieveAllPropertiesForEntity(ctx context.Context, efp *models.EntityWithProperties, provMan manager.ProviderManager) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RetrieveAllPropertiesForEntity", ctx, efp, provMan) + ret0, _ := ret[0].(error) + return ret0 +} + +// RetrieveAllPropertiesForEntity indicates an expected call of RetrieveAllPropertiesForEntity. +func (mr *MockPropertiesServiceMockRecorder) RetrieveAllPropertiesForEntity(ctx, efp, provMan any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RetrieveAllPropertiesForEntity", reflect.TypeOf((*MockPropertiesService)(nil).RetrieveAllPropertiesForEntity), ctx, efp, provMan) +} + // RetrieveProperty mocks base method. func (m *MockPropertiesService) RetrieveProperty(ctx context.Context, provider v10.Provider, projectId, providerID uuid.UUID, lookupProperties *properties.Properties, entType v1.Entity, key string) (*properties.Property, error) { m.ctrl.T.Helper() diff --git a/internal/entities/properties/service/service.go b/internal/entities/properties/service/service.go index faa18e1a98..47284eeea5 100644 --- a/internal/entities/properties/service/service.go +++ b/internal/entities/properties/service/service.go @@ -28,9 +28,11 @@ import ( "github.com/stacklok/minder/internal/db" "github.com/stacklok/minder/internal/engine/entities" + "github.com/stacklok/minder/internal/entities/models" "github.com/stacklok/minder/internal/entities/properties" + "github.com/stacklok/minder/internal/providers/manager" minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" - v1 "github.com/stacklok/minder/pkg/providers/v1" + provifv1 "github.com/stacklok/minder/pkg/providers/v1" ) //go:generate go run go.uber.org/mock/mockgen -package mock_$GOPACKAGE -destination=./mock/$GOFILE -source=./$GOFILE @@ -45,15 +47,24 @@ const ( // PropertiesService is the interface for the properties service type PropertiesService interface { + // EntityWithProperties Fetches an Entity by ID and Project in order to refresh the properties + EntityWithProperties( + ctx context.Context, entityID, projectId uuid.UUID, qtx db.ExtendQuerier, + ) (*models.EntityWithProperties, error) // RetrieveAllProperties fetches all properties for the given entity RetrieveAllProperties( - ctx context.Context, provider v1.Provider, projectId uuid.UUID, + ctx context.Context, provider provifv1.Provider, projectId uuid.UUID, providerID uuid.UUID, lookupProperties *properties.Properties, entType minderv1.Entity, ) (*properties.Properties, error) + // RetrieveAllPropertiesForEntity fetches all properties for the given entity + // for properties model. Note that properties will be updated in place. + RetrieveAllPropertiesForEntity(ctx context.Context, efp *models.EntityWithProperties, + provMan manager.ProviderManager, + ) error // RetrieveProperty fetches a single property for the given entity RetrieveProperty( - ctx context.Context, provider v1.Provider, projectId uuid.UUID, + ctx context.Context, provider provifv1.Provider, projectId uuid.UUID, providerID uuid.UUID, lookupProperties *properties.Properties, entType minderv1.Entity, key string, ) (*properties.Property, error) @@ -68,7 +79,7 @@ type PropertiesService interface { type propertiesServiceOption func(*propertiesService) type propertiesService struct { - store db.Store + store db.ExtendQuerier entityTimeout time.Duration } @@ -81,7 +92,7 @@ func WithEntityTimeout(timeout time.Duration) propertiesServiceOption { // NewPropertiesService creates a new properties service func NewPropertiesService( - store db.Store, + store db.ExtendQuerier, opts ...propertiesServiceOption, ) PropertiesService { ps := &propertiesService{ @@ -98,7 +109,7 @@ func NewPropertiesService( // RetrieveAllProperties fetches a single property for the given entity func (ps *propertiesService) RetrieveAllProperties( - ctx context.Context, provider v1.Provider, projectId uuid.UUID, + ctx context.Context, provider provifv1.Provider, projectId uuid.UUID, providerID uuid.UUID, lookupProperties *properties.Properties, entType minderv1.Entity, ) (*properties.Properties, error) { @@ -156,9 +167,34 @@ func (ps *propertiesService) RetrieveAllProperties( return refreshedProps, nil } +// RetrieveAllPropertiesForEntity fetches a single property for the given an entity +// for properties model. Note that properties will be updated in place. +func (ps *propertiesService) RetrieveAllPropertiesForEntity( + ctx context.Context, efp *models.EntityWithProperties, provMan manager.ProviderManager, +) error { + propClient, err := provMan.InstantiateFromID(ctx, efp.Entity.ProviderID) + if err != nil { + return fmt.Errorf("error instantiating provider: %w", err) + } + + props, err := ps.RetrieveAllProperties( + ctx, + propClient, + efp.Entity.ProjectID, + efp.Entity.ProviderID, + efp.Properties, + efp.Entity.Type) + if err != nil { + return fmt.Errorf("error fetching properties for repository: %w", err) + } + + efp.UpdateProperties(props) + return nil +} + // RetrieveProperty fetches a single property for the given entity func (ps *propertiesService) RetrieveProperty( - ctx context.Context, provider v1.Provider, projectId uuid.UUID, + ctx context.Context, provider provifv1.Provider, projectId uuid.UUID, providerID uuid.UUID, lookupProperties *properties.Properties, entType minderv1.Entity, key string, ) (*properties.Property, error) { @@ -273,6 +309,36 @@ func (_ *propertiesService) ReplaceProperty( return err } +func (ps *propertiesService) EntityWithProperties( + ctx context.Context, entityID, projectID uuid.UUID, + qtx db.ExtendQuerier, +) (*models.EntityWithProperties, error) { + // use the transaction if provided, otherwise use the store + var q db.Querier + if qtx != nil { + q = qtx + } else { + q = ps.store + } + + ent, err := q.GetEntityByID(ctx, db.GetEntityByIDParams{ + ID: entityID, + Projects: []uuid.UUID{projectID}, + }) + if err != nil { + return nil, fmt.Errorf("error getting entity: %w", err) + } + + fetchByProps, err := properties.NewProperties(map[string]any{ + properties.PropertyName: ent.Name, + }) + if err != nil { + return nil, fmt.Errorf("error creating properties: %w", err) + } + + return models.NewEntityWithProperties(ent, fetchByProps), nil +} + func (ps *propertiesService) areDatabasePropertiesValid(dbProps []db.Property) bool { // if the all the properties are to be valid, neither must be older than // the cache timeout diff --git a/internal/history/mock/service.go b/internal/history/mock/service.go index b5606a0b11..6a352cf686 100644 --- a/internal/history/mock/service.go +++ b/internal/history/mock/service.go @@ -43,7 +43,7 @@ func (m *MockEvaluationHistoryService) EXPECT() *MockEvaluationHistoryServiceMoc } // ListEvaluationHistory mocks base method. -func (m *MockEvaluationHistoryService) ListEvaluationHistory(ctx context.Context, qtx db.Querier, cursor *history.ListEvaluationCursor, size uint32, filter history.ListEvaluationFilter) (*history.ListEvaluationHistoryResult, error) { +func (m *MockEvaluationHistoryService) ListEvaluationHistory(ctx context.Context, qtx db.ExtendQuerier, cursor *history.ListEvaluationCursor, size uint32, filter history.ListEvaluationFilter) (*history.ListEvaluationHistoryResult, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ListEvaluationHistory", ctx, qtx, cursor, size, filter) ret0, _ := ret[0].(*history.ListEvaluationHistoryResult) diff --git a/internal/history/models.go b/internal/history/models.go index 77858a1caa..ad749b12fc 100644 --- a/internal/history/models.go +++ b/internal/history/models.go @@ -26,6 +26,7 @@ import ( "github.com/google/uuid" "github.com/stacklok/minder/internal/db" + em "github.com/stacklok/minder/internal/entities/models" ) var ( @@ -634,11 +635,18 @@ func NewListEvaluationFilter(opts ...FilterOpt) (ListEvaluationFilter, error) { return filter, nil } +// OneEvalHistoryAndEntity is a struct representing a combination of an +// evaluation and an entity. +type OneEvalHistoryAndEntity struct { + *em.EntityWithProperties + EvalHistoryRow db.ListEvaluationHistoryRow +} + // ListEvaluationHistoryResult is the return value of // ListEvaluationHistory function. type ListEvaluationHistoryResult struct { // Data is an ordered collection of evaluation events. - Data []db.ListEvaluationHistoryRow + Data []*OneEvalHistoryAndEntity // Next is an object usable as cursor to fetch the next // page. The page is absent if Next is nil. Next []byte diff --git a/internal/history/service.go b/internal/history/service.go index 98a2150fc2..4d7d762d03 100644 --- a/internal/history/service.go +++ b/internal/history/service.go @@ -26,6 +26,8 @@ import ( "github.com/stacklok/minder/internal/db" evalerrors "github.com/stacklok/minder/internal/engine/errors" + propertiessvc "github.com/stacklok/minder/internal/entities/properties/service" + "github.com/stacklok/minder/internal/providers/manager" ) //go:generate go run go.uber.org/mock/mockgen -package mock_$GOPACKAGE -destination=./mock/$GOFILE -source=./$GOFILE @@ -48,19 +50,40 @@ type EvaluationHistoryService interface { // in the history table. ListEvaluationHistory( ctx context.Context, - qtx db.Querier, + qtx db.ExtendQuerier, cursor *ListEvaluationCursor, size uint32, filter ListEvaluationFilter, ) (*ListEvaluationHistoryResult, error) } +type options func(*evaluationHistoryService) + +func withPropertiesServiceBuilder(builder func(qtx db.ExtendQuerier) propertiessvc.PropertiesService) options { + return func(ehs *evaluationHistoryService) { + ehs.propServiceBuilder = builder + } +} + // NewEvaluationHistoryService creates a new instance of EvaluationHistoryService -func NewEvaluationHistoryService() EvaluationHistoryService { - return &evaluationHistoryService{} +func NewEvaluationHistoryService(providerManager manager.ProviderManager, opts ...options) EvaluationHistoryService { + ehs := &evaluationHistoryService{ + providerManager: providerManager, + propServiceBuilder: func(qtx db.ExtendQuerier) propertiessvc.PropertiesService { + return propertiessvc.NewPropertiesService(qtx) + }, + } + for _, opt := range opts { + opt(ehs) + } + + return ehs } -type evaluationHistoryService struct{} +type evaluationHistoryService struct { + providerManager manager.ProviderManager + propServiceBuilder func(qtx db.ExtendQuerier) propertiessvc.PropertiesService +} func (e *evaluationHistoryService) StoreEvaluationStatus( ctx context.Context, @@ -76,18 +99,13 @@ func (e *evaluationHistoryService) StoreEvaluationStatus( status := evalerrors.ErrorAsEvalStatus(evalError) details := evalerrors.ErrorAsEvalDetails(evalError) - params, err := paramsFromEntity(ruleID, entityID, entityType) - if err != nil { - return uuid.Nil, err - } + params := paramsFromEntity(ruleID, entityID) // find the latest record for this rule/entity pair latestRecord, err := qtx.GetLatestEvalStateForRuleEntity(ctx, db.GetLatestEvalStateForRuleEntityParams{ - RuleID: params.RuleID, - RepositoryID: params.RepositoryID, - PullRequestID: params.PullRequestID, - ArtifactID: params.ArtifactID, + RuleID: params.RuleID, + EntityInstanceID: params.EntityID, }, ) if err != nil { @@ -96,9 +114,6 @@ func (e *evaluationHistoryService) StoreEvaluationStatus( ruleEntityID, err = qtx.InsertEvaluationRuleEntity(ctx, db.InsertEvaluationRuleEntityParams{ RuleID: params.RuleID, - RepositoryID: params.RepositoryID, - PullRequestID: params.PullRequestID, - ArtifactID: params.ArtifactID, EntityType: entityType, EntityInstanceID: params.EntityID, }, @@ -160,45 +175,21 @@ func (_ *evaluationHistoryService) createNewStatus( func paramsFromEntity( ruleID uuid.UUID, entityID uuid.UUID, - entityType db.Entities, -) (*ruleEntityParams, error) { +) *ruleEntityParams { params := ruleEntityParams{RuleID: ruleID} - nullableEntityID := uuid.NullUUID{ - UUID: entityID, - Valid: true, - } - params.EntityID = entityID - - switch entityType { - case db.EntitiesRepository: - params.RepositoryID = nullableEntityID - case db.EntitiesPullRequest: - params.PullRequestID = nullableEntityID - case db.EntitiesArtifact: - params.ArtifactID = nullableEntityID - case db.EntitiesBuildEnvironment, db.EntitiesRelease, - db.EntitiesPipelineRun, db.EntitiesTaskRun, db.EntitiesBuild: - default: - return nil, fmt.Errorf("unknown entity %q", entityType) - } - return ¶ms, nil + return ¶ms } type ruleEntityParams struct { - RuleID uuid.UUID - RepositoryID uuid.NullUUID - ArtifactID uuid.NullUUID - PullRequestID uuid.NullUUID - // Is the target entity ID. We'll be replacing the single-entity IDs - // with this one. + RuleID uuid.UUID EntityID uuid.UUID } -func (_ *evaluationHistoryService) ListEvaluationHistory( +func (ehs *evaluationHistoryService) ListEvaluationHistory( ctx context.Context, - qtx db.Querier, + qtx db.ExtendQuerier, cursor *ListEvaluationCursor, size uint32, filter ListEvaluationFilter, @@ -223,8 +214,28 @@ func (_ *evaluationHistoryService) ListEvaluationHistory( slices.Reverse(rows) } + propsvc := ehs.propServiceBuilder(qtx) + + data := make([]*OneEvalHistoryAndEntity, 0, len(rows)) + for _, row := range rows { + efp, err := propsvc.EntityWithProperties(ctx, row.EntityID, row.ProjectID, qtx) + if err != nil { + return nil, fmt.Errorf("error fetching entity for properties: %w", err) + } + + err = propsvc.RetrieveAllPropertiesForEntity(ctx, efp, ehs.providerManager) + if err != nil { + return nil, fmt.Errorf("error fetching properties for entity: %w", err) + } + + data = append(data, &OneEvalHistoryAndEntity{ + EntityWithProperties: efp, + EvalHistoryRow: row, + }) + } + result := &ListEvaluationHistoryResult{ - Data: rows, + Data: data, } if len(rows) > 0 { newest := rows[0] diff --git a/internal/history/service_test.go b/internal/history/service_test.go index 64ce0b127e..9d48524896 100644 --- a/internal/history/service_test.go +++ b/internal/history/service_test.go @@ -27,6 +27,10 @@ import ( "github.com/stacklok/minder/internal/db" dbf "github.com/stacklok/minder/internal/db/fixtures" + entmodels "github.com/stacklok/minder/internal/entities/models" + "github.com/stacklok/minder/internal/entities/properties/service" + propsSvcMock "github.com/stacklok/minder/internal/entities/properties/service/mock" + pmMock "github.com/stacklok/minder/internal/providers/manager/mock" ) func TestStoreEvaluationStatus(t *testing.T) { @@ -41,7 +45,8 @@ func TestStoreEvaluationStatus(t *testing.T) { { Name: "StoreEvaluationStatus rejects invalid entity type", EntityType: "I'm a little teapot", - ExpectedError: "unknown entity", + DBSetup: dbf.NewDBMock(withGetLatestEval(emptyLatestResult, errTest)), + ExpectedError: "error while querying DB", }, { Name: "StoreEvaluationStatus returns error when unable to query previous status", @@ -113,7 +118,8 @@ func TestStoreEvaluationStatus(t *testing.T) { store = scenario.DBSetup(ctrl) } - service := NewEvaluationHistoryService() + // provider manager is not used by this function + service := NewEvaluationHistoryService(nil) id, err := service.StoreEvaluationStatus( ctx, store, ruleID, profileID, scenario.EntityType, entityID, errTest, []byte("{}")) if scenario.ExpectedError == "" { @@ -150,16 +156,30 @@ func TestListEvaluationHistory(t *testing.T) { } tests := []struct { - name string - dbSetup dbf.DBMockBuilder - cursor *ListEvaluationCursor - size uint32 - filter ListEvaluationFilter - checkf func(*testing.T, *ListEvaluationHistoryResult) - err bool + name string + dbSetup dbf.DBMockBuilder + cursor *ListEvaluationCursor + size uint32 + filter ListEvaluationFilter + checkf func(*testing.T, *ListEvaluationHistoryResult) + err bool + efp []*entmodels.EntityWithProperties + entityForPropertiesError error + retrieveAllPropsErr error }{ { name: "records", + efp: []*entmodels.EntityWithProperties{ + entmodels.NewEntityWithPropertiesFromInstance(entmodels.EntityInstance{ + ID: uuid1, + }, nil), + entmodels.NewEntityWithPropertiesFromInstance(entmodels.EntityInstance{ + ID: uuid2, + }, nil), + entmodels.NewEntityWithPropertiesFromInstance(entmodels.EntityInstance{ + ID: uuid3, + }, nil), + }, dbSetup: dbf.NewDBMock( withListEvaluationHistory(nil, nil, makeHistoryRow( @@ -193,19 +213,19 @@ func TestListEvaluationHistory(t *testing.T) { // database order is maintained item1 := rows.Data[0] - require.Equal(t, uuid1, item1.EvaluationID) - require.Equal(t, evaluatedAt1, item1.EvaluatedAt) - require.Equal(t, uuid1, item1.EntityID) + require.Equal(t, uuid1, item1.EvalHistoryRow.EvaluationID) + require.Equal(t, evaluatedAt1, item1.EvalHistoryRow.EvaluatedAt) + require.Equal(t, uuid1, item1.Entity.ID) item2 := rows.Data[1] - require.Equal(t, uuid2, item2.EvaluationID) - require.Equal(t, evaluatedAt2, item2.EvaluatedAt) - require.Equal(t, uuid2, item2.EntityID) + require.Equal(t, uuid2, item2.EvalHistoryRow.EvaluationID) + require.Equal(t, evaluatedAt2, item2.EvalHistoryRow.EvaluatedAt) + require.Equal(t, uuid2, item2.Entity.ID) item3 := rows.Data[2] - require.Equal(t, uuid3, item3.EvaluationID) - require.Equal(t, evaluatedAt3, item3.EvaluatedAt) - require.Equal(t, uuid3, item3.EntityID) + require.Equal(t, uuid3, item3.EvalHistoryRow.EvaluationID) + require.Equal(t, evaluatedAt3, item3.EvalHistoryRow.EvaluatedAt) + require.Equal(t, uuid3, item3.Entity.ID) }, }, @@ -230,6 +250,14 @@ func TestListEvaluationHistory(t *testing.T) { ), ), ), + efp: []*entmodels.EntityWithProperties{ + entmodels.NewEntityWithPropertiesFromInstance(entmodels.EntityInstance{ + ID: uuid1, + }, nil), + entmodels.NewEntityWithPropertiesFromInstance(entmodels.EntityInstance{ + ID: uuid2, + }, nil), + }, cursor: &ListEvaluationCursor{ Direction: Next, }, @@ -241,14 +269,14 @@ func TestListEvaluationHistory(t *testing.T) { // database order is maintained item1 := rows.Data[0] - require.Equal(t, uuid1, item1.EvaluationID) - require.Equal(t, evaluatedAt1, item1.EvaluatedAt) - require.Equal(t, uuid1, item1.EntityID) + require.Equal(t, uuid1, item1.EvalHistoryRow.EvaluationID) + require.Equal(t, evaluatedAt1, item1.EvalHistoryRow.EvaluatedAt) + require.Equal(t, uuid1, item1.Entity.ID) item2 := rows.Data[1] - require.Equal(t, uuid2, item2.EvaluationID) - require.Equal(t, evaluatedAt2, item2.EvaluatedAt) - require.Equal(t, uuid2, item2.EntityID) + require.Equal(t, uuid2, item2.EvalHistoryRow.EvaluationID) + require.Equal(t, evaluatedAt2, item2.EvalHistoryRow.EvaluatedAt) + require.Equal(t, uuid2, item2.Entity.ID) }, }, { @@ -271,6 +299,14 @@ func TestListEvaluationHistory(t *testing.T) { ), ), ), + efp: []*entmodels.EntityWithProperties{ + entmodels.NewEntityWithPropertiesFromInstance(entmodels.EntityInstance{ + ID: uuid1, + }, nil), + entmodels.NewEntityWithPropertiesFromInstance(entmodels.EntityInstance{ + ID: uuid2, + }, nil), + }, cursor: &ListEvaluationCursor{ Direction: Prev, }, @@ -282,14 +318,14 @@ func TestListEvaluationHistory(t *testing.T) { // database order is maintained item1 := rows.Data[0] - require.Equal(t, uuid1, item1.EvaluationID) - require.Equal(t, evaluatedAt1, item1.EvaluatedAt) - require.Equal(t, uuid1, item1.EntityID) + require.Equal(t, uuid1, item1.EvalHistoryRow.EvaluationID) + require.Equal(t, evaluatedAt1, item1.EvalHistoryRow.EvaluatedAt) + require.Equal(t, uuid1, item1.Entity.ID) item2 := rows.Data[1] - require.Equal(t, uuid2, item2.EvaluationID) - require.Equal(t, evaluatedAt2, item2.EvaluatedAt) - require.Equal(t, uuid2, item2.EntityID) + require.Equal(t, uuid2, item2.EvalHistoryRow.EvaluationID) + require.Equal(t, evaluatedAt2, item2.EvalHistoryRow.EvaluatedAt) + require.Equal(t, uuid2, item2.Entity.ID) }, }, @@ -716,6 +752,72 @@ func TestListEvaluationHistory(t *testing.T) { ), err: true, }, + { + name: "error getting entity", + err: true, + entityForPropertiesError: errors.New("whoops"), + dbSetup: dbf.NewDBMock( + withListEvaluationHistory(nil, nil, + makeHistoryRow( + uuid1, + evaluatedAt1, + entityType, + remediation, + alert, + ), + makeHistoryRow( + uuid2, + evaluatedAt2, + entityType, + remediation, + alert, + ), + makeHistoryRow( + uuid3, + evaluatedAt3, + entityType, + remediation, + alert, + ), + ), + ), + }, + { + name: "error getting properties", + err: true, + retrieveAllPropsErr: errors.New("whoops"), + efp: []*entmodels.EntityWithProperties{ + // Only called once + entmodels.NewEntityWithPropertiesFromInstance(entmodels.EntityInstance{ + ID: uuid1, + }, nil), + }, + dbSetup: dbf.NewDBMock( + withListEvaluationHistory(nil, nil, + makeHistoryRow( + uuid1, + evaluatedAt1, + entityType, + remediation, + alert, + ), + makeHistoryRow( + uuid2, + evaluatedAt2, + entityType, + remediation, + alert, + ), + makeHistoryRow( + uuid3, + evaluatedAt3, + entityType, + remediation, + alert, + ), + ), + ), + }, } for _, tt := range tests { @@ -731,7 +833,23 @@ func TestListEvaluationHistory(t *testing.T) { store = tt.dbSetup(ctrl) } - service := NewEvaluationHistoryService() + pm := pmMock.NewMockProviderManager(ctrl) + propsSvc := propsSvcMock.NewMockPropertiesService(ctrl) + for _, efp := range tt.efp { + propsSvc.EXPECT().EntityWithProperties(ctx, gomock.Any(), gomock.Any(), gomock.Any()). + Return(efp, tt.entityForPropertiesError) + } + + if tt.entityForPropertiesError != nil && len(tt.efp) == 0 { + propsSvc.EXPECT().EntityWithProperties(ctx, gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, tt.entityForPropertiesError).AnyTimes() + } + propsSvc.EXPECT().RetrieveAllPropertiesForEntity(ctx, gomock.Any(), gomock.Any()). + Return(tt.retrieveAllPropsErr).AnyTimes() + + service := NewEvaluationHistoryService(pm, withPropertiesServiceBuilder(func(_ db.ExtendQuerier) service.PropertiesService { + return propsSvc + })) res, err := service.ListEvaluationHistory(ctx, store, tt.cursor, tt.size, tt.filter) if tt.err { require.Error(t, err) @@ -759,22 +877,6 @@ func makeHistoryRow( EvaluatedAt: evaluatedAt, EntityType: entityType, EntityID: id, - RepoOwner: sql.NullString{ - Valid: true, - String: "stacklok", - }, - RepoName: sql.NullString{ - Valid: true, - String: "minder", - }, - PrNumber: sql.NullInt64{ - Valid: true, - Int64: 12345, - }, - ArtifactName: sql.NullString{ - Valid: true, - String: "artifact1", - }, // EntityName: "repo1", RuleType: "rule_type", RuleName: "rule_name", diff --git a/internal/repositories/github/service.go b/internal/repositories/github/service.go index 5c71e0cd02..58dae6ad17 100644 --- a/internal/repositories/github/service.go +++ b/internal/repositories/github/service.go @@ -377,7 +377,7 @@ func (r *repositoryService) RefreshRepositoryByUpstreamID( if errors.Is(err, provifv1.ErrEntityNotFound) { // return the entity without properties in case the upstream entity is not found ewp := models.NewEntityWithProperties(entRepo, repoProperties) - return &ewp, nil + return ewp, nil } else if err != nil { return nil, fmt.Errorf("error fetching properties for repository: %w", err) } @@ -385,7 +385,7 @@ func (r *repositoryService) RefreshRepositoryByUpstreamID( if !isLegacy { // this is not a migration from the legacy tables, we're done ewp := models.NewEntityWithProperties(entRepo, repoProperties) - return &ewp, nil + return ewp, nil } zerolog.Ctx(ctx).Debug().Str("repo_name", entRepo.Name).Msg("migrating legacy repository") @@ -403,7 +403,7 @@ func (r *repositoryService) RefreshRepositoryByUpstreamID( // we could as well call RetrieveAllProperties again, we should just get the same data ewp := models.NewEntityWithProperties(entRepo, repoProperties.Merge(legacyProps)) - return &ewp, nil + return ewp, nil }) if err != nil { diff --git a/internal/service/service.go b/internal/service/service.go index 49ae3b00c0..4328881fdb 100644 --- a/internal/service/service.go +++ b/internal/service/service.go @@ -97,7 +97,6 @@ func AllInOneServerService( serverconfig.FallbackOAuthClientConfigValues("github", &cfg.Provider.GitHub.OAuthClientConfig) serverconfig.FallbackOAuthClientConfigValues("github-app", &cfg.Provider.GitHubApp.OAuthClientConfig) - historySvc := history.NewEvaluationHistoryService() inviteSvc := invites.NewInviteService() selChecker := selectors.NewEnv() profileSvc := profiles.NewProfileService(evt, selChecker) @@ -153,6 +152,7 @@ func AllInOneServerService( return fmt.Errorf("failed to create provider auth manager: %w", err) } propSvc := propService.NewPropertiesService(store) + historySvc := history.NewEvaluationHistoryService(providerManager) repos := github.NewRepositoryService(whManager, store, propSvc, evt, providerManager) projectDeleter := projects.NewProjectDeleter(authzClient, providerManager) sessionsService := session.NewProviderSessionService(providerManager, providerStore, store) @@ -210,7 +210,7 @@ func AllInOneServerService( store, providerManager, executorMetrics, - history.NewEvaluationHistoryService(), + historySvc, featureFlagClient, profileStore, selEnv,