From d66afedc28a60dbc3309a149d3c27f1305170f68 Mon Sep 17 00:00:00 2001 From: Matthew Staebler Date: Tue, 14 Jul 2026 22:00:21 -0400 Subject: [PATCH 1/3] TRT-2771: Return HTTP 400 for CR validation errors instead of 500 Introduce a ValidationError type recognized by IsBadRequestError so that component readiness handlers can rely on shared bad-request classification instead of hard-coding HTTP status codes. - Add ValidationError to pkg/api and extend IsBadRequestError to detect it - Wrap validation errors in test_details.go (missing test_id, missing dbGroupBy variants) and queryparamparser.go (missing releases, unknown view, bad dates, invalid int/bool params) - Switch jsonComponentReportFromBigQuery to failureResponseWithError for auto-classification (fixes false 400 on BQ server errors) - Switch jsonComponentReportTestDetailsFromBigQuery to failureResponseWithError (fixes false 500 on validation errors) - Log all errors from GetTestDetails before classifying the first Co-Authored-By: Claude Opus 4.6 --- pkg/api/api.go | 15 +++++++++++++ pkg/api/api_test.go | 10 +++++++++ pkg/api/componentreadiness/test_details.go | 10 +++++---- .../utils/queryparamparser.go | 21 +++++++++---------- pkg/sippyserver/server.go | 13 ++++++------ 5 files changed, 48 insertions(+), 21 deletions(-) diff --git a/pkg/api/api.go b/pkg/api/api.go index e23820d289..4e8ae25ee8 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -43,7 +43,22 @@ const pgUndefinedColumn = "42703" // such as unrecognized column names, invalid syntax, and type mismatches. const bqInvalidQuery = "invalidQuery" +// ValidationError represents a request validation failure (missing or invalid +// parameters). IsBadRequestError recognizes it so handlers can rely on the +// shared bad-request classification instead of hard-coding HTTP 400. +type ValidationError struct { + Message string +} + +func (e *ValidationError) Error() string { + return e.Message +} + func IsBadRequestError(err error) bool { + var validationErr *ValidationError + if errors.As(err, &validationErr) { + return true + } var pgErr *pgconn.PgError if errors.As(err, &pgErr) && pgErr.Code == pgUndefinedColumn { return true diff --git a/pkg/api/api_test.go b/pkg/api/api_test.go index f61f1edcfb..de0363d5d4 100644 --- a/pkg/api/api_test.go +++ b/pkg/api/api_test.go @@ -82,6 +82,16 @@ func TestIsBadRequestError(t *testing.T) { }), want: true, }, + { + name: "validation error", + err: &ValidationError{Message: "test_id is required"}, + want: true, + }, + { + name: "wrapped validation error", + err: fmt.Errorf("request failed: %w", &ValidationError{Message: "missing param"}), + want: true, + }, } for _, tt := range tests { diff --git a/pkg/api/componentreadiness/test_details.go b/pkg/api/componentreadiness/test_details.go index 0d69c588e0..433fb4a50d 100644 --- a/pkg/api/componentreadiness/test_details.go +++ b/pkg/api/componentreadiness/test_details.go @@ -186,14 +186,16 @@ func (c *ComponentReportGenerator) GenerateDetailsReportForTest( ) (testdetails.Report, []error) { if testIDOption.TestID == "" { - return testdetails.Report{}, []error{fmt.Errorf("test_id has to be defined for test details")} + return testdetails.Report{}, []error{&api.ValidationError{ + Message: "test_id has to be defined for test details", + }} } for _, v := range sets.List(c.ReqOptions.VariantOption.DBGroupBy) { if _, ok := testIDOption.RequestedVariants[v]; !ok { - return testdetails.Report{}, []error{ - fmt.Errorf("all dbGroupBy variants have to be defined for test details: %s is missing in %v", + return testdetails.Report{}, []error{&api.ValidationError{ + Message: fmt.Sprintf("all dbGroupBy variants have to be defined for test details: %s is missing in %v", v, testIDOption.RequestedVariants), - } + }} } } diff --git a/pkg/api/componentreadiness/utils/queryparamparser.go b/pkg/api/componentreadiness/utils/queryparamparser.go index 6a7e17bb2e..25c29cb9e4 100644 --- a/pkg/api/componentreadiness/utils/queryparamparser.go +++ b/pkg/api/componentreadiness/utils/queryparamparser.go @@ -1,7 +1,6 @@ package utils import ( - "errors" "fmt" "net/http" "strconv" @@ -124,12 +123,12 @@ func parseRequiredReleases(req *http.Request, view *crview.View, opts *reqopts.R if baseRelease := param.SafeRead(req, "baseRelease"); baseRelease != "" { opts.BaseRelease.Name = baseRelease } else if view == nil { - return fmt.Errorf("missing baseRelease") + return &api.ValidationError{Message: "missing baseRelease"} } if sampleRelease := param.SafeRead(req, "sampleRelease"); sampleRelease != "" { opts.SampleRelease.Name = sampleRelease } else if view == nil { - return fmt.Errorf("missing sampleRelease") + return &api.ValidationError{Message: "missing sampleRelease"} } return nil } @@ -243,7 +242,7 @@ func getRequestedView(req *http.Request, views []crview.View) (*crview.View, err return &view, nil } } - return nil, fmt.Errorf("unknown view: %s", viewRequested) + return nil, &api.ValidationError{Message: fmt.Sprintf("unknown view: %s", viewRequested)} } // Translate relative start/end times to actual time.Time: @@ -258,11 +257,11 @@ func GetViewReleaseOptions( opts := reqopts.Release{Name: viewRelease.Name} opts.Start, err = util.ParseCRReleaseTime(releases, opts.Name, viewRelease.RelativeStart, true, nil, roundingFactor, roundingOffset) if err != nil { - return opts, fmt.Errorf("%s start time %q in wrong format: %v", releaseType, viewRelease.RelativeStart, err) + return opts, &api.ValidationError{Message: fmt.Sprintf("%s start time %q in wrong format: %v", releaseType, viewRelease.RelativeStart, err)} } opts.End, err = util.ParseCRReleaseTime(releases, opts.Name, viewRelease.RelativeEnd, false, nil, roundingFactor, roundingOffset) if err != nil { - return opts, fmt.Errorf("%s start time %q in wrong format: %v", releaseType, viewRelease.RelativeEnd, err) + return opts, &api.ValidationError{Message: fmt.Sprintf("%s start time %q in wrong format: %v", releaseType, viewRelease.RelativeEnd, err)} } return opts, nil } @@ -340,10 +339,10 @@ func ParseIntArg(req *http.Request, name string, defaultVal int, validator func( } val, err := strconv.Atoi(valueStr) if err != nil { - return val, errors.New(name + " is not an integer") + return val, &api.ValidationError{Message: name + " is not an integer"} } if !validator(val) { - return val, errors.New("confidence is not in the correct range") + return val, &api.ValidationError{Message: name + " is not in the correct range"} } return val, nil } @@ -355,7 +354,7 @@ func ParseBoolArg(req *http.Request, name string, defaultVal bool) (bool, error) } val, err := strconv.ParseBool(valueStr) if err != nil { - return val, errors.New(name + " is not a boolean") + return val, &api.ValidationError{Message: name + " is not a boolean"} } return val, nil } @@ -428,13 +427,13 @@ func parseDateRange(allReleases []v1.Release, req *http.Request, timeStr := req.URL.Query().Get(startName) releaseOpts.Start, err = util.ParseCRReleaseTime(allReleases, releaseOpts.Name, timeStr, true, nil, roundingFactor, roundingOffset) if err != nil { - return releaseOpts, errors.New(startName + " in wrong format") + return releaseOpts, &api.ValidationError{Message: startName + " in wrong format"} } timeStr = req.URL.Query().Get(endName) releaseOpts.End, err = util.ParseCRReleaseTime(allReleases, releaseOpts.Name, timeStr, false, nil, roundingFactor, roundingOffset) if err != nil { - return releaseOpts, errors.New(endName + " in wrong format") + return releaseOpts, &api.ValidationError{Message: endName + " in wrong format"} } return releaseOpts, nil } diff --git a/pkg/sippyserver/server.go b/pkg/sippyserver/server.go index a25e7b2dac..fdbfe8af2e 100644 --- a/pkg/sippyserver/server.go +++ b/pkg/sippyserver/server.go @@ -1091,7 +1091,9 @@ func (s *Server) getRegressedTestsForRegressions(req *http.Request, regressions // getComponentReportFromRequest creates a component report based on the HTTP request parameters func (s *Server) getComponentReportFromRequest(req *http.Request) (componentreport.ComponentReport, error) { if s.crDataProvider == nil { - return componentreport.ComponentReport{}, fmt.Errorf("component report API is only available when a data provider is configured") + return componentreport.ComponentReport{}, &api.ValidationError{ + Message: "component report API is only available when a data provider is configured", + } } allJobVariants, errs := componentreadiness.GetJobVariants(req.Context(), s.crDataProvider) @@ -1133,7 +1135,7 @@ func (s *Server) getComponentReportFromRequest(req *http.Request) (componentrepo func (s *Server) jsonComponentReportFromBigQuery(w http.ResponseWriter, req *http.Request) { outputs, err := s.getComponentReportFromRequest(req) if err != nil { - failureResponse(w, http.StatusBadRequest, err.Error()) + failureResponseWithError(w, "error generating component report", err) return } @@ -1167,11 +1169,10 @@ func (s *Server) jsonComponentReportTestDetailsFromBigQuery(w http.ResponseWrite baseURL := api.GetBaseFrontendURL(req) outputs, errs := componentreadiness.GetTestDetails(req.Context(), s.crDataProvider, s.db, reqOptions, allReleases, baseURL) if len(errs) > 0 { - log.Warningf("%d errors were encountered while querying component test details:", len(errs)) - for _, err := range errs { - log.Error(err.Error()) + for _, e := range errs[1:] { + log.WithError(e).Error("additional error querying component test details") } - failureResponse(w, http.StatusInternalServerError, fmt.Sprintf("error querying component test details: %v", errs)) + failureResponseWithError(w, "error querying component test details", errs[0]) return } api.RespondWithJSON(http.StatusOK, w, outputs) From 746291990428ff5a9d36959f06c7452c7d6b5c82 Mon Sep 17 00:00:00 2001 From: Matthew Staebler Date: Wed, 15 Jul 2026 20:40:35 -0400 Subject: [PATCH 2/3] TRT-2771: Fix copy-paste typo in end time error message Co-Authored-By: Claude Opus 4.6 --- pkg/api/componentreadiness/utils/queryparamparser.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/api/componentreadiness/utils/queryparamparser.go b/pkg/api/componentreadiness/utils/queryparamparser.go index 25c29cb9e4..3e088d6ca9 100644 --- a/pkg/api/componentreadiness/utils/queryparamparser.go +++ b/pkg/api/componentreadiness/utils/queryparamparser.go @@ -261,7 +261,7 @@ func GetViewReleaseOptions( } opts.End, err = util.ParseCRReleaseTime(releases, opts.Name, viewRelease.RelativeEnd, false, nil, roundingFactor, roundingOffset) if err != nil { - return opts, &api.ValidationError{Message: fmt.Sprintf("%s start time %q in wrong format: %v", releaseType, viewRelease.RelativeEnd, err)} + return opts, &api.ValidationError{Message: fmt.Sprintf("%s end time %q in wrong format: %v", releaseType, viewRelease.RelativeEnd, err)} } return opts, nil } From 9748dd236f68b969e463b8158fbe642863c63ee2 Mon Sep 17 00:00:00 2001 From: Matthew Staebler Date: Wed, 15 Jul 2026 20:42:07 -0400 Subject: [PATCH 3/3] TRT-2771: Use failureResponseWithError in test details handler preamble Switch the remaining hardcoded failureResponse calls in jsonComponentReportTestDetailsFromBigQuery to use failureResponseWithError for consistent error classification. Co-Authored-By: Claude Opus 4.6 --- pkg/sippyserver/server.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/sippyserver/server.go b/pkg/sippyserver/server.go index fdbfe8af2e..cdbb436f2f 100644 --- a/pkg/sippyserver/server.go +++ b/pkg/sippyserver/server.go @@ -1144,26 +1144,25 @@ func (s *Server) jsonComponentReportFromBigQuery(w http.ResponseWriter, req *htt func (s *Server) jsonComponentReportTestDetailsFromBigQuery(w http.ResponseWriter, req *http.Request) { if s.crDataProvider == nil { - err := fmt.Errorf("component report API is only available when a data provider is configured") - failureResponse(w, http.StatusBadRequest, err.Error()) + failureResponseWithError(w, "error querying component test details", + &api.ValidationError{Message: "component report API is only available when a data provider is configured"}) return } allJobVariants, errs := componentreadiness.GetJobVariants(req.Context(), s.crDataProvider) if len(errs) > 0 { - err := fmt.Errorf("failed to get job variants") - failureResponse(w, http.StatusBadRequest, err.Error()) + failureResponseWithError(w, "error querying component test details", fmt.Errorf("failed to get job variants")) return } allReleases, err := s.getReleases(req.Context()) if err != nil { - failureResponse(w, http.StatusBadRequest, err.Error()) + failureResponseWithError(w, "error querying component test details", err) return } reqOptions, _, err := utils.ParseComponentReportRequest(s.views.ComponentReadiness, allReleases, req, allJobVariants, s.crTimeRoundingFactor, s.crTimeRoundingOffset) if err != nil { - failureResponse(w, http.StatusBadRequest, err.Error()) + failureResponseWithError(w, "error querying component test details", err) return } baseURL := api.GetBaseFrontendURL(req)