Conversation
|
📝 WalkthroughWalkthroughRemoved the regex pattern constraint from the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant A as API Gateway
participant V as OpenAPI Validator
participant H as Limit Handler
participant D as DB
C->>A: POST /v2/ratelimit/limit { namespace, ... }
A->>V: Validate request body
Note over V: Regex pattern on `namespace` removed\n(min/max length still enforced)
V-->>A: Validation result (valid / invalid)
alt valid
A->>H: Invoke limit handler with payload
H->>D: Insert/lookup namespace and apply limit
D-->>H: OK
H-->>A: 200 OK with result
A-->>C: 200 OK
else invalid
A-->>C: 400 Bad Request (openapi.BadRequestErrorResponse)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 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 (5)
go/apps/api/openapi/openapi-generated.yaml(0 hunks)go/apps/api/openapi/spec/paths/v2/ratelimit/limit/V2RatelimitLimitRequestBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/ratelimit/listOverrides/V2RatelimitListOverridesRequestBody.yaml(1 hunks)go/apps/api/routes/v2_ratelimit_limit/200_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_limit/400_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- go/apps/api/openapi/openapi-generated.yaml
- go/apps/api/routes/v2_ratelimit_limit/400_test.go
🧰 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/v2_ratelimit_limit/200_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/apps/api/routes/v2_ratelimit_limit/200_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/v2_ratelimit_limit/200_test.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: chronark
PR: unkeyed/unkey#2126
File: apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts:36-36
Timestamp: 2024-11-13T19:06:36.786Z
Learning: In the rate limit test files (e.g., `apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts`), URL parameters like `namespaceId` and `identifier` do not need to be URL-encoded in the test code because the values used are always considered safe within the test environment.
📚 Learning: 2024-11-13T19:06:36.786Z
Learnt from: chronark
PR: unkeyed/unkey#2126
File: apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts:36-36
Timestamp: 2024-11-13T19:06:36.786Z
Learning: In the rate limit test files (e.g., `apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts`), URL parameters like `namespaceId` and `identifier` do not need to be URL-encoded in the test code because the values used are always considered safe within the test environment.
Applied to files:
go/apps/api/routes/v2_ratelimit_limit/200_test.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/v2_ratelimit_limit/200_test.go
🧬 Code graph analysis (1)
go/apps/api/routes/v2_ratelimit_limit/200_test.go (3)
go/apps/api/integration/harness.go (1)
New(53-111)go/pkg/uid/uid.go (2)
New(94-124)RatelimitNamespacePrefix(25-25)go/pkg/testutil/http.go (1)
CallRoute(259-293)
⏰ 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: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
go/apps/api/openapi/spec/paths/v2/ratelimit/listOverrides/V2RatelimitListOverridesRequestBody.yaml (1)
4-7: Good doc clean-up.Single-line description is clearer and consistent with nearby schemas.
go/apps/api/openapi/spec/paths/v2/ratelimit/limit/V2RatelimitLimitRequestBody.yaml (2)
4-9: Removing the pattern constraint matches the objective.Allowing any namespace characters (with length bounds) is consistent with the PR’s intent and with the new tests.
1-9: ✅ No lingering pattern constraint onnamespacefieldVerified that in
go/apps/api/openapi/openapi-generated.yaml, theV2RatelimitLimitRequestBody.namespaceschema no longer includes anypatternentry, and the generated Go model ingo/apps/api/openapi/gen.goshows theNamespace stringjson:"namespace"` field without any validation tags. No further changes are required here.go/apps/api/routes/v2_ratelimit_limit/200_test.go (2)
258-337: No duplicate “namespace accepts any characters within length bounds” subtest foundI ran a grep for the subtest marker and only one occurrence was found at lines 258–260 in 200_test.go, confirming there are no duplicates to remove.
287-290: No unescaped namespace logging or ClickHouse/metrics usage detected; DB writes use parameter binding
- A scan of go/apps/api/routes/v2_ratelimit_limit/handler.go reveals no calls to h.Logger.* that include the namespace value, so there are currently no raw logs of control-character names.
- There is no metrics or ClickHouse Bufferer usage in this handler that embeds the namespace, so analytics sinks won’t receive unescaped names.
- All database inserts for namespaces (both single and bulk via InsertRatelimitNamespace and InsertRatelimitNamespaces) pass the name as a parameter to ExecContext, ensuring control characters are handled safely by the driver (no SQL-string interpolation).
No immediate changes are required for this route. If future logging or analytics code begins to include raw namespace values, consider adding a centralized “safe display” helper to escape control characters at that point.
go/apps/api/openapi/spec/paths/v2/ratelimit/limit/V2RatelimitLimitRequestBody.yaml
Show resolved
Hide resolved
...s/api/openapi/spec/paths/v2/ratelimit/listOverrides/V2RatelimitListOverridesRequestBody.yaml
Show resolved
Hide resolved
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
wondering about the commented test, otherwise we can merge
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
go/apps/api/routes/v2_ratelimit_limit/200_test.go (1)
307-313: Use the existing ctx; t.Context() may not be available on your Go versionThis can fail to compile depending on the Go toolchain, and it breaks consistency with the rest of the file which already uses ctx. Use the existing ctx variable to propagate deadlines/cancellations.
Apply this diff:
- err := db.Query.InsertRatelimitNamespace(t.Context(), h.DB.RW(), db.InsertRatelimitNamespaceParams{ + err := db.Query.InsertRatelimitNamespace(ctx, h.DB.RW(), db.InsertRatelimitNamespaceParams{
📜 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/v2_ratelimit_limit/200_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_limit/400_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/v2_ratelimit_limit/200_test.gogo/apps/api/routes/v2_ratelimit_limit/400_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/apps/api/routes/v2_ratelimit_limit/200_test.gogo/apps/api/routes/v2_ratelimit_limit/400_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/v2_ratelimit_limit/200_test.gogo/apps/api/routes/v2_ratelimit_limit/400_test.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: chronark
PR: unkeyed/unkey#2126
File: apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts:36-36
Timestamp: 2024-11-13T19:06:36.786Z
Learning: In the rate limit test files (e.g., `apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts`), URL parameters like `namespaceId` and `identifier` do not need to be URL-encoded in the test code because the values used are always considered safe within the test environment.
📚 Learning: 2024-11-13T19:06:36.786Z
Learnt from: chronark
PR: unkeyed/unkey#2126
File: apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts:36-36
Timestamp: 2024-11-13T19:06:36.786Z
Learning: In the rate limit test files (e.g., `apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts`), URL parameters like `namespaceId` and `identifier` do not need to be URL-encoded in the test code because the values used are always considered safe within the test environment.
Applied to files:
go/apps/api/routes/v2_ratelimit_limit/200_test.gogo/apps/api/routes/v2_ratelimit_limit/400_test.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/v2_ratelimit_limit/200_test.gogo/apps/api/routes/v2_ratelimit_limit/400_test.go
🧬 Code graph analysis (2)
go/apps/api/routes/v2_ratelimit_limit/200_test.go (3)
go/pkg/uid/uid.go (2)
New(94-124)RatelimitNamespacePrefix(25-25)go/apps/api/routes/v2_ratelimit_limit/handler.go (2)
Request(27-27)Response(28-28)go/pkg/testutil/http.go (1)
CallRoute(259-293)
go/apps/api/routes/v2_ratelimit_limit/400_test.go (4)
go/pkg/testutil/seed/seed.go (1)
Resources(23-28)go/apps/api/routes/v2_ratelimit_limit/handler.go (1)
Request(27-27)go/pkg/testutil/http.go (1)
CallRoute(259-293)go/apps/api/openapi/gen.go (1)
BadRequestErrorResponse(59-65)
⏰ 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). (2)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
go/apps/api/routes/v2_ratelimit_limit/400_test.go (3)
62-62: Use http.StatusBadRequest constant for status comparisonsPrefer the stdlib constant over a magic number for clarity and consistency with the new test.
- require.Equal(t, 400, res.Status, "expected 400, sent: %+v, received: %s", req, res.RawBody) + require.Equal(t, http.StatusBadRequest, res.Status, "expected 400, sent: %+v, received: %s", req, res.RawBody)
137-153: Remove duplicate “missing authorization header” subtest (wrong response type weakens assertions)This second subtest duplicates the scenario/name and uses handler.Response as the decode target. Because json.Unmarshal ignores unknown fields, this can pass without actually validating the structured error. Either delete it or make it assert the BadRequestErrorResponse like the first subtest.
Minimal fix: remove the duplicate block.
- t.Run("missing authorization header", func(t *testing.T) { - headers := http.Header{ - "Content-Type": {"application/json"}, - // No Authorization header - } - - req := handler.Request{ - Namespace: "test_namespace", - Identifier: "user_123", - Limit: 100, - Duration: 60000, - } - - res := testutil.CallRoute[handler.Request, handler.Response](h, route, headers, req) - require.Equal(t, http.StatusBadRequest, res.Status, "Got %s", res.RawBody) - require.NotNil(t, res.Body) - })
19-31: Consider table-driven tests for 400 casesThis suite is a great candidate for table-driven tests (missing namespace, negative cost, missing/malformed auth). It will deduplicate setup, make scenarios easier to extend (e.g., empty namespace vs. missing field), and aligns with team direction.
If helpful, I can open/append to the tracking issue (per Flo4604’s request) and/or submit a follow-up refactor PR. Example skeleton:
tests := []struct{ name string req handler.Request headers http.Header seed func(h *testutil.Harness) wantStatus int wantType string }{ { name: "missing namespace", req: handler.Request{ Identifier: "u", Limit: 1, Duration: 1 }, headers: auth(h, "ratelimit.*.limit"), wantStatus: http.StatusBadRequest, wantType: "https://unkey.com/docs/errors/unkey/application/invalid_input" }, // ... } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { if tc.seed != nil { tc.seed(h) } res := testutil.CallRoute[handler.Request, openapi.BadRequestErrorResponse](h, route, tc.headers, tc.req) require.Equal(t, tc.wantStatus, res.Status, "body: %s", res.RawBody) require.Equal(t, tc.wantType, res.Body.Error.Type) }) }
♻️ Duplicate comments (1)
go/apps/api/routes/v2_ratelimit_limit/400_test.go (1)
32-39: Keep request type usage consistent (prefer handler.Request alias)Earlier subtest uses the concrete OpenAPI type while the new subtest uses the route alias. Align on the alias for readability in route-focused tests.
- req := openapi.V2RatelimitLimitRequestBody{ + req := handler.Request{ Namespace: uid.New("test"), Identifier: "user_123", Limit: 100, Duration: 60000, Cost: ptr.P[int64](-5), }
📜 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/400_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/v2_ratelimit_limit/400_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/apps/api/routes/v2_ratelimit_limit/400_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/v2_ratelimit_limit/400_test.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: chronark
PR: unkeyed/unkey#2126
File: apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts:36-36
Timestamp: 2024-11-13T19:06:36.786Z
Learning: In the rate limit test files (e.g., `apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts`), URL parameters like `namespaceId` and `identifier` do not need to be URL-encoded in the test code because the values used are always considered safe within the test environment.
📚 Learning: 2024-11-13T19:06:36.786Z
Learnt from: chronark
PR: unkeyed/unkey#2126
File: apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts:36-36
Timestamp: 2024-11-13T19:06:36.786Z
Learning: In the rate limit test files (e.g., `apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts`), URL parameters like `namespaceId` and `identifier` do not need to be URL-encoded in the test code because the values used are always considered safe within the test environment.
Applied to files:
go/apps/api/routes/v2_ratelimit_limit/400_test.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/v2_ratelimit_limit/400_test.go
🧬 Code graph analysis (1)
go/apps/api/routes/v2_ratelimit_limit/400_test.go (4)
go/pkg/testutil/seed/seed.go (1)
Resources(23-28)go/apps/api/routes/v2_ratelimit_limit/handler.go (1)
Request(27-27)go/pkg/testutil/http.go (1)
CallRoute(259-293)go/apps/api/openapi/gen.go (2)
BadRequestErrorResponse(59-65)Meta(259-262)
🔇 Additional comments (1)
go/apps/api/routes/v2_ratelimit_limit/400_test.go (1)
73-100: LGTM: Solid negative test for missing namespace with structured error assertionsGood restoration and modernization of the test. Using a wildcard permission ensures auth succeeds so the failure is purely schema-validation. The assertions on Type/Detail/Status/Title/RequestId/Errors look consistent with the current error model.
|
@ogzhanolguncu could I get a second review 🙂 |
|
@Flo4604 ofc. I'm so sorry still trying to get used to cushion |
|
cc @chronark can I also get your review again |
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (08/26/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
3 participants
What does this PR do?
Allow any namespace name to be used for ratelimiting aslong as its in the range of our chars
Type of change
How should this be tested?
Create ratelimit in UI named
api:v1:calls:outboundand ensure you can ratelimit against it without the API throwing a 400 bad request error.Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Documentation
Tests