[codex] Customize validation handlers with slog#4
Open
phumberdroz wants to merge 10 commits intomainfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a rich ContractError model and expanded validation decoding; refactors request/response validation with a buffered responseBodyWriter and pluggable handlers (Logger, RequestErrorHandler, ResponseErrorHandler); updates tests for per-test routers and logging; pins CI to Go 1.26.1 and bumps golangci-lint to v9 (v2.11.3). Changes
Sequence DiagramsequenceDiagram
participant Client
participant Gin as Gin Router
participant Validator
participant ReqHandler as RequestErrorHandler
participant RespWriter as ResponseBodyWriter
participant RespHandler as ResponseErrorHandler
participant Logger
Client->>Gin: HTTP request
Gin->>Validator: middleware intercepts request
Validator->>Validator: validateIncomingRequest()
alt request validation fails
Validator->>ReqHandler: invoke RequestErrorHandler
ReqHandler->>Client: return error response
else request valid
Validator->>RespWriter: wrap response writer
Gin->>RespWriter: handler writes response
RespWriter->>RespWriter: capture body/status
Validator->>Validator: validateOutgoingResponse()
alt response validation fails
Validator->>Logger: log validation error (if configured)
Validator->>RespHandler: invoke ResponseErrorHandler
RespHandler->>Client: return/replace error response
else response valid
RespWriter->>Client: flush captured response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/gin-openapi-validator/validationerror.go (1)
188-195:⚠️ Potential issue | 🔴 CriticalGuard
e.Parameterbefore building the enum hint.Line 189 assumes every enum schema error came from a parameter, but body schema errors reach this branch with
e.Parameter == nil. An invalid enum in the request body will panic here and turn a validation failure into a 500.🐛 Proposed fix
- if (e.Parameter.Explode == nil || *e.Parameter.Explode) && + if e.Parameter != nil && + (e.Parameter.Explode == nil || *e.Parameter.Explode) && (e.Parameter.Style == "" || e.Parameter.Style == "form") && strings.Contains(value, ",") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/gin-openapi-validator/validationerror.go` around lines 188 - 195, The code builds an enum hint for query parameters but dereferences e.Parameter without checking for nil, causing a panic for body schema enum errors; update the branch that constructs the hint (the condition referencing e.Parameter.Explode, e.Parameter.Style, e.Parameter.Name and innerErr.Value) to first check e.Parameter != nil and only build/append the "?name=..." suggestion when e.Parameter is non-nil (leave enum handling for body errors untouched), ensuring accesses to e.Parameter.Explode, e.Parameter.Style and e.Parameter.Name are guarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/gin-openapi-validator/validator.go`:
- Around line 220-248: The custom response handler is given a fresh empty writer
so it cannot inspect the invalid response; modify handleResponseValidationError
to pass the captured response into the handler (or set c.Writer =
capturedWriter) instead of replacing c.Writer with newResponseBodyWriter before
calling options.ResponseErrorHandler; ensure capturedWriter is initialized with
the captured status, headers and body so the handler can read/modify them, then
detect whether the handler replaced/modified the response (using
capturedWriter.modified() or a snapshot comparison) and only flush/replace
originalWriter when appropriate; keep the StrictResponse branch behavior intact
by continuing to use newResponseBodyWriter there and invoke
defaultResponseErrorHandler(options) as before.
- Around line 82-95: The Flush method is a no-op which breaks streaming
handlers; implement responseBodyWriter.Flush to flush buffered headers/body
immediately and forward to the underlying ResponseWriter's Flush if available:
call the existing flush() helper to copy headers, set status and write buffered
bytes, then if w.ResponseWriter implements http.Flusher invoke its Flush();
after flushing clear or reset w.body to avoid re-sending data and ensure
subsequent writes continue validation/accumulate as intended (use the symbols
responseBodyWriter, Flush(), flush(), headers, statusCode,
ResponseWriter.WriteHeader, ResponseWriter.Write and
ResponseWriter.(Header/Flush) to locate code).
---
Outside diff comments:
In `@pkg/gin-openapi-validator/validationerror.go`:
- Around line 188-195: The code builds an enum hint for query parameters but
dereferences e.Parameter without checking for nil, causing a panic for body
schema enum errors; update the branch that constructs the hint (the condition
referencing e.Parameter.Explode, e.Parameter.Style, e.Parameter.Name and
innerErr.Value) to first check e.Parameter != nil and only build/append the
"?name=..." suggestion when e.Parameter is non-nil (leave enum handling for body
errors untouched), ensuring accesses to e.Parameter.Explode, e.Parameter.Style
and e.Parameter.Name are guarded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16adc2f5-1359-4c0e-b858-e1655bab90f1
📒 Files selected for processing (4)
.github/workflows/ci.ymlpkg/gin-openapi-validator/validationerror.gopkg/gin-openapi-validator/validator.gopkg/gin-openapi-validator/validator_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/gin-openapi-validator/validator.go`:
- Around line 89-95: The Flush() implementation on responseBodyWriter currently
calls w.flush() then w.body.Reset(), which discards data needed by
validateOutgoingResponse; change the behavior so flushing still writes out to
the client but does NOT clear the validation buffer: introduce a separate
validation buffer (e.g., responseBodyWriter.validationBuf) or stop calling
w.body.Reset() so validateOutgoingResponse can read the full accumulated bytes;
update responseBodyWriter.Flush (and the analogous code at the other occurrence)
to write / forward the current bytes to the underlying ResponseWriter via
w.flush() but keep a persistent copy for validation, and ensure
validateOutgoingResponse reads from that persistent buffer instead of a
resettable one.
- Around line 27-71: The responseBodyWriter currently only clears rewriteBody
and resets state in WriteHeader, so handlers that call Write or WriteString
first will append to the captured invalid response and leak stale content
headers; update the mutating methods (responseBodyWriter.Write and
responseBodyWriter.WriteString) to check if w.rewriteBody && !w.flushed and on
first mutating write call the same reset logic as in WriteHeader:
w.body.Reset(), clear w.rewriteBody, reset/replace w.headers to an empty
http.Header (and any content-* or length headers), and ensure
wroteHeader/statusCode behavior remains correct; this keeps
cloneResponseBodyWriter's replacement semantics consistent regardless of whether
WriteHeader is called first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1dd053f5-68ff-4a22-ae58-3e03499966e8
📒 Files selected for processing (3)
pkg/gin-openapi-validator/validationerror.gopkg/gin-openapi-validator/validator.gopkg/gin-openapi-validator/validator_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/gin-openapi-validator/validator_test.go`:
- Around line 224-246: Add required blank lines to satisfy the wsl_v5 linter:
put a blank line after the TestCustomRequestErrorHandlerStopsChainWithoutAbort
local variable declaration (routeCalled), add a blank line after the
gin.SetMode(gin.TestMode) call, add blank lines around the
router.Use(ginopenapivalidator.Validator(...)) middleware block (one blank line
before and one after), and ensure there is a blank line separating the
router.POST handler definition from the performRequest/assertion block so the
test has the required blank lines around those statements.
- Around line 326-352: TestStrictResponseAllowsValidChunkedResponse has a
whitespace lint error: add a blank line before the logger assignment to satisfy
wsl_v5; specifically, insert a blank line between the var logOutput bytes.Buffer
declaration and the logger := slog.New(...) line in the
TestStrictResponseAllowsValidChunkedResponse function so the block with logger,
router.Use(ginopenapivalidator.Validator(...)) and the handler remains correctly
separated.
In `@pkg/gin-openapi-validator/validator.go`:
- Around line 256-263: The lint error is due to missing required blank lines
around the error handling block in validator.go; update the
validateIncomingRequest error branch so spacing follows wsl_v5 rules by
inserting a blank line after the requestErrorHandler(...) call (and/or between
the if !c.IsAborted() { ... } block and the subsequent return) to produce a
blank line separation around the error-handling statements involving
validateIncomingRequest, requestValidationInput, requestErrorHandler,
newContractError, c.IsAborted and c.Abort.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02a1220a-8972-49d9-bfd0-0ae36664899f
📒 Files selected for processing (2)
pkg/gin-openapi-validator/validator.gopkg/gin-openapi-validator/validator_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/gin-openapi-validator/validator.go`:
- Around line 111-135: The responseBodyWriter currently allows later validation
error handlers to replace/rewind responses even after Flush()/flush() has
forwarded bytes and cleared body; update the validation/error-handling paths to
check responseBodyWriter.flushed (and/or use a helper isCommitted) and, when
true, avoid attempting full replacement—only append/log or downgrade to
pass-through behavior. Specifically, add guards in the strict-rewrite branch and
the custom response-handler branch (the code that reads responseBodyWriter.body,
statusCode, and attempts to overwrite ResponseWriter) to short-circuit if
responseBodyWriter.flushed is true, and ensure any post-flush handling uses only
append/logging semantics rather than replacing status/body.
- Around line 28-37: prepareRewrite currently stomps the entire header map
(w.headers = make(http.Header)), which discards headers seeded by
cloneResponseBodyWriter and any headers set before the first Write/WriteHeader;
remove the assignment that replaces w.headers and instead preserve the existing
map (if you need to clear values, delete keys selectively or leave them intact)
so cloned/pass-through headers and pre-write custom headers remain; apply the
same change to the other similar block referenced at the 147-165 region to avoid
wiping the header map there as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f368f34-e6a4-4f40-9ab4-728fe3877a67
📒 Files selected for processing (2)
pkg/gin-openapi-validator/validator.gopkg/gin-openapi-validator/validator_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change makes the Gin OpenAPI validator middleware customizable at the points where validation failures are handled, and removes the package's direct dependency on
logrusin favor of the standard librarylog/sloglogger.Before this change, request validation failures always returned the package's built-in JSON error response, response validation failures always logged through
logrus, and strict response mode always overwrote invalid responses with a fixed 500 payload. That made the middleware hard to integrate into applications that already have their own error handling and logging conventions.The root cause was that the middleware embedded its request abort behavior and response logging/rewrite behavior directly inside
Validator, with no option hooks for callers to intervene. The response writer also wrote non-strict responses through immediately, which made it impossible for a custom outgoing validation handler to replace the response after validation had run.This update adds
Logger *slog.Logger,RequestErrorHandler func(*gin.Context, error), andResponseErrorHandler func(*gin.Context, error)toValidatorOptions. The default request behavior is preserved through a reusable default handler built onDecode. The default response behavior is preserved by logging invalid responses withslogwhen a logger is configured and by returning the existing 500 JSON body in strict mode. The response writer now buffers output until validation completes so a custom response error handler can either observe the failure or intentionally replace the response.The test suite was rewritten to remove
logrushooks and to cover the new customization paths, including custom request handlers, custom response handlers, strict-mode fallback behavior, and non-strict logging through a testsloglogger.Validation used for this change:
go test ./...golangci-lint runSummary by CodeRabbit
New Features
Documentation
Chores
Tests