Align tests with updated middleware API #85
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. Caution Review failedThe pull request is closed. WalkthroughRefactors middleware error logging to normalize data once and log under the "data" key. Updates token middleware and tests to use API key signatures, testcontainers Postgres, and a parameterless GuardDependencies. Simplifies ValidTimestamp constructor by removing logger. Tests migrate and seed a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant TM as TokenMiddleware
participant VT as ValidTimestamp
participant R as Repo.ApiKeys
participant S as APIKeySignatures
C->>TM: Request with token + signature headers
TM->>VT: NewValidTimestamp(ts, clock)
VT-->>TM: valid / invalid
alt invalid ts
TM-->>C: ApiError (TimestampTooOld/TooNew)
else valid ts
TM->>R: Load APIKey
alt not found
TM-->>C: ApiError (NotFound)
else found
TM->>S: Retrieve signature seed / verify signature
alt signature bad or replay
TM-->>C: ApiError (Unauthenticated/RateLimited)
else verified
TM-->>C: Next handler (authenticated)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✨ 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. Comment |
![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!
This pull request focuses on updating and refining existing test suites to match recent changes in the middleware API. The primary goal is to ensure test compatibility and correctness with updated function signatures and logging practices, including adjustments for slog usage and simplified constructors.
Highlights
- Middleware Token Tests: Updated tests for ValidateAndGetHeaders and context handling to align with new API signatures.
- Valid Timestamp Tests: Refreshed tests for ValidTimestamp to reflect a simplified constructor.
- Dependency Guard Tests & Logging: Adjusted dependency guard tests and fixed slog logging calls to satisfy go vet requirements.
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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
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 primarily refactors tests to align with an updated middleware API, including changes to ValidateAndGetHeaders and context handling. The changes also simplify the ValidTimestamp constructor and fix slog logging calls to satisfy go vet. The test adjustments are correct and reflect the API changes well. My main feedback is on the slog fixes in mw_response_messages.go. While they resolve the vet issues, they miss an opportunity to include valuable structured data in the logs. I've suggested including the data parameter in the logging calls to improve observability and debugging.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/middleware/token_middleware_additional_test.go (1)
25-36: Harden testcontainers startup to reduce CI flakiness.Add an explicit wait strategy and ping DB post-open; current CI shows connection resets.
import ( "context" "fmt" "net/http" "net/http/httptest" "testing" "time" "github.com/google/uuid" "github.com/oullin/database" "github.com/oullin/database/repository" "github.com/oullin/pkg/auth" "github.com/oullin/pkg/cache" pkgHttp "github.com/oullin/pkg/http" "github.com/oullin/pkg/limiter" "github.com/testcontainers/testcontainers-go" postgrescontainer "github.com/testcontainers/testcontainers-go/modules/postgres" + "github.com/testcontainers/testcontainers-go/wait" "gorm.io/driver/postgres" "gorm.io/gorm" )pgC, err := postgrescontainer.RunContainer(ctx, testcontainers.WithImage("postgres:16-alpine"), postgrescontainer.WithDatabase("testdb"), postgrescontainer.WithUsername("test"), postgrescontainer.WithPassword("test"), + testcontainers.WithWaitStrategy( + wait.ForListeningPort("5432/tcp").WithStartupTimeout(90*time.Second), + ), )db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{}) if err != nil { t.Skipf("gorm open: %v", err) } if sqlDB, err := db.DB(); err == nil { t.Cleanup(func() { _ = sqlDB.Close() }) + // Ensure server accepts connections before proceeding + ctxPing, cancelPing := context.WithTimeout(ctx, 30*time.Second) + defer cancelPing() + for ctxPing.Err() == nil { + if err := sqlDB.PingContext(ctxPing); err == nil { + break + } + time.Sleep(250 * time.Millisecond) + } }Also applies to: 55-57
pkg/middleware/token_middleware_test.go (1)
366-373: Ensure the “future timestamp” test fails for the intended reason.Without seeding a signature, the request may fail earlier. Seed a valid signature to isolate the future-time rejection.
- if _, err := repo.Create(database.APIKeyAttr{ + apiKey, err := repo.Create(database.APIKeyAttr{ AccountName: seed.AccountName, PublicKey: seed.EncryptedPublicKey, SecretKey: seed.EncryptedSecretKey, - }); err != nil { + }) + if err != nil { t.Fatalf("repo.Create: %v", err) } ... apiErr := handler(rec, req) +// Seed signature and retry to assert future-timestamp path specifically +seedSignature(t, repo, apiKey, req) +rec = httptest.NewRecorder() +apiErr = handler(rec, req) if apiErr == nil || apiErr.Status != http.StatusUnauthorized { t.Fatalf("expected 401 for future timestamp, got %#v", apiErr) }Also applies to: 385-399
♻️ Duplicate comments (1)
pkg/middleware/mwguards/mw_response_messages.go (1)
39-46: Good move: normalized data once and added structured logging.Precomputing d and logging it under "data" improves both performance and observability. This also addresses prior review feedback about missing structured context.
Also applies to: 52-59, 65-72, 78-85, 91-98, 104-111, 117-124
🧹 Nitpick comments (5)
pkg/middleware/mwguards/mw_response_messages.go (2)
36-46: Status code for InvalidRequestError may be better as 400.If this error is meant for malformed/missing headers (not auth failure), consider baseHttp.StatusBadRequest.
36-46: DRY these error constructors.Seven nearly identical blocks. A tiny helper reduces duplication and keeps logging consistent.
+func apiErr(status int, message, logMessage string, data ...map[string]any) *http.ApiError { + message, logMessage = normaliseMessages(message, logMessage) + d := normaliseData(data...) + slog.Error(logMessage, "data", redactSensitive(d)) + return &http.ApiError{Message: message, Status: status, Data: d} +}- d := normaliseData(data...) - slog.Error(logMessage, "data", d) - return &http.ApiError{ Message: message, Status: baseHttp.StatusUnauthorized, Data: d } + return apiErr(baseHttp.StatusUnauthorized, message, logMessage, data...)(Repeat similarly in the other constructors.)
Also applies to: 49-60, 62-73, 75-86, 88-99, 101-112, 114-125
pkg/middleware/valid_timestamp_test.go (2)
24-30: Add a test for negative skew normalization.Validate flips negative skew to positive; assert behavior explicitly.
+func TestValidate_NegativeSkew_Normalized(t *testing.T) { + base := time.Unix(1_700_000_000, 0) + vt := NewValidTimestamp(strconv.FormatInt(base.Unix(), 10), fixedClock(base)) + if err := vt.Validate(-60*time.Second, false); err != nil { + t.Fatalf("expected pass with negative skew normalized, got %#v", err) + } +}Also applies to: 32-38
10-21: Add a test for nil clock fallback to time.Now.Ensures Validate works when constructor’s now is nil.
+func TestValidate_NilClock_FallbackNow(t *testing.T) { + ts := strconv.FormatInt(time.Now().Unix(), 10) + vt := NewValidTimestamp(ts, nil) + if err := vt.Validate(1*time.Minute, true); err != nil { + t.Fatalf("expected pass with nil clock fallback, got %#v", err) + } +}pkg/middleware/token_middleware_additional_test.go (1)
165-175: Rate limiter test is clear; consider asserting the pre-warm actually hit the limit.Optional: assert the last warm-up call returns 401/429 to prove the limiter state before the final request.
Also applies to: 177-190
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/middleware/mwguards/mw_response_messages.go(1 hunks)pkg/middleware/token_middleware_additional_test.go(8 hunks)pkg/middleware/token_middleware_test.go(10 hunks)pkg/middleware/valid_timestamp_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/middleware/mwguards/mw_response_messages.go (1)
pkg/http/schema.go (1)
ApiError(11-15)
pkg/middleware/valid_timestamp_test.go (1)
pkg/middleware/valid_timestamp.go (3)
NewValidTimestamp(16-21)ts(11-14)v(23-60)
pkg/middleware/token_middleware_additional_test.go (6)
database/repository/api_keys.go (1)
ApiKeys(16-18)pkg/auth/handler.go (1)
TokenHandler(10-14)pkg/auth/schema.go (1)
Token(11-17)database/model.go (2)
APIKey(27-39)APIKeySignatures(41-55)database/connection_testhelpers.go (1)
NewConnectionFromGorm(6-8)pkg/middleware/token_middleware.go (2)
TokenCheckMiddleware(22-33)MakeTokenMiddleware(35-48)
pkg/middleware/token_middleware_test.go (4)
pkg/middleware/headers.go (1)
AuthTokenHeaders(3-12)database/model.go (2)
APIKey(27-39)APIKeySignatures(41-55)database/repository/api_keys.go (2)
ApiKeys(16-18)a(58-99)database/repository/repoentity/api_keys.go (2)
APIKeyCreateSignatureFor(9-14)Key(16-21)
🪛 GitHub Actions: Tests
pkg/middleware/token_middleware_additional_test.go
[error] 48-48: failed to initialize database, could not connect to 'user=test database=testdb' (connection reset by peer).
pkg/middleware/token_middleware_test.go
[error] 204-204: make connection: failed to connect to 'user=test database=testdb' (connection reset by peer).
[error] 285-285: make connection: failed to connect to 'user=test database=testdb' (connection reset by peer).
[error] 354-354: make connection: failed to connect to 'user=test database=testdb' (connection reset by peer).
🔇 Additional comments (3)
pkg/middleware/valid_timestamp_test.go (1)
13-21: Constructor and boundary tests look solid.Covers empty/non-numeric inputs and both sides of the skew window, including disallowFuture behavior.
Also applies to: 24-30, 32-38, 40-49, 51-68, 70-94
pkg/middleware/token_middleware_additional_test.go (1)
82-91: GuardDependencies() path looks correct without a logger.Nice alignment with the new parameterless API and dependency wiring.
pkg/middleware/token_middleware_test.go (1)
184-201: Happy path and integration flows look good with signature seeding.Solid end-to-end coverage: signed request, context propagation, and next handler invocation.
Also applies to: 217-224, 252-259, 299-306, 331-338
Summary
Testing
go test ./... -count=1https://chatgpt.com/codex/tasks/task_e_68be5e8d90cc8333b4375660b893807f
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores