feat: add identity-based credits Go services and v2 API routes#4193
feat: add identity-based credits Go services and v2 API routes#4193Flo4604 wants to merge 13 commits intofeat/identity-credits-openapifrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughRefactors credits into a nested Credits model, adds a new v2/identities.updateCredits route, introduces dual credit sources (new credits table + legacy RemainingRequests), surfaces identity-level credits, updates usage-limiter to prefer CreditID, and migrates tests/DB/query surfaces to the new model. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IdentityHandler as v2/identities.updateCredits
participant DB
participant UsageLimiter
participant KeyCache
participant AuditLog
Client->>IdentityHandler: POST /v2/identities.updateCredits
IdentityHandler->>IdentityHandler: authenticate & bind
IdentityHandler->>DB: Find identity (external id) & permission check
alt Not found / no permission
IdentityHandler-->>Client: 4xx
else
IdentityHandler->>DB: Tx — Set/Increment/Decrement credits (insert/upsert/delete or adjust)
DB-->>IdentityHandler: updated credit state
IdentityHandler->>AuditLog: write audit entry
IdentityHandler->>UsageLimiter: Invalidate(updated identifier)
IdentityHandler->>KeyCache: Invalidate keys for identity
IdentityHandler-->>Client: 200 OK (Credits)
end
sequenceDiagram
participant Requester
participant KeyHandler as v2/keys.get / whoami / verify
participant DB
participant KeyDataBuilder
Requester->>KeyHandler: request key
KeyHandler->>DB: query key with LEFT JOIN credits (kc, ic)
DB-->>KeyHandler: row with key & credit fields
KeyHandler->>KeyDataBuilder: build key data
alt identity credit present
KeyDataBuilder->>KeyDataBuilder: populate IdentityCredits
end
alt key credit present
KeyDataBuilder->>KeyDataBuilder: populate KeyCredits
end
KeyHandler->>KeyHandler: populate response Credits (priority Identity -> Key -> legacy)
KeyHandler-->>Requester: response with openapi.Credits (and optional Refill)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Points needing careful review:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
Flo4604
force-pushed
the
feat/identity-credits-go-services
branch
from
6668e81 to
8a07fb8
Compare
Flo4604
force-pushed
the
feat/identity-credits-openapi
branch
from
ce27d21 to
19aadaf
Compare
Flo4604
force-pushed
the
feat/identity-credits-go-services
branch
from
8a07fb8 to
82018c9
Compare
Flo4604
force-pushed
the
feat/identity-credits-go-services
branch
from
82018c9 to
723c836
Compare
Flo4604
force-pushed
the
feat/identity-credits-openapi
branch
from
19aadaf to
03872fb
Compare
Flo4604
force-pushed
the
feat/identity-credits-go-services
branch
from
723c836 to
6e6a1ec
Compare
Flo4604
force-pushed
the
feat/identity-credits-openapi
branch
from
03872fb to
6816e5e
Compare
Flo4604
force-pushed
the
feat/identity-credits-openapi
branch
from
6816e5e to
c199c71
Compare
Flo4604
force-pushed
the
feat/identity-credits-go-services
branch
from
41dd624 to
20b9c78
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go/internal/services/usagelimiter/redis.go (1)
292-299: Treat missing credit rows as unlimited instead of denying requestsWhen
FindRemainingCreditsreturnsdb.IsNotFound(err)we currently respond withValid: false. After an identity/key is flipped to “unlimited” (which deletes the credit row) any in-flight request that is still holding the staleCreditIDwill now get rejected until it refreshes its auth object. We should instead treat the missing row as “no limit configured” and allow the request (mirroring legacy unlimited semantics).- if err != nil { - if db.IsNotFound(err) { - return UsageResponse{Valid: false, Remaining: 0}, nil - } - return UsageResponse{Valid: false, Remaining: 0}, err - } + if err != nil { + if db.IsNotFound(err) { + // Credit was deleted (unlimited) — permit the call and skip counter init. + return UsageResponse{Valid: true, Remaining: 0}, nil + } + return UsageResponse{Valid: false, Remaining: 0}, err + }
🧹 Nitpick comments (4)
go/apps/api/routes/v2_keys_update_credits/200_test.go (1)
81-84: Consider removing debug logging statements.These detailed
t.Logfstatements appear to be leftover from debugging. While they only display with-vor on failure, they add noise and can be removed unless actively troubleshooting.Apply this diff to remove the debug logging:
- t.Logf("Request Operation: %v, Value.IsNull(): %v", req.Operation, req.Value.IsNull()) - t.Logf("Remaining: %+v, IsNull: %v, IsSpecified: %v", res.Body.Data.Remaining, res.Body.Data.Remaining.IsNull(), res.Body.Data.Remaining.IsSpecified()) - t.Logf("Refill: %+v", res.Body.Data.Refill)go/apps/api/routes/v2_identities_update_credits/403_test.go (1)
56-59: Fix misleading permission commentThe test is asserting that only keys with
identity.*.update_identityare allowed, but the comment still mentionsupdate_key. Please align the wording so future readers don’t chase the wrong permission.- // Create root key with create_key permission instead of required update_key + // Create root key with create_key permission instead of the required identity.*.update_identitygo/apps/api/routes/v2_keys_whoami/handler.go (1)
131-150: Consider extracting credits construction logic.The credits construction pattern is duplicated between key credits (lines 131-150) and identity credits (lines 168-188). Both blocks:
- Check for non-nil credits data
- Build
openapi.CreditswithRemaining- Check
RefillAmount.Valid- Build
openapi.CreditsRefillwith interval/refillDay logicConsider extracting this into a helper function:
func buildCreditsResponse(remaining int32, refillAmount sql.NullInt32, refillDay sql.NullInt16) *openapi.Credits { credits := &openapi.Credits{ Remaining: nullable.NewNullableWithValue(int64(remaining)), } if refillAmount.Valid { var day *int interval := openapi.Daily if refillDay.Valid { interval = openapi.Monthly day = ptr.P(int(refillDay.Int16)) } credits.Refill = &openapi.CreditsRefill{ Amount: int64(refillAmount.Int32), Interval: interval, RefillDay: day, } } return credits }Then use it for both key and identity credits to improve maintainability.
go/apps/api/routes/v2_apis_list_keys/handler.go (1)
278-319: Consider extracting credits construction logic (duplicate pattern).The credits construction pattern here duplicates logic from:
v2_keys_whoami/handler.go(lines 131-150, 168-188)- This file's identity credits block (lines 336-356)
All three locations implement identical logic:
- Check for credits data existence
- Build
openapi.CreditswithRemaining- Conditionally add
Refillwith interval/refillDay logicConsider creating a shared helper in a common package (e.g.,
pkg/api/credits.go):package api import ( "database/sql" "github.com/oapi-codegen/nullable" "github.com/unkeyed/unkey/go/apps/api/openapi" "github.com/unkeyed/unkey/go/pkg/ptr" ) func BuildCreditsResponse(remaining int32, refillAmount sql.NullInt32, refillDay sql.NullInt16) *openapi.Credits { credits := &openapi.Credits{ Remaining: nullable.NewNullableWithValue(int64(remaining)), } if refillAmount.Valid { var day *int interval := openapi.Daily if refillDay.Valid { interval = openapi.Monthly day = ptr.P(int(refillDay.Int16)) } credits.Refill = &openapi.CreditsRefill{ Amount: int64(refillAmount.Int32), Interval: interval, RefillDay: day, } } return credits }Then use it across all handlers to reduce duplication and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (64)
go/apps/api/integration/multi_node_usagelimiting/run.go(1 hunks)go/apps/api/openapi/gen.go(1 hunks)go/apps/api/openapi/openapi-generated.yaml(1 hunks)go/apps/api/openapi/spec/common/Credits.yaml(1 hunks)go/apps/api/routes/register.go(2 hunks)go/apps/api/routes/v2_apis_list_keys/200_test.go(9 hunks)go/apps/api/routes/v2_apis_list_keys/handler.go(3 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(2 hunks)go/apps/api/routes/v2_identities_create_identity/400_test.go(2 hunks)go/apps/api/routes/v2_identities_create_identity/handler.go(2 hunks)go/apps/api/routes/v2_identities_get_identity/200_test.go(2 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(3 hunks)go/apps/api/routes/v2_identities_list_identities/handler.go(2 hunks)go/apps/api/routes/v2_identities_update_credits/200_test.go(1 hunks)go/apps/api/routes/v2_identities_update_credits/400_test.go(1 hunks)go/apps/api/routes/v2_identities_update_credits/401_test.go(1 hunks)go/apps/api/routes/v2_identities_update_credits/403_test.go(1 hunks)go/apps/api/routes/v2_identities_update_credits/404_test.go(1 hunks)go/apps/api/routes/v2_identities_update_credits/handler.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/200_test.go(3 hunks)go/apps/api/routes/v2_identities_update_identity/400_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/401_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/403_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/404_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/handler.go(14 hunks)go/apps/api/routes/v2_keys_add_permissions/401_test.go(0 hunks)go/apps/api/routes/v2_keys_add_permissions/403_test.go(2 hunks)go/apps/api/routes/v2_keys_add_roles/403_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_get_key/200_test.go(2 hunks)go/apps/api/routes/v2_keys_get_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_get_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_remove_permissions/404_test.go(2 hunks)go/apps/api/routes/v2_keys_reroll_key/200_test.go(2 hunks)go/apps/api/routes/v2_keys_reroll_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_set_permissions/403_test.go(1 hunks)go/apps/api/routes/v2_keys_update_credits/200_test.go(10 hunks)go/apps/api/routes/v2_keys_update_credits/401_test.go(1 hunks)go/apps/api/routes/v2_keys_update_credits/403_test.go(1 hunks)go/apps/api/routes/v2_keys_update_credits/handler.go(4 hunks)go/apps/api/routes/v2_keys_update_key/200_test.go(5 hunks)go/apps/api/routes/v2_keys_update_key/400_test.go(1 hunks)go/apps/api/routes/v2_keys_update_key/handler.go(7 hunks)go/apps/api/routes/v2_keys_update_key/three_state_test.go(11 hunks)go/apps/api/routes/v2_keys_verify_key/200_test.go(7 hunks)go/apps/api/routes/v2_keys_verify_key/handler.go(4 hunks)go/apps/api/routes/v2_keys_whoami/200_test.go(2 hunks)go/apps/api/routes/v2_keys_whoami/handler.go(2 hunks)go/go.mod(2 hunks)go/internal/services/keys/get.go(1 hunks)go/internal/services/keys/validation.go(2 hunks)go/internal/services/usagelimiter/interface.go(1 hunks)go/internal/services/usagelimiter/limit.go(2 hunks)go/internal/services/usagelimiter/redis.go(8 hunks)go/pkg/counter/redis.go(2 hunks)go/pkg/counter/redis_test.go(1 hunks)go/pkg/db/key_data.go(1 hunks)go/pkg/db/key_list_by_identity_id.sql_generated.go(1 hunks)go/pkg/db/key_list_live_by_key_space_id.sql_generated.go(4 hunks)go/pkg/db/querier_generated.go(2 hunks)go/pkg/db/queries/key_list_by_identity_id.sql(1 hunks)go/pkg/db/queries/key_list_live_by_key_space_id.sql(1 hunks)go/pkg/testutil/seed/seed.go(4 hunks)
💤 Files with no reviewable changes (1)
- go/apps/api/routes/v2_keys_add_permissions/401_test.go
🚧 Files skipped from review as they are similar to previous changes (28)
- go/apps/api/routes/v2_keys_set_permissions/403_test.go
- go/apps/api/openapi/openapi-generated.yaml
- go/apps/api/routes/v2_identities_get_identity/200_test.go
- go/apps/api/routes/v2_keys_add_roles/403_test.go
- go/apps/api/routes/v2_identities_update_identity/403_test.go
- go/apps/api/routes/v2_keys_remove_permissions/404_test.go
- go/go.mod
- go/apps/api/routes/v2_keys_whoami/200_test.go
- go/apps/api/openapi/spec/common/Credits.yaml
- go/apps/api/routes/v2_identities_list_identities/handler.go
- go/internal/services/keys/get.go
- go/apps/api/routes/v2_keys_add_permissions/403_test.go
- go/apps/api/routes/v2_identities_update_credits/401_test.go
- go/apps/api/openapi/gen.go
- go/apps/api/routes/v2_identities_update_credits/400_test.go
- go/apps/api/routes/v2_identities_update_identity/404_test.go
- go/apps/api/routes/v2_keys_reroll_key/200_test.go
- go/apps/api/routes/v2_identities_update_identity/400_test.go
- go/apps/api/routes/v2_keys_get_key/403_test.go
- go/pkg/db/queries/key_list_by_identity_id.sql
- go/apps/api/routes/v2_keys_reroll_key/403_test.go
- go/apps/api/routes/v2_identities_update_credits/404_test.go
- go/internal/services/keys/validation.go
- go/apps/api/routes/v2_identities_create_identity/400_test.go
- go/apps/api/routes/v2_identities_update_identity/401_test.go
- go/apps/api/routes/v2_keys_update_credits/401_test.go
- go/apps/api/integration/multi_node_usagelimiting/run.go
- go/pkg/db/querier_generated.go
🧰 Additional context used
🧠 Learnings (31)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.710Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
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.
📚 Learning: 2025-10-30T15:10:52.710Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.710Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Applied to files:
go/apps/api/routes/v2_identities_get_identity/handler.gogo/apps/api/routes/v2_identities_update_identity/200_test.gogo/apps/api/routes/v2_apis_list_keys/handler.gogo/pkg/db/queries/key_list_live_by_key_space_id.sqlgo/apps/api/routes/v2_keys_whoami/handler.gogo/apps/api/routes/v2_identities_create_identity/200_test.gogo/apps/api/routes/v2_keys_create_key/handler.gogo/apps/api/routes/v2_identities_update_credits/200_test.gogo/apps/api/routes/v2_keys_update_key/400_test.gogo/apps/api/routes/v2_identities_update_credits/403_test.gogo/pkg/testutil/seed/seed.gogo/apps/api/routes/v2_keys_verify_key/handler.gogo/internal/services/usagelimiter/redis.gogo/pkg/db/key_data.gogo/apps/api/routes/v2_apis_list_keys/200_test.gogo/apps/api/routes/v2_keys_update_key/200_test.gogo/apps/api/routes/v2_identities_create_identity/handler.gogo/apps/api/routes/v2_identities_update_credits/handler.gogo/internal/services/usagelimiter/interface.gogo/pkg/db/key_list_live_by_key_space_id.sql_generated.gogo/apps/api/routes/register.gogo/apps/api/routes/v2_keys_verify_key/200_test.gogo/apps/api/routes/v2_identities_update_identity/handler.gogo/apps/api/routes/v2_keys_reroll_key/handler.gogo/internal/services/usagelimiter/limit.gogo/pkg/db/key_list_by_identity_id.sql_generated.gogo/apps/api/routes/v2_keys_get_key/200_test.gogo/apps/api/routes/v2_keys_update_credits/403_test.gogo/apps/api/routes/v2_keys_update_key/handler.gogo/apps/api/routes/v2_keys_update_credits/handler.gogo/apps/api/routes/v2_keys_get_key/handler.gogo/apps/api/routes/v2_keys_update_key/three_state_test.gogo/apps/api/routes/v2_keys_update_credits/200_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: 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/routes/v2_identities_get_identity/handler.gogo/apps/api/routes/v2_identities_update_identity/200_test.gogo/apps/api/routes/v2_identities_create_identity/200_test.gogo/apps/api/routes/v2_keys_update_key/400_test.gogo/apps/api/routes/v2_identities_update_identity/handler.gogo/apps/api/routes/v2_keys_update_credits/403_test.gogo/apps/api/routes/v2_keys_update_credits/handler.gogo/apps/api/routes/v2_keys_update_key/three_state_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, 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/routes/v2_identities_get_identity/handler.gogo/apps/api/routes/v2_identities_update_identity/200_test.gogo/apps/api/routes/v2_identities_create_identity/200_test.gogo/apps/api/routes/v2_identities_update_identity/handler.gogo/apps/api/routes/v2_keys_update_key/three_state_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/routes/v2_identities_get_identity/handler.gogo/apps/api/routes/v2_identities_update_identity/200_test.gogo/apps/api/routes/v2_identities_update_identity/handler.go
📚 Learning: 2025-08-19T09:36:19.485Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3800
File: go/pkg/counter/redis.go:241-265
Timestamp: 2025-08-19T09:36:19.485Z
Learning: In Redis counter implementations using go-redis client, numeric values should not be returned as []byte. The go-redis client handles type conversion automatically, and if []byte appears for numeric data, it indicates a configuration problem rather than expected behavior that should be handled.
Applied to files:
go/pkg/counter/redis_test.gogo/pkg/counter/redis.go
📚 Learning: 2025-08-21T15:54:45.198Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3825
File: go/internal/services/usagelimiter/limit.go:38-0
Timestamp: 2025-08-21T15:54:45.198Z
Learning: In go/internal/services/usagelimiter/limit.go, the UpdateKeyCreditsDecrement operation cannot be safely wrapped with db.WithRetry due to the lack of idempotency mechanisms in the current tech stack. Retrying this non-idempotent write operation risks double-charging users if the first attempt commits but the client sees a transient error.
Applied to files:
go/apps/api/routes/v2_identities_update_identity/200_test.gogo/apps/api/routes/v2_keys_create_key/handler.gogo/apps/api/routes/v2_identities_update_credits/200_test.gogo/apps/api/routes/v2_keys_update_key/400_test.gogo/apps/api/routes/v2_identities_update_credits/403_test.gogo/pkg/testutil/seed/seed.gogo/apps/api/routes/v2_keys_verify_key/handler.gogo/internal/services/usagelimiter/redis.gogo/pkg/db/key_data.gogo/apps/api/routes/v2_apis_list_keys/200_test.gogo/apps/api/routes/v2_keys_update_key/200_test.gogo/apps/api/routes/v2_identities_create_identity/handler.gogo/apps/api/routes/v2_identities_update_credits/handler.gogo/internal/services/usagelimiter/interface.gogo/apps/api/routes/register.gogo/apps/api/routes/v2_keys_verify_key/200_test.gogo/apps/api/routes/v2_identities_update_identity/handler.gogo/apps/api/routes/v2_keys_reroll_key/handler.gogo/internal/services/usagelimiter/limit.gogo/apps/api/routes/v2_keys_get_key/200_test.gogo/apps/api/routes/v2_keys_update_credits/403_test.gogo/apps/api/routes/v2_keys_update_key/handler.gogo/apps/api/routes/v2_keys_update_credits/handler.gogo/apps/api/routes/v2_keys_get_key/handler.gogo/apps/api/routes/v2_keys_update_key/three_state_test.gogo/apps/api/routes/v2_keys_update_credits/200_test.go
📚 Learning: 2025-08-08T15:20:40.288Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:20:40.288Z
Learning: Repo unkeyed/unkey: oapi-codegen v2.4+ (v2.5.0 in use) supports output-options.overlay in go/apps/api/openapi/config.yaml; the generator applies overlay.yaml at codegen time, so no separate pre-step is required if oapi-codegen is invoked with -config=config.yaml.
Applied to files:
go/apps/api/routes/v2_identities_update_identity/200_test.go
📚 Learning: 2024-10-20T07:05:55.471Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 2294
File: apps/api/src/pkg/keys/service.ts:268-271
Timestamp: 2024-10-20T07:05:55.471Z
Learning: In `apps/api/src/pkg/keys/service.ts`, `ratelimitAsync` is a table relation, not a column selection. When querying, ensure that table relations are included appropriately, not as columns.
Applied to files:
go/pkg/db/queries/key_list_live_by_key_space_id.sqlgo/pkg/db/key_list_live_by_key_space_id.sql_generated.go
📚 Learning: 2025-08-19T09:46:03.702Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3800
File: go/internal/services/usagelimiter/redis.go:176-186
Timestamp: 2025-08-19T09:46:03.702Z
Learning: Zero-cost requests in the usage limiter should not change the remaining credits but should still return the actual remaining credits for that key, not treat the key as unlimited. A key with 100 credits remaining should still report 100 credits remaining after a zero-cost request.
Applied to files:
go/apps/api/routes/v2_keys_whoami/handler.gogo/apps/api/routes/v2_keys_create_key/handler.gogo/apps/api/routes/v2_identities_update_credits/200_test.gogo/apps/api/routes/v2_keys_verify_key/handler.gogo/internal/services/usagelimiter/redis.gogo/apps/api/routes/v2_keys_update_key/200_test.gogo/internal/services/usagelimiter/interface.gogo/apps/api/routes/v2_keys_verify_key/200_test.gogo/internal/services/usagelimiter/limit.gogo/apps/api/routes/v2_keys_update_key/handler.gogo/apps/api/routes/v2_keys_update_credits/handler.gogo/apps/api/routes/v2_keys_get_key/handler.gogo/apps/api/routes/v2_keys_update_key/three_state_test.gogo/apps/api/routes/v2_keys_update_credits/200_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/apps/api/routes/v2_identities_create_identity/200_test.gogo/apps/api/routes/v2_keys_create_key/handler.gogo/apps/api/routes/v2_identities_update_credits/200_test.gogo/apps/api/routes/v2_keys_update_key/400_test.gogo/apps/api/routes/v2_identities_update_credits/403_test.gogo/apps/api/routes/v2_apis_list_keys/200_test.gogo/apps/api/routes/v2_keys_update_key/200_test.gogo/apps/api/routes/v2_keys_verify_key/200_test.gogo/apps/api/routes/v2_keys_get_key/200_test.gogo/apps/api/routes/v2_keys_update_credits/403_test.gogo/apps/api/routes/v2_keys_update_key/handler.gogo/apps/api/routes/v2_keys_update_credits/handler.gogo/apps/api/routes/v2_keys_update_key/three_state_test.gogo/apps/api/routes/v2_keys_update_credits/200_test.go
📚 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/routes/v2_identities_create_identity/200_test.gogo/apps/api/routes/v2_keys_create_key/handler.gogo/apps/api/routes/v2_keys_update_key/handler.go
📚 Learning: 2025-08-19T09:42:40.919Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3800
File: go/internal/services/keys/validation.go:45-52
Timestamp: 2025-08-19T09:42:40.919Z
Learning: In go/internal/services/keys/validation.go, keys with unlimited usage (RemainingRequests.Valid = false) have an early return that bypasses the usage limiter entirely. The usage limiter is only called for keys with finite remaining request counts.
Applied to files:
go/apps/api/routes/v2_keys_create_key/handler.gogo/internal/services/usagelimiter/redis.gogo/apps/api/routes/v2_apis_list_keys/200_test.gogo/apps/api/routes/v2_keys_update_key/200_test.gogo/internal/services/usagelimiter/interface.gogo/apps/api/routes/v2_keys_verify_key/200_test.gogo/internal/services/usagelimiter/limit.gogo/apps/api/routes/v2_keys_update_key/handler.gogo/apps/api/routes/v2_keys_update_credits/handler.gogo/apps/api/routes/v2_keys_update_key/three_state_test.gogo/apps/api/routes/v2_keys_update_credits/200_test.go
📚 Learning: 2025-07-15T14:25:05.608Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3560
File: go/apps/api/routes/v2_keys_create_key/handler.go:353-466
Timestamp: 2025-07-15T14:25:05.608Z
Learning: In the Unkey codebase, input validation for API endpoints is handled at the OpenAPI schema layer, which validates request fields like permission slugs (pattern: "^[a-zA-Z0-9_]+$", length: 1-100 characters) before requests reach the handler code. This validation occurs during the zen.BindBody call in handlers.
Applied to files:
go/apps/api/routes/v2_keys_create_key/handler.go
📚 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/routes/v2_keys_create_key/handler.go
📚 Learning: 2025-04-22T11:48:39.670Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3156
File: apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/ratelimit-setup.tsx:36-47
Timestamp: 2025-04-22T11:48:39.670Z
Learning: The Unkey dashboard's form validation for numeric values like rate limits is handled through the Zod schema validation (with `.positive()` validators and additional checks in `superRefine`), rather than HTML input attributes like `min`.
Applied to files:
go/apps/api/routes/v2_keys_create_key/handler.go
📚 Learning: 2025-07-15T14:47:20.490Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3560
File: go/apps/api/routes/v2_keys_create_key/handler.go:468-581
Timestamp: 2025-07-15T14:47:20.490Z
Learning: In the Unkey codebase, role and permission names are validated at the OpenAPI schema layer with strict regex patterns: role names must match "^[a-zA-Z][a-zA-Z0-9_-]*$" (start with letter, followed by letters/numbers/underscores/hyphens) and permission names must match "^[a-zA-Z0-9_]+$" (letters, numbers, underscores only). This validation occurs during zen.BindBody call before handlers run, preventing malicious or improperly formatted names from reaching auto-creation logic.
Applied to files:
go/apps/api/routes/v2_keys_create_key/handler.go
📚 Learning: 2025-06-18T12:28:10.449Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3321
File: apps/dashboard/lib/trpc/routers/authorization/roles/keys/schema-with-helpers.ts:5-8
Timestamp: 2025-06-18T12:28:10.449Z
Learning: In the unkey dashboard application, API validation for pagination limits is controlled at the UI level rather than requiring additional server-side validation, as the APIs are internal and protected by UI logic.
Applied to files:
go/apps/api/routes/v2_keys_create_key/handler.go
📚 Learning: 2025-08-08T14:59:52.283Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Applied to files:
go/apps/api/routes/v2_keys_create_key/handler.gogo/apps/api/routes/v2_keys_update_credits/handler.go
📚 Learning: 2025-08-20T11:41:36.718Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3810
File: go/apps/api/routes/v2_ratelimit_limit/handler.go:54-56
Timestamp: 2025-08-20T11:41:36.718Z
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_keys_create_key/handler.gogo/apps/api/routes/v2_keys_reroll_key/handler.gogo/apps/api/routes/v2_keys_get_key/handler.go
📚 Learning: 2025-07-16T17:51:57.297Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3617
File: go/apps/api/openapi/openapi.yaml:3309-3312
Timestamp: 2025-07-16T17:51:57.297Z
Learning: In the Unkey API OpenAPI schema, the permissions query regex for the verifyKey endpoint intentionally allows all whitespace characters (including tabs and newlines) via `\s`. Do not flag this as an error in future reviews.
Applied to files:
go/apps/api/routes/v2_identities_update_credits/403_test.go
📚 Learning: 2025-10-21T09:45:47.560Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 4107
File: go/pkg/clickhouse/key_verifications_test.go:20-20
Timestamp: 2025-10-21T09:45:47.560Z
Learning: In ClickHouse tests (e.g., go/pkg/clickhouse/key_verifications_test.go), parallel execution with t.Parallel() is safe when tests use workspaceID-based isolation. Each test generates a unique workspaceID (via uid.New(uid.WorkspacePrefix)), and all database operations are scoped to that workspaceID, providing logical isolation even when tests share a single ClickHouse instance.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/200_test.go
📚 Learning: 2025-07-17T14:24:20.403Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.403Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.
Applied to files:
go/pkg/db/key_list_live_by_key_space_id.sql_generated.gogo/apps/api/routes/v2_keys_reroll_key/handler.gogo/pkg/db/key_list_by_identity_id.sql_generated.go
📚 Learning: 2025-08-14T17:45:14.448Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/handler.go:176-193
Timestamp: 2025-08-14T17:45:14.448Z
Learning: When rerolling keys in the v2 reroll endpoint, expiration dates should be copied from the original key to preserve business constraints like trial periods or temporary access policies. Users expect rerolled keys to maintain the same expiration limitations as the original.
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/handler.go
📚 Learning: 2025-09-12T08:01:20.792Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3944
File: go/pkg/db/acme_challenge_update_verified_with_expiry.sql_generated.go:31-39
Timestamp: 2025-09-12T08:01:20.792Z
Learning: Do not review or suggest changes to files with sql_generated.go suffix or other files marked as auto-generated (containing "Code generated by" comments), as these are generated by tools like sqlc and changes would be overwritten on regeneration.
Applied to files:
go/pkg/db/key_list_by_identity_id.sql_generated.go
📚 Learning: 2025-09-01T17:08:52.983Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3899
File: go/proto/metald/v1/storage.proto:7-12
Timestamp: 2025-09-01T17:08:52.983Z
Learning: When reviewing protobuf files, systematically check all int64 fields across all files in the changeset to ensure counters, sizes, and quantities use uint64 unless there's a specific reason they could be negative. Don't review files in isolation - look for patterns across the entire change.
Applied to files:
go/apps/api/routes/v2_keys_update_key/handler.go
📚 Learning: 2025-08-29T13:48:43.353Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3884
File: go/apps/api/routes/v2_ratelimit_delete_override/handler.go:218-228
Timestamp: 2025-08-29T13:48:43.353Z
Learning: In the unkeyed/unkey codebase, when working with ratelimit namespace caching, req.Namespace parameter is either the namespace ID or the namespace name, so cache invalidation only needs to remove cache keys for namespace.ID and namespace.Name - no need to also remove req.Namespace as a separate key.
Applied to files:
go/apps/api/routes/v2_keys_update_key/handler.go
📚 Learning: 2025-08-08T15:10:46.436Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Applied to files:
go/apps/api/routes/v2_keys_update_credits/handler.go
📚 Learning: 2025-08-08T16:10:00.224Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Applied to files:
go/apps/api/routes/v2_keys_update_credits/handler.go
📚 Learning: 2025-07-16T15:38:53.491Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.491Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.
Applied to files:
go/apps/api/routes/v2_keys_update_key/three_state_test.gogo/apps/api/routes/v2_keys_update_credits/200_test.go
📚 Learning: 2025-07-02T11:51:58.572Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3420
File: go/pkg/hydra/store/gorm/gorm.go:486-498
Timestamp: 2025-07-02T11:51:58.572Z
Learning: The Hydra package (go/pkg/hydra) is planned to be migrated from GORM to sqlc for database operations, which explains why raw SQL queries are acceptable in the current implementation.
Applied to files:
go/apps/api/routes/v2_keys_update_credits/200_test.go
🔇 Additional comments (48)
go/pkg/db/key_list_by_identity_id.sql_generated.go (1)
1-50: Skipping review of auto-generated file.This file is auto-generated by sqlc and should not be reviewed or modified directly. Any necessary changes should be made to the source SQL file (
go/pkg/db/queries/key_list_by_identity_id.sql), which will regenerate this file when sqlc runs.Based on learnings.
go/pkg/counter/redis.go (2)
22-24: LGTM! Clear documentation of zero-cost behavior.The documentation accurately describes the new early-return path for zero-cost decrements.
39-43: Excellent implementation of zero-cost decrement optimization.The early-return path for zero-cost operations is well-placed and handles the edge case correctly. The logic ensures that:
- Non-existent keys still return
existed=false, success=false(caught by the earlier existence check)- Existing keys return
existed=true, success=truewith the value unchanged- No unnecessary Redis operations are performed
This is particularly valuable in the identity credits context where operations may legitimately have zero cost.
go/pkg/counter/redis_test.go (2)
918-943: Excellent test coverage for zero-cost decrement on existing keys.This test thoroughly validates the zero-cost decrement path by:
- Verifying the operation succeeds with correct flags (
existed=true,success=true)- Confirming the counter value remains unchanged
- Testing idempotency with a second zero-cost call
Well-structured and clear test case.
945-954: Good edge case coverage for zero-cost decrement on non-existent keys.This test validates the important edge case where a zero-cost decrement is attempted on a non-existent key. The correct behavior (
existed=false,success=false) confirms that the existence check in the Lua script properly takes precedence over the zero-cost optimization path.go/apps/api/routes/v2_keys_reroll_key/handler.go (2)
177-188: LGTM! Key properties correctly preserved.The key creation properly copies all relevant fields from the original key, including the
Expirestimestamp to maintain business constraints like trial periods.
287-327: Previous RefilledAt issue resolved—credit migration logic is correct.The critical issue flagged in the previous review has been fixed. Both credit paths (lines 301, 315) now correctly preserve the original
RefilledAttimestamp by copying bothValidandInt64fields, ensuring refill cadence remains intact after reroll. The dual-path migration logic (new credits table → new table, legacy → new table) is sound and properly handles all edge cases within the transaction.go/apps/api/routes/v2_keys_update_credits/403_test.go (1)
45-47: LGTM! Test payload updated to new Credits API structure.The change correctly migrates from the legacy
Remainingfield to the nestedCreditsstructure, aligning with the new dual-credit system API.go/apps/api/routes/v2_keys_update_credits/200_test.go (2)
54-57: LGTM! Test migrated to new Credits API structure.The test correctly uses the nested
Creditsstructure withseed.CreditRequest, aligning with the new dual-credit system.
218-422: Excellent legacy credit system test coverage!These tests comprehensively cover all legacy credit operations (Set, Increment, Decrement), edge cases (unlimited, below-zero clamping), and refill configuration preservation. The tests verify both API responses and database state, ensuring backward compatibility during the migration to the new credit system.
go/apps/api/routes/v2_keys_update_credits/handler.go (1)
159-185: Document the intentional design decision or fix the refill config inconsistency in Set operation.The Set operation creates/updates credit records with RefillDay and RefillAmount set to null (with Specified flags = 0, meaning "preserve existing values"), yet populates the response from the key's legacy refill configuration rather than from the credit record itself. This creates a discrepancy: the database credit record has null or preserved values, but the API response shows the key's legacy config.
On subsequent reads (e.g., via
FindCreditsByKeyID), the credit record will return its actual stored values (null or previously set), not the legacy key config. This inconsistency should either be:
- Clarified in a comment if this is intentional interim behavior (per learning notes indicating logic is deferred to a later PR)
- Fixed by either: (a) preserving/copying refill config into the credit record, or (b) always returning credit record values, not key legacy values
go/internal/services/usagelimiter/interface.go (2)
8-16: LGTM: Clear documentation for dual identifier support.The updated
Invalidatesignature and comprehensive documentation properly handle both legacy key-based credits and the new credits system. The explanation about attempting to invalidate both formats is helpful for callers.
22-29: LGTM: Well-structured dual credit support.The
UsageRequestrefactoring clearly separates the newCreditIDfrom the legacyKeyIDpath, with explicit documentation about precedence. This design supports a smooth migration from legacy to new credits.go/apps/api/routes/v2_keys_create_key/handler.go (2)
273-284: LGTM: Clean enabled status handling and key insertion.The enabled status logic properly defaults to true when not specified, and the key insertion error handling provides clear public messages.
286-338: LGTM: Credit creation logic is well-structured.The credit creation flow correctly:
- Checks for the presence of
req.Creditsand specifiedRemaining- Handles monthly vs daily refill intervals appropriately (monthly requires
refillDay, daily sets it to NULL)- Inserts credits within the same transaction as key creation
- Provides clear error messages on failures
The transactional boundary ensures atomicity between key and credit creation.
go/apps/api/routes/v2_keys_verify_key/handler.go (3)
27-27: LGTM: Explicit type for DefaultCost.Typing
DefaultCostasint32ensures consistency with theCredits.Costfield type and prevents potential type conversion issues.
130-134: LGTM: Consistent cost handling.The logic properly defaults to
DefaultCostwhen no custom cost is specified and always appendsWithCredits(cost)to ensure credit checks are consistently applied.
183-192: LGTM: Correct credits priority implementation.The credits population follows the documented priority order:
- Identity credits (highest priority)
- Key credits
- Legacy
RemainingRequests(fallback)- Unlimited (nil)
This ensures the new credits system takes precedence while maintaining backward compatibility.
go/pkg/testutil/seed/seed.go (4)
212-216: LGTM: Clean credit data encapsulation.The
CreditRequesttype provides a clear, reusable structure for credit data across both key and identity creation paths.
198-210: LGTM: Well-structured migration support.The addition of
CreditsalongsideLegacy*fields enables tests to exercise both the new credits table path and the legacyRemainingRequestspath, supporting gradual migration.
244-264: LGTM: Consistent credit insertion for keys.The credit insertion logic correctly:
- Generates a unique credit ID
- Maps key ID to
sql.NullString{String: keyID, Valid: true}- Sets identity ID to invalid (key-scoped credits)
- Handles optional
RefillDayandRefillAmount
417-431: LGTM: Consistent credit insertion for identities.The identity credit insertion mirrors the key credit logic with appropriate field mappings (identity ID valid, key ID invalid), ensuring consistent behavior across both paths.
go/apps/api/routes/v2_keys_get_key/200_test.go (2)
211-234: LGTM: Comprehensive daily refill test coverage.The test properly validates:
- Credits creation with the nested
seed.CreditRequeststructure- Daily refill interval detection (no
RefillDayspecified)- Correct serialization in the response payload
236-259: LGTM: Thorough monthly refill test coverage.The test correctly validates monthly refill behavior, including the presence and value of
RefillDayin the response.go/pkg/db/queries/key_list_live_by_key_space_id.sql (1)
79-98: LGTM: Credit data properly surfaced in live key query.The query correctly:
- Uses LEFT JOINs to preserve keys without credits
- Fetches both key-scoped credits (
kc.key_id = k.id) and identity-scoped credits (ic.identity_id = i.id)- Includes all necessary credit fields for both types
The dual credit paths enable the priority-based credit selection logic in handlers.
go/apps/api/routes/register.go (1)
228-247: LGTM: Proper dependency wiring for credit routes.Both routes are correctly configured:
updateIdentitynow includesUsageLimiterfor credit invalidation when identity changes- New
updateCreditsroute includes all necessary dependencies (Logger,DB,Keys,Auditlogs,UsageLimiter,KeyCache)The
KeyCachedependency ensures proper cache invalidation when credits are updated.go/apps/api/routes/v2_identities_create_identity/200_test.go (6)
442-466: LGTM: Clean test for credits-only identity creation.The test properly validates that credits are created and persisted with the correct remaining value.
468-501: LGTM: Thorough monthly refill test.The test correctly validates:
- Credits creation with refill configuration
- Monthly interval with required
RefillDay- Proper persistence of all refill parameters
503-532: LGTM: Daily refill test validates NULL RefillDay.The test correctly verifies that daily refill intervals result in
RefillDaybeing NULL (Valid=false) as expected.
534-556: LGTM: Unlimited credits test validates no credit record.The test properly validates that when
Remainingis nullable null (unlimited), no credit record is created (CreditID.Valid = false).
558-578: LGTM: Test validates identities without credits.The test confirms that identities can be created without credits, maintaining backward compatibility.
580-640: LGTM: Comprehensive integration test.The test validates that all features (metadata, ratelimits, credits with refill) work together correctly, ensuring no conflicts between different components.
go/apps/api/routes/v2_identities_get_identity/handler.go (1)
124-156: LGTM! Credits construction follows consistent patterns.The logic correctly:
- Checks for valid credit ID before constructing Credits
- Populates Remaining from the identity credit data
- Constructs Refill configuration only when RefillAmount is valid
- Maps intervals appropriately (Daily by default, Monthly when RefillDay is present)
go/internal/services/usagelimiter/limit.go (2)
16-26: LGTM! Priority routing correctly implemented.The routing logic correctly prioritizes:
- CreditID (new credits system)
- KeyID (legacy system)
- Returns invalid with 0 remaining when neither is provided
This aligns with the dual-credit migration strategy described in the PR objectives.
75-96: LGTM! Shared processing logic is clean and correct.The
processLimitfunction correctly:
- Returns the actual
remainingvalue (not 0) when denying usage at line 80, which addresses the past review comment- Handles zero-cost requests without decrementing
- Uses
max(0, remaining-cost)to prevent negative values- Delegates decrement operations via the pluggable function
Based on learnings.
go/apps/api/routes/v2_keys_verify_key/200_test.go (3)
104-218: LGTM! Comprehensive test coverage for new credits system.The tests cover all key scenarios:
- Default credit cost (lines 105-124)
- Exceeding credits (lines 126-145)
- Custom credit costs (lines 147-169)
- Zero-cost requests even with 0 remaining (lines 195-217)
This ensures the new credits table system behaves correctly.
220-374: LGTM! Identity credits test coverage is thorough.These tests validate:
- Identity-level credit enforcement
- Default and custom costs with identity credits
- Denial behavior when identity credits are exhausted
- Zero-cost requests with identity credits
The pattern mirrors key credits tests, ensuring consistent behavior across both credit sources.
376-551: LGTM! Legacy credits backward compatibility validated.The test suite ensures:
- Legacy
remaining_requestsfield continues to work- Daily and monthly refill configurations are preserved
- High remaining values are handled correctly
This validates the dual-path migration strategy where both systems coexist.
go/apps/api/routes/v2_apis_list_keys/handler.go (1)
217-223: LGTM! Improved decryption error handling.The change from silent failure to logging and continuing is appropriate. This allows the endpoint to return partial results when some keys fail to decrypt, rather than failing entirely.
go/apps/api/routes/v2_apis_list_keys/200_test.go (2)
491-734: LGTM! Comprehensive credits test coverage.The test suite validates:
- New credits table system (lines 491-529)
- Daily refill configuration (lines 531-572)
- Monthly refill with refillDay (lines 574-618)
- Legacy credits system (lines 620-658)
- Legacy refill configurations (lines 660-702)
- Unlimited credits (lines 704-734)
This ensures the dual-credit system works correctly across all configurations.
736-941: LGTM! Identity credits scenarios well covered.The tests validate:
- Identity-level credits without key credits
- Daily and monthly refill for identities
- Keys with both key-level and identity-level credits (lines 887-941)
The combined scenario test is particularly valuable for ensuring the dual-credit display works correctly.
go/apps/api/routes/v2_identities_update_identity/handler.go (2)
382-582: LGTM! Credits update logic is comprehensive.The implementation correctly handles:
- Deleting credits when set to null (lines 383-422)
- Deleting credits when remaining is set to null (lines 427-466)
- Upserting credits with proper refill configuration (lines 468-580)
- Using the
*Specifiedflags to avoid touching unmodified fields- Preserving existing credit IDs or generating new ones as appropriate
- Proper audit logging for all operations
636-676: LGTM! Cache invalidation strategy is thorough.The invalidation logic correctly:
- Invalidates the old credit ID if it existed
- Invalidates the new credit ID if newly created
- Finds all keys belonging to the identity via
ListKeysByIdentityID- Invalidates the key cache for all affected keys
This ensures cache coherency when identity credits change, which is critical for the credit priority system.
go/apps/api/routes/v2_identities_update_credits/handler.go (3)
204-217: LGTM! Refill data preservation is correct.The Set operation now correctly handles refill metadata:
- Lines 205-207: When
hasCreditsis true, it copies the entirecurrentCreditsstruct (which includesRefillDay,RefillAmount,RefilledAt) and only updatesRemaining- Lines 209-216: When creating new credits without an existing record, it correctly initializes without refill configuration
This addresses the past review comment about preserving refill data in Set responses.
329-353: LGTM! Cache invalidation is comprehensive.The implementation correctly:
- Invalidates the usage limiter cache for the credit ID (lines 330-337)
- Finds all keys associated with the identity (lines 340-346)
- Invalidates the key cache for all affected keys (lines 348-353)
This ensures proper cache coherency when identity credits are updated, which is critical for the dual-credit priority system.
182-196: Refill configuration is preserved as intended.The
UpsertCreditSQL implementation correctly respects the*Specifiedflags. WhenRefillDaySpecifiedandRefillAmountSpecifiedare set to0, theCASEstatements evaluate to false and execute theELSEbranch, preserving existing refill configuration:refill_day = CASE WHEN CAST(? AS UNSIGNED) = 1 THEN VALUES(refill_day) ELSE refill_day -- preserves existing value when flag = 0 ENDThe handler's settings are correct:
RemainingSpecified=1updates the remaining balance, whileRefillDaySpecified=0andRefillAmountSpecified=0leave existing refill configuration untouched.go/apps/api/routes/v2_identities_update_credits/200_test.go (1)
161-224: Cache invalidation scenario is solidGreat job wiring the identity update through the limiter cache — the before/after verification makes it obvious that stale Redis state is flushed alongside the DB write. This should catch regressions quickly.
go/apps/api/routes/v2_keys_update_key/three_state_test.go (1)
333-661: Great coverage on credit state transitions.Appreciate how these subtests walk through every permutation (null, partial updates, refill-only tweaks, and complex sequences). It gives strong confidence the handler’s three-state logic won’t regress.
| } | ||
| } | ||
| // Set enabled status (default true) | ||
| if req.Enabled != nil { |
There was a problem hiding this comment.
nit: use ptr.SafeDeref(req.Enabled, true)
|
|
||
| // withCredits validates that the key has sufficient usage credits and deducts the specified cost. | ||
| // It updates the key's remaining request count and marks the key as invalid if the limit is exceeded. | ||
| // Priority: Identity credits > Key credits > Legacy key.remaining_requests |
There was a problem hiding this comment.
this is a bit weird
idk if we this precedence makes sense or reversed or whether we should deduct from both key and identity....
if you have an identity with 20 credits and it owns a key with 10 credits, what do you expect the outcome after a verification to be?
- 20/9
- 19/10
- 19/9
cc @perkinsjr
| return s.counter.Delete(ctx, s.redisKey(keyID)) | ||
| func (s *counterService) Invalidate(ctx context.Context, identifier string) error { | ||
| // Try both possible key formats since we don't know which system this is | ||
| _ = s.counter.Delete(ctx, s.creditRedisKey(identifier)) |
There was a problem hiding this comment.
nit: we should refactor Delete to accept multiple keys, redis command under the hood already supports that https://redis.io/docs/latest/commands/del/
| toolchain go1.25.1 | ||
|
|
||
| require ( | ||
| buf.build/gen/go/depot/api/connectrpc/go v1.19.0-20250915125527-3af9e416de91.1 |
There was a problem hiding this comment.
these seem unrelated to the changes in this PR
Flo4604
force-pushed
the
feat/identity-credits-openapi
branch
from
eed7ae0 to
d20a48c
Compare
|
Thank you for following the naming conventions for pull request titles! 🙏 |
- Update usagelimiter service to support both key and credit-based limits - Add credit validation logic to keys service - Implement v2 identities.updateCredits endpoint with comprehensive tests - Update all v2 key endpoints to support credits field - Update v2 identity endpoints to include credits information - Add support for increment/decrement/set operations on credits - Update test utilities to support credit operations
- Fixed v2_identities_update_identity handler to use single struct return from FindIdentity - Fixed v2_keys_create_key handler to use openapi.Monthly instead of undefined constant - Added ListKeysByIdentityID query to find keys for cache invalidation - Fixed integration test to use Credits field in CreateKeyRequest 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Updates go.mod and go.sum to include depot and buildkit dependencies needed by ctrl service's depot build backend. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Updated CreateKeyRequest to use Credits field instead of deprecated Remaining, RefillAmount, and RefillDay fields. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
2 participants
What does this PR do?
This pr adds identity based credit logic to the V2 API, here we also work in the assumption that we either have credits in the keys or credits table and will work of whatever is availiable for later migration.
This also adds our (first) go cli migration script!
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated