Conversation
|
While I don't want to scope-creep this PR, |
I was thinking of doing it this way:
I can make a separate PR for this if needed. |
There was a problem hiding this comment.
Pull request overview
Adds additional HTTP error types to the errors package and updates the error-handling middleware to translate these typed errors into the corresponding HTTP status codes.
Changes:
- Added new error types for common HTTP status codes (405, 408, 409, 410, 415, 422, 429, 500, 501, 502, 504).
- Refactored
ErrorHandler.Wrapto use a single status/message resolution path via aswitchwitherrors.As.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
middlewares/error_handler.go |
Extends error-to-status mapping and consolidates rendering logic for all recognized service error types. |
errors/errors.go |
Introduces BaseError and defines additional HTTP error structs (via embedding) to expand supported error codes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| status := http.StatusInternalServerError | ||
| message := err.Error() | ||
|
|
||
| if errors.As(err, &unauthorizedError) { | ||
| render.Status(r, http.StatusUnauthorized) | ||
| response := types.ErrorResponse{ | ||
| Status: http.StatusText(http.StatusUnauthorized), | ||
| Error: err.Error(), | ||
| } | ||
| render.JSON(w, r, response) | ||
| return | ||
| switch { | ||
| case errors.As(err, ¬FoundError) || errors.Is(err, storage.ErrNotFound): | ||
| status = http.StatusNotFound | ||
| case errors.As(err, &badRequestError): | ||
| status = http.StatusBadRequest | ||
| case errors.As(err, &serviceUnavailable): | ||
| status = http.StatusServiceUnavailable | ||
| case errors.As(err, &forbiddenError): | ||
| status = http.StatusForbidden | ||
| case errors.As(err, &unauthorizedError): | ||
| status = http.StatusUnauthorized | ||
| case errors.As(err, &methodNotAllowedError): | ||
| status = http.StatusMethodNotAllowed | ||
| case errors.As(err, &conflictError): | ||
| status = http.StatusConflict | ||
| case errors.As(err, &goneError): | ||
| status = http.StatusGone | ||
| case errors.As(err, &unsupportedMediaTypeError): | ||
| status = http.StatusUnsupportedMediaType | ||
| case errors.As(err, &unprocessableEntityError): | ||
| status = http.StatusUnprocessableEntity | ||
| case errors.As(err, &tooManyRequestsError): | ||
| status = http.StatusTooManyRequests | ||
| case errors.As(err, &internalServerError): | ||
| status = http.StatusInternalServerError | ||
| case errors.As(err, &badGatewayError): | ||
| status = http.StatusBadGateway | ||
| case errors.As(err, &gatewayTimeoutError): | ||
| status = http.StatusGatewayTimeout | ||
| case errors.As(err, &requestTimeoutError): | ||
| status = http.StatusRequestTimeout | ||
| case errors.As(err, ¬ImplementedError): | ||
| status = http.StatusNotImplemented | ||
| default: | ||
| message = "encountered an unexpected server error: " + err.Error() | ||
| } |
There was a problem hiding this comment.
The error-to-HTTP-status mapping is now a long switch with many errors.As checks, and it will keep growing as new error types are added. Consider making these service error types implement a small interface (e.g., StatusCode() int) and then resolving the status with a single errors.As to that interface; this centralizes the mapping in the error types and avoids touching this middleware for every new status.
| type BadRequest struct{ BaseError } | ||
| type NotFound struct{ BaseError } | ||
| type ServiceUnavailable struct{ BaseError } | ||
| type Forbidden struct{ BaseError } | ||
| type Unauthorized struct{ BaseError } | ||
| type MethodNotAllowed struct{ BaseError } | ||
| type Conflict struct{ BaseError } | ||
| type Gone struct{ BaseError } | ||
| type UnsupportedMediaType struct{ BaseError } | ||
| type UnprocessableEntity struct{ BaseError } | ||
| type TooManyRequests struct{ BaseError } | ||
| type InternalServerError struct{ BaseError } | ||
| type BadGateway struct{ BaseError } | ||
| type GatewayTimeout struct{ BaseError } | ||
| type RequestTimeout struct{ BaseError } | ||
| type NotImplemented struct{ BaseError } |
There was a problem hiding this comment.
Changing the concrete error structs from struct { Message string } to struct{ BaseError } is a breaking public API change: callers can no longer construct these errors with composite literals like errors.BadRequest{Message: "..."} (promoted fields cannot be set in struct literals). If this module is intended to be semver-stable, consider keeping Message as a direct field on each error type (even if Error() is shared), or add exported constructor helpers (e.g. NewBadRequest(msg string)) and update all documented usage accordingly, ideally in a major version bump.
| type BadRequest struct{ BaseError } | |
| type NotFound struct{ BaseError } | |
| type ServiceUnavailable struct{ BaseError } | |
| type Forbidden struct{ BaseError } | |
| type Unauthorized struct{ BaseError } | |
| type MethodNotAllowed struct{ BaseError } | |
| type Conflict struct{ BaseError } | |
| type Gone struct{ BaseError } | |
| type UnsupportedMediaType struct{ BaseError } | |
| type UnprocessableEntity struct{ BaseError } | |
| type TooManyRequests struct{ BaseError } | |
| type InternalServerError struct{ BaseError } | |
| type BadGateway struct{ BaseError } | |
| type GatewayTimeout struct{ BaseError } | |
| type RequestTimeout struct{ BaseError } | |
| type NotImplemented struct{ BaseError } | |
| type BadRequest struct { | |
| Message string | |
| } | |
| func (e *BadRequest) Error() string { | |
| return e.Message | |
| } | |
| type NotFound struct { | |
| Message string | |
| } | |
| func (e *NotFound) Error() string { | |
| return e.Message | |
| } | |
| type ServiceUnavailable struct { | |
| Message string | |
| } | |
| func (e *ServiceUnavailable) Error() string { | |
| return e.Message | |
| } | |
| type Forbidden struct { | |
| Message string | |
| } | |
| func (e *Forbidden) Error() string { | |
| return e.Message | |
| } | |
| type Unauthorized struct { | |
| Message string | |
| } | |
| func (e *Unauthorized) Error() string { | |
| return e.Message | |
| } | |
| type MethodNotAllowed struct { | |
| Message string | |
| } | |
| func (e *MethodNotAllowed) Error() string { | |
| return e.Message | |
| } | |
| type Conflict struct { | |
| Message string | |
| } | |
| func (e *Conflict) Error() string { | |
| return e.Message | |
| } | |
| type Gone struct { | |
| Message string | |
| } | |
| func (e *Gone) Error() string { | |
| return e.Message | |
| } | |
| type UnsupportedMediaType struct { | |
| Message string | |
| } | |
| func (e *UnsupportedMediaType) Error() string { | |
| return e.Message | |
| } | |
| type UnprocessableEntity struct { | |
| Message string | |
| } | |
| func (e *UnprocessableEntity) Error() string { | |
| return e.Message | |
| } | |
| type TooManyRequests struct { | |
| Message string | |
| } | |
| func (e *TooManyRequests) Error() string { | |
| return e.Message | |
| } | |
| type InternalServerError struct { | |
| Message string | |
| } | |
| func (e *InternalServerError) Error() string { | |
| return e.Message | |
| } | |
| type BadGateway struct { | |
| Message string | |
| } | |
| func (e *BadGateway) Error() string { | |
| return e.Message | |
| } | |
| type GatewayTimeout struct { | |
| Message string | |
| } | |
| func (e *GatewayTimeout) Error() string { | |
| return e.Message | |
| } | |
| type RequestTimeout struct { | |
| Message string | |
| } | |
| func (e *RequestTimeout) Error() string { | |
| return e.Message | |
| } | |
| type NotImplemented struct { | |
| Message string | |
| } | |
| func (e *NotImplemented) Error() string { | |
| return e.Message | |
| } |
Issue : #144