From 135dd94de5a4fe902c9e43635e89d79737b0b8d6 Mon Sep 17 00:00:00 2001 From: Sanne Raymaekers Date: Tue, 25 Jun 2024 12:03:48 +0200 Subject: [PATCH] cloudapi/v2: include details in case internal error is set In case details aren't explicitly given, and the internal error is set, include the internal error message in the details. --- internal/cloudapi/v2/errors.go | 53 +++++++++++++--------------- internal/cloudapi/v2/errors_test.go | 8 ++--- internal/cloudapi/v2/handler.go | 2 +- internal/cloudapi/v2/v2_koji_test.go | 2 +- 4 files changed, 31 insertions(+), 34 deletions(-) diff --git a/internal/cloudapi/v2/errors.go b/internal/cloudapi/v2/errors.go index 565078e0a..61280ffb1 100644 --- a/internal/cloudapi/v2/errors.go +++ b/internal/cloudapi/v2/errors.go @@ -177,8 +177,12 @@ func HTTPError(code ServiceErrorCode) error { // echo.HTTPError has a message interface{} field, which can be used to include the ServiceErrorCode func HTTPErrorWithInternal(code ServiceErrorCode, internalErr error) error { - se := find(code) - he := echo.NewHTTPError(se.httpStatus, detailsError{code, ""}) + de := detailsError{code, ""} + if internalErr != nil { + de.details = internalErr.Error() + } + + he := echo.NewHTTPError(find(code).httpStatus, de) if internalErr != nil { he.Internal = internalErr } @@ -202,28 +206,26 @@ func HTTPErrorWithDetails(code ServiceErrorCode, internalErr error, details stri // Convert a ServiceErrorCode into an Error as defined in openapi.v2.yml // serviceError is optional, prevents multiple find() calls -func APIError(code ServiceErrorCode, serviceError *serviceError, c echo.Context, details *interface{}) *Error { - se := serviceError - if se == nil { - se = find(code) - } - +func APIError(serviceError *serviceError, c echo.Context, details interface{}) *Error { operationID, ok := c.Get("operationID").(string) if !ok || operationID == "" { - se = find(ErrorMalformedOperationID) + serviceError = find(ErrorMalformedOperationID) } - return &Error{ + apiErr := &Error{ ObjectReference: ObjectReference{ - Href: fmt.Sprintf("%s/%d", ErrorHREF, se.code), - Id: fmt.Sprintf("%d", se.code), + Href: fmt.Sprintf("%s/%d", ErrorHREF, serviceError.code), + Id: fmt.Sprintf("%d", serviceError.code), Kind: "Error", }, - Code: fmt.Sprintf("%s%d", ErrorCodePrefix, se.code), + Code: fmt.Sprintf("%s%d", ErrorCodePrefix, serviceError.code), OperationId: operationID, // set operation id from context - Reason: se.reason, - Details: details, + Reason: serviceError.reason, } + if details != nil { + apiErr.Details = &details + } + return apiErr } // Helper to make the ErrorList as defined in openapi.v2.yml @@ -253,7 +255,7 @@ func APIErrorList(page int, pageSize int, c echo.Context) *ErrorList { for _, e := range errs { // Implicit memory alasing doesn't couse any bug in this case /* #nosec G601 */ - list.Items = append(list.Items, *APIError(e.code, &e, c, nil)) + list.Items = append(list.Items, *APIError(&e, c, nil)) } list.Size = len(list.Items) return list @@ -274,14 +276,13 @@ func apiErrorFromEchoError(echoError *echo.HTTPError) ServiceErrorCode { // Convert an echo error into an AOC compliant one so we send a correct json error response func (s *Server) HTTPErrorHandler(echoError error, c echo.Context) { - doResponse := func(details *interface{}, code ServiceErrorCode, c echo.Context, internal error) { - // don't anticipate serviceerrorcode, instead check what type it is + doResponse := func(details interface{}, code ServiceErrorCode, c echo.Context, internal error) { if !c.Response().Committed { var err error - sec := find(code) - apiErr := APIError(code, sec, c, details) + se := find(code) + apiErr := APIError(se, c, details) - if sec.httpStatus == http.StatusInternalServerError { + if se.httpStatus == http.StatusInternalServerError { errMsg := fmt.Sprintf("Internal server error. Code: %s, OperationId: %s", apiErr.Code, apiErr.OperationId) if internal != nil { @@ -292,9 +293,9 @@ func (s *Server) HTTPErrorHandler(echoError error, c echo.Context) { } if c.Request().Method == http.MethodHead { - err = c.NoContent(sec.httpStatus) + err = c.NoContent(se.httpStatus) } else { - err = c.JSON(sec.httpStatus, apiErr) + err = c.JSON(se.httpStatus, apiErr) } if err != nil { c.Logger().Errorf("Failed to return error response: %v", err) @@ -317,9 +318,5 @@ func (s *Server) HTTPErrorHandler(echoError error, c echo.Context) { doResponse(nil, apiErrorFromEchoError(he), c, he.Internal) return } - var det *interface{} - if err.details != nil { - det = &err.details - } - doResponse(det, err.errorCode, c, he.Internal) + doResponse(err.details, err.errorCode, c, he.Internal) } diff --git a/internal/cloudapi/v2/errors_test.go b/internal/cloudapi/v2/errors_test.go index 5c1044799..5332df22f 100644 --- a/internal/cloudapi/v2/errors_test.go +++ b/internal/cloudapi/v2/errors_test.go @@ -25,7 +25,7 @@ func TestAPIError(t *testing.T) { for _, se := range getServiceErrors() { ctx := e.NewContext(nil, nil) ctx.Set("operationID", "test-operation-id") - apiError := APIError(se.code, nil, ctx, nil) + apiError := APIError(&se, ctx, nil) require.Equal(t, fmt.Sprintf("/api/image-builder-composer/v2/errors/%d", se.code), apiError.Href) require.Equal(t, fmt.Sprintf("%d", se.code), apiError.Id) require.Equal(t, "Error", apiError.Kind) @@ -38,15 +38,15 @@ func TestAPIError(t *testing.T) { func TestAPIErrorOperationID(t *testing.T) { ctx := echo.New().NewContext(nil, nil) - apiError := APIError(ErrorUnauthenticated, nil, ctx, nil) + apiError := APIError(find(ErrorUnauthenticated), ctx, nil) require.Equal(t, "IMAGE-BUILDER-COMPOSER-10003", apiError.Code) ctx.Set("operationID", 5) - apiError = APIError(ErrorUnauthenticated, nil, ctx, nil) + apiError = APIError(find(ErrorUnauthenticated), ctx, nil) require.Equal(t, "IMAGE-BUILDER-COMPOSER-10003", apiError.Code) ctx.Set("operationID", "test-operation-id") - apiError = APIError(ErrorUnauthenticated, nil, ctx, nil) + apiError = APIError(find(ErrorUnauthenticated), ctx, nil) require.Equal(t, "IMAGE-BUILDER-COMPOSER-401", apiError.Code) } diff --git a/internal/cloudapi/v2/handler.go b/internal/cloudapi/v2/handler.go index 6c23d7deb..726c94968 100644 --- a/internal/cloudapi/v2/handler.go +++ b/internal/cloudapi/v2/handler.go @@ -77,7 +77,7 @@ func (h *apiHandlers) GetError(ctx echo.Context, id string) error { return HTTPError(ErrorInvalidErrorId) } - apiError := APIError(ServiceErrorCode(errorId), nil, ctx, nil) + apiError := APIError(find(ServiceErrorCode(errorId)), ctx, nil) // If the service error wasn't found, it's a 404 in this instance if apiError.Id == fmt.Sprintf("%d", ErrorServiceErrorNotFound) { return HTTPError(ErrorErrorNotFound) diff --git a/internal/cloudapi/v2/v2_koji_test.go b/internal/cloudapi/v2/v2_koji_test.go index 463b841bb..db0612147 100644 --- a/internal/cloudapi/v2/v2_koji_test.go +++ b/internal/cloudapi/v2/v2_koji_test.go @@ -597,6 +597,6 @@ func TestKojiJobTypeValidation(t *testing.T) { } badID := uuid.New() - test.TestRoute(t, handler, false, "GET", fmt.Sprintf("/api/image-builder-composer/v2/composes/%s%s", badID, path), ``, http.StatusNotFound, `{"code":"IMAGE-BUILDER-COMPOSER-15", "details": "", "href":"/api/image-builder-composer/v2/errors/15","id":"15","kind":"Error","reason":"Compose with given id not found"}`, `operation_id`) + test.TestRoute(t, handler, false, "GET", fmt.Sprintf("/api/image-builder-composer/v2/composes/%s%s", badID, path), ``, http.StatusNotFound, `{"code":"IMAGE-BUILDER-COMPOSER-15", "details": "job does not exist", "href":"/api/image-builder-composer/v2/errors/15","id":"15","kind":"Error","reason":"Compose with given id not found"}`, `operation_id`) } }