diff --git a/helpers_test.go b/helpers_test.go index dd18297e6..75884b363 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -161,7 +161,7 @@ func TestGetLimitNegativeLimit(t *testing.T) { t.Errorf(`want 0, got "%d"`, got) } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Error("ghosts infected the return") } } @@ -189,7 +189,7 @@ func TestGetLimitBadLimit(t *testing.T) { t.Errorf(`want 0, got "%d"`, got) } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Error("ghosts infected the return") } } @@ -214,7 +214,7 @@ func TestGetLimitMissingLimit(t *testing.T) { t.Errorf(`want 0, got "%d"`, got) } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Error("ghosts infected the return") } } @@ -269,7 +269,7 @@ func TestGetOffsetNegativeOffset(t *testing.T) { t.Errorf(`want 0, got "%d"`, got) } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Error("ghosts infected the return") } } @@ -297,7 +297,7 @@ func TestGetOffsetBadOffset(t *testing.T) { t.Errorf(`want 0, got "%d"`, got) } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Error("ghosts infected the return") } } @@ -322,7 +322,7 @@ func TestGetOffsetMissingOffset(t *testing.T) { t.Errorf(`want 0, got "%d"`, got) } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Error("ghosts infected the return") } } diff --git a/middleware/authorization.go b/middleware/authorization.go index 8f37738b0..d62937f81 100644 --- a/middleware/authorization.go +++ b/middleware/authorization.go @@ -44,7 +44,7 @@ func PermissionCheck(next echo.HandlerFunc) echo.HandlerFunc { } if !pskMatches(psk) { - return c.JSON(http.StatusUnauthorized, util.ErrorDoc("Unauthorized Action: Incorrect PSK", "401")) + return c.JSON(http.StatusUnauthorized, util.NewErrorDoc("Unauthorized Action: Incorrect PSK", "401")) } case c.Get(h.XRHID) != nil: @@ -64,12 +64,12 @@ func PermissionCheck(next echo.HandlerFunc) echo.HandlerFunc { method := c.Request().Method if method != http.MethodGet && method != http.MethodPost && method != http.MethodDelete { c.Response().Header().Set("Allow", "GET, POST, DELETE") - return c.JSON(http.StatusMethodNotAllowed, util.ErrorDoc("Method not allowed", "405")) + return c.JSON(http.StatusMethodNotAllowed, util.NewErrorDoc("Method not allowed", "405")) } // Secondary check for delete - we could move this to middleware if method == http.MethodDelete && !certDeleteAllowed(c) { c.Response().Header().Set("Allow", "GET, POST") - return c.JSON(http.StatusMethodNotAllowed, util.ErrorDoc("Method not allowed", "405")) + return c.JSON(http.StatusMethodNotAllowed, util.NewErrorDoc("Method not allowed", "405")) } // basically we're checking whether cn or cluster_id is set in @@ -80,7 +80,7 @@ func PermissionCheck(next echo.HandlerFunc) echo.HandlerFunc { if id.Identity.System.ClusterId != "" || id.Identity.System.CommonName != "" { return next(c) } else { - return c.JSON(http.StatusUnauthorized, util.ErrorDoc("Unauthorized Action: system authorization only supports cn/cluster_id authorization", "401")) + return c.JSON(http.StatusUnauthorized, util.NewErrorDoc("Unauthorized Action: system authorization only supports cn/cluster_id authorization", "401")) } } @@ -96,11 +96,11 @@ func PermissionCheck(next echo.HandlerFunc) echo.HandlerFunc { } if !allowed { - return c.JSON(http.StatusUnauthorized, util.ErrorDoc("Unauthorized Action: Missing RBAC permissions", "401")) + return c.JSON(http.StatusUnauthorized, util.NewErrorDoc("Unauthorized Action: Missing RBAC permissions", "401")) } default: - return c.JSON(http.StatusUnauthorized, util.ErrorDoc("Authentication required by either [x-rh-identity] or [x-rh-sources-psk]", "401")) + return c.JSON(http.StatusUnauthorized, util.NewErrorDoc("Authentication required by either [x-rh-identity] or [x-rh-sources-psk]", "401")) } return next(c) diff --git a/middleware/error_handling.go b/middleware/error_handling.go index 79e367246..14bb5a73d 100644 --- a/middleware/error_handling.go +++ b/middleware/error_handling.go @@ -19,10 +19,10 @@ func HandleErrors(next echo.HandlerFunc) echo.HandlerFunc { switch err.(type) { case util.ErrNotFound: statusCode = http.StatusNotFound - message = util.ErrorDocWithoutLogging(err.Error(), "404") + message = util.NewErrorDoc(err.Error(), "404") case util.ErrBadRequest: statusCode = http.StatusBadRequest - message = util.ErrorDocWithoutLogging(err.Error(), "400") + message = util.NewErrorDoc(err.Error(), "400") default: c.Logger().Error(err) uuid, ok := c.Get(h.InsightsRequestID).(string) diff --git a/middleware/tenancy.go b/middleware/tenancy.go index 32f147d7f..41991a0dd 100644 --- a/middleware/tenancy.go +++ b/middleware/tenancy.go @@ -27,7 +27,7 @@ func Tenancy(next echo.HandlerFunc) echo.HandlerFunc { // EbsAccount numbers, and might not work otherwise. if id.Identity.AccountNumber == "" { if id.Identity.OrgID == "" { - return c.JSON(http.StatusUnauthorized, util.ErrorDoc("the ebs account number and the org id are missing", "401")) + return c.JSON(http.StatusUnauthorized, util.NewErrorDoc("the ebs account number and the org id are missing", "401")) } else { c.Logger().Warnf(`[org_id: %s] potential anemic tenant found`, id.Identity.OrgID) } diff --git a/util/echo/binder_test.go b/util/echo/binder_test.go index 194cb07fc..fd185ed66 100644 --- a/util/echo/binder_test.go +++ b/util/echo/binder_test.go @@ -54,7 +54,7 @@ func TestNilBody(t *testing.T) { err := c.Bind(&TestStruct{}) // Check that returned err is Bad request with "no body" message - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Errorf("Expected Bad request err, got %s", err) } else if !strings.Contains(err.Error(), "no body") { t.Errorf("Expected that err message contains 'no body' but got '%s'", err) @@ -72,7 +72,7 @@ func TestNoBody(t *testing.T) { err := c.Bind(&TestStruct{}) // Check that returned err is Bad request with "no body" message - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Errorf("Expected Bad request err, got %s", err) } else if !strings.Contains(err.Error(), "no body") { t.Errorf("Expected that err message contains 'no body' but got '%s'", err) @@ -103,7 +103,7 @@ func TestEmptyJsonBody(t *testing.T) { t.Error("No error was found when there should have been a no body error") } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Errorf(`bad request error expected when passing it an empty JSON body, got "%s"`, reflect.TypeOf(err)) } } diff --git a/util/error_document.go b/util/error_document.go index 05cba9928..ae444e2a4 100644 --- a/util/error_document.go +++ b/util/error_document.go @@ -3,8 +3,6 @@ package util import ( "fmt" "reflect" - - l "github.com/RedHatInsights/sources-api-go/logger" ) var ErrNotFoundEmpty = NewErrNotFound("") @@ -20,23 +18,19 @@ type ErrorDocument struct { } func ErrorDocWithRequestId(message, status, uuid string) *ErrorDocument { - e := ErrorDocWithoutLogging(message, status) + e := NewErrorDoc(message, status) e.Errors[0].RequestId = uuid return e } -func ErrorDoc(message, status string) *ErrorDocument { - l.Log.Error(message) - - return ErrorDocWithoutLogging(message, status) -} - -func ErrorDocWithoutLogging(message, status string) *ErrorDocument { +func NewErrorDoc(message, status string) *ErrorDocument { return &ErrorDocument{ - []Error{{ - Detail: message, - Status: status, - }}, + []Error{ + { + Detail: message, + Status: status, + }, + }, } } @@ -53,10 +47,6 @@ func (e ErrNotFound) Is(err error) bool { } func NewErrNotFound(t string) error { - if l.Log != nil { - l.Log.Error(t) - } - return ErrNotFound{Type: t} } @@ -68,10 +58,6 @@ func (e ErrBadRequest) Error() string { return fmt.Sprintf("bad request: %s", e.Message) } -func (e ErrBadRequest) Is(err error) bool { - return reflect.TypeOf(err) == reflect.TypeOf(e) -} - func NewErrBadRequest(t interface{}) error { errorMessage := "" @@ -84,9 +70,5 @@ func NewErrBadRequest(t interface{}) error { panic("bad interface type for bad request: " + reflect.ValueOf(t).String()) } - if l.Log != nil { - l.Log.Error(errorMessage) - } - return ErrBadRequest{Message: errorMessage} }