Update validation error handling#3
Merged
phumberdroz merged 3 commits intomainfrom Mar 16, 2026
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRefactors golangci-lint config to a compact, versioned v2 setup; adds comprehensive README and new AGENTS.md; enhances Gin OpenAPI middleware to buffer responses and centralize validation error handling; refines validation error conversion logic; minor test formatting tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GinMiddleware as Gin Middleware
participant Validator as OpenAPI Validator
participant Handler
participant ResponseWriter
Client->>GinMiddleware: HTTP request
GinMiddleware->>Validator: route lookup + request validation
alt request validation fails
Validator-->>GinMiddleware: ValidationError
GinMiddleware->>ResponseWriter: write error response (abort)
ResponseWriter-->>Client: HTTP error
else request validation succeeds
GinMiddleware->>ResponseWriter: wrap response writer (buffer status/headers/body)
GinMiddleware->>Handler: c.Next() (invoke handler)
Handler->>ResponseWriter: write response (buffered)
GinMiddleware->>Validator: validate buffered response against spec
alt response validation fails
Validator-->>GinMiddleware: ValidationError
GinMiddleware->>ResponseWriter: write error response (abort/overwrite)
ResponseWriter-->>Client: HTTP error
else
GinMiddleware->>ResponseWriter: flush buffered status/headers/body
ResponseWriter-->>Client: send 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 Tip Migrating from UI to YAML configuration.Use the |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yaml:
- Line 37: Replace the deprecated linter name "wsl" with "wsl_v5" wherever it's
used and update its configuration keys: remove the invalid keys (strict-append,
allow-assign-and-call, allow-multiline-assign, allow-cuddle-declarations,
allow-trailing-comment, force-case-trailing-whitespace) and instead use
supported options for wsl_v5 such as allow-first-in-block, allow-whole-block,
branch-max-lines, case-max-lines, default, enable, and disable; locate the "wsl"
linter entries and their config blocks in the .golangci.yaml and map the
existing behaviors to the appropriate supported keys (or drop them) so the file
validates under golangci-lint v2.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.golangci.yaml (1)
37-37:⚠️ Potential issue | 🔴 CriticalSwitch this config from
wsltowsl_v5.Line 37 enables
wsl, and Lines 100-106 configure it with legacy keys, but in golangci-lint v2wslis deprecated, exposes no settings, and the configurable replacement iswsl_v5. As written, this block is unlikely to do what you expect and can break lint config validation in CI. (golangci-lint.run)golangci-lint v2 official docs: is `wsl` deprecated with no settings, and should `.golangci.yaml` use `wsl_v5` plus `linters.settings.wsl_v5` instead?Also applies to: 100-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yaml at line 37, Replace the deprecated linter key "wsl" with "wsl_v5" in the linter list and move/rename the existing configuration under "linters.settings.wsl" to "linters.settings.wsl_v5" (update any occurrences around the current block at lines ~100-106), ensuring the enabled linters list contains "wsl_v5" instead of "wsl" so golangci-lint v2 picks up the configurable settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.golangci.yaml:
- Line 37: Replace the deprecated linter key "wsl" with "wsl_v5" in the linter
list and move/rename the existing configuration under "linters.settings.wsl" to
"linters.settings.wsl_v5" (update any occurrences around the current block at
lines ~100-106), ensuring the enabled linters list contains "wsl_v5" instead of
"wsl" so golangci-lint v2 picks up the configurable settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0624768-26e3-4642-98d2-99aa2f381095
📒 Files selected for processing (3)
.golangci.yamlREADME.mdpkg/gin-openapi-validator/validationerror.go
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 (2)
pkg/gin-openapi-validator/validationerror.go (2)
193-212:⚠️ Potential issue | 🔴 CriticalScope the enum hint to query parameters before using
e.Parameter.Lines 205-212 dereference
e.Parameterunconditionally, but this block runs for both parameter and request-body enum errors. Whene.RequestBody != nil,e.Parameterisnil, causing a panic. Additionally, the?param=hint is only valid for query parameters, not request bodies or other parameter types.Suggested patch
- if (e.Parameter.Explode == nil || *e.Parameter.Explode) && - (e.Parameter.Style == "" || e.Parameter.Style == "form") && + if e.Parameter != nil && + e.Parameter.In == "query" && + (e.Parameter.Explode == nil || *e.Parameter.Explode) && + (e.Parameter.Style == "" || e.Parameter.Style == "form") && strings.Contains(value, ",") { parts := strings.Split(value, ",") cErr.Detail = cErr.Detail + "; " + fmt.Sprintf("perhaps you intended '?%s=%s'", e.Parameter.Name, strings.Join(parts, "&"+e.Parameter.Name+"=")) }🤖 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 193 - 212, The enum hint appends a query-style "?param=..." suggestion but dereferences e.Parameter even when the error is for a request body, causing panics; guard the whole hint logic by first checking e.Parameter != nil and that the parameter is a query parameter (e.Parameter.In == "query") before reading e.Parameter.Explode, e.Parameter.Style, or e.Parameter.Name, and only then perform the explode/style/comma check and append to cErr.Detail so request-body enum errors skip this query-only hint.
115-153:⚠️ Potential issue | 🔴 Critical
convertParseErrorhas three confirmed panic paths.The function initializes
cErras nil with no default switch case, and then dereferences it on line 151, causing a panic for any unhandledParseError.Additionally, two cases access
e.Parameterwithout guarding for nil:
- Line 129–132: The third case checks
innerErr.Reason != ""but doesn't verifye.Parameter != nilbefore accessinge.Parameter.Nameande.Parameter.In.- Line 137: The condition directly accesses
e.Parameter.Inwithout checking ife.Parameteris nil.Since
openapi3filter.RequestErrorcarries both parameter and request-body failures (withParameter == nilfor body errors), these unchecked accesses will panic on legitimate request-body parse failures.Suggested patch
func convertParseError(e *openapi3filter.RequestError, innerErr *openapi3filter.ParseError) *openapi3filter.ValidationError { - var cErr *openapi3filter.ValidationError + cErr := &openapi3filter.ValidationError{ + Status: http.StatusBadRequest, + Title: innerErr.Reason, + } // We treat path params of the wrong type like a 404 instead of a 400 switch { case innerErr.Kind == openapi3filter.KindInvalidFormat && e.Parameter != nil && e.Parameter.In == "path": cErr = &openapi3filter.ValidationError{ Status: http.StatusNotFound, Title: fmt.Sprintf("Resource not found with '%s' value: %v", e.Parameter.Name, innerErr.Value), } case strings.HasPrefix(innerErr.Reason, "unsupported content type"): cErr = &openapi3filter.ValidationError{ Status: http.StatusUnsupportedMediaType, Title: innerErr.Reason, } - case innerErr.Kind == openapi3filter.KindInvalidFormat && innerErr.Reason != "": + case innerErr.Kind == openapi3filter.KindInvalidFormat && e.Parameter != nil && innerErr.Reason != "": cErr = &openapi3filter.ValidationError{ Status: http.StatusBadRequest, Title: fmt.Sprintf("Parameter '%s' in %s is invalid: %v is %s", e.Parameter.Name, e.Parameter.In, innerErr.Value, innerErr.Reason), } case innerErr.RootCause() != nil: if rootErr, ok := innerErr.Cause.(*openapi3filter.ParseError); ok && - rootErr.Kind == openapi3filter.KindInvalidFormat && e.Parameter.In == "query" { + rootErr.Kind == openapi3filter.KindInvalidFormat && e.Parameter != nil && e.Parameter.In == "query" { cErr = &openapi3filter.ValidationError{ Status: http.StatusBadRequest, Title: fmt.Sprintf("Parameter '%s' in %s is invalid: %v is %s", e.Parameter.Name, e.Parameter.In, rootErr.Value, rootErr.Reason), } } else { cErr = &openapi3filter.ValidationError{ Status: http.StatusBadRequest, Title: innerErr.Reason, } } }🤖 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 115 - 153, convertParseError can panic because cErr is left nil if no switch branch matches and several branches access e.Parameter without nil checks; initialize cErr with a safe default ValidationError before the switch (e.g., Status BadRequest and generic Title) and add guards for e.Parameter != nil before referencing e.Parameter.Name or e.Parameter.In in the branches that build messages (specifically the branch handling innerErr.Kind==KindInvalidFormat && innerErr.Reason!="" and the branch inspecting rootErr.Kind && e.Parameter.In=="query"); when e.Parameter is nil, produce an appropriate body-parse error message instead of accessing fields. Ensure the final Title fallback uses the initialized cErr to avoid dereferencing nil.
🤖 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 69-74: The code currently asserts c.Writer.(*responseBodyWriter)
after calling c.Next(), which can panic if downstream middleware replaces
c.Writer; to fix, call wrapResponseWriter(c) and immediately capture the
returned or accessible responseBodyWriter instance (the wrapped writer created
by wrapResponseWriter) into a local variable before invoking c.Next(), then call
c.Next() and use that saved responseBodyWriter variable to read the buffered
body and any status instead of re-asserting c.Writer after c.Next(); update any
places that reference c.Writer to use the saved responseBodyWriter (look for
wrapResponseWriter, responseBodyWriter, c.Next, and c.Writer in validator.go).
- Around line 24-26: The responseBodyWriter currently defines Write with a value
receiver causing writes to go to a copy and not the actual buffer, and it lacks
WriteString so StringWriter paths bypass the buffer; change Write to have a
pointer receiver on the responseBodyWriter type and implement a WriteString(w
string) (int, error) method that writes to both w.body and delegates to the
embedded ResponseWriter (mirroring Write behavior), ensuring wrapResponseWriter
continues to assign a pointer (&responseBodyWriter{...}) so the pointer-receiver
methods operate on the same instance used for validation.
---
Outside diff comments:
In `@pkg/gin-openapi-validator/validationerror.go`:
- Around line 193-212: The enum hint appends a query-style "?param=..."
suggestion but dereferences e.Parameter even when the error is for a request
body, causing panics; guard the whole hint logic by first checking e.Parameter
!= nil and that the parameter is a query parameter (e.Parameter.In == "query")
before reading e.Parameter.Explode, e.Parameter.Style, or e.Parameter.Name, and
only then perform the explode/style/comma check and append to cErr.Detail so
request-body enum errors skip this query-only hint.
- Around line 115-153: convertParseError can panic because cErr is left nil if
no switch branch matches and several branches access e.Parameter without nil
checks; initialize cErr with a safe default ValidationError before the switch
(e.g., Status BadRequest and generic Title) and add guards for e.Parameter !=
nil before referencing e.Parameter.Name or e.Parameter.In in the branches that
build messages (specifically the branch handling
innerErr.Kind==KindInvalidFormat && innerErr.Reason!="" and the branch
inspecting rootErr.Kind && e.Parameter.In=="query"); when e.Parameter is nil,
produce an appropriate body-parse error message instead of accessing fields.
Ensure the final Title fallback uses the initialized cErr to avoid dereferencing
nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db1b6d9d-e6a7-417f-b01c-e121dd75e806
📒 Files selected for processing (5)
.golangci.yamlAGENTS.mdpkg/gin-openapi-validator/validationerror.gopkg/gin-openapi-validator/validator.gopkg/gin-openapi-validator/validator_test.go
✅ Files skipped from review due to trivial changes (2)
- AGENTS.md
- pkg/gin-openapi-validator/validator_test.go
Comment on lines
+24
to
+26
| func wrapResponseWriter(c *gin.Context) { | ||
| c.Writer = &responseBodyWriter{body: &bytes.Buffer{}, ResponseWriter: c.Writer} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the validator.go file
find . -type f -name "validator.go" | grep -E "(gin-openapi|validator)" | head -20Repository: phumberdroz/gin-openapi-validator
Length of output: 117
🏁 Script executed:
# Look at the structure of the repository
git ls-files | grep -E "validator\.go|gin-openapi" | head -20Repository: phumberdroz/gin-openapi-validator
Length of output: 277
🏁 Script executed:
# Get the full content of the validator.go file (or at least the beginning)
find . -type f -name "validator.go" -exec wc -l {} \;Repository: phumberdroz/gin-openapi-validator
Length of output: 121
🏁 Script executed:
# Read the complete validator.go file
cat -n ./pkg/gin-openapi-validator/validator.goRepository: phumberdroz/gin-openapi-validator
Length of output: 3489
🏁 Script executed:
# Search for WriteString usage in the codebase
rg "WriteString" --type goRepository: phumberdroz/gin-openapi-validator
Length of output: 59
🏁 Script executed:
# Check what gin.ResponseWriter interface includes by searching docs or references
rg "ResponseWriter.*WriteString|WriteString.*ResponseWriter" --type goRepository: phumberdroz/gin-openapi-validator
Length of output: 59
🏁 Script executed:
# Check if there are any tests that might reveal WriteString behavior
cat -n ./pkg/gin-openapi-validator/validator_test.goRepository: phumberdroz/gin-openapi-validator
Length of output: 7368
🏁 Script executed:
# Check if gin.ResponseWriter has WriteString or other write methods
# Search in go.mod to find gin-gonic version
cat -n go.mod | head -30Repository: phumberdroz/gin-openapi-validator
Length of output: 363
🏁 Script executed:
# Search for any response writing patterns in the codebase
rg "Writer\." --type go -A 2 -B 2 | head -100Repository: phumberdroz/gin-openapi-validator
Length of output: 1184
🏁 Script executed:
# Check if there are any direct assignments or calls to ResponseWriter methods
rg "c\.Writer" --type goRepository: phumberdroz/gin-openapi-validator
Length of output: 441
🏁 Script executed:
# Let's check if gin.ResponseWriter or its implementations use WriteString
# First, let's see what methods gin.ResponseWriter should have
python3 << 'EOF'
# Based on Go documentation, http.ResponseWriter interface has:
# - Header() http.Header
# - Write([]byte) (int, error)
# - WriteHeader(statusCode int)
# But concrete implementations often have WriteString from io.StringWriter interface
# which is: WriteString(s string) (int, error)
# Let's check what the actual issue is:
print("Analysis:")
print("1. gin.ResponseWriter embeds http.ResponseWriter")
print("2. http.ResponseWriter doesn't define WriteString in the interface")
print("3. BUT concrete implementations (like http.response) implement io.StringWriter")
print("4. So calling WriteString on the embedded writer would work")
print("5. If responseBodyWriter only overrides Write(), WriteString calls skip the buffer")
EOFRepository: phumberdroz/gin-openapi-validator
Length of output: 424
🏁 Script executed:
# Check if there are any uses of WriteString in the codebase or test files
rg "WriteString\|write.*string" --type go -iRepository: phumberdroz/gin-openapi-validator
Length of output: 59
🏁 Script executed:
# Let's also verify the actual issue by checking if responseBodyWriter properly implements the interface
# by looking at what methods it should have
python3 << 'EOF'
import re
# Read the validator.go file
with open('./pkg/gin-openapi-validator/validator.go', 'r') as f:
content = f.read()
# Find the responseBodyWriter definition
print("Current responseBodyWriter implementation:")
print("=" * 60)
for match in re.finditer(r'type responseBodyWriter struct \{.*?\n\}', content, re.DOTALL):
print(match.group())
print()
# Find all methods on responseBodyWriter
print("Methods on responseBodyWriter:")
print("=" * 60)
for match in re.finditer(r'func \(w.*?responseBodyWriter.*?\) \w+\(', content):
print(match.group())
EOFRepository: phumberdroz/gin-openapi-validator
Length of output: 383
🏁 Script executed:
# Let's check what methods gin.ResponseWriter and http.ResponseWriter actually have
# by looking at what the concrete ResponseWriter types support
python3 << 'EOF'
# According to Go's io package, io.StringWriter is:
# interface {
# WriteString(s string) (n int, err error)
# }
#
# The concrete http.response type does implement WriteString
# Gin's ResponseWriter embeds http.ResponseWriter and adds methods like Status(), Header(), etc.
# But it doesn't explicitly define WriteString in the interface
# The issue is:
# - responseBodyWriter embeds gin.ResponseWriter (which embeds http.ResponseWriter)
# - If code calls WriteString on the underlying http.ResponseWriter,
# it bypasses the overridden Write() method
# - The current code only overrides Write(), so WriteString calls skip the buffer
print("Issue confirmed:")
print("1. responseBodyWriter should intercept ALL write methods")
print("2. Currently only Write() is intercepted")
print("3. WriteString() on the embedded ResponseWriter would bypass the buffer")
print("4. This would cause validation to see empty body")
print()
print("The fix requirements:")
print("1. Add WriteString() method to capture string writes")
print("2. Change receiver to *responseBodyWriter for consistency (already assigning pointer)")
EOFRepository: phumberdroz/gin-openapi-validator
Length of output: 474
🏁 Script executed:
# Let's verify if the receiver type change is necessary by checking the current assignment
python3 << 'EOF'
with open('./pkg/gin-openapi-validator/validator.go', 'r') as f:
lines = f.readlines()
# Line 25 is the assignment
print("Line 25 assignment:")
print(lines[24])
# Line 19 is the Write method receiver
print("\nLine 19 receiver:")
print(lines[18])
print("\nAnalysis:")
print("- Line 25 assigns a POINTER: &responseBodyWriter{...}")
print("- Line 19 uses VALUE receiver: func (w responseBodyWriter)")
print("- This is inconsistent and problematic:")
print(" - Value receiver means the method gets a copy of the struct")
print(" - Modifying w.body in the copy doesn't affect the original")
print(" - Need pointer receiver to modify the actual embedded buffer")
EOFRepository: phumberdroz/gin-openapi-validator
Length of output: 607
Add WriteString method and fix receiver type to pointer.
The responseBodyWriter has a critical bug: it uses a value receiver on line 19, but assigns a pointer on line 25. This means the Write() method receives a copy of the struct, so writes to w.body never reach the actual buffer that validation reads from on line 84. Additionally, the wrapper only intercepts Write(); anything using WriteString() (which http.ResponseWriter implementations support via io.StringWriter) bypasses the buffer entirely. Both issues cause validation to silently see an empty body and skip response validation.
Suggested patch
-func (w responseBodyWriter) Write(b []byte) (int, error) {
- w.body.Write(b)
+func (w *responseBodyWriter) Write(b []byte) (int, error) {
+ _, _ = w.body.Write(b)
return w.ResponseWriter.Write(b)
}
+
+func (w *responseBodyWriter) WriteString(s string) (int, error) {
+ _, _ = w.body.WriteString(s)
+ return w.ResponseWriter.WriteString(s)
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/gin-openapi-validator/validator.go` around lines 24 - 26, The
responseBodyWriter currently defines Write with a value receiver causing writes
to go to a copy and not the actual buffer, and it lacks WriteString so
StringWriter paths bypass the buffer; change Write to have a pointer receiver on
the responseBodyWriter type and implement a WriteString(w string) (int, error)
method that writes to both w.body and delegates to the embedded ResponseWriter
(mirroring Write behavior), ensuring wrapResponseWriter continues to assign a
pointer (&responseBodyWriter{...}) so the pointer-receiver methods operate on
the same instance used for validation.
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.
Summary by CodeRabbit
Documentation
Chores
Improvements
Tests