Conversation
Flo4604
requested review from
MichaelUnkey,
chronark,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
📝 WalkthroughWalkthroughIntroduces a new Bad Request error code for unreadable request bodies: adds constant and struct field, maps the code in middleware, sets the code when body read fails (non-MaxBytes), updates docs/OpenAPI, and adds tests and a docs page. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SessionInit as "Session.Init"
participant IO as "io.ReadAll"
participant Middleware
participant Response
Client->>SessionInit: POST with body
SessionInit->>IO: io.ReadAll(body)
alt success
IO-->>SessionInit: body bytes
SessionInit->>Middleware: proceed
Middleware-->>Response: handler result
else read error (non-MaxBytesError)
IO-->>SessionInit: read error
SessionInit->>SessionInit: wrap error + attach UserErrorsBadRequestRequestBodyUnreadable
SessionInit->>Middleware: return wrapped error
Middleware->>Response: 400 Bad Request JSON (type includes request_body_unreadable)
else read error (MaxBytesError)
IO-->>SessionInit: MaxBytesError
SessionInit->>Middleware: existing payload-too-large path
Middleware->>Response: 413 Payload Too Large JSON
end
Response-->>Client: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go/apps/api/openapi/openapi-generated.yaml (1)
39-49: Update BadRequestErrorResponse description or add concrete 400 example to surface unreadable request body errors.The PR implements body read errors returning 400 status with error type
request_body_unreadable(verified ingo/pkg/zen/session.go), but the current BadRequestErrorResponse description at lines 39-49 only mentions "missing, malformed, or fail validation rules"—it does not reference unreadable/illegible request bodies. The error code does not appear in the OpenAPI documentation, creating a gap between runtime behavior and API docs.To align the documentation with implementation, either:
- Add a brief note to the description acknowledging unreadable request bodies as a 400 scenario, or
- Include a concrete 400 example on one public endpoint (e.g.,
/v2/keys.verifyKey) showing therequest_body_unreadableproblem type to guide clients
🧹 Nitpick comments (1)
go/apps/api/openapi/openapi-generated.yaml (1)
928-929: Minor copy consistency: reference same support channel everywhere.“You will receive this from Unkey's support staff.” Consider linking to or reusing the contact phrasing used elsewhere (e.g., “Reach out at support@unkey.dev”) for consistency across the spec.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/docs/docs.json(1 hunks)apps/docs/errors/user/bad_request/request_body_unreadable.mdx(1 hunks)go/apps/api/openapi/gen.go(1 hunks)go/apps/api/openapi/openapi-generated.yaml(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/session.go(1 hunks)go/pkg/zen/session_body_read_error_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 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.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3647
File: go/apps/api/openapi/openapi-generated.yaml:3569-3575
Timestamp: 2025-07-22T18:09:41.800Z
Learning: In the Unkey codebase, using non-standard HTTP status code 529 for internal-only endpoints is acceptable and should not be flagged as an issue in future reviews.
📚 Learning: 2025-08-27T13:48:54.016Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/queries/key_find_for_verification.sql:15-15
Timestamp: 2025-08-27T13:48:54.016Z
Learning: The pending_migration_id field in FindKeyForVerification is only used internally by get_migrated.go for migration logic and doesn't need to be exposed in KeyVerifier struct or API response DTOs since it's an internal implementation detail.
Applied to files:
go/apps/api/openapi/gen.gogo/apps/api/openapi/openapi-generated.yaml
📚 Learning: 2025-11-10T11:50:46.281Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4292
File: go/apps/api/openapi/openapi-generated.yaml:386-387
Timestamp: 2025-11-10T11:50:46.281Z
Learning: Repo unkeyed/unkey: It’s acceptable for pagination properties (e.g., V2IdentitiesListIdentitiesResponseBody.pagination) to omit x-go-type-skip-optional-pointer and x-go-type-skip-optional-pointer-with-omitzero; do not flag this in future reviews.
Applied to files:
go/apps/api/openapi/gen.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, oapi-codegen doesn’t support OpenAPI 3.1 union nullability; overlay.yaml must be applied before codegen. The overlay key in oapi-codegen config isn’t supported—use a pre-step (programmatic or CLI) to merge overlay into the bundled spec, then run oapi-codegen.
Applied to files:
go/apps/api/openapi/gen.gogo/apps/api/openapi/openapi-generated.yaml
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: For go/apps/api/openapi, oapi-codegen is used and does not support OpenAPI 3.1 union types like [T, "null"]; an overlay step is required to downconvert to 3.0-style nullable before code generation.
Applied to files:
go/apps/api/openapi/gen.gogo/apps/api/openapi/openapi-generated.yaml
📚 Learning: 2025-07-03T05:58:10.699Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3421
File: go/apps/api/openapi/openapi.yaml:196-200
Timestamp: 2025-07-03T05:58:10.699Z
Learning: In the Unkey codebase, OpenAPI 3.1 is used, which allows sibling keys (such as `description`) alongside `$ref` in schema objects. Do not flag this as an error in future reviews.
Applied to files:
go/apps/api/openapi/gen.gogo/apps/api/openapi/openapi-generated.yaml
📚 Learning: 2025-09-24T19:58:24.572Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4022
File: go/pkg/zen/middleware_timeout.go:30-35
Timestamp: 2025-09-24T19:58:24.572Z
Learning: The zen framework in go/pkg/zen passes context as a separate parameter to handlers (ctx context.Context, s *Session) rather than through a Session.Context() method. The Session struct does not have a Context() method, and handlers access context through the ctx parameter, not through the session.
Applied to files:
go/pkg/zen/session_body_read_error_test.go
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 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/pkg/zen/session_body_read_error_test.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, overlays are required to downconvert 3.1 union nullability to 3.0-style nullable before running oapi-codegen; config.yaml’s output-options.overlay is not recognized by oapi-codegen, so overlays must be applied in a pre-step (programmatic or CLI) prior to codegen.
Applied to files:
go/apps/api/openapi/openapi-generated.yaml
📚 Learning: 2025-03-19T09:25:59.751Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 2955
File: go/apps/api/routes/v2_identities_create_identity/handler.go:162-202
Timestamp: 2025-03-19T09:25:59.751Z
Learning: In the Unkey codebase, input validation for API endpoints is primarily handled through OpenAPI schema validation, which occurs before requests reach the handler code. For example, in the identities.createIdentity endpoint, minimum values for ratelimit duration and limit are defined in the OpenAPI schema rather than duplicating these checks in the handler.
Applied to files:
go/apps/api/openapi/openapi-generated.yaml
🧬 Code graph analysis (4)
go/pkg/zen/middleware_errors.go (1)
go/pkg/codes/constants_gen.go (1)
UserErrorsBadRequestRequestBodyUnreadable(20-20)
go/pkg/codes/user_request.go (1)
go/pkg/codes/codes.go (2)
SystemUser(26-26)CategoryUserBadRequest(49-49)
go/pkg/zen/session.go (2)
go/pkg/codes/user_request.go (1)
User(55-75)go/pkg/codes/constants_gen.go (1)
URN(5-5)
go/pkg/zen/session_body_read_error_test.go (2)
go/pkg/zen/session.go (1)
Session(31-48)go/pkg/zen/middleware_errors.go (1)
WithErrorHandling(15-375)
⏰ 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: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
go/apps/api/openapi/openapi-generated.yaml (1)
2710-2711: Good improvement on security clarity for hash example.Switching the example to a clearly hashed placeholder avoids implying plaintext; LGTM.
go/apps/api/openapi/gen.go (1)
1136-1136: LGTM - Documentation clarification in generated code.The updated comment clarifies that the migration identifier is provided by Unkey's support staff, which improves the developer experience.
apps/docs/docs.json (1)
270-270: LGTM - Proper documentation integration.The new error documentation entry is correctly placed in the Bad Request section and follows the existing naming conventions.
go/pkg/zen/middleware_errors.go (1)
68-68: LGTM - Proper error handling integration.The new error code is correctly integrated into the Bad Request error handling path, ensuring unreadable request bodies return 400 instead of 500. The placement in the "General validation" section is appropriate.
go/pkg/codes/constants_gen.go (1)
19-20: LGTM - New error constant properly defined.The new URN constant follows the existing naming conventions and includes a clear comment describing when this error should be used. The URN format is consistent with other error codes in the system.
go/pkg/zen/session.go (1)
81-81: LGTM - Error classification now more precise.The new error code properly distinguishes unreadable request bodies from other error types, maintaining the existing error messages while improving the error classification. This enables the middleware to return 400 instead of 500 for these scenarios.
apps/docs/errors/user/bad_request/request_body_unreadable.mdx (1)
1-73: LGTM - Comprehensive error documentation.The documentation is clear, user-friendly, and provides practical troubleshooting steps. The example JSON response matches the structure produced by the middleware, and the common causes section helps developers understand when this error occurs.
go/pkg/codes/user_request.go (2)
9-10: LGTM - Error code struct field properly added.The new field is properly documented and follows the existing pattern for other Bad Request error codes.
59-59: LGTM - Error code initialization is correct.The initialization properly wires up the new error code with the correct system (SystemUser), category (CategoryUserBadRequest), and identifier ("request_body_unreadable").
go/pkg/zen/session_body_read_error_test.go (4)
17-28: LGTM - Clean test helper implementation.The
failingReadCloserhelper provides a straightforward way to simulate read errors for testing purposes. The implementation is minimal and focused.
30-46: LGTM - Unit test validates error handling.The test properly verifies that
Session.Initreturns an error with the correct internal message when the body cannot be read.
48-113: LGTM - Integration test thoroughly validates HTTP behavior.This test comprehensively validates:
- Correct HTTP status (400 instead of 500)
- JSON response structure and content-type
- All required error fields (title, detail, status, type)
- Correct field values matching the middleware implementation
- Handler is not invoked when body read fails
Excellent coverage of the full request lifecycle.
115-149: LGTM - Parameterized test ensures error distinction.The table-driven test properly validates that
MaxBytesError(413) and generic read errors (400) remain distinct with appropriate error messages. This prevents regression in the error classification logic.
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (11/14/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
3 participants
What does this PR do?
If we cant read request body we now send a 400 instead of 500 err.
Type of change
How should this be tested?
Tests.
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated