Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pkg/api/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,14 @@ Endpoint: `/api/tests`
| sort | asc / desc | Sort type, ascending or descending | "asc" or "desc" |
| limit | Integer | The maximum amount of results to return | N/A |

`filter` supports a `lifecycle` field (`equals` or `!=` operators only; other operators return a
400) to restrict results to a test lifecycle (`blocking` or `informing`). It narrows which
underlying test runs are aggregated; it is not returned as a field on results, and
blocking/informing runs for the same test are combined into a single row when no lifecycle filter
is applied. This filter is only supported against the Postgres-backed report (`/api/tests`); using
it against `/api/tests/v2` (BigQuery) returns a 400, since the underlying BigQuery comparison
tables don't carry a lifecycle column.
Comment thread
coderabbitai[bot] marked this conversation as resolved.

<details>
<summary>Example response</summary>

Expand Down
5 changes: 5 additions & 0 deletions pkg/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"github.com/jackc/pgconn"
log "github.com/sirupsen/logrus"
"google.golang.org/api/googleapi"

"github.com/openshift/sippy/pkg/filter"
)

func RespondWithJSON(statusCode int, w http.ResponseWriter, data interface{}) {
Expand Down Expand Up @@ -68,5 +70,8 @@ func IsBadRequestError(err error) bool {
len(apiErr.Errors) > 0 && apiErr.Errors[0].Reason == bqInvalidQuery {
return true
}
if errors.Is(err, filter.ErrUnsupportedOperator) {
return true
}
return false
}
26 changes: 19 additions & 7 deletions pkg/api/tests.go
Original file line number Diff line number Diff line change
Expand Up @@ -439,11 +439,13 @@ func (spec *TestResultsSpec) buildTestsResultsFromPostgres(ctx context.Context,
func (spec *TestResultsSpec) buildTestsResultsPGGenerator(ctx context.Context, dbc *db.DB, sample, base query.DateRange) (result testResults, errs []error) {
now := time.Now()

var nameFilter, variantFilter, processedFilter *filter.Filter
var nameFilter, variantFilter, processedFilter, lifecycleFilter *filter.Filter
if spec.Filter != nil {
var rawFilter *filter.Filter
rawFilter, processedFilter = spec.Filter.Split([]string{"name", "variants"})
nameFilter, variantFilter = rawFilter.Split([]string{"name"})
var nameVariantsAndLifecycle *filter.Filter
nameVariantsAndLifecycle, processedFilter = spec.Filter.Split([]string{"name", "variants", "lifecycle"})
var variantsAndLifecycle *filter.Filter
nameFilter, variantsAndLifecycle = nameVariantsAndLifecycle.Split([]string{"name"})
variantFilter, lifecycleFilter = variantsAndLifecycle.Split([]string{"variants"})
}

testMetadataColumns := []string{"suite_name", "name", "jira_component", "jira_component_id"}
Expand All @@ -453,7 +455,7 @@ func (spec *TestResultsSpec) buildTestsResultsPGGenerator(ctx context.Context, d
if spec.Collapse {
workMem = "16MB"

collapsedQuery, err := query.TestReportQueryCollapsed(dbc, spec.Release, sample, base, variantFilter, nameFilter)
collapsedQuery, err := query.TestReportQueryCollapsed(dbc, spec.Release, sample, base, variantFilter, nameFilter, lifecycleFilter)
if err != nil {
errs = append(errs, err)
return
Expand All @@ -471,7 +473,7 @@ func (spec *TestResultsSpec) buildTestsResultsPGGenerator(ctx context.Context, d
Where("current_runs > 0 or previous_runs > 0")
finalResults = dbc.DB.Table("(?) as final_results", processedResults)
} else {
rawQuery, remainingFilter, err := query.UncollapsedTestReportWithStats(dbc, spec.Release, sample, base, nameFilter, variantFilter, processedFilter)
rawQuery, remainingFilter, err := query.UncollapsedTestReportWithStats(dbc, spec.Release, sample, base, nameFilter, variantFilter, processedFilter, lifecycleFilter)
if err != nil {
errs = append(errs, err)
return
Expand Down Expand Up @@ -588,7 +590,17 @@ func (spec *TestResultsSpec) buildTestsResultsBQGenerator(ctx context.Context, b
// assembled our final temporary table.
var rawFilter, processedFilter *filter.Filter
if spec.Filter != nil {
rawFilter, processedFilter = spec.Filter.Split([]string{"name", "variants"})
var lifecycleFilter *filter.Filter
lifecycleFilter, processedFilter = spec.Filter.Split([]string{"lifecycle"})
if len(lifecycleFilter.Items) > 0 {
// junit_7day_comparison/junit_2day_comparison are materialized BigQuery tables
// populated outside this repo and have no lifecycle column, unlike the Postgres
// cumulative summary tables. Fail clearly rather than silently ignoring the filter.
return testResultsBQ{}, []error{&ValidationError{
Message: "lifecycle filter is not supported for the BigQuery-backed tests report",
}}
}
rawFilter, processedFilter = processedFilter.Split([]string{"name", "variants"})
}
table := "junit_7day_comparison"
if spec.Period == "twoDay" {
Expand Down
49 changes: 49 additions & 0 deletions pkg/api/tests_test.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package api

import (
"context"
"errors"
"strings"
"testing"

apitype "github.com/openshift/sippy/pkg/apis/api"
"github.com/openshift/sippy/pkg/filter"
)

func TestComputeOverallTest(t *testing.T) {
Expand Down Expand Up @@ -150,3 +154,48 @@ func assertFloat(t *testing.T, name string, got, want float64) {
t.Errorf("%s = %f, want %f", name, got, want)
}
}

// TestBuildTestsResultsBQGenerator_RejectsLifecycleFilter guards the /api/tests/v2
// (BigQuery-backed) 400 response: the underlying junit_7day_comparison/
// junit_2day_comparison tables have no lifecycle column, so a lifecycle filter must be
// rejected explicitly rather than silently ignored. This never reaches bqc, so passing
// nil is safe.
func TestBuildTestsResultsBQGenerator_RejectsLifecycleFilter(t *testing.T) {
spec := &TestResultsSpec{
Release: "4.16",
Filter: &filter.Filter{Items: []filter.FilterItem{
{Field: "lifecycle", Operator: filter.OperatorEquals, Value: "blocking"},
}},
}

_, errs := spec.buildTestsResultsBQGenerator(context.Background(), nil)
if len(errs) != 1 {
t.Fatalf("errs = %v, want exactly one error", errs)
}
var validationErr *ValidationError
if !errors.As(errs[0], &validationErr) {
t.Fatalf("errs[0] = %v (%T), want *ValidationError so it surfaces as HTTP 400", errs[0], errs[0])
}
if !strings.Contains(validationErr.Message, "lifecycle") {
t.Errorf("validation message = %q, want it to mention lifecycle", validationErr.Message)
}
}

func TestBuildTestsResultsBQGenerator_RejectsLifecycleFilterAlongsideOtherFilters(t *testing.T) {
spec := &TestResultsSpec{
Release: "4.16",
Filter: &filter.Filter{Items: []filter.FilterItem{
{Field: "name", Operator: filter.OperatorContains, Value: "network"},
{Field: "lifecycle", Operator: filter.OperatorArithmeticNotEquals, Value: "informing"},
}},
}

_, errs := spec.buildTestsResultsBQGenerator(context.Background(), nil)
if len(errs) != 1 {
t.Fatalf("errs = %v, want exactly one error", errs)
}
var validationErr *ValidationError
if !errors.As(errs[0], &validationErr) {
t.Fatalf("errs[0] = %v (%T), want *ValidationError", errs[0], errs[0])
}
}
63 changes: 62 additions & 1 deletion pkg/db/query/cumulative_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,57 @@ func variantFilterConditions(variantFilter *filter.Filter) (conditions []string,
return conditions, args
}

// lifecycleWhereClause returns a single ready-to-AND-in SQL fragment (e.g. "e.lifecycle = ?",
// or for multiple items a parenthesized "(e.lifecycle = ? OR e.lifecycle <> ?)") applying
// lifecycleFilter against columnRef (a qualified reference to the "lifecycle" column on
// test_cumulative_summaries, e.g. "e.lifecycle" or "tcs.lifecycle"). A qualifier is required
// because test_cumulative_summaries is self-joined (aliased e/m/s) in the uncollapsed path,
// where an unqualified "lifecycle" would be ambiguous. Unlike variant filters, this clause
// must be applied before aggregation: lifecycle is not part of the GROUP BY in
// TestReportQueryCollapsed/UncollapsedTestReportWithStats, so it is summed away once rows
// reach the outer query. Returns an empty string when there's nothing to filter.
//
// Multiple items are joined per lifecycleFilter.LinkOperator (OR when explicitly "or", AND
// otherwise): naively ANDing regardless of LinkOperator would make an OR-linked filter like
// "blocking OR informing" into an unsatisfiable "lifecycle = 'blocking' AND lifecycle =
// 'informing'", since a row has exactly one lifecycle value, silently returning zero rows.
//
// Only equals/not-equals are supported, matching the fixed dropdown of known values
// (blocking/informing) the frontend restricts this filter to. Any other operator is
// rejected rather than silently ignored, since a filter that appears to apply but
// doesn't would return unfiltered results without any indication of the problem.
func lifecycleWhereClause(lifecycleFilter *filter.Filter, columnRef string) (clause string, args []any, err error) {
if lifecycleFilter == nil || len(lifecycleFilter.Items) == 0 {
return "", nil, nil
}
var conditions []string
for _, item := range lifecycleFilter.Items {
var op string
switch item.Operator {
case filter.OperatorEquals, filter.OperatorArithmeticEquals:
op = "="
case filter.OperatorArithmeticNotEquals:
op = "<>"
default:
return "", nil, fmt.Errorf("%w: lifecycle filter only supports equals/not-equals, got %q", filter.ErrUnsupportedOperator, item.Operator)
}
cond := fmt.Sprintf("%s %s ?", columnRef, op)
if item.Not {
cond = negateConditions([]string{cond})[0]
}
conditions = append(conditions, cond)
args = append(args, item.Value)
}
if len(conditions) == 1 {
return conditions[0], args, nil
}
joiner := " AND "
if lifecycleFilter.LinkOperator == filter.LinkOperatorOr {
joiner = " OR "
}
return "(" + strings.Join(conditions, joiner) + ")", args, nil
}

// pushdownSafeFields lists columns available in the filtered CTE (before the
// stats join). Only these fields can be pushed into post_filtered; stats-derived
// fields like working_average and delta_from_* exist only after the final SELECT
Expand Down Expand Up @@ -346,13 +397,17 @@ func testReportPreAgg(dbc *db.DB, release string, sample, base DateRange, nameMa
// It aggregates prefix sums per date partition separately (~16K groups each), then joins
// the three small results to compute period counts. This avoids the expensive 3-way
// self-join on all ~1.8M per-prow_job rows that the uncollapsed path requires.
func TestReportQueryCollapsed(dbc *db.DB, release string, sample, base DateRange, variantFilter, nameFilter *filter.Filter) (*gorm.DB, error) {
func TestReportQueryCollapsed(dbc *db.DB, release string, sample, base DateRange, variantFilter, nameFilter, lifecycleFilter *filter.Filter) (*gorm.DB, error) {
end, boundary, start, err := resolvePrefixSumDates(dbc, release, &sample, &base)
if err != nil {
return nil, err
}
nameConds, nameArgs := nameFilterConditions(nameFilter)
variantConds, variantArgs := variantFilterConditions(variantFilter)
lifecycleClause, lifecycleArgs, err := lifecycleWhereClause(lifecycleFilter, "tcs.lifecycle")
if err != nil {
return nil, err
}

var nameJoinClause string
var nameJoinArgs []any
Expand Down Expand Up @@ -392,6 +447,12 @@ func TestReportQueryCollapsed(dbc *db.DB, release string, sample, base DateRange
}
args = append(args, variantArgs...)

if lifecycleClause != "" {
buf.WriteString("\n AND ")
buf.WriteString(lifecycleClause)
}
args = append(args, lifecycleArgs...)

fmt.Fprintf(&buf, "\n GROUP BY tcs.test_id, tcs.suite_id, tcs.release\n) AS %s", alias)
}

Expand Down
142 changes: 142 additions & 0 deletions pkg/db/query/cumulative_query_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package query

import (
"errors"
"slices"
"testing"
"time"

Expand Down Expand Up @@ -184,6 +186,146 @@ func TestNameFilterConditions(t *testing.T) {
}
}

func TestLifecycleWhereClause(t *testing.T) {
tests := []struct {
name string
filter *filter.Filter
wantClause string
wantArgs []any
wantErr bool
}{
{
name: "nil filter",
filter: nil,
wantClause: "",
},
{
name: "empty items",
filter: &filter.Filter{Items: []filter.FilterItem{}},
wantClause: "",
},
{
name: "equals",
filter: &filter.Filter{Items: []filter.FilterItem{
{Field: "lifecycle", Operator: filter.OperatorEquals, Value: "blocking"},
}},
wantClause: "e.lifecycle = ?",
wantArgs: []any{"blocking"},
},
{
name: "arithmetic equals",
filter: &filter.Filter{Items: []filter.FilterItem{
{Field: "lifecycle", Operator: filter.OperatorArithmeticEquals, Value: "informing"},
}},
wantClause: "e.lifecycle = ?",
wantArgs: []any{"informing"},
},
{
name: "negative equals",
filter: &filter.Filter{Items: []filter.FilterItem{
{Field: "lifecycle", Operator: filter.OperatorEquals, Value: "informing", Not: true},
}},
wantClause: "NOT(e.lifecycle = ?)",
wantArgs: []any{"informing"},
},
{
name: "not equals",
filter: &filter.Filter{Items: []filter.FilterItem{
{Field: "lifecycle", Operator: filter.OperatorArithmeticNotEquals, Value: "blocking"},
}},
wantClause: "e.lifecycle <> ?",
wantArgs: []any{"blocking"},
},
{
name: "negated not equals",
filter: &filter.Filter{Items: []filter.FilterItem{
{Field: "lifecycle", Operator: filter.OperatorArithmeticNotEquals, Value: "informing", Not: true},
}},
wantClause: "NOT(e.lifecycle <> ?)",
wantArgs: []any{"informing"},
},
{
name: "multiple items default to AND",
filter: &filter.Filter{Items: []filter.FilterItem{
{Field: "lifecycle", Operator: filter.OperatorEquals, Value: "blocking"},
{Field: "lifecycle", Operator: filter.OperatorEquals, Value: "informing"},
}},
wantClause: "(e.lifecycle = ? AND e.lifecycle = ?)",
wantArgs: []any{"blocking", "informing"},
},
{
name: "multiple items with explicit AND",
filter: &filter.Filter{
LinkOperator: filter.LinkOperatorAnd,
Items: []filter.FilterItem{
{Field: "lifecycle", Operator: filter.OperatorEquals, Value: "blocking"},
{Field: "lifecycle", Operator: filter.OperatorArithmeticNotEquals, Value: "informing"},
},
},
wantClause: "(e.lifecycle = ? AND e.lifecycle <> ?)",
wantArgs: []any{"blocking", "informing"},
},
{
// This is the case the OR-linked bug affected: selecting both lifecycles via
// two "equals" items must produce a union, not an unsatisfiable AND.
name: "multiple items with OR",
filter: &filter.Filter{
LinkOperator: filter.LinkOperatorOr,
Items: []filter.FilterItem{
{Field: "lifecycle", Operator: filter.OperatorEquals, Value: "blocking"},
{Field: "lifecycle", Operator: filter.OperatorEquals, Value: "informing"},
},
},
wantClause: "(e.lifecycle = ? OR e.lifecycle = ?)",
wantArgs: []any{"blocking", "informing"},
},
{
name: "starts with is rejected",
filter: &filter.Filter{Items: []filter.FilterItem{
{Field: "lifecycle", Operator: filter.OperatorStartsWith, Value: "block"},
}},
wantErr: true,
},
{
name: "contains is rejected",
filter: &filter.Filter{Items: []filter.FilterItem{
{Field: "lifecycle", Operator: filter.OperatorContains, Value: "lock"},
}},
wantErr: true,
},
{
name: "ends with is rejected",
filter: &filter.Filter{Items: []filter.FilterItem{
{Field: "lifecycle", Operator: filter.OperatorEndsWith, Value: "ing"},
}},
wantErr: true,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
clause, args, err := lifecycleWhereClause(tc.filter, "e.lifecycle")
if tc.wantErr {
if err == nil {
t.Fatalf("expected an error, got nil")
}
if !errors.Is(err, filter.ErrUnsupportedOperator) {
t.Errorf("err = %v, want wrapped filter.ErrUnsupportedOperator", err)
}
return
}
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if clause != tc.wantClause {
t.Errorf("clause = %q, want %q", clause, tc.wantClause)
}
if !slices.Equal(args, tc.wantArgs) {
t.Errorf("args = %v, want %v", args, tc.wantArgs)
}
})
}
}

func TestPeriodsForReportType(t *testing.T) {
tomorrow := civil.DateOf(time.Now().UTC()).AddDays(1)

Expand Down
Loading