Refactor API handler wrapping

traffic_ops/traffic_ops_golang/api/errors.go

@@ -23,6 +23,8 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+
+	"github.com/apache/trafficcontrol/v8/lib/go-util"
 )
 
 // Errs is the concrete implementation of Errors, which is used so that we can
@@ -221,7 +223,7 @@ func NewSystemErrorf(format string, args ...any) Errors {
 // and has the appropriate response code.
 func NewUserError(err error) Errors {
 	return &Errs{
-		code:        http.StatusInternalServerError,
+		code:        http.StatusBadRequest,
 		systemError: nil,
 		userError:   err,
 	}
@@ -231,7 +233,7 @@ func NewUserError(err error) Errors {
 // having the appropriate response code and containing the given message.
 func NewUserErrorString(err string) Errors {
 	return &Errs{
-		code:        http.StatusInternalServerError,
+		code:        http.StatusBadRequest,
 		systemError: nil,
 		userError:   errors.New(err),
 	}
@@ -243,7 +245,7 @@ func NewUserErrorString(err string) Errors {
 // wrapping).
 func NewUserErrorf(format string, args ...any) Errors {
 	return &Errs{
-		code:        http.StatusInternalServerError,
+		code:        http.StatusBadRequest,
 		systemError: nil,
 		userError:   fmt.Errorf(format, args...),
 	}
@@ -258,3 +260,25 @@ func NewResourceModifiedError() Errors {
 		userError:   ResourceModifiedError,
 	}
 }
+
+// NewUserErrorFromErrorList creates a new user-facing error (400 Bad Request)
+// by concatenating the given list of errors. Uniquely, this can return nil
+// if the passed slice is empty (or nil).
+func NewUserErrorFromErrorList(errs []error) Errors {
+	err := util.JoinErrs(errs)
+	if err == nil {
+		return nil
+	}
+	return NewUserError(err)
+}
+
+// NewNotFoundError creates an Errors that contains an HTTP Not Found status
+// code and the given error message as a user-visible error (supports wrapping
+// with the '%w' format specifier verb).
+func NewNotFoundError(format string, args ...any) Errors {
+	return &Errs{
+		code:        http.StatusNotFound,
+		systemError: nil,
+		userError:   fmt.Errorf(format, args...),
+	}
+}

traffic_ops/traffic_ops_golang/api/errors_test.go

@@ -160,17 +160,32 @@ func ExampleNewSystemErrorf() {
 }
 func ExampleNewUserError() {
 	fmt.Println(NewUserError(errors.New("testquest")).String())
-	// Output: 500 Internal Server Error, SystemError='<nil>', UserError='testquest'
+	// Output: 400 Bad Request, SystemError='<nil>', UserError='testquest'
 }
 func ExampleNewUserErrorString() {
 	fmt.Println(NewUserErrorString("testquest").String())
-	// Output: 500 Internal Server Error, SystemError='<nil>', UserError='testquest'
+	// Output: 400 Bad Request, SystemError='<nil>', UserError='testquest'
 }
 func ExampleNewUserErrorf() {
 	fmt.Println(NewUserErrorf("test: %w", errors.New("quest")).String())
-	// Output: 500 Internal Server Error, SystemError='<nil>', UserError='test: quest'
+	// Output: 400 Bad Request, SystemError='<nil>', UserError='test: quest'
 }
 func ExampleNewResourceModifiedError() {
 	fmt.Println(NewResourceModifiedError().String())
 	// Output: 412 Precondition Failed, SystemError='<nil>', UserError='resource was modified since the time specified by the request headers'
 }
+func ExampleNewUserErrorFromErrorList() {
+	errs := []error{}
+	fmt.Println(NewUserErrorFromErrorList(errs))
+
+	errs = append(errs, errors.New("Test"))
+	errs = append(errs, errors.New("Quest"))
+	fmt.Println(NewUserErrorFromErrorList(errs))
+
+	// Output: <nil>
+	// Test, Quest
+}
+func ExampleNewNotFoundError() {
+	fmt.Println(NewNotFoundError("test: %s", "quest").String())
+	// Output: 404 Not Found, SystemError='<nil>', UserError='test: quest'
+}

traffic_ops/traffic_ops_golang/api/info.go

@@ -257,9 +257,9 @@ func (inf *Info) Close() {
 //
 // This CANNOT be used by any Info that wasn't constructed for the caller by
 // Wrap - ing a Handler (yet).
-func (inf Info) WriteOKResponse(resp any) (int, error, error) {
+func (inf Info) WriteOKResponse(resp any) error {
 	WriteResp(inf.w, inf.request, resp)
-	return http.StatusOK, nil, nil
+	return nil
 }
 
 // WriteOKResponseWithSummary writes a 200 OK response with the given object as
@@ -272,41 +272,41 @@ func (inf Info) WriteOKResponse(resp any) (int, error, error) {
 // Deprecated: Summary sections on responses were intended to cover up for a
 // deficiency in jQuery-based tables on the front-end, so now that we aren't
 // using those anymore it serves no purpose.
-func (inf Info) WriteOKResponseWithSummary(resp any, count uint64) (int, error, error) {
+func (inf Info) WriteOKResponseWithSummary(resp any, count uint64) error {
 	WriteRespWithSummary(inf.w, inf.request, resp, count)
-	return http.StatusOK, nil, nil
+	return nil
 }
 
 // WriteNotModifiedResponse writes a 304 Not Modified response with the given
 // time as the last modified time in the headers.
 //
 // This CANNOT be used by any Info that wasn't constructed for the caller by
 // Wrap - ing a Handler (yet).
-func (inf Info) WriteNotModifiedResponse(lastModified time.Time) (int, error, error) {
+func (inf Info) WriteNotModifiedResponse(lastModified time.Time) error {
 	inf.w.Header().Set(rfc.LastModified, FormatLastModified(lastModified))
 	inf.w.WriteHeader(http.StatusNotModified)
 	setRespWritten(inf.request)
-	return http.StatusNotModified, nil, nil
+	return nil
 }
 
 // WriteSuccessResponse writes the given response object as the `response`
 // property of the response body, with the accompanying message as a
 // success-level Alert.
-func (inf Info) WriteSuccessResponse(resp any, message string) (int, error, error) {
+func (inf Info) WriteSuccessResponse(resp any, message string) error {
 	WriteAlertsObj(inf.w, inf.request, http.StatusOK, tc.CreateAlerts(tc.SuccessLevel, message), resp)
-	return http.StatusOK, nil, nil
+	return nil
 }
 
 // WriteCreatedResponse writes the given response object as the `response`
 // property of the response body of a 201 created response, with the
 // accompanying message as a success-level Alert. It also sets the Location
 // header to the given path. This will be automatically prefaced with the
 // correct path to the API version the client requested.
-func (inf Info) WriteCreatedResponse(resp any, message, path string) (int, error, error) {
+func (inf Info) WriteCreatedResponse(resp any, message, path string) error {
 	inf.w.Header().Set(rfc.Location, strings.Join([]string{"/api", inf.Version.String(), strings.TrimPrefix(path, "/")}, "/"))
 	inf.w.WriteHeader(http.StatusCreated)
 	WriteAlertsObj(inf.w, inf.request, http.StatusCreated, tc.CreateAlerts(tc.SuccessLevel, message), resp)
-	return http.StatusCreated, nil, nil
+	return nil
 }
 
 // RequestHeaders returns the headers sent by the client in the API request.
@@ -427,3 +427,18 @@ func (inf Info) DefaultSort(param string) {
 		inf.Params["orderby"] = param
 	}
 }
+
+// HandleErrors handles errors that occur during handling API operations - as
+// represented by an appropriate Errors.
+func (inf Info) HandleErrors(errs Errors) {
+	HandleErr(inf.w, inf.request, inf.Tx.Tx, errs.Code(), errs.UserError(), errs.SystemError())
+}
+
+// HandleDBError handles errors from database actions. This is identical to
+// ParseDBError followed by handling what that returns, but does the
+// intermediary step for you.
+func (inf Info) HandleDBError(err error) error {
+	userErr, sysErr, code := ParseDBError(err)
+	HandleErr(inf.w, inf.request, inf.Tx.Tx, code, userErr, sysErr)
+	return nil
+}

traffic_ops/traffic_ops_golang/api/info_test.go

@@ -330,15 +330,9 @@ func TestInfo_WriteOKResponse(t *testing.T) {
 		request: r,
 		w:       w,
 	}
-	code, userErr, sysErr := inf.WriteOKResponse("test")
-	if code != http.StatusOK {
-		t.Errorf("WriteOKResponse should return a %d %s code, got: %d %s", http.StatusOK, http.StatusText(http.StatusOK), code, http.StatusText(code))
-	}
-	if userErr != nil {
-		t.Errorf("Unexpected user error: %v", userErr)
-	}
-	if sysErr != nil {
-		t.Errorf("Unexpected system error: %v", sysErr)
+	err := inf.WriteOKResponse("test")
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
 	}
 
 	if w.Code != http.StatusOK {
@@ -354,15 +348,9 @@ func TestInfo_WriteOKResponseWithSummary(t *testing.T) {
 		request: r,
 		w:       w,
 	}
-	code, userErr, sysErr := inf.WriteOKResponseWithSummary("test", 42)
-	if code != http.StatusOK {
-		t.Errorf("WriteOKResponseWithSummary should return a %d %s code, got: %d %s", http.StatusOK, http.StatusText(http.StatusOK), code, http.StatusText(code))
-	}
-	if userErr != nil {
-		t.Errorf("Unexpected user error: %v", userErr)
-	}
-	if sysErr != nil {
-		t.Errorf("Unexpected system error: %v", sysErr)
+	err := inf.WriteOKResponseWithSummary("test", 42)
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
 	}
 
 	if w.Code != http.StatusOK {
@@ -378,15 +366,9 @@ func TestInfo_WriteNotModifiedResponse(t *testing.T) {
 		request: r,
 		w:       w,
 	}
-	code, userErr, sysErr := inf.WriteNotModifiedResponse(time.Time{})
-	if code != http.StatusNotModified {
-		t.Errorf("WriteNotModifiedResponse should return a %d %s code, got: %d %s", http.StatusNotModified, http.StatusText(http.StatusNotModified), code, http.StatusText(code))
-	}
-	if userErr != nil {
-		t.Errorf("Unexpected user error: %v", userErr)
-	}
-	if sysErr != nil {
-		t.Errorf("Unexpected system error: %v", sysErr)
+	err := inf.WriteNotModifiedResponse(time.Time{})
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
 	}
 
 	if w.Code != http.StatusNotModified {
@@ -402,15 +384,9 @@ func TestInfo_WriteSuccessResponse(t *testing.T) {
 		request: r,
 		w:       w,
 	}
-	code, userErr, sysErr := inf.WriteSuccessResponse("test", "quest")
-	if code != http.StatusOK {
-		t.Errorf("WriteSuccessResponse should return a %d %s code, got: %d %s", http.StatusOK, http.StatusText(http.StatusOK), code, http.StatusText(code))
-	}
-	if userErr != nil {
-		t.Errorf("Unexpected user error: %v", userErr)
-	}
-	if sysErr != nil {
-		t.Errorf("Unexpected system error: %v", sysErr)
+	err := inf.WriteSuccessResponse("test", "quest")
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
 	}
 
 	if w.Code != http.StatusOK {
@@ -442,15 +418,9 @@ func TestInfo_WriteCreatedResponse(t *testing.T) {
 		Version: &Version{Major: 420, Minor: 9001},
 		w:       w,
 	}
-	code, userErr, sysErr := inf.WriteCreatedResponse("test", "quest", "mypath")
-	if code != http.StatusCreated {
-		t.Errorf("WriteCreatedResponse should return a %d %s code, got: %d %s", http.StatusCreated, http.StatusText(http.StatusCreated), code, http.StatusText(code))
-	}
-	if userErr != nil {
-		t.Errorf("Unexpected user error: %v", userErr)
-	}
-	if sysErr != nil {
-		t.Errorf("Unexpected system error: %v", sysErr)
+	err := inf.WriteCreatedResponse("test", "quest", "mypath")
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
 	}
 
 	if w.Code != http.StatusCreated {
@@ -802,3 +772,70 @@ func ExampleInfo_DefaultSort() {
 	// Output: testquest
 	// testquest
 }
+
+func TestInfo_HandleErrors(t *testing.T) {
+	w := httptest.NewRecorder()
+	r := httptest.NewRequest(http.MethodPost, "/", nil)
+
+	inf := Info{
+		request: r,
+		w:       w,
+		Tx:      &sqlx.Tx{},
+	}
+
+	code := http.StatusFailedDependency
+	inf.HandleErrors(NewErrors(code, errors.New("test"), errors.New("quest")))
+
+	if w.Code != code {
+		t.Errorf("incorrect response status code; want: %d, got: %d", code, w.Code)
+	}
+
+	var alerts tc.Alerts
+	if err := json.NewDecoder(w.Body).Decode(&alerts); err != nil {
+		t.Fatalf("couldn't decode response body: %v", err)
+	}
+
+	if len(alerts.Alerts) != 1 {
+		t.Fatalf("expected exactly one alert; got: %d", len(alerts.Alerts))
+	}
+	alert := alerts.Alerts[0]
+	if alert.Level != tc.ErrorLevel.String() {
+		t.Errorf("Incorrect alert level; want: %s, got: %s", tc.ErrorLevel, alert.Level)
+	}
+	if alert.Text != "test" {
+		t.Errorf("Incorrect alert text; want: 'test', got: '%s'", alert.Text)
+	}
+}
+
+func TestInfo_HandleDBError(t *testing.T) {
+	w := httptest.NewRecorder()
+	r := httptest.NewRequest(http.MethodPost, "/", nil)
+
+	inf := Info{
+		request: r,
+		w:       w,
+		Tx:      &sqlx.Tx{},
+	}
+
+	inf.HandleDBError(errors.New("a non-parsable error"))
+
+	if code := http.StatusInternalServerError; w.Code != code {
+		t.Errorf("incorrect response status code; want: %d, got: %d", code, w.Code)
+	}
+
+	var alerts tc.Alerts
+	if err := json.NewDecoder(w.Body).Decode(&alerts); err != nil {
+		t.Fatalf("couldn't decode response body: %v", err)
+	}
+
+	if len(alerts.Alerts) != 1 {
+		t.Fatalf("expected exactly one alert; got: %d", len(alerts.Alerts))
+	}
+	alert := alerts.Alerts[0]
+	if alert.Level != tc.ErrorLevel.String() {
+		t.Errorf("Incorrect alert level; want: %s, got: %s", tc.ErrorLevel, alert.Level)
+	}
+	if expected := http.StatusText(http.StatusInternalServerError); alert.Text != expected {
+		t.Errorf("Incorrect alert text; want: '%s', got: '%s'", expected, alert.Text)
+	}
+}

traffic_ops/traffic_ops_golang/api/shared_handlers.go

@@ -679,10 +679,13 @@ func parseMultipleCreates(data []byte, desiredType reflect.Type, inf *Info) ([]C
 }
 
 // A Handler is an API endpoint handlers. They take in Info helper objects and
-// return - in order - an HTTP response status code, a user-facing error (if one
-// occurred), and a system-only error not safe for exposure to clients (if one
-// occurred).
-type Handler = func(*Info) (int, error, error)
+// return errors that occur in handling. If no error occurs, the response MUST
+// have been written. It is strongly recommended that the returned error be an
+// Errors type as defined by this package, as that will allow for the most
+// versatile handling. Any other kind of error returned by a Handler is treated
+// as a "system-only" error, which returns no information to the user beyond
+// that the request failed.
+type Handler = func(*Info) error
 
 // Wrap wraps an API endpoint handler in the more generic HTTP request handler
 // type from the http package. This constructs and provides the Info for the
@@ -709,9 +712,16 @@ func Wrap(h Handler, requiredParams, intParams []string) http.HandlerFunc {
 		}
 		inf.w = w
 
-		errCode, userErr, sysErr = h(inf)
-		if userErr != nil || sysErr != nil {
-			HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+		err := h(inf)
+		if err == nil {
+			return
+		}
+
+		var apiErr Errors
+		if errors.As(err, &apiErr) {
+			inf.HandleErrors(apiErr)
+		} else {
+			HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
 		}
 	}
 }