Skip to content

Commit

Permalink
ListEvaluationHistory filtering fixes. (#3866)
Browse files Browse the repository at this point in the history 
Filtering on entity name was not accounting for repo owner in case of
repositories. Additionally, wrong values for enumerations were
reported as internal errors rather than validation errors.
  • Loading branch information
@blkt
blkt authored Jul 11, 2024
1 parent df6cf1c commit 9877dba
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 26 deletions.
4 changes: 2 additions & 2 deletions database/query/eval_history.sql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ SELECT s.id::uuid AS evaluation_id,
AND (sqlc.narg(prev)::timestamp without time zone IS NULL OR sqlc.narg(prev) < s.most_recent_evaluation)
-- inclusion filters
AND (sqlc.slice(entityTypes)::entities[] IS NULL OR entity_type::entities = ANY(sqlc.slice(entityTypes)::entities[]))
AND (sqlc.slice(entityNames)::text[] IS NULL OR ere.repository_id IS NULL OR r.repo_name = ANY(sqlc.slice(entityNames)::text[]))
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(profileNames)::text[] IS NULL OR p.name = ANY(sqlc.slice(profileNames)::text[]))
Expand All @@ -150,7 +150,7 @@ SELECT s.id::uuid AS evaluation_id,
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 entity_type::entities != ANY(sqlc.slice(notEntityTypes)::entities[]))
AND (sqlc.slice(notEntityNames)::text[] IS NULL OR ere.repository_id IS NULL OR r.repo_name != ANY(sqlc.slice(notEntityNames)::text[]))
AND (sqlc.slice(notEntityNames)::text[] IS NULL OR ere.repository_id IS NULL OR CONCAT(r.repo_owner, '/', r.repo_name) != ANY(sqlc.slice(notEntityNames)::text[]))
AND (sqlc.slice(notEntityNames)::text[] IS NULL OR ere.pull_request_id IS NULL OR pr.pr_number::text != ANY(sqlc.slice(notEntityNames)::text[]))
AND (sqlc.slice(notEntityNames)::text[] IS NULL OR ere.artifact_id IS NULL OR a.artifact_name != ANY(sqlc.slice(notEntityNames)::text[]))
AND (sqlc.slice(notProfileNames)::text[] IS NULL OR p.name != ANY(sqlc.slice(notProfileNames)::text[]))
Expand Down
4 changes: 2 additions & 2 deletions internal/db/eval_history.sql.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 20 additions & 4 deletions internal/history/models.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"encoding/base64"
"errors"
"fmt"
"slices"
"strconv"
"strings"
"time"
Expand All @@ -44,6 +45,13 @@ var (
ErrInvalidIdentifier = errors.New("invalid identifier")
)

var (
allowedEntityTypes = []string{"repository", "build_environment", "artifact", "pull_request"}
allowedEvaluationStatuses = []string{"success", "failure", "error", "skipped", "pending"}
allowedRemediationStatuses = []string{"success", "failure", "error", "skipped", "not_available", "pending"}
allowedAlertStatuses = []string{"on", "off", "error", "skipped", "not_available"}
)

// Direction enumerates the direction of the Cursor.
type Direction string

Expand Down Expand Up @@ -294,6 +302,9 @@ func (filter *listEvaluationFilter) AddEntityType(entityType string) error {
} else {
filter.includedEntityTypes = append(filter.includedEntityTypes, entityType)
}
if !slices.Contains(allowedEntityTypes, entityType) {
return fmt.Errorf("%w: entity type", ErrInvalidIdentifier)
}

// Prevent filtering for both inclusion and exclusion
if len(filter.includedEntityTypes) > 0 &&
Expand Down Expand Up @@ -363,6 +374,9 @@ func (filter *listEvaluationFilter) AddStatus(status string) error {
} else {
filter.includedStatuses = append(filter.includedStatuses, status)
}
if !slices.Contains(allowedEvaluationStatuses, status) {
return fmt.Errorf("%w: status", ErrInvalidIdentifier)
}

