jcleira/gin-go

Gin Framework Review

Review Go code using the Gin web framework for best practices in routing, middleware, request/response handling, and error management.

1. Router & Route Groups

Review router configuration for explicit control and consistent patterns.

Review Checklist

• Issue if using gin.Default() without explicit middleware control
• Issue if related routes aren't grouped with router.Group()
• Issue if paths use camelCase or snake_case (should use kebab-case)
• Issue if resource paths are singular (should be plural)
• Issue if incorrect HTTP verbs for CRUD operations

// Issue: implicit middleware, no explicit control
router := gin.Default()

// Correct: explicit middleware configuration
router := gin.New()
router.Use(gin.Recovery())
router.Use(loggingMiddleware)

// Issue: ungrouped related routes
router.GET("/users/:id", h.GetUser)
router.POST("/users", h.CreateUser)
router.GET("/users/:id/orders", h.GetUserOrders)

// Correct: grouped routes
users := router.Group("/users")
{
    users.GET("/:id", h.GetUser)
    users.POST("", h.CreateUser)
    users.GET("/:id/orders", h.GetUserOrders)
}

// Issue: wrong path naming
router.GET("/getUserById/:id", h.GetUser)     // camelCase
router.GET("/user_orders", h.GetOrders)       // snake_case
router.GET("/user/:id", h.GetUser)            // singular

// Correct: kebab-case, plural resources
router.GET("/users/:id", h.GetUser)
router.GET("/user-orders", h.GetOrders)

---

2. Handler Structure

Handlers should be thin and delegate business logic to services.

Review Checklist

• Issue if handlers contain business logic beyond HTTP concerns
• Issue if handler structs use value receivers
• Issue if errors aren't handled with early returns
• Issue if parameter extraction is inconsistent

// Issue: business logic in handler
func (h UserHandler) Create(c *gin.Context) {  // value receiver
    var req CreateRequest
    c.BindJSON(&req)

    // Business logic - should be in service
    if req.Email != "" {
        // validate email format...
    }
    user := &User{Name: req.Name}
    h.db.Create(user)

    c.JSON(200, user)
}

// Correct: thin handler, pointer receiver
func (h *UserHandler) Create(c *gin.Context) {
    var req CreateRequest
    if err := c.ShouldBindJSON(&req); err != nil {
        c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
        return
    }

    user, err := h.userService.Create(c.Request.Context(), &req)
    if err != nil {
        c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to create user"})
        return
    }

    c.JSON(http.StatusCreated, user)
}

---

3. Middleware Patterns

Middleware should follow standard patterns for cross-cutting concerns.

Review Checklist

• Issue if c.Next() is missing or misplaced in middleware
• Issue if no recovery middleware for panic handling
• Issue if logging middleware lacks request context (method, path, latency)
• Issue if auth middleware doesn't abort on failure

// Issue: missing c.Next(), won't execute subsequent handlers
func LoggingMiddleware() gin.HandlerFunc {
    return func(c *gin.Context) {
        log.Println("request received")
        // Missing c.Next()!
    }
}

// Correct: structured logging middleware
func LoggingMiddleware(logger *slog.Logger) gin.HandlerFunc {
    return func(c *gin.Context) {
        start := time.Now()
        path := c.Request.URL.Path

        c.Next()

        logger.Info("request",
            "method", c.Request.Method,
            "path", path,
            "status", c.Writer.Status(),
            "latency", time.Since(start),
        )
    }
}

// Issue: auth middleware doesn't abort
func AuthMiddleware() gin.HandlerFunc {
    return func(c *gin.Context) {
        token := c.GetHeader("Authorization")
        if token == "" {
            c.JSON(401, gin.H{"error": "unauthorized"})
            // Missing return! Continues to handler
        }
        c.Next()
    }
}

// Correct: abort on auth failure
func AuthMiddleware() gin.HandlerFunc {
    return func(c *gin.Context) {
        token := c.GetHeader("Authorization")
        if token == "" {
            c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "unauthorized"})
            return
        }
        // Validate token...
        c.Set("userID", userID)
        c.Next()
    }
}

---

4. Request Binding & Validation

Use proper binding methods and struct tag validation.

Review Checklist

• Issue if using BindJSON instead of ShouldBindJSON
• Issue if binding errors aren't handled explicitly
• Issue if required fields lack binding:"required" tag
• Issue if validation errors expose internal details

// Issue: BindJSON aborts on error, loses control
func (h *Handler) Create(c *gin.Context) {
    var req Request
    c.BindJSON(&req)  // Aborts with 400 on error
    // Code below may not execute
}

// Correct: ShouldBindJSON for explicit error handling
func (h *Handler) Create(c *gin.Context) {
    var req Request
    if err := c.ShouldBindJSON(&req); err != nil {
        c.JSON(http.StatusBadRequest, gin.H{
            "error": "invalid request body",
            "details": formatValidationErrors(err),
        })
        return
    }
}

// Issue: missing validation tags
type CreateUserRequest struct {
    Email string `json:"email"`
    Name  string `json:"name"`
}

// Correct: validation with struct tags
type CreateUserRequest struct {
    Email string `json:"email" binding:"required,email"`
    Name  string `json:"name" binding:"required,min=1,max=100"`
    Age   int    `json:"age" binding:"omitempty,gte=0,lte=150"`
}

---

5. Response Conventions

Maintain consistent response structure and status codes.

Review Checklist

• Issue if JSON responses lack consistent structure
• Issue if wrong HTTP status codes for operations
• Issue if c.JSON called after c.Abort* methods
• Issue if internal errors exposed to clients

// Issue: inconsistent response structures
c.JSON(200, user)                           // Direct object
c.JSON(200, gin.H{"user": user})            // Wrapped
c.JSON(200, gin.H{"data": user, "ok": true})// Different wrapper

