From fd71b773da1b024c35b4cfe2b0a95fc546adc321 Mon Sep 17 00:00:00 2001 From: Doron Galambos Date: Tue, 16 Sep 2025 19:25:56 -0400 Subject: [PATCH 1/2] improve error handling in redisurl package - return explicit error values instead of naked returns - handle error from SELECT command when switching databases --- redis/redisurl/redisurl.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/redis/redisurl/redisurl.go b/redis/redisurl/redisurl.go index a02a3cd..bf79e8a 100644 --- a/redis/redisurl/redisurl.go +++ b/redis/redisurl/redisurl.go @@ -17,13 +17,11 @@ func Connect() (redis.Conn, error) { func ConnectToURL(s string) (c redis.Conn, err error) { redisURL, err := url.Parse(s) - if err != nil { - return + return c, err } auth := "" - if redisURL.User != nil { if password, ok := redisURL.User.Password(); ok { auth = password @@ -31,25 +29,26 @@ func ConnectToURL(s string) (c redis.Conn, err error) { } c, err = redis.Dial("tcp", redisURL.Host) - if err != nil { fmt.Println(err) - return + return c, err } if len(auth) > 0 { _, err = c.Do("AUTH", auth) - if err != nil { fmt.Println(err) - return + return c, err } } if len(redisURL.Path) > 1 { db := strings.TrimPrefix(redisURL.Path, "/") - c.Do("SELECT", db) + _, err = c.Do("SELECT", db) + if err != nil { + return c, err + } } - return + return c, err } From 9312ffa2597146ff5ad626dd4070c48f7e7f4c0e Mon Sep 17 00:00:00 2001 From: Doron Galambos Date: Fri, 24 Oct 2025 18:27:36 -0400 Subject: [PATCH 2/2] complete error handling fixes in redisurl - return nil on parse/dial errors for consistency - return connection with error after dial for caller-managed cleanup - add support for newer Redis AUTH error message - use errors.Is() for error checking in pool - add tests for error cases --- redis/connection_test.go | 1 - redis/pool.go | 6 ++--- redis/redis.go | 5 ++-- redis/redisurl/redisurl.go | 4 +-- redis/redisurl/redisurl_test.go | 47 +++++++++++++++++++++++++++++++-- 5 files changed, 53 insertions(+), 10 deletions(-) diff --git a/redis/connection_test.go b/redis/connection_test.go index 2eccde5..657f973 100644 --- a/redis/connection_test.go +++ b/redis/connection_test.go @@ -2,7 +2,6 @@ package redis_test import ( "fmt" - netURL "net/url" . "github.com/onsi/ginkgo" diff --git a/redis/pool.go b/redis/pool.go index 1278e14..59f4bcb 100644 --- a/redis/pool.go +++ b/redis/pool.go @@ -50,11 +50,11 @@ func (m *hosts) Get(host string) bool { } func generateConnection(url *netURL.URL) (redigo.Conn, error) { - // Then we expec the server to not ask for a password + // Then we expect the server to not ask for a password if hostsNotUsingAuth.Get(url.Host) { url.User = nil conn, err := redisurl.ConnectToURL(url.String()) - if err == redigoErrNoAuth { + if errors.Is(err, redigoErrNoAuth) { hostsNotUsingAuth.Remove(url.Host) return generateConnection(url) } @@ -63,7 +63,7 @@ func generateConnection(url *netURL.URL) (redigo.Conn, error) { // Then we expect the server to potentially ask for a password conn, err := redisurl.ConnectToURL(url.String()) - if err == redigoErrSentAuth { + if errors.Is(err, redigoErrSentAuth) || errors.Is(err, redigoErrSentAuth2) { hostsNotUsingAuth.Add(url.Host) return generateConnection(url) } diff --git a/redis/redis.go b/redis/redis.go index 3411afe..86e416e 100644 --- a/redis/redis.go +++ b/redis/redis.go @@ -7,8 +7,9 @@ import ( var ( ErrNil = redigo.ErrNil - redigoErrNoAuth = redigo.Error("NOAUTH Authentication required.") - redigoErrSentAuth = redigo.Error("ERR Client sent AUTH, but no password is set") + redigoErrNoAuth = redigo.Error("NOAUTH Authentication required.") + redigoErrSentAuth = redigo.Error("ERR Client sent AUTH, but no password is set") + redigoErrSentAuth2 = redigo.Error("ERR AUTH called without any password configured for the default user. Are you sure your configuration is correct?") ) type Z struct { diff --git a/redis/redisurl/redisurl.go b/redis/redisurl/redisurl.go index bf79e8a..92e6bbd 100644 --- a/redis/redisurl/redisurl.go +++ b/redis/redisurl/redisurl.go @@ -18,7 +18,7 @@ func Connect() (redis.Conn, error) { func ConnectToURL(s string) (c redis.Conn, err error) { redisURL, err := url.Parse(s) if err != nil { - return c, err + return nil, err } auth := "" @@ -31,7 +31,7 @@ func ConnectToURL(s string) (c redis.Conn, err error) { c, err = redis.Dial("tcp", redisURL.Host) if err != nil { fmt.Println(err) - return c, err + return nil, err } if len(auth) > 0 { diff --git a/redis/redisurl/redisurl_test.go b/redis/redisurl/redisurl_test.go index 3fb1921..1b0b34d 100644 --- a/redis/redisurl/redisurl_test.go +++ b/redis/redisurl/redisurl_test.go @@ -9,10 +9,14 @@ import ( func TestConnect(t *testing.T) { c, err := redisurl.Connect() - if err != nil { - t.Errorf("Error returned") + t.Fatalf("Error returned: %v", err) + } + + if c == nil { + t.Fatal("Connection is nil") } + defer c.Close() pong, err := redis.String(c.Do("PING")) @@ -24,3 +28,42 @@ func TestConnect(t *testing.T) { t.Errorf("Wanted PONG, got %v\n", pong) } } + +func TestConnectToURL_InvalidURL(t *testing.T) { + c, err := redisurl.ConnectToURL("://invalid url") + if err == nil { + t.Error("Expected error for invalid URL, got nil") + } + + if c != nil { + t.Error("Expected nil connection for invalid URL") + } +} + +func TestConnectToURL_InvalidHost(t *testing.T) { + c, err := redisurl.ConnectToURL("redis://nonexistent-host-12345:6379") + if err == nil { + t.Error("Expected error for invalid host, got nil") + } + + // This is the critical check - connection should be nil on error + if c != nil { + t.Error("Expected nil connection when dial fails") + } +} + +func TestConnectToURL_WithAuth(t *testing.T) { + c, err := redisurl.ConnectToURL("redis://:password@localhost:6379/10") + // When AUTH fails, connection is returned so caller can check error type and retry + if err != nil { + if c == nil { + t.Error("Connection should be returned even when AUTH fails (for fallback logic)") + } else { + c.Close() // Caller's responsibility to close on error + } + } + + if c != nil && err == nil { + defer c.Close() + } +}