Conversation
chronark
requested review from
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
📝 WalkthroughWalkthroughThis PR refactors and extends the API for rate limit overrides. It introduces a new constant and type for handling rate limit overrides, updates the OpenAPI specification with new schemas and a POST endpoint ( Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant H as ListOverrides Handler
participant DB as Database
C->>H: POST /v2/ratelimit.listOverrides (with auth & request data)
H->>H: Validate authentication and bind request
H->>H: Retrieve namespace and validate permissions
H->>DB: Execute ListRatelimitOverrides query
DB-->>H: Return overrides data
H->>C: Respond with 200 OK and overrides list
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
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
🧹 Nitpick comments (9)
go/pkg/db/queries/ratelimit_override_list_by_namespace_id.sql (1)
1-5: SQL query looks good but consider filtering deleted records.The query correctly retrieves rate limit overrides filtered by workspace_id and namespace_id, using parameterized queries for security. However, you might want to consider:
- Adding
AND deleted_at_m IS NULLto exclude soft-deleted records- Adding an ORDER BY clause for consistent results
-- name: ListRatelimitOverrides :many SELECT * FROM ratelimit_overrides WHERE workspace_id = sqlc.arg(workspace_id) - AND namespace_id = sqlc.arg(namespace_id); + AND namespace_id = sqlc.arg(namespace_id) + AND deleted_at_m IS NULL +ORDER BY created_at_m DESC;go/apps/api/routes/v2_ratelimit_list_overrides/403_test.go (1)
23-23: Ensure consistent use of UID prefixes across test files.While this file correctly uses
uid.RatelimitNamespacePrefixfor namespace IDs, the 404_test.go file uses a custom prefix "test_ns". Consider standardizing on the official prefixes across all test files for consistency.-namespaceID := uid.New(uid.RatelimitNamespacePrefix) +namespaceID := uid.New(uid.RatelimitNamespacePrefix)go/apps/api/routes/v2_ratelimit_list_overrides/404_test.go (1)
23-23: Use standard UID prefix for namespace IDs.For consistency with other test files (like 403_test.go), use the standard
uid.RatelimitNamespacePrefixinstead of a custom "test_ns" prefix.-namespaceID := uid.New("test_ns") +namespaceID := uid.New(uid.RatelimitNamespacePrefix)go/apps/api/routes/v2_ratelimit_list_overrides/200_test.go (2)
17-20: Consider a multi-override test scenario.
While this test checks for a single override, testing multiple overrides would provide greater confidence in the list's handling of multiple entries.
82-97: Test empty namespace scenario.
Additionally, consider a scenario where no overrides exist for a namespace to ensure the handler correctly returns an empty list.go/apps/api/routes/v2_ratelimit_list_overrides/400_test.go (1)
69-83: Add a test for both namespace fields set.
Currently, there's no explicit test confirming how the system behaves if bothNamespaceIdandNamespaceNameare sent together. Consider adding one to validate the expected handling.go/pkg/db/ratelimit_override_list_by_namespace_id.sql_generated.go (1)
12-17: Consider filtering out deleted overrides.
The query as written retrieves all entries, including those havingdeleted_at_mset. If you intend to exclude deleted overrides, add a condition to ignore them.WHERE workspace_id = ? AND namespace_id = ? + AND deleted_at_m = 0go/apps/api/routes/v2_ratelimit_list_overrides/handler.go (2)
29-30: Evaluate using GET over POST.
The endpoint retrieves data rather than modifying it, so using the POST method may be semantically less intuitive. Consider using GET for listing resources unless there is a requirement (such as request-body parameters) that strictly demands POST.
119-138: Clarify behavior when both namespace fields are set.
The current logic processesNamespaceIdfirst and then only triesNamespaceNameifNamespaceIdis nil. If both fields are set, it silently ignores the name. Make your expectation explicit, either by returning an error or choosing which field has priority.switch { case req.NamespaceId != nil && req.NamespaceName != nil: - // Currently unhandled scenario + return db.RatelimitNamespace{}, fault.New("both fields provided", + fault.WithTag(fault.BAD_REQUEST), + fault.WithDesc("ambiguous namespace", "Please specify either namespaceId or namespaceName, but not both."), + ) case req.NamespaceId != nil: return db.Query.FindRatelimitNamespaceByID(ctx, svc.DB.RO(), *req.NamespaceId) case req.NamespaceName != nil: ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go/gen/proto/ratelimit/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
go/apps/api/openapi/gen.go(5 hunks)go/apps/api/openapi/openapi.json(3 hunks)go/apps/api/routes/register.go(2 hunks)go/apps/api/routes/v2_ratelimit_list_overrides/200_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_list_overrides/400_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_list_overrides/401_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_list_overrides/403_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_list_overrides/404_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_list_overrides/handler.go(1 hunks)go/pkg/db/querier_generated.go(1 hunks)go/pkg/db/queries/ratelimit_override_list_by_namespace_id.sql(1 hunks)go/pkg/db/ratelimit_override_list_by_namespace_id.sql_generated.go(1 hunks)packages/ratelimit/package.json(1 hunks)packages/ratelimit/src/ratelimit.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (8)
go/pkg/db/querier_generated.go (3)
go/pkg/db/ratelimit_override_list_by_namespace_id.sql_generated.go (1)
ListRatelimitOverridesParams(19-22)go/apps/api/openapi/gen.go (1)
RatelimitOverride(68-83)go/pkg/db/models_generated.go (1)
RatelimitOverride(440-452)
go/apps/api/routes/v2_ratelimit_list_overrides/403_test.go (2)
go/apps/api/routes/v2_ratelimit_list_overrides/handler.go (3)
New(29-117)Services(22-27)Request(19-19)go/apps/api/openapi/gen.go (1)
BadRequestError(11-32)
go/apps/api/routes/v2_ratelimit_list_overrides/200_test.go (2)
go/apps/api/routes/v2_ratelimit_list_overrides/handler.go (4)
New(29-117)Services(22-27)Request(19-19)Response(20-20)go/apps/api/routes/register.go (1)
Register(15-100)
go/apps/api/routes/v2_ratelimit_list_overrides/handler.go (2)
go/apps/api/openapi/gen.go (3)
V2RatelimitListOverridesRequestBody(161-167)V2RatelimitListOverridesResponseBody(170-172)RatelimitOverride(68-83)go/pkg/db/ratelimit_override_list_by_namespace_id.sql_generated.go (1)
ListRatelimitOverridesParams(19-22)
go/pkg/db/ratelimit_override_list_by_namespace_id.sql_generated.go (1)
go/apps/api/openapi/gen.go (1)
RatelimitOverride(68-83)
go/apps/api/routes/v2_ratelimit_list_overrides/404_test.go (2)
go/apps/api/routes/v2_ratelimit_list_overrides/handler.go (3)
New(29-117)Services(22-27)Request(19-19)go/apps/api/openapi/gen.go (1)
NotFoundError(62-62)
go/apps/api/routes/register.go (2)
go/apps/api/routes/v2_ratelimit_list_overrides/handler.go (2)
New(29-117)Services(22-27)go/apps/api/routes/services.go (1)
Services(19-28)
go/apps/api/routes/v2_ratelimit_list_overrides/400_test.go (3)
go/apps/api/routes/v2_ratelimit_list_overrides/handler.go (3)
New(29-117)Services(22-27)Request(19-19)go/apps/api/routes/register.go (1)
Register(15-100)go/apps/api/openapi/gen.go (2)
V2RatelimitListOverridesRequestBody(161-167)BadRequestError(11-32)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (18)
packages/ratelimit/package.json (1)
29-29: Dependency version locked to specific alpha version.The version of
@unkey/apihas been changed from a flexible^2.0.0-alpha.1to a specific2.0.0-alpha.5. This is appropriate for alpha releases where breaking changes might occur between minor versions.go/pkg/db/querier_generated.go (1)
387-393: New query interface method looks good.The addition of the
ListRatelimitOverridesmethod to the Querier interface follows the established pattern in the codebase. The method signature is correct with appropriate parameters and return types.go/apps/api/routes/register.go (2)
8-8: Import for new route handler is properly added.The addition of the import for the v2RatelimitListOverrides package is correct and follows the existing pattern.
77-85: Route registration is well structured.The registration of the new route for listing rate limit overrides follows the established pattern in the codebase. The route uses the standard middleware stack and provides the necessary services to the handler.
Looking at the handler implementation from the relevant code snippet, it correctly:
- Verifies the API key
- Checks permissions
- Retrieves data using the new database query
- Transforms the database results into the API response format
packages/ratelimit/src/ratelimit.ts (1)
171-177: Code simplification looks good.The change simplifies the
_limitmethod by directly returning the promise fromthis.unkey.ratelimit.limitwithout additional promise chain processing. This is a valid improvement since:
- Error handling is already present in the public
limitmethod (lines 147-158)- The timeout handling via
Promise.raceis still intact- The cleanup in the
finallyblock is preservedgo/apps/api/routes/v2_ratelimit_list_overrides/401_test.go (1)
1-40: Well-structured test for unauthorized access.This test properly validates the 401 Unauthorized response when an invalid token is provided. It includes all necessary components for a comprehensive test:
- Proper test harness setup
- Service initialization
- Request construction with invalid credentials
- Clear assertions for status code and response body
go/apps/api/routes/v2_ratelimit_list_overrides/403_test.go (1)
1-75: Good security test for workspace isolation.This test correctly verifies that users from one workspace cannot access rate limit overrides from another workspace. The implementation follows security best practices by masking the existence of resources with a 404 Not Found error rather than revealing unauthorized resources exist.
go/apps/api/routes/v2_ratelimit_list_overrides/404_test.go (1)
18-73: Comprehensive test for not found scenarios.This test thoroughly validates the API's handling of not found scenarios for both namespace ID and namespace name, which is essential for robust error handling. The test structure is clear and includes appropriate assertions to verify both the status code and error type in the response.
go/apps/api/openapi/openapi.json (5)
210-212: Good refactoring approach using schema referenceThe change to reference the common
RatelimitOverrideschema inV2RatelimitGetOverrideResponseBodyimproves code reuse and consistency.
213-231: LGTM: Well-structured request schema for listing overridesThe new request schema for listing overrides is well-designed, requiring either
namespaceIdornamespaceNameas expected. This follows the same pattern as other endpoints in the API.
232-244: LGTM: Clear response structure for list operationThe response schema properly defines an array of overrides with the required property constraint, making the API contract clear.
343-379: LGTM: Comprehensive schema definition for RatelimitOverrideThe
RatelimitOverrideschema is well-defined with all necessary properties and constraints:
- All required fields are specified
- Clear descriptions for each field
- Appropriate validation rules (minLength, maxLength, minimum)
- Good documentation reference for the wildcard rules in identifiers
This standardization will help maintain consistency across the API.
633-715: LGTM: Well-defined endpoint for listing overridesThe new endpoint
/v2/ratelimit.listOverridesis properly configured with:
- Consistent security requirements (rootKey)
- Appropriate request and response schemas
- Standard error responses matching other endpoints
- Clear tagging and operation ID
This maintains consistency with the API's existing patterns.
go/apps/api/openapi/gen.go (5)
67-83: LGTM: Well-structured RatelimitOverride typeThe new
RatelimitOverridetype is well-structured with:
- Appropriate JSON tags for each field
- Clear documentation comments
- Fields aligned with the OpenAPI schema
This type will provide a standardized structure for rate limit overrides across the API.
122-122: Good use of type alias for response modelUsing a type alias for
V2RatelimitGetOverrideResponseBodythat points toRatelimitOverrideimproves code reuse and consistency.
160-172: LGTM: Clear request and response types for list operationThe new request and response types for listing overrides are well-structured:
- Request has optional fields with appropriate pointer types
- Response has a clearly defined slice of overrides
- Field documentation is clear and consistent
These types align well with the OpenAPI schema definitions.
210-223: Good standardization of naming conventionsRenaming the request body types to remove the "V2" prefix improves naming consistency while maintaining the same functionality. This makes the code more maintainable.
6-8:Details
❓ Verification inconclusive
Verify usage of the new constant
The new
RootKeyScopesconstant has been added, but ensure it's used appropriately throughout the codebase.
🏁 Script executed:
#!/bin/bash # Check for usages of the RootKeyScopes constant rg "RootKeyScopes" --type goLength of output: 89
Double-check Usage of
RootKeyScopesConstant
- The constant is defined in go/apps/api/openapi/gen.go (lines 6-8) as:
const ( RootKeyScopes = "rootKey.Scopes" )- A repository-wide search indicates that the constant appears only in its definition file.
- Please verify whether this is intentional—if
RootKeyScopesis meant to be used elsewhere in the codebase (e.g., for API permissions or configuration logic), ensure that those references are added. Otherwise, confirm that its current usage meets the intended design.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores