Conversation
📝 WalkthroughWalkthroughReorganizes packages into pkg/{middleware,response,server}, renames public APIs (ginkgo → middleware/response/server; HttpServer→Http; WrapHandler→Handler), reimplements middleware (auth, CORS, permission, rate limit, logging, error handling), updates response envelope, adds example app, adjusts CI/Makefile, updates go.mod, and moves many tests from root Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gin as "Gin Engine"
participant Auth as "MiddlewareProvider.NewAuthMiddleware"
participant Rate as "MiddlewareProvider.NewRateLimitMiddleware"
participant Handler as "Application Handler"
participant ServerResp as "pkg/response"
Client->>Gin: HTTP request (Authorization header)
Gin->>Rate: pass through rate-limit middleware
Rate-->>Gin: allow / reject (429)
Gin->>Auth: extract token & call tokenCheckFunc
Auth-->>Gin: set ctx values or abort (401/500)
Gin->>Handler: invoke business handler
Handler-->>Gin: return (data, error)
Gin->>ServerResp: build JSON envelope (NewResponse/NewErrorResponse)
ServerResp-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
utils.go (1)
57-63: Consider refactoring to reduce code duplication.The BindJSON function duplicates logic from BindRequest. While the specialized API is convenient, you could eliminate duplication by implementing it as:
func BindJSON[T any](ctx *gin.Context) (T, error) { return BindRequest[T](ctx, binding.JSON) }This would maintain the same API while reusing the existing BindRequest logic.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
responses.go(1 hunks)utils.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: responses.go:31-35
Timestamp: 2025-08-27T10:05:59.429Z
Learning: The `omitzero` JSON tag was introduced in Go 1.24, allowing struct fields to be omitted during JSON marshaling if their value is zero (as determined by an IsZero() method if present, otherwise based on the zero value for the type). This is different from `omitempty` which omits empty values.
📚 Learning: 2025-08-27T10:05:59.429Z
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: responses.go:31-35
Timestamp: 2025-08-27T10:05:59.429Z
Learning: The `omitzero` JSON tag was introduced in Go 1.24, allowing struct fields to be omitted during JSON marshaling if their value is zero (as determined by an IsZero() method if present, otherwise based on the zero value for the type). This is different from `omitempty` which omits empty values.
Applied to files:
responses.go
🧬 Code graph analysis (1)
utils.go (1)
responses.go (1)
JSONResponse(30-35)
🔇 Additional comments (1)
responses.go (1)
31-31: LGTM! Consistent field marshaling behavior.The
omitzerotag aligns with the other fields in the struct and enables the Message field to be omitted when empty, which works well with the new Handler function in utils.go that creates responses without explicit messages.Based on learnings,
omitzerowas introduced in Go 1.24 and provides appropriate zero-value omission semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
Makefile (1)
7-7: Minor formatting inconsistency in help text.The spacing before the dash appears inconsistent compared to other help entries (mixing tabs and spaces).
🔎 Proposed fix for consistent spacing
- @echo " test - Run all tests" + @echo " test - Run all tests"pkg/middleware/logging.go (1)
36-61: Consider consolidating duplicate log calls.The logging logic duplicates the format string between the case with an error message and without. A minor refactor could reduce this duplication.
🔎 Proposed refactor
if statusCode >= 400 { - errorMsg := "" - if len(ctx.Errors) > 0 { - errorMsg = ctx.Errors.String() - } - - if errorMsg != "" { - mp.logger.Errorf( - "[HTTP] method=%s path=%s status=%d duration=%s client_ip=%s error=%s", - method, - fullPath, - statusCode, - elapsed, - clientIP, - errorMsg, - ) - } else { - mp.logger.Errorf( - "[HTTP] method=%s path=%s status=%d duration=%s client_ip=%s", - method, - fullPath, - statusCode, - elapsed, - clientIP, - ) - } + baseMsg := "[HTTP] method=%s path=%s status=%d duration=%s client_ip=%s" + args := []any{method, fullPath, statusCode, elapsed, clientIP} + if len(ctx.Errors) > 0 { + mp.logger.Errorf(baseMsg+" error=%s", append(args, ctx.Errors.String())...) + } else { + mp.logger.Errorf(baseMsg, args...) + } } else {example/server.go (1)
20-20: Consider adding logging middleware for observability.The example only uses the error middleware. For a complete demonstration, consider also adding
mp.NewLoggingMiddleware()to show how request logging integrates with error handling.🔎 Proposed addition
r.Use(mp.NewErrorMiddleware()) + r.Use(mp.NewLoggingMiddleware())pkg/middleware/permission.go (2)
16-20: Consider validating input parameters for consistency with other middlewares.Other middlewares in this package (CORS, Auth) validate their inputs and fail-fast on invalid configuration. This middleware doesn't validate
roleContextKey,requiredPermission, orpermissionMapduring construction, which could lead to confusing runtime behavior (e.g., emptyroleContextKeywould always fail at line 23).🔎 Proposed validation
func (mp *MiddlewareProvider) NewPermissionMiddleware( roleContextKey string, requiredPermission string, permissionMap map[string][]string, ) gin.HandlerFunc { + if roleContextKey == "" { + mp.logger.Fatal("roleContextKey cannot be empty") + } + if requiredPermission == "" { + mp.logger.Fatal("requiredPermission cannot be empty") + } + if permissionMap == nil { + mp.logger.Fatal("permissionMap cannot be nil") + } return func(ctx *gin.Context) {Based on learnings, this codebase uses fail-fast design for configuration validation.
23-27: Themp.loggerfield is unused in this middleware.Unlike other middlewares, this one doesn't use the logger for any debug or trace logging. While not critical, logging permission checks could aid debugging in production.
pkg/middleware/auth.go (3)
12-16: Docstring mentions unsupported strategies.The comment mentions
"header"or"cookie"strategies, but the implementation only supports"Bearer". Update the documentation to reflect the actual supported strategy.🔎 Suggested docstring fix
// NewAuthMiddleware creates an authentication middleware for Gin. -// It extracts a token using the given strategy (e.g., "header" or "cookie") via internal.ExtractToken, +// It extracts a token using the given strategy (e.g., "Bearer") via extractToken, // calls tokenCheckFunc to validate the token and retrieve user data, // stores user data in the Gin context, and aborts the request on errors. // Returns a Gin HandlerFunc for authentication handling.
58-66: Consider returning a single error instead of mixed (token, errMsg, error) tuple.The
extractTokenfunction returns both an error message string and an error, which creates an awkward API. The caller must check botherr != nilanderrMsg != ""separately. Consider consolidating into a single error return using typed errors (e.g.,ungerr.UnauthorizedError).🔎 Suggested refactor
-func extractToken(ctx *gin.Context, authStrategy string) (string, string, error) { +func extractToken(ctx *gin.Context, authStrategy string) (string, error) { switch authStrategy { case "Bearer": - token, errMsg := extractBearerToken(ctx) - return token, errMsg, nil + return extractBearerToken(ctx) default: - return "", "", eris.Errorf("unsupported auth strategy: %s", authStrategy) + return "", eris.Errorf("unsupported auth strategy: %s", authStrategy) } } -func extractBearerToken(ctx *gin.Context) (string, string) { +func extractBearerToken(ctx *gin.Context) (string, error) { token := ctx.GetHeader("Authorization") if token == "" { - return "", "missing token" + return "", ungerr.UnauthorizedError("missing token") } isValid, token := validateAndExtractBearerToken(token) if !isValid { - return "", "invalid token" + return "", ungerr.UnauthorizedError("invalid token") } - return token, "" + return token, nil }
82-96: ConstantTimeCompare on "bearer" prefix has limited security benefit.Using
subtle.ConstantTimeComparefor comparing the"bearer"prefix doesn't prevent timing attacks on the actual token value—only on the prefix comparison. The real token validation happens intokenCheckFunc. This is harmless but provides little security value here.pkg/server/http_server.go (1)
15-20: Type naming is terse but follows Go conventions.Renaming
HttpServertoHttpwithin theserverpackage results in usage likeserver.Http, which is acceptable. However,Httpalone doesn't convey it's a server struct. Consider ifServerwould be clearer (usage:server.Serverorserver.New() *Server).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
.gitignoreMakefileerrors.goexample/handler.goexample/main.goexample/server.goextract_token.gogo.modmiddlewares.gopkg/middleware/auth.gopkg/middleware/cors.gopkg/middleware/error.gopkg/middleware/logging.gopkg/middleware/middlewares.gopkg/middleware/permission.gopkg/response/responses.gopkg/server/http_server.gopkg/server/utils.gotest/cors_middleware_test.gotest/error_middleware_comprehensive_test.gotest/error_middleware_test.gotest/errors_test.gotest/extract_token_test.gotest/http_server_test.gotest/logging_middleware_test.gotest/middlewares_test.gotest/responses_test.gotest/utils_test.go
💤 Files with no reviewable changes (13)
- test/responses_test.go
- errors.go
- test/http_server_test.go
- test/extract_token_test.go
- test/errors_test.go
- test/logging_middleware_test.go
- test/error_middleware_test.go
- test/error_middleware_comprehensive_test.go
- test/utils_test.go
- extract_token.go
- middlewares.go
- test/cors_middleware_test.go
- test/middlewares_test.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: middlewares.go:23-28
Timestamp: 2025-08-27T09:29:56.699Z
Learning: In the ginkgo package, the NewCorsMiddleware function intentionally uses log.Fatalf for CORS configuration validation as a fail-fast design choice rather than degrading gracefully.
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: error_middleware.go:3-16
Timestamp: 2025-08-27T09:12:03.396Z
Learning: In the ginkgo package error middleware, unwrapped errors (including known types like validator.ValidationErrors and json.SyntaxError) are intentionally treated as 500 Internal Server Errors to enforce the use of eris.Wrap for consistent stack trace collection and better observability.
📚 Learning: 2025-08-27T09:29:56.699Z
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: middlewares.go:23-28
Timestamp: 2025-08-27T09:29:56.699Z
Learning: In the ginkgo package, the NewCorsMiddleware function intentionally uses log.Fatalf for CORS configuration validation as a fail-fast design choice rather than degrading gracefully.
Applied to files:
pkg/middleware/logging.gopkg/middleware/cors.goexample/handler.gopkg/middleware/error.gopkg/middleware/middlewares.gopkg/server/utils.gopkg/server/http_server.go
📚 Learning: 2025-08-27T09:32:11.080Z
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: middlewares.go:39-44
Timestamp: 2025-08-27T09:32:11.080Z
Learning: In the ginkgo package, the NewAuthMiddleware function intentionally uses log.Fatalf for tokenCheckFunc validation as a fail-fast design choice, similar to the CORS middleware pattern.
Applied to files:
pkg/middleware/logging.gopkg/middleware/cors.goexample/handler.gopkg/middleware/error.gopkg/middleware/auth.gopkg/middleware/middlewares.gopkg/server/utils.gopkg/server/http_server.gopkg/middleware/permission.go
📚 Learning: 2025-08-27T09:18:34.994Z
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: http_server.go:22-37
Timestamp: 2025-08-27T09:18:34.994Z
Learning: In the ginkgo package, the NewHttpServer constructor intentionally uses log.Fatal for parameter validation (nil server, invalid timeout, nil logger) as a fail-fast design choice rather than returning errors.
Applied to files:
pkg/middleware/logging.goexample/handler.gopkg/middleware/error.gopkg/server/utils.goexample/server.gopkg/server/http_server.go
📚 Learning: 2025-08-27T09:12:03.396Z
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: error_middleware.go:3-16
Timestamp: 2025-08-27T09:12:03.396Z
Learning: In the ginkgo package error middleware, unwrapped errors (including known types like validator.ValidationErrors and json.SyntaxError) are intentionally treated as 500 Internal Server Errors to enforce the use of eris.Wrap for consistent stack trace collection and better observability.
Applied to files:
pkg/middleware/logging.goexample/handler.gopkg/middleware/error.gopkg/middleware/middlewares.gopkg/server/utils.go
🧬 Code graph analysis (6)
pkg/middleware/cors.go (1)
pkg/middleware/middlewares.go (1)
MiddlewareProvider(10-12)
example/handler.go (1)
pkg/server/utils.go (1)
Handler(99-107)
pkg/middleware/auth.go (1)
pkg/middleware/middlewares.go (1)
MiddlewareProvider(10-12)
pkg/server/utils.go (1)
pkg/response/responses.go (1)
JSONResponse(30-34)
example/server.go (3)
pkg/server/http_server.go (2)
Http(15-20)New(22-37)pkg/middleware/middlewares.go (1)
NewMiddlewareProvider(14-19)pkg/server/utils.go (1)
Handler(99-107)
pkg/middleware/permission.go (1)
pkg/middleware/middlewares.go (1)
MiddlewareProvider(10-12)
🔇 Additional comments (23)
.gitignore (1)
10-11: LGTM!Standard and appropriate ignore patterns for a Go project. The additions of
bin/andtmp/align well with the package restructuring in this PR, and uncommenting.vscode/keeps IDE-specific files out of the repository.Also applies to: 34-34
Makefile (1)
18-38: No tests currently exist; scope expansion is appropriate for restructured package layout.The verification confirms no test files exist in the repository (no
_test.gofiles found, and thetest/directory has been removed). The Makefile scope change from./test/...to./...is correctly aligned with the package restructuring and follows standard Go practice for testing across multiple packages. This allows future tests to be added throughout thepkg/andexample/directories as needed.go.mod (1)
10-10: Pre-release dependency in use.Using
v0.2.0-pre2(a pre-release version) ofungerrmay introduce instability or breaking changes before the final release. Consider pinning to a stable release version if available, or ensure adequate testing covers this dependency's behavior.example/main.go (1)
1-6: LGTM!Clean and minimal entry point for the example application. The separation of setup logic into
setup()keepsmain()focused on orchestration.pkg/middleware/logging.go (1)
1-1: Package rename looks good.The package name change from
ginkgotomiddlewarealigns with the new modular package structure.example/server.go (1)
39-39: Passing nil for shutdownFunc is acceptable.Per the
server.Newimplementation inpkg/server/http_server.go, a nilshutdownFunctriggers a warning log but continues execution. This is fine for an example application.pkg/middleware/cors.go (1)
8-23: LGTM!The CORS middleware implementation follows the established fail-fast pattern for validation errors (consistent with learnings) while gracefully falling back to defaults when no config is provided. The validation check before creating the middleware is a good defensive practice.
example/handler.go (1)
12-52: LGTM! Comprehensive error scenario coverage.These handlers effectively demonstrate the different error handling paths:
- Success case
ungerrwrapped errors (Unknown, Wrap, BadRequestError)- Unwrapped errors (intentionally treated as 500 per error middleware design)
- Known error types
- Panic recovery
This provides good examples for testing the
server.Handlerwrapper behavior.pkg/middleware/middlewares.go (2)
14-19: LGTM!The constructor correctly uses
log.Fatalfor the nil logger check since the injected logger isn't available yet at that point. This follows the fail-fast pattern established in the codebase.
21-23: FunctionnewErrorMiddlewareis properly defined.The function exists in
pkg/middleware/error.go:37with the correct signaturefunc newErrorMiddleware(logger ezutil.Logger) gin.HandlerFunc, returningmiddleware.handleas expected.pkg/server/utils.go (2)
58-64: LGTM - Clean convenience wrapper.The
BindJSONfunction provides a consistent API alongsideBindRequest. The error wrapping with eris is appropriate for stack trace collection, aligning with the project's error handling philosophy.
99-106: Handler wrapper design is clean; nil responses are properly handled byomitzero.The
Datafield inJSONResponseuses thejson:"data,omitzero"tag, which is supported by Go 1.25.0. Whenrespisnil, thedatafield will be omitted from the JSON output rather than serialized asnull. This provides a clean separation of concerns and proper handling of endpoints that return no data.pkg/middleware/auth.go (1)
21-23: Fail-fast validation is consistent with project patterns.Based on learnings, using
log.FatalffortokenCheckFuncvalidation is an intentional fail-fast design choice, consistent withNewCorsMiddlewareand other constructors.pkg/server/http_server.go (2)
22-37: Constructor validation is well-structured.The fail-fast design using
log.Fatalandlogger.Fatalfor critical parameters is consistent with project patterns per learnings. The validation order correctly checks logger first (using standardlog.Fatal) before using the logger for subsequent checks.
39-67: ServeGracefully implementation is solid.The graceful shutdown logic properly handles signal capture, context timeout, and resource cleanup. The only change is the receiver type rename, which doesn't affect functionality.
pkg/middleware/error.go (4)
23-30: The errorObject type correctly implements the error interface.The struct provides structured error data with
CodeandDetailfields, and theError()method satisfies theerrorinterface for compatibility with[]errorinJSONResponse.
76-85: UnknownError handling logic looks correct.The flow properly distinguishes between:
UnknownErrorwith an unwrappable original error → logs original error typeUnknownErrorwithout wrapped content → logs the unknown error messageBoth paths correctly mask and return
InternalServerError, consistent with the project's error handling philosophy per learnings.
122-141: Good consolidated logging for unwrapped errors.The
logUnwrappedErrorfunction provides comprehensive debugging information in a single log entry, making it easier to identify where errors need proper wrapping. The message clearly guides developers to useungerr.Wrap()orungerr.AppError.
144-198: Comprehensive panic recovery with detailed analysis.The panic handling provides excellent observability with:
- Type-specific analysis for common panic scenarios
- Full stack trace inclusion
- Request context (method, path, handler)
- Proper fallback when response is already written
pkg/response/responses.go (4)
8-9: Form tags are correct for query parameter binding.Using
formtags instead ofqueryis the correct approach for Gin'sShouldBindQueryand related binding methods.
38-42: Simplified NewResponse API is clean.Directly accepting
datainstead of a message with chained methods reduces boilerplate. The change aligns with the updatedHandlerwrapper inpkg/server/utils.go.
46-50: Variadic error signature provides flexibility.The variadic
...errorsignature allows passing single or multiple errors, which is useful for validation scenarios that may produce multiple errors.
30-34: The errors field correctly serializes structured error objects.Go's JSON marshaler respects struct field tags when marshaling concrete types through interfaces. When
errorObjectinstances are placed in the[]errorfield, they serialize to{code, detail}objects with their fields preserved, not as strings. The current implementation correctly maintains the error structure during JSON serialization.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
pkg/middleware/error_test.go (3)
21-41: Remove duplicate code and unused variables.The test logic is correct, but there are code quality issues:
- Line 27 duplicates line 24 (redundant request creation)
- Lines 22-24 create a context that's never used since line 31 creates a router
- Lines 26-30 contain confusing comments that don't add clarity
🔎 Proposed cleanup
t.Run("app error", func(t *testing.T) { w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Request = httptest.NewRequest("GET", "/", nil) - - // Setup handlers properly to simulate middleware chain - c.Request = httptest.NewRequest("GET", "/", nil) + req := httptest.NewRequest("GET", "/", nil) - // We manually execute the middleware logic sequence since gin.CreateTestContext doesn't easily chaining without router - // Better approach: use a router r := gin.New() r.Use(mw) r.GET("/", func(c *gin.Context) { _ = c.Error(ungerr.NotFoundError("resource not found")) }) - r.ServeHTTP(w, c.Request) + r.ServeHTTP(w, req) assert.Equal(t, http.StatusNotFound, w.Code) assert.Contains(t, w.Body.String(), "Not Found") })
43-58: Remove unused context creation.The test correctly verifies that raw errors return 500 (consistent with the learned behavior that unwrapped errors are intentionally treated as Internal Server Errors). However, lines 44-46 create an unused context.
🔎 Proposed cleanup
t.Run("raw error conversion", func(t *testing.T) { w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Request = httptest.NewRequest("GET", "/", nil) + req := httptest.NewRequest("GET", "/", nil) r := gin.New() r.Use(mw) r.GET("/", func(c *gin.Context) { _ = c.Error(errors.New("something broke")) }) - r.ServeHTTP(w, c.Request) + r.ServeHTTP(w, req) assert.Equal(t, http.StatusInternalServerError, w.Code) assert.Contains(t, w.Body.String(), "Internal Server Error") })Based on learnings, this correctly tests that unwrapped errors are intentionally treated as 500 Internal Server Errors.
60-75: Remove unused context creation.The test correctly verifies panic recovery returns 500 Internal Server Error. However, lines 61-63 create an unused context.
🔎 Proposed cleanup
t.Run("panic recovery", func(t *testing.T) { w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Request = httptest.NewRequest("GET", "/", nil) + req := httptest.NewRequest("GET", "/", nil) r := gin.New() r.Use(mw) r.GET("/", func(c *gin.Context) { panic("oops") }) - r.ServeHTTP(w, c.Request) + r.ServeHTTP(w, req) assert.Equal(t, http.StatusInternalServerError, w.Code) assert.Contains(t, w.Body.String(), "Internal Server Error") })pkg/response/responses_test.go (3)
11-21: Consider adding test cases for edge cases.The current test covers the happy path well. For more comprehensive coverage, consider adding test cases for:
nildata input- Empty data structures
- Verification of omitzero behavior on the Data field when marshaled to JSON
23-30: Consider testing variadic behavior with multiple errors.Since
NewErrorResponseaccepts variadic errors (err ...error), consider adding test cases for:
- Multiple errors:
NewErrorResponse(err1, err2, err3)- Zero errors:
NewErrorResponse()(edge case)This would provide more comprehensive coverage of the function's API contract.
32-76: Pagination test logic is correct.The test cases cover important scenarios:
- Middle page pagination with correct HasNextPage/HasPrevPage flags
- Zero limit edge case
- Last page scenario
- IsZero method
For additional coverage, consider adding a test for the first page scenario (page 1) to verify that
HasPrevPageis false andHasNextPageis true.pkg/server/http_server_test.go (1)
12-36: Strong test coverage for the constructor.The test effectively covers the main scenarios: successful initialization with all parameters and handling of nil
shutdownFunc. The subtest organization is clear and the assertions appropriately verify the internal state.💡 Optional: Add logger field assertion for completeness
Consider verifying that the logger field is properly set in both subtests:
assert.NotNil(t, s) assert.Equal(t, srv, s.srv) assert.Equal(t, timeout, s.timeout) + assert.Equal(t, logger, s.logger) assert.NotNil(t, s.shutdownFunc)And similarly in the "nil shutdown func" subtest:
assert.NotNil(t, s) + assert.Equal(t, logger, s.logger) assert.Nil(t, s.shutdownFunc)pkg/server/utils_test.go (1)
76-84: Add Content-Type header for more realistic test.The test should set the Content-Type header to "application/json" to better reflect real-world HTTP requests and ensure the binding behavior is validated under realistic conditions.
🔎 Suggested improvement
t.Run("valid json", func(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("POST", "/", bytes.NewBufferString(`{"name":"test"}`)) + c.Request.Header.Set("Content-Type", "application/json") val, err := BindJSON[TestStruct](c) assert.NoError(t, err) assert.Equal(t, "test", val.Name) })pkg/middleware/logging_test.go (2)
20-21: Remove the developer comment.This appears to be a temporary implementation note that doesn't add value for future maintainers.
🔎 Proposed fix
- // Corrected: No arguments locally, it seems from logging.go content mw := mp.NewLoggingMiddleware()
15-47: Consider verifying actual logging behavior.The tests validate that the middleware doesn't crash and returns a 200 status, but they don't verify that the middleware performs its primary function—logging request details. Consider enhancing the tests to:
- Capture and assert on log output (method, path, status code, duration)
- Verify the logger was called with expected parameters
- For POST requests, confirm that the request body was logged
This would provide stronger confidence that the logging middleware works correctly beyond just not panicking.
Example approach using a mock logger
You could use a mock logger or capture log output to verify behavior:
// Example: Add a test recorder to capture logs type testLogRecorder struct { logs []string } func (r *testLogRecorder) Log(msg string) { r.logs = append(r.logs, msg) } // Then in your test: recorder := &testLogRecorder{} // Use recorder to verify expected log entries were createdpkg/middleware/permission_test.go (1)
48-67: Consider adding error assertions for consistency.The "unknown role" and "missing role" subtests only check
c.IsAborted()but don't verifyc.Errorslike the "no permission" test does. Addingassert.NotEmpty(t, c.Errors)would improve consistency and catch regressions in error recording.🔎 Proposed fix for consistency
t.Run("unknown role", func(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("POST", "/", nil) c.Set("role", "guest") mw(c) assert.True(t, c.IsAborted()) + assert.NotEmpty(t, c.Errors) }) t.Run("missing role", func(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("POST", "/", nil) mw(c) assert.True(t, c.IsAborted()) + assert.NotEmpty(t, c.Errors) })pkg/middleware/ratelimit_test.go (1)
57-67: Consider handling JSON unmarshal error and using safer type assertions.The JSON unmarshal error is silently ignored, and the type assertions on lines 60 and 62 could panic if the response structure changes unexpectedly.
🔎 Proposed fix for safer response parsing
var response map[string]interface{} - _ = json.Unmarshal(w2.Body.Bytes(), &response) + err := json.Unmarshal(w2.Body.Bytes(), &response) + assert.NoError(t, err, "failed to unmarshal response") errors, ok := response["errors"].([]interface{}) - if ok && len(errors) > 0 { - errObj := errors[0].(map[string]interface{}) + if assert.True(t, ok && len(errors) > 0, "expected errors in response") { + errObj, ok := errors[0].(map[string]interface{}) + assert.True(t, ok, "expected error object") assert.Equal(t, "Too Many Requests", errObj["code"]) assert.Equal(t, "rate limit exceeded", errObj["detail"]) - } else { - assert.Fail(t, "expected errors in response") }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
Makefileexample/server.gogo.modpkg/middleware/auth_test.gopkg/middleware/error_test.gopkg/middleware/logging_test.gopkg/middleware/permission_test.gopkg/middleware/ratelimit.gopkg/middleware/ratelimit_test.gopkg/response/responses_test.gopkg/server/http_server_test.gopkg/server/utils_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- example/server.go
- go.mod
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: middlewares.go:23-28
Timestamp: 2025-08-27T09:29:56.699Z
Learning: In the ginkgo package, the NewCorsMiddleware function intentionally uses log.Fatalf for CORS configuration validation as a fail-fast design choice rather than degrading gracefully.
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: error_middleware.go:3-16
Timestamp: 2025-08-27T09:12:03.396Z
Learning: In the ginkgo package error middleware, unwrapped errors (including known types like validator.ValidationErrors and json.SyntaxError) are intentionally treated as 500 Internal Server Errors to enforce the use of eris.Wrap for consistent stack trace collection and better observability.
📚 Learning: 2025-08-27T09:32:11.080Z
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: middlewares.go:39-44
Timestamp: 2025-08-27T09:32:11.080Z
Learning: In the ginkgo package, the NewAuthMiddleware function intentionally uses log.Fatalf for tokenCheckFunc validation as a fail-fast design choice, similar to the CORS middleware pattern.
Applied to files:
pkg/middleware/error_test.gopkg/middleware/permission_test.gopkg/middleware/ratelimit.gopkg/middleware/ratelimit_test.gopkg/middleware/auth_test.gopkg/middleware/logging_test.go
📚 Learning: 2025-08-27T09:29:56.699Z
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: middlewares.go:23-28
Timestamp: 2025-08-27T09:29:56.699Z
Learning: In the ginkgo package, the NewCorsMiddleware function intentionally uses log.Fatalf for CORS configuration validation as a fail-fast design choice rather than degrading gracefully.
Applied to files:
pkg/middleware/error_test.gopkg/middleware/ratelimit.gopkg/middleware/auth_test.gopkg/middleware/logging_test.go
📚 Learning: 2025-08-27T09:12:03.396Z
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: error_middleware.go:3-16
Timestamp: 2025-08-27T09:12:03.396Z
Learning: In the ginkgo package error middleware, unwrapped errors (including known types like validator.ValidationErrors and json.SyntaxError) are intentionally treated as 500 Internal Server Errors to enforce the use of eris.Wrap for consistent stack trace collection and better observability.
Applied to files:
pkg/middleware/error_test.go
📚 Learning: 2025-08-27T09:18:34.994Z
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: http_server.go:22-37
Timestamp: 2025-08-27T09:18:34.994Z
Learning: In the ginkgo package, the NewHttpServer constructor intentionally uses log.Fatal for parameter validation (nil server, invalid timeout, nil logger) as a fail-fast design choice rather than returning errors.
Applied to files:
pkg/server/http_server_test.go
🧬 Code graph analysis (8)
pkg/server/utils_test.go (1)
pkg/server/utils.go (4)
GetPathParam(15-29)GetRequiredPathParam(34-43)BindJSON(58-64)GetFromContext(69-83)
pkg/middleware/error_test.go (2)
pkg/middleware/middlewares.go (1)
NewMiddlewareProvider(14-19)pkg/server/http_server.go (1)
New(22-37)
pkg/middleware/permission_test.go (1)
pkg/middleware/middlewares.go (1)
NewMiddlewareProvider(14-19)
pkg/middleware/ratelimit.go (2)
pkg/middleware/middlewares.go (1)
MiddlewareProvider(10-12)pkg/response/responses.go (1)
NewErrorResponse(46-50)
pkg/server/http_server_test.go (1)
pkg/server/http_server.go (1)
New(22-37)
pkg/middleware/ratelimit_test.go (1)
pkg/middleware/middlewares.go (1)
NewMiddlewareProvider(14-19)
pkg/middleware/auth_test.go (2)
pkg/middleware/middlewares.go (1)
NewMiddlewareProvider(14-19)pkg/server/http_server.go (1)
New(22-37)
pkg/middleware/logging_test.go (1)
pkg/middleware/middlewares.go (1)
NewMiddlewareProvider(14-19)
🔇 Additional comments (18)
pkg/middleware/error_test.go (1)
15-19: LGTM!The test setup correctly initializes the logger, middleware provider, and error middleware following the expected patterns.
Makefile (1)
18-38: Test path migration to./pkg/...is complete and properly implemented.Verification confirms all 8 test files have been successfully migrated to the new package structure (middleware, response, and server subdirectories), with no tests orphaned in the old
./test/directory. The Makefile changes align correctly with the actual repository organization. The restructuring follows Go conventions for co-locating tests with package code.pkg/response/responses_test.go (1)
1-9: LGTM!Package declaration and imports are appropriate for the test file.
pkg/server/http_server_test.go (2)
1-10: LGTM: Clean package and import structure.The package declaration and imports are appropriate for the test file, with all dependencies properly declared.
38-41: Good documentation of testing limitations.The comment appropriately explains why fatal exit paths aren't tested and acknowledges the technical constraints. Based on learnings, the fail-fast design using
log.Fatal/logger.Fatalis an intentional choice, so this limitation is acceptable and well-documented.pkg/server/utils_test.go (3)
12-45: LGTM! Comprehensive test coverage for path parameter handling.The test cases cover all critical paths: successful parsing, missing parameters, and type conversion failures. Assertions correctly validate all return values.
47-67: LGTM! Adequate coverage for required parameter validation.Tests cover both success and error scenarios appropriately.
96-125: LGTM! Thorough coverage for context value retrieval.Tests comprehensively cover success, missing value, and type assertion failure scenarios with appropriate assertions.
pkg/middleware/auth_test.go (4)
14-38: LGTM! Comprehensive test for success path.The success test correctly verifies that the middleware passes with a valid Bearer token, context is not aborted, and user data is properly stored in the context.
40-55: LGTM! Missing token scenario covered.The test correctly verifies that requests without an Authorization header result in an aborted context with errors.
57-91: LGTM! Error scenarios well covered.Both "user not found" and "check error" subtests properly validate that the middleware aborts and records errors when the token check fails or returns an error.
93-109: LGTM! Unsupported strategy handling verified.The test correctly verifies that using an unsupported auth strategy ("Basic" when only "Bearer" is supported) results in proper abort behavior.
pkg/middleware/permission_test.go (2)
12-23: LGTM! Well-structured test setup.The test correctly initializes the middleware provider with a logger and defines a clear permission map for testing different role scenarios.
25-34: LGTM! Happy path correctly verified.The test properly verifies that an admin role with write permission is allowed through without aborting.
pkg/middleware/ratelimit_test.go (1)
16-34: LGTM! First request allowed test is correct.The test properly verifies that the first request from an IP passes through without being rate-limited.
pkg/middleware/ratelimit.go (3)
13-23: LGTM! Clean struct definitions.The
visitorandrateLimiterstructs are well-defined with appropriate fields for tracking rate limits per IP.
35-47: LGTM! Thread-safe visitor management.The mutex protection is correctly applied around map access, and the limiter is properly created or updated on each access.
62-84: LGTM! Middleware implementation is correct.The middleware properly extracts client IP, checks rate limits, and returns appropriate 429 responses with structured error objects when limits are exceeded.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @go.mod:
- Around line 9-12: The go.mod contains two invalid dependencies
(github.com/itsLeonB/ezutil/v2 v2.2.1 and github.com/itsLeonB/ungerr v0.2.0);
verify the correct module paths and versions on GitHub/pkg.go.dev for
github.com/itsLeonB/ezutil (confirm whether the module uses /v2 in path and pick
an existing tag) and for ungerr (confirm exact module name or remove if unused),
update or remove those require lines accordingly, update any import paths in the
code that reference those modules to match the corrected module path/version (or
remove usages), then run go mod tidy and go mod download to regenerate go.sum
and ensure the remaining dependencies (github.com/stretchr/testify and
golang.org/x/time) resolve cleanly.
In @pkg/middleware/permission.go:
- Around line 21-38: The role validation currently returns ungerr.Unknownf
(which maps to HTTP 500); update the two checks that use ungerr.Unknownf (the
missing-role check using role := ctx.GetString(roleContextKey) and the
unknown-role lookup against permissionMap[role]) to use
ungerr.ForbiddenError(...) instead so access-control failures return 403 like
the existing permission denial (ctx.Error(ungerr.ForbiddenError("..."))). Ensure
you replace both ungerr.Unknownf calls with ungerr.ForbiddenError and keep the
same descriptive messages.
🧹 Nitpick comments (6)
pkg/server/utils.go (1)
58-64: Consider delegating to BindRequest to reduce duplication.The
BindJSONfunction duplicates the error handling pattern fromBindRequest. You could simplify this by delegating to the existing function.♻️ Proposed refactor
func BindJSON[T any](ctx *gin.Context) (T, error) { - var zero T - if err := ctx.ShouldBindJSON(&zero); err != nil { - return zero, ungerr.Wrap(err, "failed to bind JSON request") - } - return zero, nil + return BindRequest[T](ctx, binding.JSON) }pkg/server/utils_test.go (2)
48-68: Consider adding test for invalid type conversion.The test covers valid and missing parameter scenarios but doesn't test the case where parsing fails (e.g.,
GetRequiredPathParam[int](c, "id")withid="abc"). SinceGetRequiredPathParamcallsezutil.Parse[T], which can return an error, this scenario should be covered.📝 Suggested additional test case
t.Run("invalid type", func(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Params = gin.Params{{Key: "id", Value: "abc"}} _, err := GetRequiredPathParam[int](c, "id") assert.Error(t, err) })
187-213: Consider verifying response body structure in success test.The success test checks the status code but doesn't verify the response body structure. Since
Handlerwraps responses inresponse.JSONResponse{Data: resp}, it would be valuable to verify this envelope structure.📝 Enhanced success test
t.Run("success", func(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) handler := Handler(200, func(ctx *gin.Context) (any, error) { return map[string]string{"message": "success"}, nil }) handler(c) assert.Equal(t, 200, w.Code) + assert.Contains(t, w.Body.String(), `"data"`) + assert.Contains(t, w.Body.String(), `"message":"success"`) })pkg/middleware/permission.go (1)
15-19: Validate middleware configuration at construction time (fail fast), not per-request.Right now
roleContextKey == "",requiredPermission == "", orpermissionMap == nilwill quietly turn into runtime denials (or confusing errors) on every request. Given your existing fail-fast pattern in other middleware/provider constructors (per learnings), consider validating these inputs before returning the handler.Proposed change
package middleware import ( + "log" "slices" "github.com/gin-gonic/gin" "github.com/itsLeonB/ungerr" ) @@ func (mp *MiddlewareProvider) NewPermissionMiddleware( roleContextKey string, requiredPermission string, permissionMap map[string][]string, ) gin.HandlerFunc { + if roleContextKey == "" { + log.Fatal("roleContextKey cannot be empty") + } + if requiredPermission == "" { + log.Fatal("requiredPermission cannot be empty") + } + if permissionMap == nil { + log.Fatal("permissionMap cannot be nil") + } return func(ctx *gin.Context) { role := ctx.GetString(roleContextKey) if role == "" {pkg/middleware/middlewares_test.go (1)
10-25: Consider adding a subprocess test forNewMiddlewareProvider(nil)to lock in the fail-fast contract.Since
NewMiddlewareProvideruseslog.Fatal(exits), verifying it requires anexec.Command-style test. Optional, but it prevents accidental future softening of this behavior. (Based on learnings, this fail-fast pattern is intentional.)pkg/middleware/cors_test.go (1)
11-28: Non-nil assertions are low-signal; prefer asserting actual CORS headers viahttptest.Consider spinning up a tiny
gin.Engine, installing the middleware, issuing an OPTIONS / preflight request, and asserting the expectedAccess-Control-*headers for both default config and custom config.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
.github/workflows/ci.ymlMakefilego.modpkg/middleware/auth.gopkg/middleware/cors_test.gopkg/middleware/middlewares_test.gopkg/middleware/permission.gopkg/server/utils.gopkg/server/utils_test.goscripts/git-pre-push.sh
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/middleware/auth.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: middlewares.go:23-28
Timestamp: 2025-08-27T09:29:56.699Z
Learning: In the ginkgo package, the NewCorsMiddleware function intentionally uses log.Fatalf for CORS configuration validation as a fail-fast design choice rather than degrading gracefully.
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: middlewares.go:39-44
Timestamp: 2025-08-27T09:32:11.080Z
Learning: In the ginkgo package, the NewAuthMiddleware function intentionally uses log.Fatalf for tokenCheckFunc validation as a fail-fast design choice, similar to the CORS middleware pattern.
📚 Learning: 2025-08-27T09:29:56.699Z
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: middlewares.go:23-28
Timestamp: 2025-08-27T09:29:56.699Z
Learning: In the ginkgo package, the NewCorsMiddleware function intentionally uses log.Fatalf for CORS configuration validation as a fail-fast design choice rather than degrading gracefully.
Applied to files:
pkg/middleware/cors_test.gopkg/middleware/middlewares_test.gogo.mod
📚 Learning: 2025-08-27T09:32:11.080Z
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: middlewares.go:39-44
Timestamp: 2025-08-27T09:32:11.080Z
Learning: In the ginkgo package, the NewAuthMiddleware function intentionally uses log.Fatalf for tokenCheckFunc validation as a fail-fast design choice, similar to the CORS middleware pattern.
Applied to files:
pkg/middleware/cors_test.gopkg/middleware/middlewares_test.gopkg/middleware/permission.go
📚 Learning: 2025-08-27T09:18:34.994Z
Learnt from: itsLeonB
Repo: itsLeonB/ginkgo PR: 1
File: http_server.go:22-37
Timestamp: 2025-08-27T09:18:34.994Z
Learning: In the ginkgo package, the NewHttpServer constructor intentionally uses log.Fatal for parameter validation (nil server, invalid timeout, nil logger) as a fail-fast design choice rather than returning errors.
Applied to files:
pkg/server/utils.go
🧬 Code graph analysis (4)
pkg/server/utils_test.go (1)
pkg/server/utils.go (7)
GetPathParam(15-29)GetRequiredPathParam(34-43)BindJSON(58-64)GetFromContext(69-83)BindRequest(48-56)GetAndParseFromContext(88-97)Handler(99-107)
pkg/middleware/cors_test.go (1)
pkg/middleware/middlewares.go (1)
NewMiddlewareProvider(14-19)
pkg/server/utils.go (1)
pkg/response/responses.go (1)
JSONResponse(30-34)
pkg/middleware/middlewares_test.go (1)
pkg/middleware/middlewares.go (1)
NewMiddlewareProvider(14-19)
🪛 checkmake (0.2.2)
Makefile
[warning] 3-3: Target body for "help" exceeds allowed length of 5 (11).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (9)
pkg/server/utils.go (2)
1-9: LGTM! Package reorganization looks good.The package rename from
ginkgotoserverand updated import paths align well with the PR's objective to reorganize packages intopkg/{middleware,response,server}.
99-107: Verify error handling middleware is configured.The
Handlerwrapper delegates error responses to middleware by callingctx.Error(err)without sending a response directly. Error handling middleware inpkg/middleware/error.gois properly registered and converts these context errors into appropriate HTTP responses.pkg/server/utils_test.go (1)
13-46: LGTM! Comprehensive test coverage.The test covers all three critical scenarios: valid parameter parsing, missing parameters, and invalid type conversion.
scripts/git-pre-push.sh (1)
11-16: LGTM! Excellent addition to the pre-push workflow.Adding a build validation step between linting and testing is a sound practice. This ensures compilation succeeds before running tests, catching build-breaking changes early in the development workflow.
Makefile (3)
19-21: LGTM! Clean implementation of the build target.The new build target provides a straightforward way to compile all packages, which integrates well with the pre-push hook validation.
24-24: LGTM! Test path expansion aligns with package reorganization.Changing from
./test/...to./...expands test coverage to all packages, which correctly reflects the reorganization intopkg/{middleware,response,server}structure mentioned in the PR summary.Also applies to: 28-28, 50-50
30-46: LGTM! Per-package coverage is well-implemented.The updated coverage targets generate individual coverage reports for each package under
./pkg/..., which is excellent for modular projects. The implementation correctly:
- Uses
go list ./pkg/...to discover packages- Transforms package paths to safe filenames using
tr '/' '-'- Generates both text and HTML reports per package
pkg/middleware/permission.go (1)
4-5: Confirm Go version supportsslices(stdlib).This won’t compile if the module Go version is < 1.21.
Also applies to: 35-36
pkg/middleware/middlewares_test.go (1)
10-17: Good basic sanity test of provider construction.Covers the happy path and confirms
loggerwiring.
1 participant
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.