Conversation
WalkthroughUpdates CI push triggers to only run for Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Gin as Gin Router
participant LM as LoggingMiddleware
participant H as Handler
participant L as Logger
Client->>Gin: HTTP Request
Gin->>LM: Invoke middleware
alt Method == OPTIONS
LM-->>Gin: Bypass logging
else Proceed
LM->>LM: Record start time & build full path
LM->>H: ctx.Next()
H-->>LM: Response (status, errors)
LM->>LM: Compute duration, gather status, client IP
alt status >= 400
LM->>L: Errorf(method, path, status, duration, ip, optional err)
else status < 400
LM->>L: Infof(method, path, status, duration, ip)
end
end
Gin-->>Client: HTTP Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
3-3: Align go.mod directive with CI-tested Go versions
Go 1.21+ toolchains refuse to load a module whosegodirective exceeds their version, so CI builds on Go 1.23/1.24 will fail withgo 1.25.0. Lower the directive to the minimum CI version or raise the CI matrix to only test 1.25+.-go 1.25.0 +go 1.23
🧹 Nitpick comments (8)
.github/workflows/ci.yml (1)
5-5: Push trigger now excludes dev — confirm policy is intentionalLimiting CI on push to main can delay feedback on dev. If unintentional, add dev back to push branches.
on: push: - branches: [main] + branches: [main, dev]error_middleware.go (2)
23-28: Fix doc comment/name mismatches after unexportingComments still reference exported names; align to identifiers to avoid lint drift.
-// NewErrorMiddleware creates an error handling middleware for Gin. +// newErrorMiddleware creates an error handling middleware for Gin. @@ -// Handle is the main middleware function that processes errors and panics +// handle is the main middleware function that processes errors and panicsAlso applies to: 35-36
23-33: API consistency: consider exporting NewErrorMiddlewareOther middleware constructors are exported (e.g., NewCorsMiddleware, NewAuthMiddleware). Consider exporting for consistency unless you explicitly want it internal-only.
Please confirm no external consumers rely on an exported constructor here; I can open a follow-up PR to rename safely.
logging_middleware.go (4)
12-15: Skip logging for OPTIONS — good callPrevents noisy logs for CORS preflights. Consider also skipping HEAD or well-known health endpoints if desired.
21-25: Build full path more simply (preserve exact query/encoding)Use RequestURI to avoid recomposing and to preserve original encoding and duplicate keys.
- // Build full path with query string for logging - fullPath := path - if rawQuery := ctx.Request.URL.RawQuery; rawQuery != "" { - fullPath = path + "?" + rawQuery - } + // Build full path with query string for logging + fullPath := ctx.Request.RequestURI
35-61: Optional: include route pattern and keep errors conciseTo reduce log cardinality, include ctx.FullPath() when available; also consider logging only the last error to avoid multi-line blobs.
- if statusCode >= 400 { - errorMsg := "" - if len(ctx.Errors) > 0 { - errorMsg = ctx.Errors.String() - } + if statusCode >= 400 { + errorMsg := "" + if last := ctx.Errors.Last(); last != nil && last.Err != nil { + errorMsg = last.Error() + } + route := ctx.FullPath() + if route == "" { + route = method + " " + path + } @@ - method, - fullPath, + method, + fullPath, // consider adding route=%s as a separate field
10-73: Privacy noteQuery strings can contain PII/secrets. If that’s a concern, add redaction or an allowlist for logged query params.
test/logging_middleware_test.go (1)
73-82: Strengthen OPTIONS test: assert no logging occurredOptionally assert no Infof/Errorf calls were made to catch regressions.
{ name: "OPTIONS request skipped", @@ setupMock: func(m *MockLogger) { - // No logging expected for OPTIONS + // No expectations registered; we’ll assert none later }, },Add after ServeHTTP for this case:
if tt.method == "OPTIONS" { mockLogger.AssertNotCalled(t, "Infof", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) mockLogger.AssertNotCalled(t, "Errorf", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
.github/workflows/ci.yml(1 hunks)error_middleware.go(1 hunks)go.mod(2 hunks)logging_middleware.go(1 hunks)test/logging_middleware_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T09:32:11.080Z
Learnt from: itsLeonB
PR: itsLeonB/ginkgo#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:
test/logging_middleware_test.gologging_middleware.go
📚 Learning: 2025-08-27T09:29:56.699Z
Learnt from: itsLeonB
PR: itsLeonB/ginkgo#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:
test/logging_middleware_test.gologging_middleware.go
🧬 Code graph analysis (2)
test/logging_middleware_test.go (2)
test/http_server_test.go (1)
MockLogger(15-17)middlewares.go (1)
NewMiddlewareProvider(18-23)
logging_middleware.go (1)
middlewares.go (1)
MiddlewareProvider(14-16)
🪛 GitHub Check: Lint
test/logging_middleware_test.go
[failure] 108-108:
Error return value of c.Error is not checked (errcheck)
🪛 GitHub Actions: CI
test/logging_middleware_test.go
[error] 108-108: golangci-lint: test/logging_middleware_test.go:108:13: Error return value of c.Error is not checked (errcheck)
🔇 Additional comments (3)
.github/workflows/ci.yml (1)
21-21: Keep Go versions; 1.24 and 1.25 are official and supported
Go 1.24 was released on 11 February 2025(go.dev) and Go 1.25 on 12 August 2025(go.dev). actions/setup-go@v5 installs released Go versions from the actions/go-versions manifest (falling back to the official Go distribution when needed), so both are supported.(github.com)error_middleware.go (1)
32-33: LGTM: returning the unexported handlerSwitching to middleware.handle keeps the surface minimal without changing behavior.
go.mod (1)
9-9: Dependency bump: ezutil v2.1.0Assuming no breaking changes in Logger interface. If there are, please confirm all call sites (including tests) compile and behave as expected.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/logging_middleware_test.go (1)
106-109: Good fix for errcheck (c.Error) lint failureAssigning the returned error to the blank identifier resolves golangci-lint’s errcheck.
🧹 Nitpick comments (4)
test/logging_middleware_test.go (4)
34-36: Make expectations strict: assert exactly one log per requestGuard against accidental double-logging by constraining each expected call to happen once.
- m.On("Infof", mock.AnythingOfType("string"), mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + m.On("Infof", mock.AnythingOfType("string"), mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Once()- m.On("Infof", mock.AnythingOfType("string"), mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + m.On("Infof", mock.AnythingOfType("string"), mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Once()- m.On("Errorf", mock.AnythingOfType("string"), mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + m.On("Errorf", mock.AnythingOfType("string"), mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Once()- m.On("Errorf", mock.AnythingOfType("string"), mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + m.On("Errorf", mock.AnythingOfType("string"), mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Once()Also applies to: 46-47, 57-58, 69-70
72-82: Explicitly assert no logging for OPTIONSStrengthen the “skipped” scenario by asserting no Infof/Errorf calls occurred.
Add import:
import ( "errors" + "net/http" "net/http/httptest" "testing"Add assertions after ServeHTTP:
- mockLogger.AssertExpectations(t) + if tt.method == http.MethodOptions { + mockLogger.AssertNotCalled(t, "Infof") + mockLogger.AssertNotCalled(t, "Errorf") + } + mockLogger.AssertExpectations(t)Also applies to: 112-116
112-113: Also assert the HTTP status codeThis ensures the handler behaved as intended alongside logging.
- engine.ServeHTTP(w, ctx.Request) + engine.ServeHTTP(w, ctx.Request) + assert.Equal(t, tt.statusCode, w.Code)
94-101: Minor cleanup: build request without ctxNot required, but you can drop CreateTestContext’s returned ctx and build req directly to reduce indirection:
Example:
router := gin.New() router.Use(middleware) router.Any("/*path", handler) req := httptest.NewRequest(tt.method, url, nil) w := httptest.NewRecorder() router.ServeHTTP(w, req)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/logging_middleware_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T09:32:11.080Z
Learnt from: itsLeonB
PR: itsLeonB/ginkgo#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:
test/logging_middleware_test.go
📚 Learning: 2025-08-27T09:29:56.699Z
Learnt from: itsLeonB
PR: itsLeonB/ginkgo#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:
test/logging_middleware_test.go
🧬 Code graph analysis (1)
test/logging_middleware_test.go (2)
test/http_server_test.go (1)
MockLogger(15-17)middlewares.go (1)
NewMiddlewareProvider(18-23)
🔇 Additional comments (2)
test/logging_middleware_test.go (2)
14-16: Solid test harness setupUsing gin.TestMode and a table-driven test is appropriate here.
34-70: Mock expectations correctly match middleware logging signatures. No argument-count mismatches detected.
1 participant
Summary by CodeRabbit