Add comprehensive TTL cache tests #78
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds three test-related files: unit tests for TTLCache UseOnce and pruning behavior, an integration test suite for token middleware using a PostgreSQL testcontainer exercising signature, nonce, and rate-limit checks, and a test helper to construct a database Connection from a GORM *gorm.DB. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Repo
participant NonceCache
participant RateLimiter
participant Handler
Client->>Middleware: HTTP request (signature, public, nonce, headers)
Middleware->>Repo: Lookup API key / public key
alt key not found or public mismatch
Middleware-->>Client: 401 Unauthorized
else
Middleware->>Middleware: Verify signature
alt signature invalid
Middleware-->>Client: 401 Unauthorized
else
Middleware->>NonceCache: UseOnce/Mark(nonce, ttl)
alt nonce replay detected
Middleware-->>Client: 401 Unauthorized
else
Middleware->>RateLimiter: Check limit for scope
alt rate limited
Middleware-->>Client: 429 Too Many Requests
else
Middleware->>Handler: Forward request
Handler-->>Client: 200 OK (or handler response)
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Suggested labels
Poem
✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @gocanto, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've added comprehensive test cases for the TTLCache to ensure its robustness and correctness. These new tests specifically validate the UseOnce functionality across various scenarios and confirm that expired entries are properly pruned from the cache, improving the reliability of the cache implementation.
Highlights
- TTLCache UseOnce Behavior: I've added TestTTLCache_UseOnce to thoroughly test the
UseOncemethod, ensuring it behaves as expected for initial usage, subsequent attempts within the TTL, and successful reuse once the TTL has expired. - TTLCache Expiry Pruning: I've included TestTTLCache_Mark_PrunesExpiredEntries to confirm that the
Markmethod correctly triggers the pruning of expired entries from the cache, maintaining cache efficiency.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds valuable test coverage for the TTL cache's UseOnce and expiry pruning logic. The tests cover the main scenarios correctly. I've provided a couple of suggestions to improve the tests: one to enhance readability and another to fix a logic issue in a test case that could lead to false positives. Once these are addressed, the changes will be in great shape.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/cache/ttl_cache_useonce_test.go (2)
26-28: Doc comment formatting: remove blank line to keep GoDoc associationKeep the test’s doc comment contiguous so it remains attached to the function.
-// TestTTLCache_Mark_PrunesExpiredEntries ensures that calling Mark prunes -// -// any expired keys in the cache. +// TestTTLCache_Mark_PrunesExpiredEntries ensures that calling Mark prunes +// any expired keys in the cache.
1-37: Optional additional coverage ideas
- UseOnce TTL refresh behavior: verify whether successive UseOnce calls before expiry extend or preserve the original TTL window.
- Mark refresh semantics: Mark the same key before expiry and assert it remains Used after the original TTL passes but before the refreshed TTL elapses.
- Zero/negative TTL handling: define expected behavior (immediate expiry vs. rejection).
- Multiple keys: ensure independence and no cross-talk between keys.
Happy to draft these tests if you want them in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/cache/ttl_cache_useonce_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/cache/ttl_cache_useonce_test.go (1)
pkg/cache/ttl_cache.go (1)
NewTTLCache(18-22)
🔇 Additional comments (1)
pkg/cache/ttl_cache_useonce_test.go (1)
1-37: Overall: good, focused tests that capture core TTL semanticsClear coverage for UseOnce behavior and expiry-triggered pruning intent.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
pkg/middleware/token_middleware_additional_test.go (2)
92-105: Public token mismatch path covered; additionally assert middleware didn’t call nextOptional: ensure the next handler wasn’t invoked when unauthorized (defensive regression check).
- next := func(w http.ResponseWriter, r *http.Request) *pkgHttp.ApiError { return nil } + nextCalled := 0 + next := func(w http.ResponseWriter, r *http.Request) *pkgHttp.ApiError { nextCalled++; return nil } handler := tm.Handle(next) @@ if err := handler(rec, req); err == nil || err.Status != http.StatusUnauthorized { t.Fatalf("expected unauthorized for public token mismatch, got %#v", err) } + if nextCalled != 0 { + t.Fatalf("next should not be invoked on unauthorized") + }
107-121: Signature mismatch path covered; additionally assert middleware didn’t call nextAnalogous to the token mismatch test, assert that next isn’t called.
- next := func(w http.ResponseWriter, r *http.Request) *pkgHttp.ApiError { return nil } + nextCalled := 0 + next := func(w http.ResponseWriter, r *http.Request) *pkgHttp.ApiError { nextCalled++; return nil } handler := tm.Handle(next) @@ if err := handler(rec, req); err == nil || err.Status != http.StatusUnauthorized { t.Fatalf("expected unauthorized for signature mismatch, got %#v", err) } + if nextCalled != 0 { + t.Fatalf("next should not be invoked on unauthorized") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/cache/ttl_cache_useonce_test.go(1 hunks)pkg/middleware/token_middleware_additional_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cache/ttl_cache_useonce_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/middleware/token_middleware_additional_test.go (10)
database/repository/api_keys.go (1)
ApiKeys(11-13)pkg/auth/handler.go (2)
TokenHandler(10-14)MakeTokensHandler(16-26)pkg/auth/schema.go (1)
Token(11-17)database/model.go (1)
APIKey(26-35)database/connection.go (1)
Connection(12-17)pkg/cache/ttl_cache.go (1)
NewTTLCache(18-22)pkg/limiter/limiter.go (1)
NewMemoryLimiter(21-27)pkg/http/schema.go (1)
ApiError(10-13)handler/tests/db.go (1)
MakeTestDB(18-88)database/repository/repository_test.go (1)
setupDB(18-77)
🔇 Additional comments (2)
pkg/middleware/token_middleware_additional_test.go (2)
1-168: Overall: solid integration coverage; minor lifecycle and assertion tweaks recommendedThe scenarios covered are thorough and valuable. Address the container lifecycle context, close GORM DB, and consider the suggested assertion improvements. The reflect/unsafe injection deserves a safer test helper.
78-90: slogNoop helper is present and accessible
TheslogNoop()function is defined inpkg/middleware/valid_timestamp_test.gounderpackage middleware, so it’s visible to your new test. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/middleware/token_middleware_additional_test.go (1)
1-1: Consider isolating these as integration testsSpinning containers on plain go test ./... increases runtime. Consider an integration build tag or env-gated skip to control when these run.
Example:
//go:build integration // or inside TestMain / each test: if os.Getenv("INTEGRATION") == "" { t.Skip("set INTEGRATION=1 to run container-backed tests") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/cache/ttl_cache_useonce_test.go(1 hunks)pkg/middleware/token_middleware_additional_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cache/ttl_cache_useonce_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/middleware/token_middleware_additional_test.go (9)
database/repository/api_keys.go (1)
ApiKeys(11-13)pkg/auth/handler.go (2)
TokenHandler(10-14)MakeTokensHandler(16-26)pkg/auth/schema.go (1)
Token(11-17)database/model.go (1)
APIKey(26-35)database/connection.go (1)
Connection(12-17)pkg/middleware/token_middleware.go (2)
TokenCheckMiddleware(48-82)MakeTokenMiddleware(84-97)pkg/cache/ttl_cache.go (1)
NewTTLCache(18-22)pkg/limiter/limiter.go (1)
NewMemoryLimiter(21-27)pkg/http/schema.go (1)
ApiError(10-13)
🔇 Additional comments (5)
pkg/middleware/token_middleware_additional_test.go (5)
78-90: Guard dependencies test looks goodCovers both failure (missing deps) and success paths. Clear and deterministic.
92-105: Public token mismatch test is solidGood coverage of header construction and unauthorized path.
123-148: Nonce replay coverage is correct and asserts next() call onceGood use of nextCalled to ensure the happy path passes and replay is blocked.
55-57: All required test helpers are present
Verified that the following helpers are defined in this package’s tests, so the code will compile and no stubs are needed:
- pkg/middleware/valid_timestamp_test.go:
• func slogNoop() *slog.Logger { … }- pkg/middleware/token_middleware_test.go:
• func generate32(t *testing.T) []byte { … }
• func makeSignedRequest(t *testing.T, method, rawURL, body, account, public, secret string, ts time.Time, nonce, reqID string) *http.Request { … }
154-157: Rate limiter key composition verifiedThe test’s prefill key
"9.9.9.9|"+strings.ToLower(seed.AccountName)exactly matches the middleware’s
limiterKey := clientIP + "|" + strings.ToLower(accountName)logic. No drift in normalization was found—no changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/middleware/token_middleware_additional_test.go (1)
145-149: Nonce replay: assert downstream isn’t invoked on replayAdd an assertion that next is not called a second time.
rec = httptest.NewRecorder() if err := handler(rec, req); err == nil || err.Status != http.StatusUnauthorized { t.Fatalf("expected unauthorized on nonce replay, got %#v", err) } +if nextCalled != 1 { + t.Fatalf("next should not be invoked on nonce replay") +}
🧹 Nitpick comments (2)
database/connection_testhelpers.go (1)
5-8: Optional: constrain test-only helper visibilityIf you want to avoid shipping this in production builds, consider moving it to a dedicated test helper package (e.g., database/testutil or database/databasetest) or guarding it behind a build tag. Note that build tags would require running tests with go test -tags test.
I can draft a follow-up refactor (databasetest.NewConnectionFromGorm) and update call sites if you’d like.
pkg/middleware/token_middleware_additional_test.go (1)
28-43: Container startup/cleanup handling — solid, with a small tweakGood: longer startup timeout and fresh context in Cleanup. Consider bumping the cleanup timeout from 15s to 30s to reduce flakiness on slow CI.
- t.Cleanup(func() { - cctx, ccancel := context.WithTimeout(context.Background(), 15*time.Second) + t.Cleanup(func() { + cctx, ccancel := context.WithTimeout(context.Background(), 30*time.Second) defer ccancel() _ = pgC.Terminate(cctx) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
database/connection_testhelpers.go(1 hunks)pkg/middleware/token_middleware_additional_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/middleware/token_middleware_additional_test.go (9)
database/repository/api_keys.go (2)
ApiKeys(11-13)Create(15-33)pkg/auth/handler.go (2)
TokenHandler(10-14)MakeTokensHandler(16-26)pkg/auth/schema.go (1)
Token(11-17)database/model.go (1)
APIKey(26-35)pkg/middleware/token_middleware.go (2)
TokenCheckMiddleware(48-82)MakeTokenMiddleware(84-97)pkg/cache/ttl_cache.go (1)
NewTTLCache(18-22)pkg/limiter/limiter.go (1)
NewMemoryLimiter(21-27)pkg/http/schema.go (1)
ApiError(10-13)handler/tests/db.go (1)
MakeTestDB(18-88)
database/connection_testhelpers.go (1)
database/connection.go (4)
Connection(12-17)Ping(49-70)MakeConnection(19-32)Close(34-47)
🔇 Additional comments (4)
database/connection_testhelpers.go (1)
5-8: Safe replacement for reflect/unsafe — LGTMUsing a dedicated helper to set the GORM driver avoids brittle reflection and is appropriate for tests.
pkg/middleware/token_middleware_additional_test.go (3)
52-54: Close underlying sql.DB — LGTMProperly closing the GORM connection pool via t.Cleanup is correct and prevents leaks.
74-76: Use of database.NewConnectionFromGorm — LGTMClean wiring into repository.ApiKeys without unsafe reflection.
162-189: Rate limiter test drives public API and verifies next suppression — LGTMPre-warming via invalid requests and asserting 429 plus nextCalled==0 is the right approach and avoids brittle internal coupling.
Summary
Testing
go test ./pkg/cache -covergo test ./... -coverprofile=coverage.outhttps://chatgpt.com/codex/tasks/task_e_6899723937988333a0b498341706a566
Summary by CodeRabbit