feat: add redis session driver#696
Open
henrywhitaker3 wants to merge 3 commits intosteveiliop56:mainfrom
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds rueidis dependency and Redis configuration; initializes a Rueidis client when session driver is "redis"; introduces a Redis-backed SessionRepository and a SessionRepository interface; AuthService now depends on the session abstraction and bootstrap selects DB or Redis session storage. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Bootstrap
participant Config as Configuration
participant Redis as Redis
participant Services as Service Init
participant SessionRepo as Session Repository
participant AuthSvc as Auth Service
App->>Config: load config (Session.Driver)
Config-->>App: config
alt Session.Driver == "redis"
App->>Redis: SetupRedis(url, password, db)
Redis-->>App: rueidis.Client
else
App-->>App: skip Redis setup
end
App->>Services: initServices(queries, redisClient)
alt Session.Driver == "redis"
Services->>SessionRepo: NewRedisSessionRepository(redisClient)
SessionRepo-->>Services: RedisSessionRepository
else
Services->>SessionRepo: use DB-backed session repo (queries)
SessionRepo-->>Services: DB Session Repository
end
Services->>AuthSvc: NewAuthService(..., SessionRepository)
AuthSvc-->>Services: AuthService ready
Services-->>App: services initialized
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
internal/config/config.go (1)
99-112: Consider addingPasswordFileoption for Redis password.For consistency with other secret handling in this codebase (e.g.,
ClientSecretFilefor OAuth,PasswordFilefor basic auth), consider adding aPasswordFileoption for the Redis password. This allows users to manage secrets via files rather than environment variables.Suggested addition
type RedisConfig struct { URL string `description:"The url of the redis instance to connect to" yaml:"url"` Password string `description:"The password to connect to redis with" yaml:"password"` + PasswordFile string `description:"Path to file containing the redis password" yaml:"passwordFile"` DB int `description:"The DB index to use" yaml:"db"` }Then in
app_bootstrap.go, useutils.GetSecret(app.config.Database.Redis.Password, app.config.Database.Redis.PasswordFile)similar to OAuth secret handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 99 - 112, Add a PasswordFile field to the RedisConfig struct so users can supply Redis password via a file (consistent with other secret handling); update RedisConfig (and any usage in DatabaseConfig) to include PasswordFile string `yaml:"passwordFile"` and then in app_bootstrap.go replace direct uses of app.config.Database.Redis.Password with utils.GetSecret(app.config.Database.Redis.Password, app.config.Database.Redis.PasswordFile) (or equivalent) where the Redis password is read; ensure any documentation/validation that checks Redis password considers the new PasswordFile option.internal/repository/redis_session_queries.go (1)
10-18: Consider adding a key prefix to avoid collisions in shared Redis instances.Using raw UUIDs as keys could conflict with other applications sharing the same Redis instance. A prefix like
tinyauth:session:would provide namespace isolation.Suggested approach
const sessionKeyPrefix = "tinyauth:session:" func sessionKey(uuid string) string { return sessionKeyPrefix + uuid } // Then use sessionKey(cookie) instead of cookie directly cmd := r.c.B().Get().Key(sessionKey(cookie)).Build()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repository/redis_session_queries.go` around lines 10 - 18, Redis keys use raw UUIDs which can collide in shared Redis; add a namespace prefix constant (e.g. sessionKeyPrefix) and helper function sessionKey(uuid string) string to build keys, then update RedisSessionRepository methods (methods on RedisSessionRepository that call r.c.B().Get().Key(...), Set, Del, etc.) to use sessionKey(cookie) instead of the raw cookie/uuid; keep constructor NewRedisSessionRepository unchanged but ensure all internal key usages are replaced to avoid cross-application collisions.internal/bootstrap/db_bootstrap.go (1)
60-70: Add explicit connection timeout and TLS configuration to the Redis client setup.The current implementation lacks resilience and production-readiness:
Connection timeout: Relying on implicit defaults (5s) is fragile. Explicitly configure timeout via
Dialer.Timeoutto handle unreachable Redis servers gracefully.TLS support: Production deployments typically require TLS. The current config has no
TLSConfigfield, preventing secure Redis connections.Suggested improvement with timeout and TLS
func (app *BootstrapApp) SetupRedis(url, password string, db int) (rueidis.Client, error) { client, err := rueidis.NewClient(rueidis.ClientOption{ InitAddress: []string{url}, Password: password, SelectDB: db, + Dialer: net.Dialer{ + Timeout: 5 * time.Second, + KeepAlive: 30 * time.Second, + }, + TLSConfig: nil, // Set to *tls.Config for production Redis })Alternatively, use URL-based configuration:
rueidis.MustParseURL("rediss://host:port?dial_timeout=5s")for automatic TLS and timeout setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bootstrap/db_bootstrap.go` around lines 60 - 70, The SetupRedis function currently calls rueidis.NewClient with minimal ClientOption; update it to explicitly set connection timeout and TLS by populating the ClientOption.Dialer.Timeout and ClientOption.TLSConfig (or use rueidis.MustParseURL with a rediss:// URL and dial_timeout query) so unreachable servers and secure connections are handled; modify the SetupRedis implementation (where rueidis.NewClient and rueidis.ClientOption are used) to accept/create a tls.Config when TLS is required and set Dialer.Timeout to a sensible duration (e.g., 5s) before returning the client or error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/bootstrap/app_bootstrap.go`:
- Around line 133-144: The bootstrap currently creates a rueidis.Client via
app.SetupRedis (assigned to variable redisClient) but never closes it and
Redis-backed sessions lack an expiry; modify app bootstrap to persist the redis
client reference (e.g., store on app struct) and ensure Close() is called during
graceful shutdown alongside other resources (refer to where dbCleanup or
shutdown sequence runs), and update CreateSession in redis_session_queries.go to
include an expiry/TTL on the SET ... NX command using the session's expiry time
(set EX seconds) so Redis auto-evicts expired sessions instead of relying solely
on DeleteExpiredSessions.
In `@internal/repository/redis_session_queries.go`:
- Around line 41-70: CreateSession is currently using
r.c.B().Set().Key(...).Value(...).Nx().Build() without any TTL so Redis sessions
never expire; compute TTLSeconds = params.Expiry - time.Now().Unix(), ensure
TTLSeconds > 0 (return an error or treat as immediate expiry if non-positive),
and include that TTL when building the Redis SET command (use the builder's
expiry option, e.g., Ex/Px/Expire-at equivalent) instead of the current call so
the stored session respects params.Expiry; update the command built in
CreateSession accordingly and keep the Nx() behavior.
- Around line 72-103: UpdateSession currently does a non-atomic GET→modify→SET
via GetSession and then r.c.B().Set().Key(...).Value(...).Build(), which can
lose concurrent updates and also doesn't refresh the Redis TTL when Expiry
changes; replace this with an atomic write that sets both the new session JSON
and the key TTL in one command. Either: 1) avoid the initial GetSession and
construct the updated Session from UpdateSessionParams then use a single atomic
SET with an expiry (EX/PX) so the value and TTL are applied together (use the
r.c builder SET with TTL args or SETEX equivalent), or 2) use a small Redis EVAL
Lua script that GETs, merges the stored session with params, and SETs the new
JSON plus TTL in one atomic operation; ensure the code paths referencing
UpdateSession, GetSession, r.c.B().Set().Key(...).Value(...).Build() are updated
so the TTL reflects params.Expiry and no read-modify-write window remains.
- Around line 20-39: GetSession in RedisSessionRepository currently returns
res.Error() directly when a key is missing, which yields a rueidis nil error
incompatible with auth service checks; add a sentinel error (e.g.
ErrSessionNotFound) in the repository package and update
RedisSessionRepository.GetSession to detect a nil-key using
rueidis.IsRedisNil(err) and return ErrSessionNotFound for missing sessions,
otherwise return the original error; then update callers (or auth service) to
compare against repository.ErrSessionNotFound instead of sql.ErrNoRows (or add a
handling branch) so "session not found" is recognized consistently.
---
Nitpick comments:
In `@internal/bootstrap/db_bootstrap.go`:
- Around line 60-70: The SetupRedis function currently calls rueidis.NewClient
with minimal ClientOption; update it to explicitly set connection timeout and
TLS by populating the ClientOption.Dialer.Timeout and ClientOption.TLSConfig (or
use rueidis.MustParseURL with a rediss:// URL and dial_timeout query) so
unreachable servers and secure connections are handled; modify the SetupRedis
implementation (where rueidis.NewClient and rueidis.ClientOption are used) to
accept/create a tls.Config when TLS is required and set Dialer.Timeout to a
sensible duration (e.g., 5s) before returning the client or error.
In `@internal/config/config.go`:
- Around line 99-112: Add a PasswordFile field to the RedisConfig struct so
users can supply Redis password via a file (consistent with other secret
handling); update RedisConfig (and any usage in DatabaseConfig) to include
PasswordFile string `yaml:"passwordFile"` and then in app_bootstrap.go replace
direct uses of app.config.Database.Redis.Password with
utils.GetSecret(app.config.Database.Redis.Password,
app.config.Database.Redis.PasswordFile) (or equivalent) where the Redis password
is read; ensure any documentation/validation that checks Redis password
considers the new PasswordFile option.
In `@internal/repository/redis_session_queries.go`:
- Around line 10-18: Redis keys use raw UUIDs which can collide in shared Redis;
add a namespace prefix constant (e.g. sessionKeyPrefix) and helper function
sessionKey(uuid string) string to build keys, then update RedisSessionRepository
methods (methods on RedisSessionRepository that call r.c.B().Get().Key(...),
Set, Del, etc.) to use sessionKey(cookie) instead of the raw cookie/uuid; keep
constructor NewRedisSessionRepository unchanged but ensure all internal key
usages are replaced to avoid cross-application collisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b610ed60-d0da-4114-8fca-6000bdd7f3b5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.modinternal/bootstrap/app_bootstrap.gointernal/bootstrap/db_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/config/config.gointernal/repository/redis_session_queries.gointernal/service/auth_service.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/repository/redis_session_queries.go (2)
65-67:⚠️ Potential issue | 🟠 MajorGuard
EXagainst zero or negative TTL.
params.Expiry - time.Now().Unix()can evaluate to<= 0under misconfiguration or timing edges, and Redis rejectsSET ... EX 0/-n. That turns an already-expired session into a write error during create/refresh instead of expiring it cleanly. Clamp the TTL or return a domain error before building the command.Also applies to: 100-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repository/redis_session_queries.go` around lines 65 - 67, The SET command uses params.Expiry - time.Now().Unix() directly which can be <= 0 and causes Redis to reject the EX argument; compute a TTL variable before building the command (ttl := params.Expiry - time.Now().Unix()), then either return a domain error when ttl <= 0 or clamp it to a minimum positive value (e.g., ttl = 1) and pass that to ExSeconds(ttl) on the r.c.B().Set() call; apply the same change to the other occurrences referenced (around lines 100-103) so all uses of params.Expiry subtracting time.Now().Unix() are guarded.
79-103:⚠️ Potential issue | 🔴 CriticalDon't let
UpdateSessionrecreate a deleted session.This is still a
GET→ mutate → unconditionalSET. If logout deletes the key after Line 79 but before Line 100, the finalSETwrites the session back and effectively undoes the logout. At minimum, make the write conditional withXXand treat a nil reply as "not found"; if you want to remove the full race window, move this to a Lua script / transaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repository/redis_session_queries.go` around lines 79 - 103, UpdateSession currently does a GetSession -> mutate -> unconditional Set which can recreate a session deleted concurrently (e.g., logout); change the Set to use the Redis "XX" option so it only updates existing keys (use the builder's XX flag or method on r.c.B().Set() to require existing key), and treat a nil/empty reply from the Set as "not found" (return a not-found error) instead of assuming success; alternatively consider moving the read-modify-write into a Lua script/transaction to eliminate the race—apply this change around the UpdateSession code that calls r.GetSession(...) and builds the Set command (r.c.B().Set().Key(...).Value(...).ExSeconds(...).Build()).
🧹 Nitpick comments (1)
internal/service/auth_service.go (1)
49-54: Keep backend-specific miss handling out ofAuthService.
SessionRepositoryis storage-agnostic, butGetSessionCookiestill has to know about bothsql.ErrNoRowsandrueidis.IsRedisNil. That leaks backend details through the interface and leaves other callers to remember the same translation. Prefer a shared sentinel such asrepository.ErrSessionNotFound, and have each repository implementation map its own miss to that error.Also applies to: 363-367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/auth_service.go` around lines 49 - 54, Add a repository-level sentinel error (e.g., repository.ErrSessionNotFound) and update each storage implementation of SessionRepository (the SQL-based GetSession and the Redis-based GetSession) to translate their backend-specific "not found" results (sql.ErrNoRows, rueidis.IsRedisNil, etc.) into that sentinel; then change AuthService.GetSessionCookie (and other callers around the other referenced call site) to check for repository.ErrSessionNotFound instead of backend-specific errors so the storage details are hidden behind the SessionRepository interface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/repository/redis_session_queries.go`:
- Around line 65-67: The SET command uses params.Expiry - time.Now().Unix()
directly which can be <= 0 and causes Redis to reject the EX argument; compute a
TTL variable before building the command (ttl := params.Expiry -
time.Now().Unix()), then either return a domain error when ttl <= 0 or clamp it
to a minimum positive value (e.g., ttl = 1) and pass that to ExSeconds(ttl) on
the r.c.B().Set() call; apply the same change to the other occurrences
referenced (around lines 100-103) so all uses of params.Expiry subtracting
time.Now().Unix() are guarded.
- Around line 79-103: UpdateSession currently does a GetSession -> mutate ->
unconditional Set which can recreate a session deleted concurrently (e.g.,
logout); change the Set to use the Redis "XX" option so it only updates existing
keys (use the builder's XX flag or method on r.c.B().Set() to require existing
key), and treat a nil/empty reply from the Set as "not found" (return a
not-found error) instead of assuming success; alternatively consider moving the
read-modify-write into a Lua script/transaction to eliminate the race—apply this
change around the UpdateSession code that calls r.GetSession(...) and builds the
Set command (r.c.B().Set().Key(...).Value(...).ExSeconds(...).Build()).
---
Nitpick comments:
In `@internal/service/auth_service.go`:
- Around line 49-54: Add a repository-level sentinel error (e.g.,
repository.ErrSessionNotFound) and update each storage implementation of
SessionRepository (the SQL-based GetSession and the Redis-based GetSession) to
translate their backend-specific "not found" results (sql.ErrNoRows,
rueidis.IsRedisNil, etc.) into that sentinel; then change
AuthService.GetSessionCookie (and other callers around the other referenced call
site) to check for repository.ErrSessionNotFound instead of backend-specific
errors so the storage details are hidden behind the SessionRepository interface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dbf44334-719a-4da0-b753-3aed69e4e2a1
📒 Files selected for processing (2)
internal/repository/redis_session_queries.gointernal/service/auth_service.go
Signed-off-by: Henry Whitaker <henrywhitaker3@outlook.com>
Signed-off-by: Henry Whitaker <henrywhitaker3@outlook.com>
Signed-off-by: Henry Whitaker <henrywhitaker3@outlook.com>
henrywhitaker3
force-pushed
the
feat/redis-sessions
branch
from
0303ec8 to
e93fb77
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/repository/redis_session_queries.go (2)
100-103:⚠️ Potential issue | 🟡 MinorSame TTL edge case applies here.
Similar to
CreateSession, ifparams.Expiry - time.Now().Unix()is zero or negative, the SET command may fail or behave unexpectedly.Proposed fix
+ ttl := params.Expiry - time.Now().Unix() + if ttl <= 0 { + return session, fmt.Errorf("session expiry is in the past") + } + cmd := r.c.B().Set().Key(params.UUID). Value(string(raw)). - ExSeconds(params.Expiry - time.Now().Unix()). + ExSeconds(ttl). Build()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repository/redis_session_queries.go` around lines 100 - 103, The TTL calculation passed to ExSeconds can be zero or negative (params.Expiry - time.Now().Unix()), causing the SET command to fail; update the session save logic around the r.c.B().Set().Key(params.UUID).Value(string(raw)).ExSeconds(...) call to compute ttl := params.Expiry - time.Now().Unix() and only call ExSeconds(ttl) when ttl > 0, otherwise call Set without ExSeconds (or return an error/handle as your CreateSession does) so you avoid passing non-positive expiry values to the builder.
65-67:⚠️ Potential issue | 🟡 MinorHandle edge case when computed TTL is non-positive.
If
params.Expiryis set to a past timestamp or very close totime.Now().Unix(), the computed TTL could be zero or negative. RedisSET ... EX 0will fail, and negative values may cause unexpected behavior.Add a guard to ensure TTL is at least 1 second or return an error for already-expired sessions.
Proposed fix
+ ttl := params.Expiry - time.Now().Unix() + if ttl <= 0 { + return out, fmt.Errorf("session expiry is in the past") + } + cmd := r.c.B().Set().Key(params.UUID).Value(string(raw)). - Nx().ExSeconds(params.Expiry - time.Now().Unix()). + Nx().ExSeconds(ttl). Build()You'll also need to add
"fmt"to the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repository/redis_session_queries.go` around lines 65 - 67, Compute TTL before building the Redis command by doing ttl := params.Expiry - time.Now().Unix(); if ttl <= 0 return an appropriate error (e.g., "session already expired") to avoid issuing SET ... EX 0/negative, otherwise use ttl (or ttl := max(1, ttl) if you prefer to allow a 1s minimum) when calling ExSeconds; update the builder call (r.c.B().Set().Key(params.UUID).Value(string(raw)).Nx().ExSeconds(ttl).Build()) and add "fmt" to imports for the error formatting if needed.
🧹 Nitpick comments (5)
internal/bootstrap/db_bootstrap.go (2)
60-70: Consider adding TLS support for secure Redis connections.The current implementation doesn't expose TLS configuration. For production environments using Redis over TLS (e.g., Redis Cloud, AWS ElastiCache with encryption), users would need additional options like
TLSConfig.This could be a follow-up enhancement to add
TLS boolandInsecureTLS boolfields toRedisConfig.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bootstrap/db_bootstrap.go` around lines 60 - 70, The SetupRedis function currently calls rueidis.NewClient without TLS options; update it to accept TLS settings from your RedisConfig (add fields like TLS bool and InsecureTLS bool) and pass a configured tls.Config into rueidis.ClientOption when TLS is enabled; specifically, modify SetupRedis to read RedisConfig.TLS and RedisConfig.InsecureTLS, build a tls.Config (e.g., InsecureSkipVerify when InsecureTLS is true), and set the appropriate TLS fields on rueidis.ClientOption so the rueidis.NewClient call uses TLS for secure Redis connections.
60-70: Consider adding a connection health check.Rueidis creates connections lazily. Adding a simple ping after client creation would provide earlier feedback if Redis is misconfigured or unreachable.
Proposed enhancement
func (app *BootstrapApp) SetupRedis(url, password string, db int) (rueidis.Client, error) { client, err := rueidis.NewClient(rueidis.ClientOption{ InitAddress: []string{url}, Password: password, SelectDB: db, }) if err != nil { return nil, fmt.Errorf("init redis client: %w", err) } + + // Verify connectivity + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := client.Do(ctx, client.B().Ping().Build()).Error(); err != nil { + client.Close() + return nil, fmt.Errorf("ping redis: %w", err) + } + return client, nil }This requires adding
"context"and"time"to imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bootstrap/db_bootstrap.go` around lines 60 - 70, The SetupRedis function creates a rueidis client lazily but doesn't verify connectivity; modify SetupRedis to perform an immediate health check by creating a context with timeout (e.g., context.WithTimeout(ctx, 2*time.Second)), sending a PING command via the created client (using the client’s request builder/Do method, e.g., client.Do(ctx, client.B().Ping().Build())), and returning a wrapped error if the ping fails; also add the "context" and "time" imports so the timeout context is available.internal/config/config.go (1)
85-85: Minor: Missing period in description tag.For consistency with other fields in the
Configstruct (e.g.,Database,Analytics), theSessionfield's description should end with a period.- Session SessionConfig `description:"Session configuration" yaml:"session"` + Session SessionConfig `description:"Session configuration." yaml:"session"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` at line 85, The description tag for the Config struct's Session field is missing a trailing period; update the struct field "Session SessionConfig `description:\"Session configuration\" yaml:\"session\"`" so the description ends with a period (i.e., `description:"Session configuration."`) to match the style of other fields like Database and Analytics.internal/repository/redis_session_queries.go (1)
75-109: Non-atomic read-modify-write pattern remains.The
UpdateSessionmethod fetches the session, modifies it in memory, and writes it back. If two concurrent requests update the same session, one update may be lost. For session refresh operations this is likely acceptable (both would extend the session), but be aware this differs from the database implementation which uses a single UPDATE statement.Consider documenting this behavior or using a Lua script for full atomicity if stricter guarantees are needed in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repository/redis_session_queries.go` around lines 75 - 109, UpdateSession currently does a non-atomic read-modify-write (calls GetSession then Set) which can lose concurrent updates; either document this concurrency caveat on RedisSessionRepository/UpdateSession or make the update atomic by replacing the RMW with a Redis EVAL Lua script that: GETs the key, unmarshals JSON, applies the same field updates (UUID, Email, Username, Name, Provider, TotpPending, OAuthName, OAuthGroups, OAuthSub, Expiry), re-marshals and SETs the key with EX seconds computed from params.Expiry - time.Now().Unix(), and returns the final JSON (or error); locate UpdateSession and GetSession to implement or reference the Lua-based atomic path.internal/service/auth_service.go (1)
13-13: Consider extracting the "not found" check to avoid coupling.Importing
rueidisin the auth service creates a compile-time dependency on the Redis library even when using the database backend. While this is minor (the library is already a dependency), you could consider having the Redis repository return a common sentinel error (e.g.,repository.ErrSessionNotFound) to keep the service layer backend-agnostic.This is a minor architectural consideration and the current approach works correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/auth_service.go` at line 13, Extract the Redis-specific "not found" check into a repository-level sentinel error so auth_service.go no longer imports rueidis; add a package-level error like repository.ErrSessionNotFound in the repository package, update the Redis implementation (the redis repository type / its GetSession/FindSession method) to translate rueidis.NilReply (or other Redis nil) into repository.ErrSessionNotFound, and change auth_service.go to check against repository.ErrSessionNotFound instead of referencing rueidis.NilReply so the service layer stays backend-agnostic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/repository/redis_session_queries.go`:
- Around line 100-103: The TTL calculation passed to ExSeconds can be zero or
negative (params.Expiry - time.Now().Unix()), causing the SET command to fail;
update the session save logic around the
r.c.B().Set().Key(params.UUID).Value(string(raw)).ExSeconds(...) call to compute
ttl := params.Expiry - time.Now().Unix() and only call ExSeconds(ttl) when ttl >
0, otherwise call Set without ExSeconds (or return an error/handle as your
CreateSession does) so you avoid passing non-positive expiry values to the
builder.
- Around line 65-67: Compute TTL before building the Redis command by doing ttl
:= params.Expiry - time.Now().Unix(); if ttl <= 0 return an appropriate error
(e.g., "session already expired") to avoid issuing SET ... EX 0/negative,
otherwise use ttl (or ttl := max(1, ttl) if you prefer to allow a 1s minimum)
when calling ExSeconds; update the builder call
(r.c.B().Set().Key(params.UUID).Value(string(raw)).Nx().ExSeconds(ttl).Build())
and add "fmt" to imports for the error formatting if needed.
---
Nitpick comments:
In `@internal/bootstrap/db_bootstrap.go`:
- Around line 60-70: The SetupRedis function currently calls rueidis.NewClient
without TLS options; update it to accept TLS settings from your RedisConfig (add
fields like TLS bool and InsecureTLS bool) and pass a configured tls.Config into
rueidis.ClientOption when TLS is enabled; specifically, modify SetupRedis to
read RedisConfig.TLS and RedisConfig.InsecureTLS, build a tls.Config (e.g.,
InsecureSkipVerify when InsecureTLS is true), and set the appropriate TLS fields
on rueidis.ClientOption so the rueidis.NewClient call uses TLS for secure Redis
connections.
- Around line 60-70: The SetupRedis function creates a rueidis client lazily but
doesn't verify connectivity; modify SetupRedis to perform an immediate health
check by creating a context with timeout (e.g., context.WithTimeout(ctx,
2*time.Second)), sending a PING command via the created client (using the
client’s request builder/Do method, e.g., client.Do(ctx,
client.B().Ping().Build())), and returning a wrapped error if the ping fails;
also add the "context" and "time" imports so the timeout context is available.
In `@internal/config/config.go`:
- Line 85: The description tag for the Config struct's Session field is missing
a trailing period; update the struct field "Session SessionConfig
`description:\"Session configuration\" yaml:\"session\"`" so the description
ends with a period (i.e., `description:"Session configuration."`) to match the
style of other fields like Database and Analytics.
In `@internal/repository/redis_session_queries.go`:
- Around line 75-109: UpdateSession currently does a non-atomic
read-modify-write (calls GetSession then Set) which can lose concurrent updates;
either document this concurrency caveat on RedisSessionRepository/UpdateSession
or make the update atomic by replacing the RMW with a Redis EVAL Lua script
that: GETs the key, unmarshals JSON, applies the same field updates (UUID,
Email, Username, Name, Provider, TotpPending, OAuthName, OAuthGroups, OAuthSub,
Expiry), re-marshals and SETs the key with EX seconds computed from
params.Expiry - time.Now().Unix(), and returns the final JSON (or error); locate
UpdateSession and GetSession to implement or reference the Lua-based atomic
path.
In `@internal/service/auth_service.go`:
- Line 13: Extract the Redis-specific "not found" check into a repository-level
sentinel error so auth_service.go no longer imports rueidis; add a package-level
error like repository.ErrSessionNotFound in the repository package, update the
Redis implementation (the redis repository type / its GetSession/FindSession
method) to translate rueidis.NilReply (or other Redis nil) into
repository.ErrSessionNotFound, and change auth_service.go to check against
repository.ErrSessionNotFound instead of referencing rueidis.NilReply so the
service layer stays backend-agnostic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: daa5a97c-173f-4ee9-a2a2-1d2dcb3dd259
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.modinternal/bootstrap/app_bootstrap.gointernal/bootstrap/db_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/config/config.gointernal/repository/redis_session_queries.gointernal/service/auth_service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- internal/bootstrap/app_bootstrap.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a new session driver option to add redis as a backend for sessions. To use it, set the following vars:
TINYAUTH_DATABASE_REDIS_URL=redis.redis.svc:6379TINYAUTH_SESSION_DRIVER=redisSummary by CodeRabbit