Merge pull request #728 from MikelAlejoBR/RHCLOUD-36529-reduce-noise-…

…logs

RHCLOUD-3652 | refactor: reduce noise in the logs

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")
 	}
 }

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)

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)

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

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

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