// Correct: consistent response structure
type Response struct {
    Data  interface{} `json:"data,omitempty"`
    Error *ErrorInfo  `json:"error,omitempty"`
}

c.JSON(http.StatusOK, Response{Data: user})

// Issue: c.JSON after abort
func (h *Handler) Get(c *gin.Context) {
    if !authorized {
        c.AbortWithStatus(http.StatusUnauthorized)
        c.JSON(http.StatusUnauthorized, gin.H{"error": "unauthorized"}) // Wrong!
        return
    }
}

// Correct: use AbortWithStatusJSON
func (h *Handler) Get(c *gin.Context) {
    if !authorized {
        c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "unauthorized"})
        return
    }
}

HTTP Status Code Reference

| Operation | Success | Client Error | Not Found | |-----------|---------|--------------|-----------| | GET | 200 OK | 400 Bad Request | 404 Not Found | | POST (create) | 201 Created | 400/422 | - | | PUT/PATCH | 200 OK | 400/422 | 404 Not Found | | DELETE | 204 No Content | 400 | 404 Not Found |

---

6. Context Usage

Handle Gin context correctly to avoid memory leaks and race conditions.

Review Checklist

• Issue if *gin.Context is stored in structs or passed to goroutines
• Issue if c is passed to service layer instead of c.Request.Context()
• Issue if context values not retrieved safely with type assertion
• Issue if context cancellation not handled in long operations

// Issue: storing gin.Context
type Handler struct {
    ctx *gin.Context  // Never do this!
}

// Issue: passing gin.Context to service
func (h *Handler) Get(c *gin.Context) {
    h.service.Process(c)  // Wrong! Pass standard context
}

// Correct: pass request context
func (h *Handler) Get(c *gin.Context) {
    result, err := h.service.Process(c.Request.Context())
}

// Issue: unsafe context value retrieval
func (h *Handler) Get(c *gin.Context) {
    userID := c.Get("userID").(string)  // Panics if not set or wrong type
}

// Correct: safe retrieval with MustGet or type check
func (h *Handler) Get(c *gin.Context) {
    userID, exists := c.Get("userID")
    if !exists {
        c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "unauthorized"})
        return
    }
    uid, ok := userID.(string)
    if !ok {
        c.AbortWithStatusJSON(http.StatusInternalServerError, gin.H{"error": "internal error"})
        return
    }
}

// Issue: gin.Context in goroutine
func (h *Handler) Process(c *gin.Context) {
    go func() {
        // c may be reused by this point!
        data := c.Query("data")
    }()
}

// Correct: copy needed values before goroutine
func (h *Handler) Process(c *gin.Context) {
    data := c.Query("data")
    ctx := c.Request.Context()
    go func() {
        // Use copied values
        processAsync(ctx, data)
    }()
}

---

7. Error Handling

Centralize error handling and avoid leaking internal details.

Review Checklist

• Issue if no centralized error handling middleware
• Issue if internal error messages exposed to clients
• Issue if errors not logged with request context
• Issue if custom error types don't map to HTTP status codes

// Issue: exposing internal errors
func (h *Handler) Get(c *gin.Context) {
    user, err := h.service.GetUser(c.Request.Context(), id)
    if err != nil {
        c.JSON(500, gin.H{"error": err.Error()})  // Leaks internal details!
        return
    }
}

// Correct: generic message, log internal error
func (h *Handler) Get(c *gin.Context) {
    user, err := h.service.GetUser(c.Request.Context(), id)
    if err != nil {
        h.logger.ErrorContext(c.Request.Context(), "failed to get user",
            "error", err,
            "user_id", id,
        )
        c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to retrieve user"})
        return
    }
}

// Correct: centralized error middleware with custom errors
type AppError struct {
    Code    string
    Message string
    Status  int
    Err     error
}

func ErrorMiddleware(logger *slog.Logger) gin.HandlerFunc {
    return func(c *gin.Context) {
        c.Next()

        if len(c.Errors) > 0 {
            err := c.Errors.Last().Err
            if appErr, ok := err.(*AppError); ok {
                logger.ErrorContext(c.Request.Context(), appErr.Message,
                    "error", appErr.Err,
                    "code", appErr.Code,
                )
                c.JSON(appErr.Status, gin.H{
                    "error": gin.H{
                        "code":    appErr.Code,
                        "message": appErr.Message,
                    },
                })
                return
            }
            // Unknown error
            logger.ErrorContext(c.Request.Context(), "unhandled error", "error", err)
            c.JSON(http.StatusInternalServerError, gin.H{"error": "internal server error"})
        }
    }
}

---

Output Format

When reviewing Gin code, report findings as:

**Gin Framework Review**

Issues found:
- `handlers/user.go:15` - Using `gin.Default()`, prefer `gin.New()` with explicit middleware
- `handlers/user.go:42` - `BindJSON` should be `ShouldBindJSON` for explicit error handling
- `handlers/user.go:58` - `*gin.Context` passed to service, use `c.Request.Context()`
- `middleware/auth.go:23` - Missing `return` after `c.AbortWithStatusJSON`

Recommendations:
- Add centralized error handling middleware
- Group related routes under `/api/v1`
- Add `binding:"required"` tags to request struct fields

No issues:
- Recovery middleware configured
- Consistent JSON response structure
- Proper use of HTTP status codes

---

When to Run

This skill auto-invokes when reviewing Go files that:

• Import github.com/gin-gonic/gin
• Contain Gin handler functions (*gin.Context parameter)
• Define middleware (gin.HandlerFunc return type)
• Configure router setup (gin.New(), gin.Default(), router.Group())