HMS-1668 refactor: Structured HTTP error (nil arg)

Add helpers to create an HTTP Error from a format string and an optional internal error object. Replace nil arg checks with new helper `NilArgError(name)`. Signed-off-by: Christian Heimes <[email protected]>
podengo-project · Sep 19, 2023 · 5968fd0 · 5968fd0
1 parent 79f1845
commit 5968fd0
Show file tree

Hide file tree

Showing 19 changed files with 177 additions and 92 deletions.
diff --git a/internal/domain/model/domain.go b/internal/domain/model/domain.go
@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/google/uuid"
+	internal_errors "github.com/podengo-project/idmsvc-backend/internal/errors"
 	"gorm.io/gorm"
 )
 
@@ -73,7 +74,7 @@ func (d *Domain) BeforeCreate(tx *gorm.DB) (err error) {
 
 func (d *Domain) AfterCreate(tx *gorm.DB) (err error) {
 	if d.Type == nil {
-		return fmt.Errorf("'Type' is nil")
+		return internal_errors.NilArgError("Type")
 	}
 	switch *d.Type {
 	case DomainTypeIpa:
@@ -90,7 +91,7 @@ func (d *Domain) AfterCreate(tx *gorm.DB) (err error) {
 // TODO: use "AfterFind" hook instead?
 func (d *Domain) FillAndPreload(db *gorm.DB) (err error) {
 	if d.Type == nil {
-		return fmt.Errorf("'Type' is nil")
+		return internal_errors.NilArgError("Type")
 	}
 	switch *d.Type {
 	case DomainTypeIpa:

diff --git a/internal/domain/model/domain_test.go b/internal/domain/model/domain_test.go
@@ -35,7 +35,7 @@ func TestDomainAfterCreate(t *testing.T) {
 	var item *Domain
 
 	item = &Domain{}
-	assert.EqualError(t, item.AfterCreate(nil), "'Type' is nil")
+	assert.EqualError(t, item.AfterCreate(nil), "code=500, message='Type' cannot be nil")
 
 	item = &Domain{
 		Model: gorm.Model{

diff --git a/internal/errors/http.go b/internal/errors/http.go
@@ -0,0 +1,31 @@
+package errors
+
+import (
+	"fmt"
+	"net/http"
+
+	"github.com/labstack/echo/v4"
+)
+
+// NewHTTPErrorWithInternal creates a new HTTPError instance from a format
+// string and an error object.
+func NewHTTPErrorWithInternal(internal error, code int, format string, a ...any) error {
+	var msg string
+	if len(a) != 0 {
+		msg = fmt.Sprintf(format, a...)
+	} else {
+		msg = format
+	}
+	return &echo.HTTPError{Code: code, Message: msg, Internal: internal}
+}
+
+// NewHTTPErrorF creates a new HTTPError instance from a format string.
+func NewHTTPErrorF(code int, format string, a ...any) error {
+	return NewHTTPErrorWithInternal(nil, code, format, a...)
+}
+
+// NilArgError creates a new HTTPError instance with "'name' cannot be nil"
+// error message.
+func NilArgError(name string) error {
+	return NewHTTPErrorF(http.StatusInternalServerError, "'%s' cannot be nil", name)
+}
diff --git a/internal/errors/http_test.go b/internal/errors/http_test.go
@@ -0,0 +1,40 @@
+package errors
+
+import (
+	"errors"
+	"net/http"
+	"testing"
+
+	"github.com/labstack/echo/v4"
+	"github.com/stretchr/testify/assert"
+)
+
+func httpErrorFromErr(t *testing.T, err error) (he *echo.HTTPError) {
+	assert.Error(t, err)
+	he, ok := err.(*echo.HTTPError)
+	assert.True(t, ok)
+	return he
+}
+
+func TestNewHTTPError(t *testing.T) {
+	internal := errors.New("internal error")
+	err := NewHTTPErrorWithInternal(internal, http.StatusForbidden, "forbidden %s!", "resource")
+	he := httpErrorFromErr(t, err)
+	assert.Equal(t, he.Code, http.StatusForbidden)
+	assert.Equal(t, he.Message, "forbidden resource!")
+	assert.ErrorIs(t, he.Internal, internal)
+
+	err = NewHTTPErrorWithInternal(internal, http.StatusForbidden, "forbidden")
+	he = httpErrorFromErr(t, err)
+	assert.Equal(t, he.Code, http.StatusForbidden)
+	assert.Equal(t, he.Message, "forbidden")
+	assert.ErrorIs(t, he.Internal, internal)
+}
+
+func TestNilArgError(t *testing.T) {
+	err := NilArgError("param")
+	he := httpErrorFromErr(t, err)
+	assert.Equal(t, he.Code, http.StatusInternalServerError)
+	assert.Equal(t, he.Message, "'param' cannot be nil")
+	assert.Nil(t, he.Internal)
+}
diff --git a/internal/handler/impl/domain_handler_private.go b/internal/handler/impl/domain_handler_private.go
@@ -8,6 +8,7 @@ import (
 	"github.com/labstack/echo/v4"
 	"github.com/openlyinc/pointy"
 	"github.com/podengo-project/idmsvc-backend/internal/domain/model"
+	internal_errors "github.com/podengo-project/idmsvc-backend/internal/errors"
 	"gorm.io/gorm"
 )
 
@@ -55,7 +56,7 @@ func (a *application) isSubscriptionManagerIDAuthorizedToUpdate(
 		return fmt.Errorf("'subscriptionManagerID' is empty")
 	}
 	if servers == nil {
-		return fmt.Errorf("'servers' is nil")
+		return internal_errors.NilArgError("servers")
 	}
 	for i := range servers {
 		if servers[i].HCCUpdateServer &&
@@ -83,7 +84,7 @@ func (a *application) fillDomain(
 		return fmt.Errorf("'source' cannot be nil")
 	}
 	if source.Type == nil {
-		return fmt.Errorf("'Type' is nil")
+		return internal_errors.NilArgError("Type")
 	}
 	if source.AutoEnrollmentEnabled != nil {
 		target.AutoEnrollmentEnabled = pointy.Bool(*source.AutoEnrollmentEnabled)
@@ -105,7 +106,7 @@ func (a *application) fillDomain(
 	switch *target.Type {
 	case model.DomainTypeIpa:
 		if source.IpaDomain == nil {
-			return fmt.Errorf("'source.IpaDomain' is nil")
+			return internal_errors.NilArgError("source.IpaDomain")
 		}
 		target.IpaDomain = &model.Ipa{}
 		return a.fillDomainIpa(target.IpaDomain, source.IpaDomain)

diff --git a/internal/infrastructure/middleware/authenticator.go b/internal/infrastructure/middleware/authenticator.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	"github.com/getkin/kin-openapi/openapi3filter"
+	internal_errors "github.com/podengo-project/idmsvc-backend/internal/errors"
 	"github.com/redhatinsights/platform-go-middlewares/identity"
 )
 
@@ -20,13 +21,13 @@ func NewAuthenticator(v XRhIValidator) openapi3filter.AuthenticationFunc {
 
 func checkGuardsAuthenticate(v XRhIValidator, ctx context.Context, input *openapi3filter.AuthenticationInput) error {
 	if v == nil {
-		return fmt.Errorf("'v' is nil")
+		return internal_errors.NilArgError("v")
 	}
 	if ctx == nil {
-		return fmt.Errorf("'ctx' is nil")
+		return internal_errors.NilArgError("ctx")
 	}
 	if input == nil {
-		return fmt.Errorf("'input' is nil")
+		return internal_errors.NilArgError("input")
 	}
 	return nil
 }

diff --git a/internal/infrastructure/middleware/enforce_identity_test.go b/internal/infrastructure/middleware/enforce_identity_test.go
@@ -11,6 +11,7 @@ import (
 	echo_middleware "github.com/labstack/echo/v4/middleware"
 	"github.com/openlyinc/pointy"
 	"github.com/podengo-project/idmsvc-backend/internal/api/header"
+	internal_errors "github.com/podengo-project/idmsvc-backend/internal/errors"
 	"github.com/redhatinsights/platform-go-middlewares/identity"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -21,7 +22,7 @@ const testPath = "/test"
 func helperCreatePredicate(username string) IdentityPredicate {
 	return func(data *identity.XRHID) error {
 		if data == nil {
-			return fmt.Errorf("'data' is nil")
+			return internal_errors.NilArgError("data")
 		}
 		if data.Identity.User.Username == username {
 			return fmt.Errorf("username='%s' is not accepted", username)

diff --git a/internal/usecase/interactor/domain_interactor.go b/internal/usecase/interactor/domain_interactor.go
@@ -12,6 +12,7 @@ import (
 	"github.com/podengo-project/idmsvc-backend/internal/api/public"
 	api_public "github.com/podengo-project/idmsvc-backend/internal/api/public"
 	"github.com/podengo-project/idmsvc-backend/internal/domain/model"
+	internal_errors "github.com/podengo-project/idmsvc-backend/internal/errors"
 	"github.com/podengo-project/idmsvc-backend/internal/infrastructure/token"
 	"github.com/podengo-project/idmsvc-backend/internal/interface/interactor"
 	"github.com/redhatinsights/platform-go-middlewares/identity"
@@ -82,7 +83,7 @@ func helperDomainTypeToUint(domainType public.DomainType) uint {
 // TODO Document method
 func (i domainInteractor) Delete(xrhid *identity.XRHID, UUID uuid.UUID, params *api_public.DeleteDomainParams) (string, uuid.UUID, error) {
 	if xrhid == nil {
-		return "", uuid.Nil, fmt.Errorf("'xrhid' is nil")
+		return "", uuid.Nil, internal_errors.NilArgError("xrhid")
 	}
 	if params == nil {
 		return "", uuid.Nil, fmt.Errorf("'params' cannot be nil")
@@ -98,10 +99,10 @@ func (i domainInteractor) Delete(xrhid *identity.XRHID, UUID uuid.UUID, params *
 // zero values and an error interface filled on error.
 func (i domainInteractor) List(xrhid *identity.XRHID, params *api_public.ListDomainsParams) (orgID string, offset int, limit int, err error) {
 	if xrhid == nil {
-		return "", -1, -1, fmt.Errorf("'xrhid' is nil")
+		return "", -1, -1, internal_errors.NilArgError("xrhid")
 	}
 	if params == nil {
-		return "", -1, -1, fmt.Errorf("'params' is nil")
+		return "", -1, -1, internal_errors.NilArgError("params")
 	}
 	if params.Offset == nil {
 		offset = 0
@@ -124,10 +125,10 @@ func (i domainInteractor) List(xrhid *identity.XRHID, params *api_public.ListDom
 // an empty organizaion id and a filled error with the situation details.
 func (i domainInteractor) GetByID(xrhid *identity.XRHID, params *public.ReadDomainParams) (orgID string, err error) {
 	if xrhid == nil {
-		return "", fmt.Errorf("'xrhid' is nil")
+		return "", internal_errors.NilArgError("xrhid")
 	}
 	if params == nil {
-		return "", fmt.Errorf("'params' is nil")
+		return "", internal_errors.NilArgError("params")
 	}
 
 	return xrhid.Identity.OrgID, nil
@@ -199,7 +200,7 @@ func (i domainInteractor) UpdateAgent(xrhid *identity.XRHID, UUID uuid.UUID, par
 		return "", nil, nil, err
 	}
 	if params == nil {
-		return "", nil, nil, fmt.Errorf("'params' is nil")
+		return "", nil, nil, internal_errors.NilArgError("params")
 	}
 	orgID := xrhid.Identity.Internal.OrgID
 
@@ -232,7 +233,7 @@ func (i domainInteractor) UpdateUser(xrhid *identity.XRHID, UUID uuid.UUID, para
 		return "", nil, err
 	}
 	if params == nil {
-		return "", nil, fmt.Errorf("'params' is nil")
+		return "", nil, internal_errors.NilArgError("params")
 	}
 	orgID := xrhid.Identity.Internal.OrgID
 
@@ -250,16 +251,16 @@ func (i domainInteractor) CreateDomainToken(
 	body *public.DomainRegTokenRequest,
 ) (orgID string, domainType public.DomainType, err error) {
 	if xrhid == nil {
-		return "", "", fmt.Errorf("'xrhid' is nil")
+		return "", "", internal_errors.NilArgError("xrhid")
 	}
 	if xrhid.Identity.Type != "User" {
 		return "", "", fmt.Errorf("invalid identity type '%s'", xrhid.Identity.Type)
 	}
 	if params == nil {
-		return "", "", fmt.Errorf("'params' is nil")
+		return "", "", internal_errors.NilArgError("params")
 	}
 	if body == nil {
-		return "", "", fmt.Errorf("'body' is nil")
+		return "", "", internal_errors.NilArgError("body")
 	}
 
 	orgID = xrhid.Identity.OrgID
@@ -360,27 +361,27 @@ func (i domainInteractor) registerOrUpdateRhelIdmLocations(body *public.Domain,
 
 func (i domainInteractor) guardRegister(xrhid *identity.XRHID, params *api_public.RegisterDomainParams, body *public.Domain) (err error) {
 	if xrhid == nil {
-		return fmt.Errorf("'xrhid' is nil")
+		return internal_errors.NilArgError("xrhid")
 	}
 	if params == nil {
-		return fmt.Errorf("'params' is nil")
+		return internal_errors.NilArgError("params")
 	}
 	if body == nil {
-		return fmt.Errorf("'body' is nil")
+		return internal_errors.NilArgError("body")
 	}
 
 	return nil
 }
 
 func (i domainInteractor) guardUpdate(xrhid *identity.XRHID, UUID uuid.UUID, body *public.Domain) (err error) {
 	if xrhid == nil {
-		return fmt.Errorf("'xrhid' is nil")
+		return internal_errors.NilArgError("xrhid")
 	}
 	if UUID == uuid.Nil {
 		return fmt.Errorf("'UUID' is invalid")
 	}
 	if body == nil {
-		return fmt.Errorf("'body' is nil")
+		return internal_errors.NilArgError("body")
 	}
 
 	return nil