fix: create key with remaining null shouldn't panic by Flo4604 · Pull Request #4297 · unkeyed/unkey

Flo4604 · 2025-11-10T12:44:08Z

What does this PR do?

Type of change

Bug fix (non-breaking change which fixes an issue)
Chore (refactoring code, technical debt, workflow improvements)
Enhancement (small improvements)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How should this be tested?

See issue example code.

Test remaining: null without refill (should return 200 - success) and not panic

  curl -X POST http://127.0.0.1:7070/v2/keys.createKey \
    -H "Authorization: Bearer YOUR_ROOT_KEY" \
    -H "Content-Type: application/json" \
    -d '{
      "apiId": "YOUR_API_ID",
      "credits": {
        "remaining": null
      }
    }'

Test remaining: null with refill (should return 400 - error):

  curl -X POST http://127.0.0.1:7070/v2/keys.createKey \
    -H "Authorization: Bearer YOUR_ROOT_KEY" \
    -H "Content-Type: application/json" \
    -d '{
      "apiId": "YOUR_API_ID",
      "credits": {
        "remaining": null,
        "refill": {
          "amount": 100,
          "interval": "daily"
        }
      }
    }'

replace root key apiId and port if necessary.

Checklist

Required

Appreciated

If a UI change was made: Added a screen recording or screenshots to this PR
Updated the Unkey Docs if changes were necessary

changeset-bot · 2025-11-10T12:44:12Z

⚠️ No Changeset found

Latest commit: 08ef81a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel · 2025-11-10T12:44:15Z

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
dashboard	Ignored	Preview		Nov 10, 2025 0:45am
engineering	Ignored	Preview		Nov 10, 2025 0:45am

coderabbitai · 2025-11-10T12:44:24Z

📝 Walkthrough

Walkthrough

The PR fixes a panic occurring when creating an API key with null credits.remaining by adding validation that requires credits.remaining to be specified (non-null) when credits.refill is provided. Adds corresponding test cases for both success and error scenarios.

Changes

Cohort / File(s)	Summary
Test Cases `go/apps/api/routes/v2_keys_create_key/200_test.go`, `400_test.go`	Adds new test `TestCreateKeyWithCreditsRemainingNull` to verify null credits.remaining without refill succeeds (200), and new subtest in `TestCreateKeyBadRequest` to verify null credits.remaining with refill fails (400). Both import nullable from oapi-codegen.
Validation Logic `go/apps/api/routes/v2_keys_create_key/handler.go`	Introduces guard clause requiring credits.remaining to be non-null when credits.refill is provided; returns validation fault if constraint violated. Preserves existing behavior for optional credits.remaining when refill is absent.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Handler
    participant Validator
    
    Client->>Handler: POST /v2/keys.createKey
    
    alt Refill provided with null credits.remaining
        Handler->>Validator: Check credits.remaining is non-null
        Validator->>Handler: Constraint violated
        Handler->>Client: 400 (Validation Fault)
    else Null credits.remaining without refill
        Handler->>Validator: Check constraints
        Validator->>Handler: Valid
        Handler->>Client: 200 (Success)
    else Non-null credits.remaining
        Handler->>Validator: Check constraints
        Validator->>Handler: Valid
        Handler->>Client: 200 (Success)
    end

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Straightforward validation logic with guard clause pattern
Test cases follow established patterns in the codebase
Changes are cohesive and narrowly scoped to address the specific panic issue
No complex branching or edge cases beyond the refill-dependency constraint

Possibly related PRs

#3443: Modifies credits/refill handling logic in the same handler file for key creation

Suggested labels

