TRT-2771: Return HTTP 400 for CR validation errors instead of 500

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

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 {

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),
-			}
+			}}
 		}
 	}
 

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 end 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
 }

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
 	}
 
@@ -1142,36 +1144,34 @@ 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)
 	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)