Conversation
There was a problem hiding this comment.
Pull request overview
Standardizes JSON error responses across selected endpoints by introducing a shared error payload type.
Changes:
- Added a shared
handlers.Errorstruct for consistent{"error": "..."} - Updated health + product handlers to return the new error payload type
- Updated Swagger annotations for product endpoints to reference the new error model
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/health/get.go | Uses shared error payload for health DB ping failure response |
| internal/handlers/product/getId.go | Uses shared error payload + updates Swagger failure response types |
| internal/handlers/product/get.go | Uses shared error payload + updates Swagger failure response types |
| internal/handlers/error.go | Introduces the shared error payload type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // @Param q query string true "Search query (min 3 chars)" | ||
| // @Param limit query int false "Limit results (default 10, max 100)" | ||
| // @Success 200 {object} map[string][]product.Product | ||
| // @Failure 400 {object} map[string]string | ||
| // @Failure 401 {object} map[string]string | ||
| // @Failure 500 {object} map[string]string | ||
| // @Failure 400 {object} handlers.Error | ||
| // @Failure 401 {object} handlers.Error | ||
| // @Failure 500 {object} handlers.Error |
There was a problem hiding this comment.
The API doc says limit has a max of 100, but the validation rejects limit >= 100 and the error message says it must be less than 100 (i.e., max 99). Update either the docs or the validation/message so they are consistent.
| if err != nil { | ||
| log.Println(err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) | ||
| c.JSON(http.StatusInternalServerError, handlers.Error{Error: err.Error()}) |
There was a problem hiding this comment.
The 500 response body includes the raw internal error string (err.Error()), which can leak implementation/database details to clients. Consider returning a generic message (and optionally a stable error code) while keeping the detailed error only in logs.
| c.JSON(http.StatusInternalServerError, handlers.Error{Error: err.Error()}) | |
| c.JSON(http.StatusInternalServerError, handlers.Error{Error: "Internal server error"}) |
| type Error struct { | ||
| Error string `json:"error"` |
There was a problem hiding this comment.
The new response type name/field combo results in awkward usage like handlers.Error{Error: ...} and can be confused with Go’s built-in error concept. Consider renaming the struct to something like ErrorResponse (keeping the JSON tag error) and using a field name like Message to make call sites clearer.
| type Error struct { | |
| Error string `json:"error"` | |
| type ErrorResponse struct { | |
| Message string `json:"error"` |
| if err != nil { | ||
| log.Println(err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) | ||
| c.JSON(http.StatusInternalServerError, handlers.Error{Error: err.Error()}) |
There was a problem hiding this comment.
The 500 response body includes the raw internal error string (err.Error()), which can leak implementation/database details to clients. Consider returning a generic message (and optionally a stable error code) while keeping the detailed error only in logs.
| c.JSON(http.StatusInternalServerError, handlers.Error{Error: err.Error()}) | |
| c.JSON(http.StatusInternalServerError, handlers.Error{Error: "internal server error"}) |
2 participants
No description provided.