Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds configurable OpenAPI response validation: new server config and modes ( ChangesResponse Validation
sequenceDiagram
participant Client as Client
participant Middleware as Middleware
participant Router as Router
participant Handler as Handler
participant Validator as Validator
participant ErrorHook as ErrorHook
Client->>Middleware: send HTTP request
Middleware->>Handler: invoke handler (buffer response)
Handler-->>Middleware: buffered status + body
Middleware->>Router: find matching route
Router-->>Middleware: route
Middleware->>Middleware: call RouteFilterHook(route)
alt RouteFilterHook returns false
Middleware-->>Client: write original buffered response
else
Middleware->>Validator: validate buffered response against route
alt validation fails
Validator-->>ErrorHook: report validation error
ErrorHook-->>Middleware: hook executed
Middleware-->>Client: write original buffered response
else
Middleware-->>Client: write original buffered response
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
rolosp
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v3/server/server.go (1)
70-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast on invalid response-validation modes.
This new public knob is added to
Config, but it still bypassesConfig.Validate(). A typo that reaches this constructor can be treated as enabled downstream instead of rejecting the config.🔧 Suggested fix
func (c *Config) Validate() error { var errs []error + + if err := c.ResponseValidation.Mode.Validate(); err != nil { + errs = append(errs, err) + } if c.BaseURL == "" { errs = append(errs, errors.New("base URL is required")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/server/server.go` around lines 70 - 77, The ResponseValidation field on Config is currently accepted without being checked; ensure the config is validated at construction by calling Config.Validate() (or equivalent validation helper) from the server constructor (e.g., wherever a new server is created) and return an error if validation fails so typos in ResponseValidation are rejected; specifically, add a validation call that inspects Config.ResponseValidation via Config.Validate() before using it in server initialization.
🧹 Nitpick comments (1)
api/v3/oasmiddleware/validator_test.go (1)
23-31: Swap tot.Context()here and at lines 62 and 98.These test setups should use the test context instead of
context.Background(). It ties the router lifecycle to the test harness and matches the pattern already used throughout the codebase.Suggested refactor
- router, err := oasmiddleware.NewValidationRouter(context.Background(), swagger, &oasmiddleware.ValidationRouterOpts{ + router, err := oasmiddleware.NewValidationRouter(t.Context(), swagger, &oasmiddleware.ValidationRouterOpts{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/oasmiddleware/validator_test.go` around lines 23 - 31, Replace context.Background() with the test's cancellable context by using t.Context() when calling NewValidationRouter in this test; specifically update the NewValidationRouter invocation that currently passes context.Background() (and the two other occurrences in the same test where context.Background() is used) so the router lifecycle is tied to the test harness. Ensure you only change the context argument passed into oasmiddleware.NewValidationRouter and leave the rest of the ValidationRouterOpts (e.g., DeleteServers) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/v3/server/server.go`:
- Around line 70-77: The ResponseValidation field on Config is currently
accepted without being checked; ensure the config is validated at construction
by calling Config.Validate() (or equivalent validation helper) from the server
constructor (e.g., wherever a new server is created) and return an error if
validation fails so typos in ResponseValidation are rejected; specifically, add
a validation call that inspects Config.ResponseValidation via Config.Validate()
before using it in server initialization.
---
Nitpick comments:
In `@api/v3/oasmiddleware/validator_test.go`:
- Around line 23-31: Replace context.Background() with the test's cancellable
context by using t.Context() when calling NewValidationRouter in this test;
specifically update the NewValidationRouter invocation that currently passes
context.Background() (and the two other occurrences in the same test where
context.Background() is used) so the router lifecycle is tied to the test
harness. Ensure you only change the context argument passed into
oasmiddleware.NewValidationRouter and leave the rest of the ValidationRouterOpts
(e.g., DeleteServers) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60d78ef9-3cdd-4f53-97b8-98cd8d76ff64
📒 Files selected for processing (8)
api/v3/oasmiddleware/validator.goapi/v3/oasmiddleware/validator_test.goapi/v3/server/server.goapp/config/config_test.goapp/config/server.gocmd/server/main.goconfig.example.yamlopenmeter/server/server.go
Collaborator
Author
No change needed. Configuration.Validate() is already |
rolosp
force-pushed
the
feat/oapi-codegen-response-validator
branch
from
ddab792 to
43a4ecb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v3/server/server.go`:
- Around line 393-395: The ResponseValidationErrorHook currently logs the raw
validation error (err) via slog.WarnContext which can leak sensitive response
fields; update the handler for ResponseValidationErrorHook to log only a
sanitized summary (HTTP method, path, response status or a short error
category/string) at warning level and remove slog.Any("error", err) from the
warn entry, and if you need full details send the original err to
slog.DebugContext or behind an explicit opt-in/redaction helper so raw error
contents are not written to standard warning logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6df4e6d-29d4-4571-9fce-754c8646a304
📒 Files selected for processing (8)
api/v3/oasmiddleware/validator.goapi/v3/oasmiddleware/validator_test.goapi/v3/server/server.goapp/config/config_test.goapp/config/server.gocmd/server/main.goconfig.example.yamlopenmeter/server/server.go
✅ Files skipped from review due to trivial changes (2)
- app/config/config_test.go
- api/v3/oasmiddleware/validator_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- app/config/server.go
- config.example.yaml
- openmeter/server/server.go
- api/v3/oasmiddleware/validator.go
Comment on lines
+393
to
+395
| ResponseValidationErrorHook: func(err error, r *http.Request) { | ||
| slog.WarnContext(r.Context(), "response validation failed", slog.String("method", r.Method), slog.String("path", r.URL.Path), slog.Any("error", err)) | ||
| }, |
Contributor
There was a problem hiding this comment.
Please avoid logging raw validation errors here (possible sensitive response data leak).
err from response validation can include offending response fields; logging it verbatim at warning level can leak PII/secrets into logs. Consider logging a sanitized summary (method/path/status + error category) and keep raw details behind debug or explicit opt-in.
Suggested minimal hardening
- ResponseValidationErrorHook: func(err error, r *http.Request) {
- slog.WarnContext(r.Context(), "response validation failed", slog.String("method", r.Method), slog.String("path", r.URL.Path), slog.Any("error", err))
- },
+ ResponseValidationErrorHook: func(err error, r *http.Request) {
+ slog.WarnContext(
+ r.Context(),
+ "response validation failed",
+ slog.String("method", r.Method),
+ slog.String("path", r.URL.Path),
+ slog.String("error_type", fmt.Sprintf("%T", err)),
+ )
+ // Optional (debug-only): log full details in controlled environments.
+ // slog.DebugContext(r.Context(), "response validation details", slog.Any("error", err))
+ },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v3/server/server.go` around lines 393 - 395, The
ResponseValidationErrorHook currently logs the raw validation error (err) via
slog.WarnContext which can leak sensitive response fields; update the handler
for ResponseValidationErrorHook to log only a sanitized summary (HTTP method,
path, response status or a short error category/string) at warning level and
remove slog.Any("error", err) from the warn entry, and if you need full details
send the original err to slog.DebugContext or behind an explicit
opt-in/redaction helper so raw error contents are not written to standard
warning logs.
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
2 participants
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.
Overview
Adds optional v3 response body validation against the OpenAPI spec. Violations are
logged as warnings; client responses are never affected.
Configured via server.responseValidation.mode:
The mode enum mirrors the existing AutoMigrate pattern in app/config/postgres.go.
Notes for reviewer
rollout target. all is fine for staging but watch memory on large list pages.
as boolean false by YAML 1.1.
fields. Acceptable for observability but worth knowing.
Summary by CodeRabbit
New Features
Configuration
Tests