refactor: Redis store to use functional options pattern #354
Conversation
…s pattern - Replace all Redis store convenience methods with a functional options pattern for configuration - Remove legacy methods for enabling and configuring the Redis store - Add new methods and types for Redis configuration via options (e.g., WithRedisAddr, WithRedisAuth, WithRedisCache, WithRedisPool, WithRedisKeyPrefix) - Update documentation and examples to use the functional options API for Redis setup - Refactor initialization logic to use a unified EnableRedisStore method and helper for fallback - Simplify token response generation by making generateTokenResponse return just the response - Update all tests and test helpers to cover the new options-based Redis configuration API - Remove explicit Redis config struct manipulation in examples and tests; use options instead - Translate all related README sections in English, Simplified Chinese, and Traditional Chinese - Improve clarity and maintainability of Redis middleware integration Signed-off-by: appleboy <[email protected]>
Summary of ChangesHello @appleboy, 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 significantly refactors the Redis store configuration within the Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| redisStore, err := store.NewRedisRefreshTokenStore(redisConfig) | ||
| if err != nil { | ||
| // Fallback to in-memory store | ||
| log.Printf("Failed to connect to Redis: %v, falling back to in-memory store", err) |
There was a problem hiding this comment.
🚫 [Bearer] <go_lang_logger_leak> reported by reviewdog 🐶
Leakage of information in logger message
Description
Information leakage through logger messages can result in data breaches. This vulnerability arises when dynamic data or variables, which may contain sensitive information, are included in log messages.
Remediations
- Do not include variables or dynamic data containing sensitive information in logger messages. This can inadvertently expose sensitive data.
logger.info(f"User is: '{user.email}'") // unsafe
- Do sanitize or remove sensitive information from data before logging. Ensure that logged information does not contain any personal or confidential data.
appleboy
changed the title
WalkthroughReplaces legacy Redis configuration helpers with a new functional options API. Adds EnableRedisStore(opts ...RedisOption) and WithRedis* option builders, updates examples and docs, and refactors tests accordingly. Changes generateTokenResponse to return gin.H without error. Introduces Redis initialization with fallback to in-memory storage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as App
participant MW as GinJWTMiddleware
participant Opts as RedisOptions
participant Redis as Redis Server
participant Mem as In-Memory Store
Dev->>MW: Configure<br/>EnableRedisStore(WithRedisAddr/Auth/Cache/Pool/KeyPrefix)
activate MW
MW->>Opts: Apply options to RedisConfig
note right of MW: Redis enabled flag set
MW->>MW: initializeRedisStore()
MW->>Redis: Connect + init store
alt Redis init OK
Redis-->>MW: Store ready
note right of MW: Use Redis-backed refresh token store
else Redis init fails
Redis--x MW: Error
MW->>Mem: Fallback initialize
note right of MW: Use in-memory refresh token store
end
deactivate MW
Dev->>MW: Login/Refresh
MW->>MW: generateTokenResponse(token, expire) -> gin.H
MW-->>Dev: JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Code Review
This is an excellent pull request that significantly improves the library's API by refactoring the Redis store configuration to use a functional options pattern. The changes are consistently applied across the codebase, including documentation in all languages, examples, and tests. The new API is much cleaner and more extensible. I've added a couple of suggestions to improve a log message for accuracy and to restore some dynamic functionality in one of the examples that was lost during the refactor. Overall, great work.
| { | ||
| auth.GET("/hello", helloHandler) | ||
| auth.GET("/store-info", storeInfoHandler(tokenStore)) | ||
| auth.GET("/store-info", storeInfoHandler()) |
There was a problem hiding this comment.
To allow storeInfoHandler to access the middleware state and provide dynamic information, you should pass the authMiddleware instance to it. This is a companion change to my other comment on storeInfoHandler.
| auth.GET("/store-info", storeInfoHandler()) | |
| auth.GET("/store-info", storeInfoHandler(authMiddleware)) |
| func storeInfoHandler() gin.HandlerFunc { | ||
| return func(c *gin.Context) { | ||
| count, err := tokenStore.Count() | ||
| if err != nil { | ||
| c.JSON(500, gin.H{"error": err.Error()}) | ||
| return | ||
| } | ||
|
|
||
| storeType := "unknown" | ||
| switch tokenStore.(type) { | ||
| case *store.InMemoryRefreshTokenStore: | ||
| storeType = "memory" | ||
| case *store.RedisRefreshTokenStore: | ||
| storeType = "redis" | ||
| } | ||
|
|
||
| c.JSON(200, gin.H{ | ||
| "store_type": storeType, | ||
| "token_count": count, | ||
| "message": "Token store information", | ||
| "configuration": "functional_options", | ||
| "redis_enabled": true, | ||
| "message": "Using functional options pattern for Redis configuration", | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The storeInfoHandler has been simplified to return a static response. This removes the dynamic aspect of the example, which previously demonstrated how to inspect the token store type and count. It would be more informative to retain the dynamic behavior to better showcase the library's capabilities, even with the new configuration pattern.
func storeInfoHandler(mw *jwt.GinJWTMiddleware) gin.HandlerFunc {
return func(c *gin.Context) {
storeType := "memory"
if mw.UseRedisStore {
// This is a simplification. In a real app, you might want to check the actual type.
storeType = "redis"
}
count, err := mw.RefreshTokenStore.Count()
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
return
}
c.JSON(http.StatusOK, gin.H{
"store_type": storeType,
"token_count": count,
"message": "Token store information",
})
}
}| } else { | ||
| log.Println("Successfully connected to Redis store with client-side cache enabled") | ||
| mw.RefreshTokenStore = redisStore | ||
| } |
There was a problem hiding this comment.
The log message unconditionally states that client-side caching is enabled. However, caching can be disabled by setting the cache size to 0. The log message should reflect the actual cache status to avoid confusion during debugging.
} else {
if redisConfig.CacheSize > 0 {
log.Println("Successfully connected to Redis store with client-side cache enabled")
} else {
log.Println("Successfully connected to Redis store")
}
mw.RefreshTokenStore = redisStore
}![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)
_example/redis_store/main.go (1)
103-107: Avoid logging full JWT claims in examples.Printing claims with
%#vmay leak user info to logs. Remove or log minimal fields.- log.Printf("NoRoute claims: %#v\n", claims) + // Avoid logging full claims in production/examples.
♻️ Duplicate comments (1)
auth_jwt_redis.go (1)
79-85: Sanitize Redis error logging; avoid leaking details.Log a generic message instead of interpolating the raw error. This aligns with the previous static-analysis warning.
- log.Printf("Failed to connect to Redis: %v, falling back to in-memory store", err) + log.Print("Failed to connect to Redis; falling back to in-memory store")
🧹 Nitpick comments (20)
README.zh-TW.md (2)
339-346: Remove redundant MiddlewareInit after jwt.New.
jwt.Newalready callsMiddlewareInit(). Drop the extra call to avoid double init.- errInit := authMiddleware.MiddlewareInit() - if errInit != nil { - log.Fatal("authMiddleware.MiddlewareInit() Error:" + errInit.Error()) - } + // No need to call MiddlewareInit() again; jwt.New() already did it.
350-359: Make refresh endpoint public (RFC 6749).Don’t protect
/auth/refresh_tokenwith JWT. Define it beforeauth.Use(...)or outside the group.auth := r.Group("/auth") auth.GET("/refresh_token", authMiddleware.RefreshHandler) // public auth.Use(authMiddleware.MiddlewareFunc()) { auth.GET("/hello", func(c *gin.Context) { /* ... */ }) }README.md (2)
508-511: Remove redundant MiddlewareInit after jwt.New.
jwt.NewinvokesMiddlewareInit()internally. Avoid calling it twice.- errInit := authMiddleware.MiddlewareInit() - if errInit != nil { - log.Fatal("authMiddleware.MiddlewareInit() Error:" + errInit.Error()) - } + // No manual MiddlewareInit() needed here.
516-524: Expose refresh endpoint without JWT.Per RFC 6749 and earlier docs, the refresh endpoint should be public. Move it before
auth.Use(...)or outside the group.auth := r.Group("/auth") auth.GET("/refresh_token", authMiddleware.RefreshHandler) // public auth.Use(authMiddleware.MiddlewareFunc()) { auth.GET("/hello", /* ... */) }README.zh-CN.md (2)
343-346: 移除重复的 MiddlewareInit 调用。
jwt.New已自动调用MiddlewareInit()，无需再次调用。- errInit := authMiddleware.MiddlewareInit() - if errInit != nil { - log.Fatal("authMiddleware.MiddlewareInit() Error:" + errInit.Error()) - } + // 无需重复调用 MiddlewareInit()。
350-359: 将刷新端点设为公开。按 RFC 6749，刷新端点应为公开端点。请在
auth.Use(...)之前定义，或放到分组外。auth := r.Group("/auth") auth.GET("/refresh_token", authMiddleware.RefreshHandler) // 公开 auth.Use(authMiddleware.MiddlewareFunc()) { auth.GET("/hello", /* ... */) }auth_jwt_redis.go (1)
84-85: Remove success log to reduce library log noise.Libraries should avoid emitting info-level logs on success by default.
- log.Println("Successfully connected to Redis store with client-side cache enabled")_example/redis_store/main.go (2)
93-99: Drop duplicate MiddlewareInit.
jwt.New(middleware)already initializes the middleware.- errInit := authMiddleware.MiddlewareInit() - - if errInit != nil { - log.Fatal("authMiddleware.MiddlewareInit() Error:" + errInit.Error()) - } + // No need to call MiddlewareInit() again.
137-146: Reflect actual store status in /store-info.Hardcoding
"redis_enabled": truecan mislead when fallback occurs. Detect from middleware.-func storeInfoHandler() gin.HandlerFunc { +func storeInfoHandler(mw *jwt.GinJWTMiddleware) gin.HandlerFunc { return func(c *gin.Context) { + _, usingRedis := mw.RefreshTokenStore.(interface{ /* marker of Redis store type */ }) c.JSON(200, gin.H{ "configuration": "functional_options", - "redis_enabled": true, + "redis_enabled": mw.UseRedisStore, + "using_redis": usingRedis, "message": "Using functional options pattern for Redis configuration", }) } }And register with:
auth.GET("/store-info", storeInfoHandler(authMiddleware))_example/redis_simple/main.go (1)
123-128: Remove redundant MiddlewareInit after jwt.New.Initialization is already performed inside
jwt.New.- errInit := authMiddleware.MiddlewareInit() - if errInit != nil { - log.Fatal("authMiddleware.MiddlewareInit() Error:" + errInit.Error()) - } + // No manual MiddlewareInit() necessary here.auth_jwt.go (3)
400-405: Use the provided HTTP status code.Default
LoginResponseignores thecodeparameter. Respect it to keep behavior consistent with custom callers.- c.JSON(http.StatusOK, response) + c.JSON(code, response)
415-420: Use the provided HTTP status code.Default
RefreshResponsealso ignorescode. Use it.- c.JSON(http.StatusOK, response) + c.JSON(code, response)
1041-1053: Prevent negative expires_in due to clock skew.Clamp to non-negative seconds for RFC 6749 compliance and stability.
func (mw *GinJWTMiddleware) generateTokenResponse(c *gin.Context, token string, expire time.Time) gin.H { - response := gin.H{ - "access_token": token, - "token_type": "Bearer", - "expires_in": int(time.Until(expire).Seconds()), - } + sec := int(time.Until(expire).Seconds()) + if sec < 0 { + sec = 0 + } + response := gin.H{ + "access_token": token, + "token_type": "Bearer", + "expires_in": sec, + }auth_jwt_redis_test.go (5)
83-109: Also cover fallback using functional optionsThis test validates fallback via legacy fields. Add a variant using EnableRedisStore(WithRedisAddr("invalid-host:6379")) to ensure fallback works with the new API as well.
Example subtest:
t.Run("RedisStoreFallback_WithFunctionalOptions", func(t *testing.T) { mw := &GinJWTMiddleware{ Realm: "test zone", Key: []byte("secret key"), Timeout: time.Hour, MaxRefresh: 24 * time.Hour, IdentityKey: "id", Authenticator: testAuthenticator, PayloadFunc: testPayloadFunc, } mw.EnableRedisStore(WithRedisAddr("invalid-host:6379")) err := mw.MiddlewareInit() require.NoError(t, err) assert.NotNil(t, mw.inMemoryStore) assert.Equal(t, mw.inMemoryStore, mw.RefreshTokenStore) })
271-275: Remove redundant EnableRedisStore callYou already set all options (including addr). Re-calling EnableRedisStore with only addr is unnecessary and may unintentionally alter earlier options depending on implementation.
- // Test that it actually works (but use working address for actual initialization) - middleware.EnableRedisStore(WithRedisAddr(redisAddr)) // Reset to working address err := middleware.MiddlewareInit() assert.NoError(t, err, "configuration with all options should initialize successfully")
426-431: Reduce flakiness: increase wait beyond cache TTLCache TTL is 50ms; sleeping only 100ms can be flaky on slower CI. Bump to a safer margin.
- time.Sleep(100 * time.Millisecond) + time.Sleep(300 * time.Millisecond)
328-337: Mark helpers as test helpersAdd t.Helper() at the top of helper funcs to improve failure locations.
func testLoginAndRefreshFlow(t *testing.T, r *gin.Engine) { + t.Helper() // Test login ... }func testTokenPersistenceAcrossRequests(t *testing.T, r *gin.Engine) { + t.Helper() // Login and get refresh token ... }func testRedisStoreOperations(t *testing.T, middleware *GinJWTMiddleware) { + t.Helper() // Verify that Redis store is being used ... }Also applies to: 366-375, 398-406
26-27: Pin Redis image for CI stability and add timeout
- Wrap container startup in
context.WithTimeout(e.g., 2 min) and callt.Cleanup(cancel).- Allow
TEST_REDIS_IMAGEoverride; default to a pinned tag, e.g."redis:8.2.2-alpine".Apply these diffs:
import ( "context" "encoding/json" "fmt" "net/http" "net/http/httptest" "strings" "testing" "time" "github.com/appleboy/gin-jwt/v2/store" "github.com/gin-gonic/gin" gojwt "github.com/golang-jwt/jwt/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/testcontainers/testcontainers-go/modules/redis" + "os" )-func setupRedisContainerForJWT(t *testing.T) (*redis.RedisContainer, string, string) { - ctx := context.Background() +func setupRedisContainerForJWT(t *testing.T) (*redis.RedisContainer, string, string) { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + t.Cleanup(cancel) // Start Redis container - redisContainer, err := redis.Run(ctx, "redis:8-alpine") + image := os.Getenv("TEST_REDIS_IMAGE") + if image == "" { + image = "redis:8.2.2-alpine" + } + redisContainer, err := redis.Run(ctx, image) require.NoError(t, err, "failed to start Redis container")auth_jwt_methods_test.go (2)
14-29: Run subtests in parallelThese are pure unit tests; enable t.Parallel() to speed up the suite.
t.Run("EnableRedisStoreDefault", func(t *testing.T) { + t.Parallel() middleware := &GinJWTMiddleware{Repeat for other subtests in this file (SingleOption, MultipleOptions, CacheOptions, PoolOptions, KeyPrefix, AllOptions, MultipleEnableRedisStoreCalls, DefaultConfiguration).
185-213: Clarify semantics for repeated calls: merge vs resetMultipleEnableRedisStoreCalls verifies overridden fields only. Add an assertion for a field not set in the second call (e.g., PoolSize or CacheTTL) to pin down whether subsequent calls reset config or preserve prior values. This prevents accidental behavior changes later.
Example:
// After first call also set a pool size: middleware.EnableRedisStore( WithRedisAddr("first.redis.com:6379"), WithRedisAuth("first-pass", 1), WithRedisPool(30, time.Hour, 2*time.Hour), ) // Second call without pool option: middleware.EnableRedisStore( WithRedisAddr("second.redis.com:6379"), WithRedisAuth("second-pass", 2), ) // Decide expected behavior, then assert either: assert.Equal(t, 30, middleware.RedisConfig.PoolSize) // persist // or assert.Equal(t, 10, middleware.RedisConfig.PoolSize) // reset to default
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(3 hunks)README.zh-CN.md(3 hunks)README.zh-TW.md(3 hunks)_example/redis_simple/main.go(1 hunks)_example/redis_store/main.go(4 hunks)auth_jwt.go(5 hunks)auth_jwt_methods_test.go(7 hunks)auth_jwt_redis.go(1 hunks)auth_jwt_redis_test.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
_example/redis_simple/main.go (2)
auth_jwt_redis.go (1)
WithRedisCache(29-34)auth_jwt.go (1)
New(259-265)
auth_jwt_redis_test.go (3)
store/redis.go (1)
RedisConfig(23-40)auth_jwt_redis.go (5)
WithRedisAddr(14-18)WithRedisAuth(21-26)WithRedisCache(29-34)WithRedisPool(37-43)WithRedisKeyPrefix(46-50)auth_jwt.go (2)
GinJWTMiddleware(27-188)IdentityKey(249-249)
auth_jwt_methods_test.go (3)
store/redis.go (1)
RedisConfig(23-40)auth_jwt_redis.go (5)
WithRedisAddr(14-18)WithRedisAuth(21-26)WithRedisCache(29-34)WithRedisPool(37-43)WithRedisKeyPrefix(46-50)auth_jwt.go (1)
GinJWTMiddleware(27-188)
_example/redis_store/main.go (2)
auth_jwt.go (2)
GinJWTMiddleware(27-188)New(259-265)auth_jwt_redis.go (2)
WithRedisAddr(14-18)WithRedisCache(29-34)
auth_jwt.go (1)
store/memory.go (1)
NewInMemoryRefreshTokenStore(20-24)
auth_jwt_redis.go (2)
store/redis.go (3)
RedisConfig(23-40)DefaultRedisConfig(43-55)NewRedisRefreshTokenStore(58-96)auth_jwt.go (1)
GinJWTMiddleware(27-188)
🔇 Additional comments (2)
auth_jwt_redis_test.go (1)
70-76: LGTM: helper flow now router-basedPassing the router instead of the middleware simplifies helper usage and matches the refactor.
auth_jwt_methods_test.go (1)
228-238: Defaults assertions look correctThe asserted defaults match store/redis.go comments: addr, password, DB, cache size/TTL, pool settings, and key prefix.
| // WithRedisPool configures connection pool | ||
| func WithRedisPool(poolSize int, maxIdleTime, maxLifetime time.Duration) RedisOption { | ||
| return func(config *store.RedisConfig) { | ||
| config.PoolSize = poolSize | ||
| config.ConnMaxIdleTime = maxIdleTime | ||
| config.ConnMaxLifetime = maxLifetime | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify WithRedisPool is actually applied in store client options.
Pool fields set here don’t appear used when building the Redis client in store.NewRedisRefreshTokenStore (per provided snippet). This would make the option a no-op.
Run to confirm usage across the repo:
🏁 Script executed:
#!/bin/bash
rg -n -C2 -g '!**/vendor/**' -e '\bPoolSize\b|\bConnMaxIdleTime\b|\bConnMaxLifetime\b'Length of output: 4620
🏁 Script executed:
#!/bin/bash
# Locate the Redis refresh token store constructor
rg -n -C3 -g '!**/vendor/**' -e 'func NewRedisRefreshTokenStore'
# Inspect how the Redis client is initialized
rg -n -C3 -g '!**/vendor/**' -e 'redis\.NewClient'Length of output: 370
🏁 Script executed:
#!/bin/bash
# Show implementation of NewRedisRefreshTokenStore
rg -n -C20 'func NewRedisRefreshTokenStore' -g 'store/redis.go'
# Show how redis client is initialized
rg -n -C10 'redis\.NewClient' -g 'store/redis.go'Length of output: 1936
Apply Redis pool options in NewRedisRefreshTokenStore
The PoolSize, ConnMaxIdleTime and ConnMaxLifetime fields from RedisConfig (set via WithRedisPool) are never applied when building the rueidis.ClientOption in store/redis.go, making those options no-ops. Wire these fields into the client configuration or remove them.
🤖 Prompt for AI Agents
In auth_jwt_redis.go around lines 36 to 43, the RedisConfig pool fields set by
WithRedisPool are not applied when constructing the rueidis.ClientOption in
NewRedisRefreshTokenStore; update NewRedisRefreshTokenStore in store/redis.go to
wire config.PoolSize, config.ConnMaxIdleTime and config.ConnMaxLifetime into the
rueidis client options (or remove the unused config fields). Specifically, map
PoolSize to the rueidis client pool size option and map
ConnMaxIdleTime/ConnMaxLifetime to the corresponding connection idle and
lifetime options provided by rueidis so the WithRedisPool settings take effect.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores