Conversation
|
📝 WalkthroughWalkthroughAdds per-request body-size enforcement and caching, session-level ClickHouse logging control, int64 CLI flag support with wiring of MaxRequestBodySize, removes two usagelimiter Prometheus metrics and their increments, introduces a 413 error code/path and docs, and updates tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Session
participant Handler
Note over Server,Session: Request handling with body-size enforcement & cached body
Client->>Server: HTTP request (with body)
Server->>Session: create session
Server->>Session: init(w, r, maxBodySize)
alt maxBodySize > 0
Session->>Session: wrap r.Body with http.MaxBytesReader
end
Session->>Session: read full body into s.requestBody (or return 413 on MaxBytesError)
Server->>Handler: invoke Handler.Handle(sess)
Note right of Handler: Handler may call sess.DisableClickHouseLogging() early
Handler->>Session: sess.BindBody(...) reads from cached s.requestBody
Handler-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
go/pkg/zen/session.go (1)
156-171: BindBody re-reads and consumes Body; reuse cached body and reset for downstream consumersinit() already caches the body and installs a new reader. BindBody re-reading consumes it again and closes it, breaking the “safe re-read” guarantee and doing extra work.
Refactor to use the cached body if present, read-and-cache only if necessary, and always reset Body afterwards:
func (s *Session) BindBody(dst any) error { - var err error - s.requestBody, err = io.ReadAll(s.r.Body) - if err != nil { - return fault.Wrap(err, fault.Internal("unable to read request body"), fault.Public("The request body is malformed.")) - } - defer s.r.Body.Close() - - err = json.Unmarshal(s.requestBody, dst) + // Prefer the cached body populated by init(); if absent, read and cache now. + if len(s.requestBody) == 0 { + orig := s.r.Body + defer orig.Close() + b, err := io.ReadAll(s.r.Body) + if err != nil { + return fault.Wrap(err, fault.Internal("unable to read request body"), fault.Public("The request body is malformed.")) + } + s.requestBody = b + } + + err := json.Unmarshal(s.requestBody, dst) if err != nil { return fault.Wrap(err, fault.Internal("failed to unmarshal request body"), fault.Public("The request body was not valid json."), ) } + // Reset Body so later middleware/handlers can read it again. + s.r.Body = io.NopCloser(bytes.NewReader(s.requestBody)) return nil }go/pkg/cli/command.go (1)
297-301: Exit signature returns error; broader cleanup may be warranted.Not a blocker here, but returning error from Exit forces
_ = Exit(...)or nolints at call sites (errcheck). Consider changing signature to not return error and panic after ExitFunc(code) for unreachable paths, in a separate PR.I can draft the cross-file refactor to make Exit non-error-returning and update call sites.
go/pkg/zen/session_bind_body_test.go (1)
94-111: Retain and assert stored raw body even on error cases.For cases that expect BindBody to fail (malformed/non-object/empty), it would be valuable to still assert sess.requestBody captured the original bytes after init to protect the “bind body at start of request” behavior.
I can add assertions for error paths to verify requestBody is preserved.
go/pkg/zen/server.go (1)
254-259: Log the init error details for debuggabilityWe swallow the specific reason for init failure (e.g., body too large). Include the error in logs. Assuming sess.init handles writing the response (e.g., 413) when it fails, logging the error won’t risk double writes.
Apply this diff:
- err := sess.init(w, r, s.config.MaxRequestBodySize) - if err != nil { - s.logger.Error("failed to init session") - return - } + err := sess.init(w, r, s.config.MaxRequestBodySize) + if err != nil { + s.logger.Error("failed to init session", "error", err.Error()) + return + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (20)
go/apps/api/config.go(1 hunks)go/apps/api/routes/chproxy_metrics/handler.go(1 hunks)go/apps/api/routes/chproxy_ratelimits/handler.go(1 hunks)go/apps/api/routes/chproxy_verifications/handler.go(1 hunks)go/apps/api/routes/openapi/handler.go(1 hunks)go/apps/api/routes/reference/handler.go(1 hunks)go/apps/api/run.go(1 hunks)go/cmd/api/main.go(2 hunks)go/internal/services/usagelimiter/limit.go(0 hunks)go/internal/services/usagelimiter/redis.go(0 hunks)go/pkg/cli/command.go(2 hunks)go/pkg/cli/flag.go(7 hunks)go/pkg/prometheus/metrics/usagelimiter.go(0 hunks)go/pkg/zen/auth_test.go(1 hunks)go/pkg/zen/middleware_metrics.go(1 hunks)go/pkg/zen/server.go(5 hunks)go/pkg/zen/session.go(4 hunks)go/pkg/zen/session_bind_body_test.go(4 hunks)go/pkg/zen/session_bind_query_test.go(1 hunks)go/pkg/zen/session_body_limit_test.go(1 hunks)
💤 Files with no reviewable changes (3)
- go/internal/services/usagelimiter/limit.go
- go/pkg/prometheus/metrics/usagelimiter.go
- go/internal/services/usagelimiter/redis.go
🧰 Additional context used
🪛 golangci-lint (2.2.2)
go/pkg/zen/session_body_limit_test.go
49-49: "POST" can be replaced by http.MethodPost
(usestdlibvars)
74-74: "POST" can be replaced by http.MethodPost
(usestdlibvars)
go/pkg/cli/flag.go
585-585: Error return value is not checked
(errcheck)
🔇 Additional comments (23)
go/pkg/zen/session.go (1)
44-46: Per-request logging controls: LGTMdisableLogging flag, DisableLogging(), ShouldLog(), and reset() behavior look consistent and safe. Good defaults and documentation.
Also applies to: 86-99, 405-406
go/pkg/zen/auth_test.go (1)
106-107: LGTM: init() signature update reflected in testTestBearer_Integration now initializes Session with the third arg. No further issues.
go/pkg/zen/session_bind_query_test.go (1)
256-259: LGTM: test updated for init() signatureThe test now calls sess.init(w, req, 0) and remains otherwise unchanged. Looks good.
go/pkg/cli/flag.go (6)
15-17: New int64 error kind is appropriate and consistent.Adding ErrInvalidInt64Value aligns with existing error taxonomy (bool/int/float). No issues.
154-160: Int64Flag implementation mirrors existing flags correctly.
- Parse uses strconv.ParseInt base 10, 64 bits — correct for CLI.
- Validation runs on the raw string consistently with other types.
- Value/HasValue semantics match Int/Float flags.
One minor gotcha: HasValue returns false for an explicitly provided 0 unless it came from env. If any consumer needs to distinguish “provided as 0” vs “unset,” rely on IsSet().
Please confirm that any logic consuming MaxRequestBodySize uses IsSet() or treats 0 as a meaningful sentinel and does not depend solely on HasValue().
Also applies to: 161-178, 180-185
266-284: Required() option integration covers Int64Flag.Switch case extended properly. No issues.
286-304: EnvVar() option integration covers Int64Flag.Consistent with other flags. No issues.
306-324: Validate() option integration covers Int64Flag.Consistent with other flags. No issues.
349-355: Default() gains int64 support; type safety is preserved.Type assertion to int64 is strict and matches pattern used elsewhere. Looks good.
go/pkg/cli/command.go (3)
128-137: Int64 getter follows established pattern.Returns 0 on missing/wrong type, matching Int()/Float(). LGTM.
139-154: RequireInt64 mirrors existing Require behavior.*Panics with structured errors on missing/type mismatch; returns Value() otherwise. LGTM.
242-252: getFlagType extended for Int64Flag.Ensures error messages are clear on type mismatches. LGTM.
go/pkg/zen/session_bind_body_test.go (2)
166-169: Large body test remains valid with new init flow.This ensures BindBody continues to work with sizable JSON under default limit behavior. LGTM.
190-193: Integration test updated correctly.Init + BindBody sequence is clear and mirrors real server setup. LGTM.
go/apps/api/routes/chproxy_metrics/handler.go (1)
35-37: Disabling logging at entry is the right call for internal ingestion.Calling s.DisableLogging() before auth and body read prevents logs/metrics for this route as intended.
Confirm middleware gating for logging/metrics checks Session.ShouldLog() so this takes effect consistently across the stack.
go/apps/api/routes/chproxy_verifications/handler.go (1)
35-37: Per-request logging disabled early: LGTM.Consistent with other chproxy routes; reduces noise and risk of sensitive payloads in logs.
go/apps/api/routes/reference/handler.go (1)
30-31: Disabling per-request logging here is correct for a static/reference routeCalling s.DisableLogging() at the very start ensures this endpoint won’t be logged to ClickHouse. This matches the PR’s per-route logging objective and avoids unnecessary noise for a static page.
go/apps/api/routes/chproxy_ratelimits/handler.go (1)
36-37: Good: disable logging for internal CH proxy ingestionTurning off request logging for this internal ingestion route avoids duplicative/recursive ClickHouse entries and keeps logs clean, especially since you already record explicit metrics for this path.
go/apps/api/routes/openapi/handler.go (1)
29-30: LGTM: aligns with per-route logging controlDisabling logs for serving the OpenAPI spec makes sense to prevent large/low-value logs for a static asset and is consistent with other routes in this PR.
go/apps/api/run.go (1)
148-150: Wiring MaxRequestBodySize into zen.New is correctThe server now receives the configured body size limit via zen.Config, which aligns with the PR’s body-limit objective.
go/pkg/zen/server.go (3)
31-31: LGTM: Server now stores full ConfigReplacing the dedicated tlsConfig field with a single Config simplifies wiring and future extensions (like body size limit).
110-110: LGTM: Config plumbed into ServerAssigning the full config at construction time is clean; there’s no concurrent mutation risk since it’s immutable thereafter.
204-208: TLS wiring is correct and safer than passing cert pathsUsing s.config.TLS and assigning to s.srv.TLSConfig before ServeTLS is the right approach for in-memory certs and dynamic TLS config.
|
Thank you for following the naming conventions for pull request titles! 🙏 |
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)
go/pkg/zen/session.go (1)
176-190: Avoid re-reading and re-closing Body in BindBody; reuse cached bodyBindBody currently re-reads and closes s.r.Body even though init already cached the body and replaced s.r.Body. Prefer using the cached bytes, falling back to reading only if absent.
func (s *Session) BindBody(dst any) error { - var err error - s.requestBody, err = io.ReadAll(s.r.Body) - if err != nil { - return fault.Wrap(err, fault.Internal("unable to read request body"), fault.Public("The request body is malformed.")) - } - defer s.r.Body.Close() - - err = json.Unmarshal(s.requestBody, dst) + var err error + var data []byte + if s.requestBody != nil { + data = s.requestBody + } else { + data, err = io.ReadAll(s.r.Body) + if err != nil { + return fault.Wrap(err, fault.Internal("unable to read request body"), fault.Public("The request body is malformed.")) + } + defer s.r.Body.Close() + // Cache for downstream consumers/metrics + s.requestBody = data + } + + err = json.Unmarshal(data, dst) if err != nil { return fault.Wrap(err, fault.Internal("failed to unmarshal request body"), fault.Public("The request body was not valid json."), ) } return nil }
♻️ Duplicate comments (3)
go/pkg/zen/session_body_limit_test.go (2)
3-12: Use stdlib constants for HTTP methods (http.MethodPost)Replace raw "POST" literals; add missing net/http import.
@@ -import ( - "fmt" - "net/http/httptest" - "strings" - "testing" +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" @@ - req := httptest.NewRequest("POST", "/", strings.NewReader(tt.bodyContent)) + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(tt.bodyContent)) @@ - req := httptest.NewRequest("POST", "/", strings.NewReader(bodyContent)) + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(bodyContent)) @@ - req := httptest.NewRequest("POST", "/", strings.NewReader(bodyContent)) + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(bodyContent))Also applies to: 51-53, 76-78, 134-136
3-12: Prefer type assertion over substring matching for size-limit errorsUse errors.As to assert *http.MaxBytesError instead of brittle substring checks; add errors and net/http imports.
@@ -import ( - "fmt" - "net/http/httptest" - "strings" - "testing" +import ( + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" @@ - if tt.wantErr { - require.Error(t, err) - if tt.errSubstr != "" { - assert.Contains(t, err.Error(), tt.errSubstr) - } - return - } + if tt.wantErr { + require.Error(t, err) + var mbe *http.MaxBytesError + if errors.As(err, &mbe) { + // ok: body exceeded limit + } else if tt.errSubstr != "" { + assert.Contains(t, err.Error(), tt.errSubstr) + } + return + }Also applies to: 58-64
go/pkg/zen/session.go (1)
59-66: Optional: skip eager body read for clearly body-less requestsMicro-optimization: avoid allocations/syscalls when method is GET/HEAD and Content-Length is 0 with no Transfer-Encoding. Behavior for edge cases remains unchanged.
- // Read and cache the request body so metrics middleware can access it even on early errors. - // We need to replace r.Body with a fresh reader afterwards so other middleware - // can still read the body if necessary. - var err error - s.requestBody, err = io.ReadAll(s.r.Body) - closeErr := s.r.Body.Close() + // Read and cache the request body so metrics middleware can access it even on early errors. + // For clearly body-less requests, skip reading to save work. + if s.r.ContentLength == 0 && + (s.r.Method == http.MethodGet || s.r.Method == http.MethodHead) && + s.r.Header.Get("Transfer-Encoding") == "" { + s.requestBody = nil + // No replacement needed; keep existing Body as-is. + return nil + } + + var err error + s.requestBody, err = io.ReadAll(s.r.Body) + closeErr := s.r.Body.Close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (3)
go/cmd/api/main.go(2 hunks)go/pkg/zen/session.go(4 hunks)go/pkg/zen/session_body_limit_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/pkg/zen/session_body_limit_test.gogo/cmd/api/main.gogo/pkg/zen/session.go
**/*_test.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*_test.go: Use table-driven tests in Go
Organize Go integration tests with real dependencies
Organize Go tests by HTTP status codes
Files:
go/pkg/zen/session_body_limit_test.go
**/*.{env,js,ts,go}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/pkg/zen/session_body_limit_test.gogo/cmd/api/main.gogo/pkg/zen/session.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: Flo4604
PR: unkeyed/unkey#3810
File: go/apps/api/config.go:82-85
Timestamp: 2025-08-20T08:23:05.076Z
Learning: The zen framework's MaxRequestBodySize defaults to 0 (no limit) for flexibility, while individual applications using zen (like the API) set their own defaults through CLI flags. This is intentional architectural layering.
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Organize Go tests by HTTP status codes
Applied to files:
go/pkg/zen/session_body_limit_test.go
🧬 Code Graph Analysis (3)
go/pkg/zen/session_body_limit_test.go (3)
go/pkg/zen/session.go (1)
Session(30-47)go/pkg/zen/request_util.go (1)
BindBody(10-22)go/pkg/fault/wrapped.go (1)
UserFacingMessage(161-195)
go/cmd/api/main.go (1)
go/pkg/cli/flag.go (3)
Int64(565-602)Default(327-373)EnvVar(287-304)
go/pkg/zen/session.go (4)
go/pkg/zen/server.go (1)
New(75-126)go/pkg/uid/uid.go (1)
New(94-124)go/pkg/fault/wrapped.go (1)
New(52-66)go/pkg/fault/wrap.go (1)
Wrap(25-67)
🪛 golangci-lint (2.2.2)
go/pkg/zen/session_body_limit_test.go
51-51: "POST" can be replaced by http.MethodPost
(usestdlibvars)
76-76: "POST" can be replaced by http.MethodPost
(usestdlibvars)
134-134: "POST" can be replaced by http.MethodPost
(usestdlibvars)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
🔇 Additional comments (3)
go/cmd/api/main.go (1)
154-156: Wiring of MaxRequestBodySize looks correctThe flag is propagated into api.Config via cmd.Int64(...) and aligns with downstream zen behavior (≤0 disables). Good.
go/pkg/zen/session_body_limit_test.go (1)
96-147: Good coverage of public error messages for varied limitsAsserting fault.UserFacingMessage(err) against the exact "max allowed size of X bytes" string ensures stable UX. Nice.
go/pkg/zen/session.go (1)
105-118: Per-request logging control is clean and self-explanatoryDisableLogging/ShouldLog are documented, default to logging, and reset in reset(). Looks good.
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
Can we add a test that an exceeded body size results in a clean 400, right now I think we'd return a 500 cause the session's init method failed
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (4)
go/pkg/zen/session_body_limit_test.go (2)
54-54: Prefer stdlib HTTP method constantsUse http.MethodPost instead of string literals for clarity and lint compliance.
- req := httptest.NewRequest("POST", "/", strings.NewReader(tt.bodyContent)) + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(tt.bodyContent))- req := httptest.NewRequest("POST", "/", strings.NewReader(bodyContent)) + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(bodyContent))- req := httptest.NewRequest("POST", "/", strings.NewReader(bodyContent)) + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(bodyContent))- testRoute := NewRoute("POST", "/test", func(ctx context.Context, s *Session) error { + testRoute := NewRoute(http.MethodPost, "/test", func(ctx context.Context, s *Session) error {- req := httptest.NewRequest("POST", "/test", strings.NewReader(bodyContent)) + req := httptest.NewRequest(http.MethodPost, "/test", strings.NewReader(bodyContent))- req := httptest.NewRequest("POST", "/", strings.NewReader("test")) + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("test"))Also applies to: 79-79, 137-137, 166-166, 180-180, 197-197
61-66: Assert on http.MaxBytesError instead of brittle substring matchSubstrings are brittle across Go versions/localizations. Prefer type assertion via errors.As to detect body-size limit failures.
- if tt.wantErr { - require.Error(t, err) - if tt.errSubstr != "" { - assert.Contains(t, err.Error(), tt.errSubstr) - } - return - } + if tt.wantErr { + require.Error(t, err) + var mbe *http.MaxBytesError + // Prefer the concrete type when applicable + if errors.As(err, &mbe) { + // OK + } else if tt.errSubstr != "" { + assert.Contains(t, err.Error(), tt.errSubstr) + } + return + }Add missing import:
import ( + "errors" "context" "fmt" "net/http"go/pkg/zen/session.go (2)
86-92: Typo in comment: “incase” → “in case”Fix the spelling.
Apply:
- // Handle close error (incase that ever happens) + // Handle close error (in case that ever happens)
61-64: Optional micro-optimization: skip reading/caching for clearly body-less requestsSkip the read/cache path for GET/HEAD with Content-Length == 0 and no Transfer-Encoding to avoid allocs and syscalls on hot paths.
Apply (and ensure WorkspaceID remains initialized before early return):
@@ - // Read and cache the request body so metrics middleware can access it even on early errors. + // Fast-path: skip reading for clearly body-less requests + if (s.r.Method == http.MethodGet || s.r.Method == http.MethodHead) && + s.r.ContentLength == 0 && + s.r.Header.Get("Transfer-Encoding") == "" { + // Nothing to cache; leave Body as-is (likely http.NoBody) + s.requestBody = nil + s.WorkspaceID = "" + return nil + } + + // Read and cache the request body so metrics middleware can access it even on early errors.If you adopt this, also adjust BindBody to treat nil/empty equally (see separate comment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (16)
go/apps/api/routes/chproxy_metrics/handler.go(1 hunks)go/apps/api/routes/chproxy_ratelimits/handler.go(1 hunks)go/apps/api/routes/chproxy_verifications/handler.go(1 hunks)go/apps/api/routes/openapi/handler.go(1 hunks)go/apps/api/routes/reference/handler.go(1 hunks)go/apps/api/routes/v2_liveness/handler.go(1 hunks)go/apps/api/routes/v2_ratelimit_limit/handler.go(3 hunks)go/cmd/api/main.go(2 hunks)go/pkg/codes/constants_gen.go(1 hunks)go/pkg/codes/user_request.go(2 hunks)go/pkg/zen/middleware_errors.go(1 hunks)go/pkg/zen/middleware_metrics.go(1 hunks)go/pkg/zen/request_util.go(1 hunks)go/pkg/zen/server.go(5 hunks)go/pkg/zen/session.go(6 hunks)go/pkg/zen/session_body_limit_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/apps/api/routes/openapi/handler.gogo/apps/api/routes/v2_liveness/handler.gogo/apps/api/routes/chproxy_ratelimits/handler.gogo/pkg/zen/request_util.gogo/apps/api/routes/chproxy_metrics/handler.gogo/apps/api/routes/chproxy_verifications/handler.gogo/pkg/zen/middleware_errors.gogo/apps/api/routes/reference/handler.gogo/pkg/codes/constants_gen.gogo/cmd/api/main.gogo/pkg/zen/session_body_limit_test.gogo/apps/api/routes/v2_ratelimit_limit/handler.gogo/pkg/zen/server.gogo/pkg/zen/middleware_metrics.gogo/pkg/codes/user_request.gogo/pkg/zen/session.go
**/*.{env,js,ts,go}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/apps/api/routes/openapi/handler.gogo/apps/api/routes/v2_liveness/handler.gogo/apps/api/routes/chproxy_ratelimits/handler.gogo/pkg/zen/request_util.gogo/apps/api/routes/chproxy_metrics/handler.gogo/apps/api/routes/chproxy_verifications/handler.gogo/pkg/zen/middleware_errors.gogo/apps/api/routes/reference/handler.gogo/pkg/codes/constants_gen.gogo/cmd/api/main.gogo/pkg/zen/session_body_limit_test.gogo/apps/api/routes/v2_ratelimit_limit/handler.gogo/pkg/zen/server.gogo/pkg/zen/middleware_metrics.gogo/pkg/codes/user_request.gogo/pkg/zen/session.go
**/*_test.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*_test.go: Use table-driven tests in Go
Organize Go integration tests with real dependencies
Organize Go tests by HTTP status codes
Files:
go/pkg/zen/session_body_limit_test.go
🧠 Learnings (1)
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Organize Go tests by HTTP status codes
Applied to files:
go/pkg/zen/session_body_limit_test.go
🧬 Code Graph Analysis (9)
go/apps/api/routes/v2_liveness/handler.go (1)
go/apps/api/openapi/gen.go (2)
Meta(259-262)V2LivenessResponseData(1432-1435)
go/pkg/zen/request_util.go (1)
go/pkg/fault/wrap.go (2)
Internal(75-89)Public(97-111)
go/pkg/zen/middleware_errors.go (1)
go/pkg/codes/constants_gen.go (2)
UserErrorsBadRequestPermissionsQuerySyntaxError(16-16)UserErrorsBadRequestRequestBodyTooLarge(18-18)
go/pkg/codes/constants_gen.go (1)
go/pkg/urn/urn.go (1)
URN(12-19)
go/cmd/api/main.go (1)
go/pkg/cli/flag.go (3)
Int64(565-602)Default(327-373)EnvVar(287-304)
go/pkg/zen/session_body_limit_test.go (4)
go/pkg/zen/session.go (1)
Session(31-48)go/pkg/fault/wrapped.go (1)
UserFacingMessage(161-195)go/pkg/zen/server.go (2)
New(75-126)Config(43-57)go/pkg/zen/middleware_errors.go (1)
WithErrorHandling(15-218)
go/pkg/zen/server.go (3)
go/apps/api/config.go (1)
Config(18-86)go/pkg/zen/middleware_errors.go (1)
WithErrorHandling(15-218)go/pkg/zen/session.go (1)
Session(31-48)
go/pkg/codes/user_request.go (1)
go/pkg/codes/codes.go (2)
SystemUser(26-26)CategoryUserBadRequest(46-46)
go/pkg/zen/session.go (3)
go/pkg/fault/wrapped.go (1)
New(52-66)go/pkg/fault/wrap.go (2)
Wrap(25-67)Code(119-133)go/pkg/codes/user_request.go (1)
User(21-26)
🪛 golangci-lint (2.2.2)
go/pkg/zen/session_body_limit_test.go
54-54: "POST" can be replaced by http.MethodPost
(usestdlibvars)
79-79: "POST" can be replaced by http.MethodPost
(usestdlibvars)
137-137: "POST" can be replaced by http.MethodPost
(usestdlibvars)
180-180: "POST" can be replaced by http.MethodPost
(usestdlibvars)
197-197: "POST" can be replaced by http.MethodPost
(usestdlibvars)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
go/pkg/zen/request_util.go (1)
17-19: LGTM: clearer error wrapping; behavior unchangedFormatting-only change; wrapper order and semantics remain correct.
go/apps/api/routes/chproxy_metrics/handler.go (1)
36-37: Disable ClickHouse logging early — good placementTurning off CH logging at the very start is correct for this internal endpoint.
go/apps/api/routes/chproxy_verifications/handler.go (1)
36-37: LGTM: CH logging disabled for internal routeConsistent with other internal CH proxy handlers; good to prevent logging sensitive/internal traffic.
go/apps/api/routes/reference/handler.go (1)
30-31: LGTM: disable ClickHouse logging for reference pageAppropriate to avoid logging this static/reference endpoint.
go/apps/api/routes/chproxy_ratelimits/handler.go (1)
36-37: Correct: disable ClickHouse request logging for this internal CHProxy routePlacing
s.DisableClickHouseLogging()at the top of the handler prevents noisy per-request logs while preserving the explicit buffering of ratelimit events below. This aligns with the PR’s route-level control pattern.go/apps/api/routes/v2_liveness/handler.go (1)
30-31: Good: disable request logging for livenessThis endpoint is high-volume and low-value for request logging; disabling at entry keeps ClickHouse clean.
go/pkg/codes/constants_gen.go (1)
17-18: All generated references are in placeThe
UserErrorsBadRequestRequestBodyTooLargeconstant is correctly emitted by the code generator and fully wired through the codebase:
- Defined in
go/pkg/codes/constants_gen.go(lines 17–18)- Referenced in
go/pkg/codes/user_request.go(line 24)- Mapped to HTTP 400 in
go/pkg/zen/middleware_errors.go(lines 63–66)No manual edits to the generated file are needed—the generator’s source-of-truth is up-to-date.
go/apps/api/routes/v2_ratelimit_limit/handler.go (1)
213-223: Session-based logging gate is correct and consistentSwitching to s.ShouldLogRequestToClickHouse() aligns this route with the new session-level logging control and centralizes the decision. LGTM.
go/pkg/codes/user_request.go (1)
7-9: New error code for oversized request bodies is well-placed and consistentThe RequestBodyTooLarge code and its documentation are consistent with constants and middleware mapping. Good addition.
Also applies to: 24-25
go/cmd/api/main.go (2)
81-84: Int64 flag with typed default and env var support: goodUsing cli.Int64 with cli.Default(int64(...)) avoids type mismatches and matches retrieval via cmd.Int64. Description is clear.
154-156: Wiring MaxRequestBodySize through config is correctPassing cmd.Int64("max-request-body-size") into api.Config is consistent with server handling (0 or negative = unlimited).
go/pkg/zen/server.go (2)
53-56: Config docs align with CLI semanticsComment explicitly states “0 or negative” disables the limit, matching flag help and tests. LGTM.
204-208: TLS selection via Config is cleanUsing s.config.TLS to switch between HTTP/HTTPS and assigning s.srv.TLSConfig is straightforward and removes prior duplication.
go/pkg/zen/session.go (3)
46-47: Per-route ClickHouse logging: naming, defaults, and docs look solidField name and default are clear. Public methods are documented per guidelines. Reset restores the default. Good addition.
Also applies to: 108-121, 428-429
68-79: Oversized-body handling: correct mapping and messagingGood use of http.MaxBytesError detection, codes.User.BadRequest.RequestBodyTooLarge, and a clear public message including the limit.
50-55: Session.init signature change verified: all call sites updatedI’ve confirmed that every
sess.initcall now includes the thirdmaxBodySizeparameter. No calls with only one or two arguments remain.Call sites updated:
- go/pkg/zen/server.go (l.255)
- go/pkg/zen/session_bind_body_test.go (l.90, 122, 167, 191)
- go/pkg/zen/session_bind_query_test.go (l.258)
- go/pkg/zen/session_body_limit_test.go (l.59, 84, 141, 201)
- go/pkg/zen/auth_test.go (l.106)
Flo4604
requested review from
MichaelUnkey,
mcstepp and
ogzhanolguncu
as code owners
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
go/pkg/zen/session_body_limit_test.go (2)
54-54: Use stdlib constants for HTTP methods (repeat).Replace string literals with http.MethodPost for consistency and lint compliance. This mirrors the prior suggestion and golangci-lint hint.
- req := httptest.NewRequest("POST", "/", strings.NewReader(tt.bodyContent)) + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(tt.bodyContent))- req := httptest.NewRequest("POST", "/", strings.NewReader(bodyContent)) + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(bodyContent))- req := httptest.NewRequest("POST", "/", strings.NewReader(bodyContent)) + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(bodyContent))- req := httptest.NewRequest("POST", "/test", strings.NewReader(bodyContent)) + req := httptest.NewRequest(http.MethodPost, "/test", strings.NewReader(bodyContent))- req := httptest.NewRequest("POST", "/", strings.NewReader("test")) + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("test"))Also applies to: 79-79, 137-137, 180-180, 197-197
61-68: Prefer type assertion over substring matching for body-size errors (repeat).Avoid brittle substring checks; assert the concrete type via errors.As against *http.MaxBytesError. Keep the substring check as a fallback only when the error is not a MaxBytesError.
if tt.wantErr { require.Error(t, err) - if tt.errSubstr != "" { - assert.Contains(t, err.Error(), tt.errSubstr) - } + var mbe *http.MaxBytesError + if errors.As(err, &mbe) { + // OK: body exceeded limit + } else if tt.errSubstr != "" { + assert.Contains(t, err.Error(), tt.errSubstr) + } return }- require.Error(t, err) - assert.Contains(t, err.Error(), tt.wantErrMsg) + require.Error(t, err) + var mbe *http.MaxBytesError + if !errors.As(err, &mbe) { + t.Fatalf("expected *http.MaxBytesError; got %T: %v", err, err) + } + assert.Contains(t, err.Error(), tt.wantErrMsg)Also applies to: 143-151
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (2)
go/apps/api/routes/openapi/handler.go(1 hunks)go/pkg/zen/session_body_limit_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/apps/api/routes/openapi/handler.gogo/pkg/zen/session_body_limit_test.go
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/apps/api/routes/openapi/handler.gogo/pkg/zen/session_body_limit_test.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Use table-driven tests in Go
Organize Go integration tests with real dependencies
Organize Go tests by HTTP status codes
Files:
go/pkg/zen/session_body_limit_test.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: Flo4604
PR: unkeyed/unkey#3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
📚 Learning: 2025-08-20T10:52:19.312Z
Learnt from: Flo4604
PR: unkeyed/unkey#3810
File: go/apps/api/routes/chproxy_metrics/handler.go:36-37
Timestamp: 2025-08-20T10:52:19.312Z
Learning: In the Unkey codebase, ClickHouse logging should be controlled explicitly per-handler using s.DisableClickHouseLogging() calls rather than through middleware, to allow for granular control and handler-specific logic around when logging should be disabled.
Applied to files:
go/apps/api/routes/openapi/handler.go
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
PR: unkeyed/unkey#3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/apps/api/routes/openapi/handler.go
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Organize Go tests by HTTP status codes
Applied to files:
go/pkg/zen/session_body_limit_test.go
🧬 Code graph analysis (1)
go/pkg/zen/session_body_limit_test.go (4)
go/pkg/zen/session.go (1)
Session(31-48)go/pkg/zen/request_util.go (1)
BindBody(10-23)go/pkg/fault/wrapped.go (1)
UserFacingMessage(161-195)go/pkg/zen/server.go (2)
New(75-126)Config(43-57)
🪛 golangci-lint (2.2.2)
go/pkg/zen/session_body_limit_test.go
54-54: "POST" can be replaced by http.MethodPost
(usestdlibvars)
79-79: "POST" can be replaced by http.MethodPost
(usestdlibvars)
137-137: "POST" can be replaced by http.MethodPost
(usestdlibvars)
180-180: "POST" can be replaced by http.MethodPost
(usestdlibvars)
197-197: "POST" can be replaced by http.MethodPost
(usestdlibvars)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
go/apps/api/routes/openapi/handler.go (1)
29-29: Per-route ClickHouse logging disabled early — correct and aligned with project guidanceGood placement at the top of the handler and consistent with the per-handler gating pattern we’ve standardized. This prevents noisy writes for a static-docs endpoint.
go/pkg/zen/session_body_limit_test.go (1)
38-49: Good coverage of “no limit” edge cases.Covers both zero and negative sizes as “disabled,” which aligns with the Config contract. Nice.
ogzhanolguncu
left a comment
ogzhanolguncu
left a comment
There was a problem hiding this comment.
✔️ Auth header redacting still works as expected
✔️ Body size works
✔️ CH disable logging flag works for endpoints
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
left 2 comments open, you can address them or skip, they're not critical
|
fixed. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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)
go/apps/api/routes/v2_ratelimit_limit/handler.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/apps/api/routes/v2_ratelimit_limit/handler.go
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/apps/api/routes/v2_ratelimit_limit/handler.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: Flo4604
PR: unkeyed/unkey#3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
📚 Learning: 2025-08-20T10:52:19.312Z
Learnt from: Flo4604
PR: unkeyed/unkey#3810
File: go/apps/api/routes/chproxy_metrics/handler.go:36-37
Timestamp: 2025-08-20T10:52:19.312Z
Learning: In the Unkey codebase, ClickHouse logging should be controlled explicitly per-handler using s.DisableClickHouseLogging() calls rather than through middleware, to allow for granular control and handler-specific logic around when logging should be disabled.
Applied to files:
go/apps/api/routes/v2_ratelimit_limit/handler.go
📚 Learning: 2025-08-20T11:41:36.683Z
Learnt from: Flo4604
PR: unkeyed/unkey#3810
File: go/apps/api/routes/v2_ratelimit_limit/handler.go:54-56
Timestamp: 2025-08-20T11:41:36.683Z
Learning: In the Unkey codebase, the X-Unkey-Metrics: disabled header is used by the v1 API when replaying ratelimit requests to the v2 API (go/apps/api/routes/v2_ratelimit_limit/handler.go) to prevent double billing/logging to ClickHouse. This is a legitimate internal service-to-service communication pattern for API migration scenarios, not intended for external client usage.
Applied to files:
go/apps/api/routes/v2_ratelimit_limit/handler.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
🔇 Additional comments (1)
go/apps/api/routes/v2_ratelimit_limit/handler.go (1)
213-223: LGTM: Session-gated ClickHouse emission is correct and aligns with the per-session logging controlUsing s.ShouldLogRequestToClickHouse() here is the right integration point; the emitted fields (RequestID, WorkspaceID, Time, NamespaceID, Identifier, Passed) look consistent for analytics. No changes needed.
|
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
What does this PR do?
This PR does 4 things
Type of change
How should this be tested?
Run Tests
Run API manually
make build &&&& ./unkey run api --color=true --http-port 7070 --region test --max-request-body-size=1and ensure api request fails against it when there is a body set.Change body size to anything else and ensure the limits work.
Test if we log openapi requests to clickhouse.
Test if we log validation requests bodies correctly into clickhouse
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Improvements
Chores
Tests
Documentation