wakes you up

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: preventing a panic when creating a key with credits.remaining set to null.
Description check	✅ Passed	The PR description covers all required sections: issue reference (#4296), bug fix type, detailed testing instructions with curl examples, and completed checklist items.
Linked Issues check	✅ Passed	The PR fully addresses issue #4296 by fixing the panic caused by null credits.remaining, adding validation for the refill case, and including tests for both scenarios.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the panic issue #4296: handler validation logic, test coverage for null cases, and imports supporting nullable type handling.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/create_key-panic

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6a3148c and 08ef81a.

📒 Files selected for processing (3)

go/apps/api/routes/v2_keys_create_key/200_test.go (2 hunks)
go/apps/api/routes/v2_keys_create_key/400_test.go (2 hunks)
go/apps/api/routes/v2_keys_create_key/handler.go (1 hunks)

🧰 Additional context used

🧠 Learnings (8)

📓 Common learnings

Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.

Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
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: 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.

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.

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.

📚 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_keys_create_key/400_test.go
go/apps/api/routes/v2_keys_create_key/200_test.go

📚 Learning: 2025-10-30T15:10:52.743Z

Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
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_keys_create_key/400_test.go
go/apps/api/routes/v2_keys_create_key/200_test.go
go/apps/api/routes/v2_keys_create_key/handler.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_keys_create_key/400_test.go
go/apps/api/routes/v2_keys_create_key/handler.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_keys_create_key/400_test.go
go/apps/api/routes/v2_keys_create_key/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_keys_create_key/400_test.go
go/apps/api/routes/v2_keys_create_key/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: 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_keys_create_key/400_test.go
go/apps/api/routes/v2_keys_create_key/200_test.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.go

🧬 Code graph analysis (2)

go/apps/api/routes/v2_keys_create_key/400_test.go (3)

go/apps/api/routes/v2_keys_create_key/handler.go (1)

Request (31-31)

go/apps/api/openapi/gen.go (4)

KeyCreditsData (190-196)

KeyCreditsRefill (199-210)

KeyCreditsRefillIntervalDaily (16-16)

BadRequestErrorResponse (65-71)

go/pkg/testutil/http.go (1)

CallRoute (385-419)

go/apps/api/routes/v2_keys_create_key/200_test.go (3)

go/pkg/testutil/http.go (1)

CallRoute (385-419)

go/apps/api/routes/v2_keys_create_key/handler.go (3)

Handler (35-41)

Request (31-31)

Response (32-32)

go/apps/api/openapi/gen.go (1)

KeyCreditsData (190-196)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: Test Go API Local / Test
GitHub Check: Test API / API Test Local
GitHub Check: Build / Build

🔇 Additional comments (6)

go/apps/api/routes/v2_keys_create_key/400_test.go (2)

9-9: LGTM!

Import added to support the new test case using nullable values.

172-188: Excellent test coverage for the validation rule.

The test correctly verifies that providing a null credits.remaining with a non-null refill returns HTTP 400 with an appropriate error message. This directly addresses the panic issue from #4296.

go/apps/api/routes/v2_keys_create_key/handler.go (2)

275-284: Excellent validation to prevent the panic.

This validation correctly addresses issue #4296 by ensuring credits.remaining must be specified and non-null when credits.refill is provided. The error message clearly explains the requirement to the user.

286-291: Proper defensive guard for MustGet().

The additional !req.Credits.Remaining.IsNull() check prevents the panic by ensuring MustGet() is only called when the value is actually present. Combined with the validation above, this provides defense in depth.

go/apps/api/routes/v2_keys_create_key/200_test.go (2)

9-11: LGTM!

Necessary imports added for the new test case.

295-336: Good test coverage for the success case.

This test correctly verifies that creating a key with credits.remaining explicitly set to null (for unlimited credits) succeeds when no refill is provided. This ensures the fix doesn't break the existing behavior for unlimited credit keys.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2025-11-10T12:45:04Z

Thank you for following the naming conventions for pull request titles! 🙏

ogzhanolguncu

LGTM. Great test cases LFG

graphite-app · 2025-11-10T13:01:19Z

graphite-app · 2025-11-10T13:05:11Z

Graphite Automations

"Post a GIF when PR approved" took an action on this PR • (11/10/25)

1 gif was posted to this PR based on Andreas Thomas's automation.

fix: add tests

18a5ca2

Flo4604 requested review from chronark, imeyer and perkinsjr as code owners November 10, 2025 12:44

fix: move test into correct place

08ef81a

Flo4604 changed the title ~~fix: add tests~~ fix: create key with remaining null shouldn't panic Nov 10, 2025

chronark approved these changes Nov 10, 2025

View reviewed changes

ogzhanolguncu approved these changes Nov 10, 2025

View reviewed changes

Flo4604 added this pull request to the merge queue Nov 10, 2025

Merged via the queue into main with commit fc9aa20 Nov 10, 2025
25 of 26 checks passed

Flo4604 deleted the fix/create_key-panic branch November 10, 2025 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: create key with remaining null shouldn't panic#4297

fix: create key with remaining null shouldn't panic#4297
Flo4604 merged 2 commits intomainfrom
fix/create_key-panic

Flo4604 commented Nov 10, 2025 •

edited

Loading

Uh oh!

changeset-bot bot commented Nov 10, 2025 •

edited

Loading

Uh oh!

vercel bot commented Nov 10, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Nov 10, 2025 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested labels

Uh oh!

github-actions bot commented Nov 10, 2025 •

edited

Loading

Uh oh!

ogzhanolguncu left a comment

Uh oh!

graphite-app bot commented Nov 10, 2025

Uh oh!

graphite-app bot commented Nov 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Flo4604 commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Type of change

How should this be tested?

Checklist

Required

Appreciated

Uh oh!

changeset-bot bot commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

vercel bot commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested labels

Pre-merge checks and finishing touches

Uh oh!

github-actions bot commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogzhanolguncu left a comment

Choose a reason for hiding this comment

Uh oh!

graphite-app bot commented Nov 10, 2025

Uh oh!

graphite-app bot commented Nov 10, 2025

Graphite Automations

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Flo4604 commented Nov 10, 2025 •

edited

Loading

changeset-bot bot commented Nov 10, 2025 •

edited

Loading

vercel bot commented Nov 10, 2025 •

edited

Loading

coderabbitai bot commented Nov 10, 2025 •

edited

Loading

github-actions bot commented Nov 10, 2025 •

edited

Loading