From 28877f1b9ba6af68a1b1dcf1f68f092d3c822c22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Wysoki=C5=84ski?= Date: Wed, 31 Jan 2024 07:14:18 +0000 Subject: [PATCH] feat: api - error handling (#54) Reviewed-on: https://gitea.dwysokinski.me/twhelp/corev3/pulls/54 --- api/openapi3.yml | 16 +- go.mod | 1 + go.sum | 2 + internal/domain/validation.go | 50 ++++-- internal/domain/version.go | 19 ++- internal/domain/version_test.go | 19 ++- internal/port/handler_http_api.go | 49 +++++- internal/port/handler_http_api_error.go | 191 ++++++++++++++++++++++ internal/port/handler_http_api_openapi.go | 6 +- internal/port/handler_http_api_version.go | 29 +++- internal/port/http_utils.go | 17 ++ 11 files changed, 353 insertions(+), 46 deletions(-) create mode 100644 internal/port/handler_http_api_error.go create mode 100644 internal/port/http_utils.go diff --git a/api/openapi3.yml b/api/openapi3.yml index b528197..2832c73 100644 --- a/api/openapi3.yml +++ b/api/openapi3.yml @@ -47,10 +47,18 @@ components: properties: slug: type: string + example: length-out-of-range message: type: string + path: + type: array + description: References field where an error occurred. + items: + type: string + x-go-type-skip-optional-pointer: true params: type: object + description: Additional data related to the error. Can be used for i18n. additionalProperties: true x-go-type-skip-optional-pointer: true Version: @@ -132,8 +140,10 @@ components: schema: type: object required: - - error + - errors additionalProperties: false properties: - error: - $ref: "#/components/schemas/Error" + errors: + type: array + items: + $ref: "#/components/schemas/Error" diff --git a/go.mod b/go.mod index 6031a32..3b62b15 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/brianvoe/gofakeit/v6 v6.28.0 github.com/cenkalti/backoff/v4 v4.2.1 github.com/elliotchance/phpserialize v1.3.3 + github.com/ettle/strcase v0.2.0 github.com/getkin/kin-openapi v0.123.0 github.com/go-chi/chi/v5 v5.0.11 github.com/go-chi/render v1.0.3 diff --git a/go.sum b/go.sum index 87a747e..0a4802a 100644 --- a/go.sum +++ b/go.sum @@ -48,6 +48,8 @@ github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkp github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto= github.com/elliotchance/phpserialize v1.3.3 h1:hV4QVmGdCiYgoBbw+ADt6fNgyZ2mYX0OgpnON1adTCM= github.com/elliotchance/phpserialize v1.3.3/go.mod h1:gt7XX9+ETUcLXbtTKEuyrqW3lcLUAeS/AnGZ2e49TZs= +github.com/ettle/strcase v0.2.0 h1:fGNiVF21fHXpX1niBgk0aROov1LagYsOwV/xqKDKR/Q= +github.com/ettle/strcase v0.2.0/go.mod h1:DajmHElDSaX76ITe3/VHVyMin4LWSJN5Z909Wp+ED1A= github.com/fatih/color v1.16.0 h1:zmkK9Ngbjj+K0yRhTVONQh1p/HknKYSlNT+vZCzyokM= github.com/fatih/color v1.16.0/go.mod h1:fL2Sau1YI5c0pdGEVCbKQbLXB6edEj1ZgiY4NijnWvE= github.com/frankban/quicktest v1.11.3/go.mod h1:wRf/ReqHper53s+kmmSZizM8NamnL3IM0I9ntUbOk+k= diff --git a/internal/domain/validation.go b/internal/domain/validation.go index 382d2fd..9b5ca42 100644 --- a/internal/domain/validation.go +++ b/internal/domain/validation.go @@ -15,14 +15,7 @@ type ValidationError struct { var _ ErrorWithParams = ValidationError{} func (e ValidationError) Error() string { - prefix := e.Model - - if e.Field != "" { - if len(prefix) > 0 { - prefix += "." - } - prefix += e.Field - } + prefix := e.Path() if prefix != "" { prefix += ": " @@ -31,6 +24,19 @@ func (e ValidationError) Error() string { return prefix + e.Err.Error() } +func (e ValidationError) Path() string { + path := e.Model + + if e.Field != "" { + if len(path) > 0 { + path += "." + } + path += e.Field + } + + return path +} + func (e ValidationError) Code() ErrorCode { var domainErr Error if errors.As(e.Err, &domainErr) { @@ -69,14 +75,7 @@ type SliceElementValidationError struct { var _ ErrorWithParams = SliceElementValidationError{} func (e SliceElementValidationError) Error() string { - prefix := e.Model - - if e.Field != "" { - if len(prefix) > 0 { - prefix += "." - } - prefix += e.Field + "[" + strconv.Itoa(e.Index) + "]" - } + prefix := e.Path() if prefix != "" { prefix += ": " @@ -85,6 +84,19 @@ func (e SliceElementValidationError) Error() string { return prefix + e.Err.Error() } +func (e SliceElementValidationError) Path() string { + path := e.Model + + if e.Field != "" { + if len(path) > 0 { + path += "." + } + path += e.Field + "[" + strconv.Itoa(e.Index) + "]" + } + + return path +} + func (e SliceElementValidationError) Code() ErrorCode { var domainErr Error if errors.As(e.Err, &domainErr) { @@ -229,6 +241,12 @@ var ErrNil error = simpleError{ slug: "nil", } +var ErrInvalidCursor error = simpleError{ + msg: "invalid cursor", + code: ErrorCodeIncorrectInput, + slug: "invalid-cursor", +} + func validateSliceLen[S ~[]E, E any](s S, min, max int) error { if l := len(s); l > max || l < min { return LenOutOfRangeError{ diff --git a/internal/domain/version.go b/internal/domain/version.go index 2cc155a..3d85c44 100644 --- a/internal/domain/version.go +++ b/internal/domain/version.go @@ -109,13 +109,19 @@ type VersionCursor struct { code NullString } +const versionCursorModelName = "VersionCursor" + func NewVersionCursor(code NullString) (VersionCursor, error) { if !code.Valid { return VersionCursor{}, nil } if err := validateVersionCode(code.Value); err != nil { - return VersionCursor{}, err + return VersionCursor{}, ValidationError{ + Model: versionCursorModelName, + Field: "code", + Err: err, + } } return VersionCursor{ @@ -125,7 +131,7 @@ func NewVersionCursor(code NullString) (VersionCursor, error) { const ( encodedVersionCursorMinLength = 1 - encodedVersionCursorMaxLength = 5000 + encodedVersionCursorMaxLength = 1000 ) func decodeVersionCursor(encoded string) (VersionCursor, error) { @@ -135,13 +141,18 @@ func decodeVersionCursor(encoded string) (VersionCursor, error) { decoded, err := base64.StdEncoding.DecodeString(encoded) if err != nil { - return VersionCursor{}, err + return VersionCursor{}, ErrInvalidCursor } - return NewVersionCursor(NullString{ + vc, err := NewVersionCursor(NullString{ Value: string(decoded), Valid: true, }) + if err != nil { + return VersionCursor{}, ErrInvalidCursor + } + + return vc, nil } func (vc VersionCursor) Code() NullString { diff --git a/internal/domain/version_test.go b/internal/domain/version_test.go index 9d6d496..3f89120 100644 --- a/internal/domain/version_test.go +++ b/internal/domain/version_test.go @@ -1,7 +1,6 @@ package domain_test import ( - "encoding/base64" "fmt" "testing" @@ -53,7 +52,11 @@ func TestNewVersionCursor(t *testing.T) { Valid: true, }, }, - expectedErr: versionCodeTest.expectedErr, + expectedErr: domain.ValidationError{ + Model: "VersionCursor", + Field: "code", + Err: versionCodeTest.expectedErr, + }, }) } @@ -182,23 +185,23 @@ func TestListVersionsParams_SetEncodedCursor(t *testing.T) { Field: "cursor", Err: domain.LenOutOfRangeError{ Min: 1, - Max: 5000, + Max: 1000, Current: 0, }, }, }, { - name: "ERR: len(cursor) > 5000", + name: "ERR: len(cursor) > 1000", args: args{ - cursor: gofakeit.LetterN(5001), + cursor: gofakeit.LetterN(1001), }, expectedErr: domain.ValidationError{ Model: "ListVersionsParams", Field: "cursor", Err: domain.LenOutOfRangeError{ Min: 1, - Max: 5000, - Current: 5001, + Max: 1000, + Current: 1001, }, }, }, @@ -210,7 +213,7 @@ func TestListVersionsParams_SetEncodedCursor(t *testing.T) { expectedErr: domain.ValidationError{ Model: "ListVersionsParams", Field: "cursor", - Err: base64.CorruptInputError(4), + Err: domain.ErrInvalidCursor, }, }, } diff --git a/internal/port/handler_http_api.go b/internal/port/handler_http_api.go index e9eb4d0..8e76728 100644 --- a/internal/port/handler_http_api.go +++ b/internal/port/handler_http_api.go @@ -9,12 +9,12 @@ import ( "gitea.dwysokinski.me/twhelp/corev3/internal/port/internal/swgui" "github.com/getkin/kin-openapi/openapi3" "github.com/go-chi/chi/v5" - "github.com/go-chi/render" + "github.com/go-chi/chi/v5/middleware" ) type apiHTTPHandler struct { - versionSvc *app.VersionService - getOpenAPISchema func() (*openapi3.T, error) + versionSvc *app.VersionService + openAPISchema func() (*openapi3.T, error) } func WithOpenAPIConfig(oapiCfg OpenAPIConfig) APIHTTPHandlerOption { @@ -28,7 +28,7 @@ func NewAPIHTTPHandler(versionSvc *app.VersionService, opts ...APIHTTPHandlerOpt h := &apiHTTPHandler{ versionSvc: versionSvc, - getOpenAPISchema: sync.OnceValues(func() (*openapi3.T, error) { + openAPISchema: sync.OnceValues(func() (*openapi3.T, error) { return getOpenAPISchema(cfg.openAPI) }), } @@ -38,6 +38,9 @@ func NewAPIHTTPHandler(versionSvc *app.VersionService, opts ...APIHTTPHandlerOpt if cfg.openAPI.Enabled { r.Group(func(r chi.Router) { if cfg.openAPI.SwaggerEnabled { + r.HandleFunc("/v2/swagger", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, r.RequestURI+"/", http.StatusMovedPermanently) + }) r.Handle("/v2/swagger/*", swgui.Handler( cfg.openAPI.BasePath+"/v2/swagger", cfg.openAPI.BasePath+"/v2/openapi3.json", @@ -47,10 +50,40 @@ func NewAPIHTTPHandler(versionSvc *app.VersionService, opts ...APIHTTPHandlerOpt }) } - return apimodel.HandlerWithOptions(h, apimodel.ChiServerOptions{BaseRouter: r}) + // these handlers must be set after the swagger/openapi routes + // because we don't want to override the default handlers for these routes + r.NotFound(h.handleNotFound) + r.MethodNotAllowed(h.handleMethodNotAllowed) + + return apimodel.HandlerWithOptions(h, apimodel.ChiServerOptions{ + BaseRouter: r, + Middlewares: []apimodel.MiddlewareFunc{middleware.NoCache}, + ErrorHandlerFunc: func(w http.ResponseWriter, r *http.Request, err error) { + apiErrorRenderer{errors: []error{err}}.render(w, r) + }, + }) } -func (h *apiHTTPHandler) renderJSON(w http.ResponseWriter, r *http.Request, status int, body any) { - render.Status(r, status) - render.JSON(w, r, body) +func (h *apiHTTPHandler) handleNotFound(w http.ResponseWriter, r *http.Request) { + apiErrorRenderer{ + errors: []error{ + apiError{ + status: http.StatusNotFound, + slug: "route-not-found", + message: "route not found", + }, + }, + }.render(w, r) +} + +func (h *apiHTTPHandler) handleMethodNotAllowed(w http.ResponseWriter, r *http.Request) { + apiErrorRenderer{ + errors: []error{ + apiError{ + status: http.StatusMethodNotAllowed, + slug: "method-not-allowed", + message: "method not allowed", + }, + }, + }.render(w, r) } diff --git a/internal/port/handler_http_api_error.go b/internal/port/handler_http_api_error.go new file mode 100644 index 0000000..f3eea12 --- /dev/null +++ b/internal/port/handler_http_api_error.go @@ -0,0 +1,191 @@ +package port + +import ( + "errors" + "net/http" + + "gitea.dwysokinski.me/twhelp/corev3/internal/domain" + "gitea.dwysokinski.me/twhelp/corev3/internal/port/internal/apimodel" + "github.com/ettle/strcase" +) + +type apiError struct { + status int + slug string + path []string + params map[string]any + message string +} + +func (e apiError) Error() string { + return e.message +} + +func (e apiError) toResponse() apimodel.ErrorResponse { + return apimodel.ErrorResponse{ + Errors: []apimodel.Error{ + { + Message: e.message, + Params: e.params, + Path: e.path, + Slug: e.slug, + }, + }, + } +} + +type apiErrors []apiError + +func (es apiErrors) status() int { + if len(es) == 0 { + return http.StatusOK + } + + status := es[0].status + + for _, e := range es { + if e.status > status { + status = e.status + } + } + + return status +} + +func (es apiErrors) toResponse() apimodel.ErrorResponse { + resp := apimodel.ErrorResponse{ + Errors: make([]apimodel.Error, 0, len(es)), + } + + for _, e := range es { + resp.Errors = append(resp.Errors, e.toResponse().Errors...) + } + + return resp +} + +type errorPathElement struct { + model string + field string + index int // index may be <0 and this means that it is unset +} + +type apiErrorRenderer struct { + errors []error + // formatErrorPath allows to override the default path formatter + // for domain.ValidationError and domain.SliceElementValidationError. + // If formatErrorPath returns an empty slice, an internal server error is rendered. + formatErrorPath func(elems []errorPathElement) []string +} + +var errInternalServerError = apiError{ + status: http.StatusInternalServerError, + slug: "internal-server-error", + message: "internal server error", +} + +func (re apiErrorRenderer) render(w http.ResponseWriter, r *http.Request) { + errs := make(apiErrors, 0, len(re.errors)) + + for _, err := range re.errors { + var apiErr apiError + var domainErr domain.Error + var paramFormatErr *apimodel.InvalidParamFormatError + + switch { + case errors.As(err, &apiErr): + case errors.As(err, ¶mFormatErr): + apiErr = apiError{ + status: http.StatusBadRequest, + slug: "invalid-param-format", + path: []string{"$query", paramFormatErr.ParamName}, + message: paramFormatErr.Err.Error(), + } + case errors.As(err, &domainErr): + apiErr = re.domainErrorToAPIError(domainErr) + default: + apiErr = errInternalServerError + } + + errs = append(errs, apiErr) + } + + renderJSON(w, r, errs.status(), errs.toResponse()) +} + +func (re apiErrorRenderer) domainErrorToAPIError(domainErr domain.Error) apiError { + message := domainErr.Error() + var pathElems []errorPathElement + + var err error = domainErr + for err != nil { + var validationErr domain.ValidationError + var sliceElementValidationErr domain.SliceElementValidationError + + switch { + case errors.As(err, &validationErr): + pathElems = append(pathElems, errorPathElement{ + model: validationErr.Model, + field: validationErr.Field, + index: -1, + }) + err = validationErr.Unwrap() + message = err.Error() + case errors.As(err, &sliceElementValidationErr): + pathElems = append(pathElems, errorPathElement{ + model: sliceElementValidationErr.Model, + field: sliceElementValidationErr.Field, + index: sliceElementValidationErr.Index, + }) + err = sliceElementValidationErr.Unwrap() + message = err.Error() + default: + err = nil + } + } + + var path []string + + if len(pathElems) > 0 { + if re.formatErrorPath == nil { + return errInternalServerError + } + + path = re.formatErrorPath(pathElems) + + if len(path) == 0 { + return errInternalServerError + } + } + + var params map[string]any + if withParams, ok := domainErr.(domain.ErrorWithParams); ok { + params = withParams.Params() + } + + cloned := make(map[string]any, len(params)) + for k, v := range params { + cloned[strcase.ToCamel(k)] = v + } + + return apiError{ + status: errorCodeToStatusCode(domainErr.Code()), + slug: domainErr.Slug(), + path: path, + params: cloned, + message: message, + } +} + +func errorCodeToStatusCode(code domain.ErrorCode) int { + switch code { + case domain.ErrorCodeIncorrectInput: + return http.StatusBadRequest + case domain.ErrorCodeNotFound: + return http.StatusNotFound + case domain.ErrorCodeUnknown: + fallthrough + default: + return http.StatusInternalServerError + } +} diff --git a/internal/port/handler_http_api_openapi.go b/internal/port/handler_http_api_openapi.go index 270657e..dd5902f 100644 --- a/internal/port/handler_http_api_openapi.go +++ b/internal/port/handler_http_api_openapi.go @@ -9,13 +9,13 @@ import ( ) func (h *apiHTTPHandler) sendOpenAPIJSON(w http.ResponseWriter, r *http.Request) { - schema, err := h.getOpenAPISchema() + schema, err := h.openAPISchema() if err != nil { - w.WriteHeader(http.StatusInternalServerError) + renderPlainText(w, r, http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError)) return } - h.renderJSON(w, r, http.StatusOK, schema) + renderJSON(w, r, http.StatusOK, schema) } func getOpenAPISchema(cfg OpenAPIConfig) (*openapi3.T, error) { diff --git a/internal/port/handler_http_api_version.go b/internal/port/handler_http_api_version.go index 45e5896..7a29d1e 100644 --- a/internal/port/handler_http_api_version.go +++ b/internal/port/handler_http_api_version.go @@ -11,18 +11,39 @@ func (h *apiHTTPHandler) ListVersions(w http.ResponseWriter, r *http.Request, pa domainParams := domain.NewListVersionsParams() if params.Limit != nil { - _ = domainParams.SetLimit(*params.Limit) + if err := domainParams.SetLimit(*params.Limit); err != nil { + apiErrorRenderer{errors: []error{err}, formatErrorPath: formatListVersionsParamsErrorPath}.render(w, r) + return + } } if params.Cursor != nil { - _ = domainParams.SetEncodedCursor(*params.Cursor) + if err := domainParams.SetEncodedCursor(*params.Cursor); err != nil { + apiErrorRenderer{errors: []error{err}, formatErrorPath: formatListVersionsParamsErrorPath}.render(w, r) + return + } } res, err := h.versionSvc.List(r.Context(), domainParams) if err != nil { - w.WriteHeader(http.StatusInternalServerError) + apiErrorRenderer{errors: []error{err}}.render(w, r) return } - h.renderJSON(w, r, http.StatusOK, apimodel.NewListVersionsResponse(res)) + renderJSON(w, r, http.StatusOK, apimodel.NewListVersionsResponse(res)) +} + +func formatListVersionsParamsErrorPath(elems []errorPathElement) []string { + if elems[0].model != "ListVersionsParams" { + return nil + } + + switch elems[0].field { + case "cursor": + return []string{"$query", "cursor"} + case "limit": + return []string{"$query", "limit"} + default: + return nil + } } diff --git a/internal/port/http_utils.go b/internal/port/http_utils.go new file mode 100644 index 0000000..e3b251f --- /dev/null +++ b/internal/port/http_utils.go @@ -0,0 +1,17 @@ +package port + +import ( + "net/http" + + "github.com/go-chi/render" +) + +func renderJSON(w http.ResponseWriter, r *http.Request, status int, v any) { + render.Status(r, status) + render.JSON(w, r, v) +} + +func renderPlainText(w http.ResponseWriter, r *http.Request, status int, v string) { + render.Status(r, status) + render.PlainText(w, r, v) +}