// Prevent filtering for both inclusion and exclusion
if len(filter.includedStatuses) > 0 &&
Expand All @@ -386,6 +400,9 @@ func (filter *listEvaluationFilter) AddRemediation(remediation string) error {
} else {
filter.includedRemediations = append(filter.includedRemediations, remediation)
}
if !slices.Contains(allowedRemediationStatuses, remediation) {
return fmt.Errorf("%w: remediation", ErrInvalidIdentifier)
}

// Prevent filtering for both inclusion and exclusion
if len(filter.includedRemediations) > 0 &&
Expand All @@ -409,6 +426,9 @@ func (filter *listEvaluationFilter) AddAlert(alert string) error {
} else {
filter.includedAlerts = append(filter.includedAlerts, alert)
}
if !slices.Contains(allowedAlertStatuses, alert) {
return fmt.Errorf("%w: alert", ErrInvalidIdentifier)
}

// Prevent filtering for both inclusion and exclusion
if len(filter.includedAlerts) > 0 &&
Expand Down Expand Up @@ -483,7 +503,6 @@ func WithEntityType(entityType string) FilterOpt {
if !ok {
return fmt.Errorf("%w: wrong filter type", ErrInvalidIdentifier)
}
// TODO add validation on enumerated types
return inner.AddEntityType(entityType)
}
}
Expand Down Expand Up @@ -532,7 +551,6 @@ func WithStatus(status string) FilterOpt {
if !ok {
return fmt.Errorf("%w: wrong filter type", ErrInvalidIdentifier)
}
// TODO add validation on enumerated types
return inner.AddStatus(status)
}
}
Expand All @@ -549,7 +567,6 @@ func WithRemediation(remediation string) FilterOpt {
if !ok {
return fmt.Errorf("%w: wrong filter type", ErrInvalidIdentifier)
}
// TODO add validation on enumerated types
return inner.AddRemediation(remediation)
}
}
Expand All @@ -566,7 +583,6 @@ func WithAlert(alert string) FilterOpt {
if !ok {
return fmt.Errorf("%w: wrong filter type", ErrInvalidIdentifier)
}
// TODO add validation on enumerated types
return inner.AddAlert(alert)
}
}
Expand Down
36 changes: 18 additions & 18 deletions internal/history/models_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -314,8 +314,8 @@ func TestListEvaluationFilter(t *testing.T) {
filter: func(t *testing.T) (ListEvaluationFilter, error) {
t.Helper()
return NewListEvaluationFilter(
WithAlert("success"),
WithAlert("!failure"),
WithAlert("on"),
WithAlert("!off"),
)
},
err: true,
Expand Down Expand Up @@ -491,7 +491,7 @@ func TestFilterOptions(t *testing.T) {
name: "bogus entity type",
option: func(t *testing.T) FilterOpt {
t.Helper()
return WithEntityType("!")
return WithEntityType("foo")
},
filter: func(t *testing.T) Filter {
t.Helper()
Expand Down Expand Up @@ -665,7 +665,7 @@ func TestFilterOptions(t *testing.T) {
name: "status in filter",
option: func(t *testing.T) FilterOpt {
t.Helper()
return WithStatus("repository")
return WithStatus("success")
},
filter: func(t *testing.T) Filter {
t.Helper()
Expand All @@ -675,15 +675,15 @@ func TestFilterOptions(t *testing.T) {
t.Helper()
f := filter.(StatusFilter)
require.NotNil(t, f.IncludedStatuses())
require.Equal(t, []string{"repository"}, f.IncludedStatuses())
require.Equal(t, []string{"success"}, f.IncludedStatuses())
require.Nil(t, f.ExcludedStatuses())
},
},
{
name: "status not in filter",
option: func(t *testing.T) FilterOpt {
t.Helper()
return WithStatus("!repository")
return WithStatus("!success")
},
filter: func(t *testing.T) Filter {
t.Helper()
Expand All @@ -694,7 +694,7 @@ func TestFilterOptions(t *testing.T) {
f := filter.(StatusFilter)
require.Nil(t, f.IncludedStatuses())
require.NotNil(t, f.ExcludedStatuses())
require.Equal(t, []string{"repository"}, f.ExcludedStatuses())
require.Equal(t, []string{"success"}, f.ExcludedStatuses())
},
},
{
Expand All @@ -713,7 +713,7 @@ func TestFilterOptions(t *testing.T) {
name: "bogus status",
option: func(t *testing.T) FilterOpt {
t.Helper()
return WithStatus("!")
return WithStatus("foo")
},
filter: func(t *testing.T) Filter {
t.Helper()
Expand All @@ -739,7 +739,7 @@ func TestFilterOptions(t *testing.T) {
name: "remediation in filter",
option: func(t *testing.T) FilterOpt {
t.Helper()
return WithRemediation("repository")
return WithRemediation("success")
},
filter: func(t *testing.T) Filter {
t.Helper()
Expand All @@ -749,15 +749,15 @@ func TestFilterOptions(t *testing.T) {
t.Helper()
f := filter.(RemediationFilter)
require.NotNil(t, f.IncludedRemediations())
require.Equal(t, []string{"repository"}, f.IncludedRemediations())
require.Equal(t, []string{"success"}, f.IncludedRemediations())
require.Nil(t, f.ExcludedRemediations())
},
},
{
name: "remediation not in filter",
option: func(t *testing.T) FilterOpt {
t.Helper()
return WithRemediation("!repository")
return WithRemediation("!success")
},
filter: func(t *testing.T) Filter {
t.Helper()
Expand All @@ -768,7 +768,7 @@ func TestFilterOptions(t *testing.T) {
f := filter.(RemediationFilter)
require.Nil(t, f.IncludedRemediations())
require.NotNil(t, f.ExcludedRemediations())
require.Equal(t, []string{"repository"}, f.ExcludedRemediations())
require.Equal(t, []string{"success"}, f.ExcludedRemediations())
},
},
{
Expand All @@ -787,7 +787,7 @@ func TestFilterOptions(t *testing.T) {
name: "bogus remediation",
option: func(t *testing.T) FilterOpt {
t.Helper()
return WithRemediation("!")
return WithRemediation("foo")
},
filter: func(t *testing.T) Filter {
t.Helper()
Expand All @@ -813,7 +813,7 @@ func TestFilterOptions(t *testing.T) {
name: "alert in filter",
option: func(t *testing.T) FilterOpt {
t.Helper()
return WithAlert("repository")
return WithAlert("on")
},
filter: func(t *testing.T) Filter {
t.Helper()
Expand All @@ -823,15 +823,15 @@ func TestFilterOptions(t *testing.T) {
t.Helper()
f := filter.(AlertFilter)
require.NotNil(t, f.IncludedAlerts())
require.Equal(t, []string{"repository"}, f.IncludedAlerts())
require.Equal(t, []string{"on"}, f.IncludedAlerts())
require.Nil(t, f.ExcludedAlerts())
},
},
{
name: "alert not in filter",
option: func(t *testing.T) FilterOpt {
t.Helper()
return WithAlert("!repository")
return WithAlert("!on")
},
filter: func(t *testing.T) Filter {
t.Helper()
Expand All @@ -842,7 +842,7 @@ func TestFilterOptions(t *testing.T) {
f := filter.(AlertFilter)
require.Nil(t, f.IncludedAlerts())
require.NotNil(t, f.ExcludedAlerts())
require.Equal(t, []string{"repository"}, f.ExcludedAlerts())
require.Equal(t, []string{"on"}, f.ExcludedAlerts())
},
},
{
Expand All @@ -861,7 +861,7 @@ func TestFilterOptions(t *testing.T) {
name: "bogus alert",
option: func(t *testing.T) FilterOpt {
t.Helper()
return WithAlert("!")
return WithAlert("foo")
},
filter: func(t *testing.T) Filter {
t.Helper()
Expand Down

0 comments on commit 9877dba

Please sign in to comment.