From 3ac3452fe5381ac1e639d2b73d16c07d4dd249c1 Mon Sep 17 00:00:00 2001 From: Dominik Menke Date: Fri, 10 Sep 2021 19:15:16 +0200 Subject: [PATCH 1/2] Refactor TestParseURL This is in preparation for supporting query parameters in ParseURL: - use an expected *Options instance to execute assertions on - extract assertions into helper function - enable parallel testing - condense test table --- options_test.go | 217 ++++++++++++++++++++---------------------------- 1 file changed, 88 insertions(+), 129 deletions(-) diff --git a/options_test.go b/options_test.go index 6b5c4534..3c1218e7 100644 --- a/options_test.go +++ b/options_test.go @@ -4,6 +4,7 @@ package redis import ( + "crypto/tls" "errors" "testing" "time" @@ -11,150 +12,108 @@ import ( func TestParseURL(t *testing.T) { cases := []struct { - u string - addr string - db int - tls bool - err error - user string - pass string + url string + o *Options // expected value + err error }{ { - "redis://localhost:123/1", - "localhost:123", - 1, false, nil, - "", "", - }, - { - "redis://localhost:123", - "localhost:123", - 0, false, nil, - "", "", - }, - { - "redis://localhost/1", - "localhost:6379", - 1, false, nil, - "", "", - }, - { - "redis://12345", - "12345:6379", - 0, false, nil, - "", "", - }, - { - "rediss://localhost:123", - "localhost:123", - 0, true, nil, - "", "", - }, - { - "redis://:bar@localhost:123", - "localhost:123", - 0, false, nil, - "", "bar", - }, - { - "redis://foo@localhost:123", - "localhost:123", - 0, false, nil, - "foo", "", - }, - { - "redis://foo:bar@localhost:123", - "localhost:123", - 0, false, nil, - "foo", "bar", - }, - { - "unix:///tmp/redis.sock", - "/tmp/redis.sock", - 0, false, nil, - "", "", - }, - { - "unix://foo:bar@/tmp/redis.sock", - "/tmp/redis.sock", - 0, false, nil, - "foo", "bar", - }, - { - "unix://foo:bar@/tmp/redis.sock?db=3", - "/tmp/redis.sock", - 3, false, nil, - "foo", "bar", - }, - { - "unix://foo:bar@/tmp/redis.sock?db=test", - "/tmp/redis.sock", - 0, false, errors.New("redis: invalid database number: strconv.Atoi: parsing \"test\": invalid syntax"), - "", "", - }, - { - "redis://localhost/?abc=123", - "", - 0, false, errors.New("redis: no options supported"), - "", "", - }, - { - "http://google.com", - "", - 0, false, errors.New("redis: invalid URL scheme: http"), - "", "", - }, - { - "redis://localhost/1/2/3/4", - "", - 0, false, errors.New("redis: invalid URL path: /1/2/3/4"), - "", "", - }, - { - "12345", - "", - 0, false, errors.New("redis: invalid URL scheme: "), - "", "", - }, - { - "redis://localhost/iamadatabase", - "", - 0, false, errors.New(`redis: invalid database number: "iamadatabase"`), - "", "", + url: "redis://localhost:123/1", + o: &Options{Addr: "localhost:123", DB: 1}, + }, { + url: "redis://localhost:123", + o: &Options{Addr: "localhost:123"}, + }, { + url: "redis://localhost/1", + o: &Options{Addr: "localhost:6379", DB: 1}, + }, { + url: "redis://12345", + o: &Options{Addr: "12345:6379"}, + }, { + url: "rediss://localhost:123", + o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ /* no deep comparison */ }}, + }, { + url: "redis://:bar@localhost:123", + o: &Options{Addr: "localhost:123", Password: "bar"}, + }, { + url: "redis://foo@localhost:123", + o: &Options{Addr: "localhost:123", Username: "foo"}, + }, { + url: "redis://foo:bar@localhost:123", + o: &Options{Addr: "localhost:123", Username: "foo", Password: "bar"}, + }, { + url: "unix:///tmp/redis.sock", + o: &Options{Addr: "/tmp/redis.sock"}, + }, { + url: "unix://foo:bar@/tmp/redis.sock", + o: &Options{Addr: "/tmp/redis.sock", Username: "foo", Password: "bar"}, + }, { + url: "unix://foo:bar@/tmp/redis.sock?db=3", + o: &Options{Addr: "/tmp/redis.sock", Username: "foo", Password: "bar", DB: 3}, + }, { + url: "unix://foo:bar@/tmp/redis.sock?db=test", + err: errors.New(`redis: invalid database number: strconv.Atoi: parsing "test": invalid syntax`), + }, { + url: "redis://localhost/?abc=123", + err: errors.New("redis: no options supported"), + }, { + url: "http://google.com", + err: errors.New("redis: invalid URL scheme: http"), + }, { + url: "redis://localhost/1/2/3/4", + err: errors.New("redis: invalid URL path: /1/2/3/4"), + }, { + url: "12345", + err: errors.New("redis: invalid URL scheme: "), + }, { + url: "redis://localhost/iamadatabase", + err: errors.New(`redis: invalid database number: "iamadatabase"`), }, } - for _, c := range cases { - t.Run(c.u, func(t *testing.T) { - o, err := ParseURL(c.u) - if c.err == nil && err != nil { + for i := range cases { + tc := cases[i] + t.Run(tc.url, func(t *testing.T) { + t.Parallel() + + actual, err := ParseURL(tc.url) + if tc.err == nil && err != nil { t.Fatalf("unexpected error: %q", err) return } - if c.err != nil && err != nil { - if c.err.Error() != err.Error() { - t.Fatalf("got %q, expected %q", err, c.err) + if tc.err != nil && err != nil { + if tc.err.Error() != err.Error() { + t.Fatalf("got %q, expected %q", err, tc.err) } return } - if o.Addr != c.addr { - t.Errorf("got %q, want %q", o.Addr, c.addr) - } - if o.DB != c.db { - t.Errorf("got %q, expected %q", o.DB, c.db) - } - if c.tls && o.TLSConfig == nil { - t.Errorf("got nil TLSConfig, expected a TLSConfig") - } - if o.Username != c.user { - t.Errorf("got %q, expected %q", o.Username, c.user) - } - if o.Password != c.pass { - t.Errorf("got %q, expected %q", o.Password, c.pass) - } + comprareOptions(t, actual, tc.o) }) } } +func comprareOptions(t *testing.T, actual, expected *Options) { + t.Helper() + + if actual.Addr != expected.Addr { + t.Errorf("got %q, want %q", actual.Addr, expected.Addr) + } + if actual.DB != expected.DB { + t.Errorf("got %q, expected %q", actual.DB, expected.DB) + } + if actual.TLSConfig == nil && expected.TLSConfig != nil { + t.Errorf("got nil TLSConfig, expected a TLSConfig") + } + if actual.TLSConfig != nil && expected.TLSConfig == nil { + t.Errorf("got TLSConfig, expected no TLSConfig") + } + if actual.Username != expected.Username { + t.Errorf("got %q, expected %q", actual.Username, expected.Username) + } + if actual.Password != expected.Password { + t.Errorf("got %q, expected %q", actual.Password, expected.Password) + } +} + // Test ReadTimeout option initialization, including special values -1 and 0. // And also test behaviour of WriteTimeout option, when it is not explicitly set and use // ReadTimeout value. From dfedc31d208c9891e21ac50c5df7a12d325768ca Mon Sep 17 00:00:00 2001 From: Dominik Menke Date: Fri, 10 Sep 2021 21:12:33 +0200 Subject: [PATCH 2/2] Add query parameter parsing to ParseURL() Before this change, ParseURL would only accept a very restricted set of URLs (it returned an error, if it encountered any parameter). This commit introduces the ability to process URLs like redis://localhost/1?dial_timeout=10s and similar. Go programs which were providing a configuration tunable (e.g. CLI flag, config entry or environment variable) to configure the Redis connection now don't need to perform this task themselves. --- example_test.go | 4 +- options.go | 151 +++++++++++++++++++++++++++++++++++++++++++----- options_test.go | 85 +++++++++++++++++++++++++-- 3 files changed, 221 insertions(+), 19 deletions(-) diff --git a/example_test.go b/example_test.go index 7977bf92..f0158096 100644 --- a/example_test.go +++ b/example_test.go @@ -39,13 +39,14 @@ func ExampleNewClient() { } func ExampleParseURL() { - opt, err := redis.ParseURL("redis://:qwerty@localhost:6379/1") + opt, err := redis.ParseURL("redis://:qwerty@localhost:6379/1?dial_timeout=5s") if err != nil { panic(err) } fmt.Println("addr is", opt.Addr) fmt.Println("db is", opt.DB) fmt.Println("password is", opt.Password) + fmt.Println("dial timeout is", opt.DialTimeout) // Create client as usually. _ = redis.NewClient(opt) @@ -53,6 +54,7 @@ func ExampleParseURL() { // Output: addr is localhost:6379 // db is 1 // password is qwerty + // dial timeout is 5s } func ExampleNewFailoverClient() { diff --git a/options.go b/options.go index 5d39bf04..a4abe32c 100644 --- a/options.go +++ b/options.go @@ -8,6 +8,7 @@ import ( "net" "net/url" "runtime" + "sort" "strconv" "strings" "time" @@ -192,9 +193,32 @@ func (opt *Options) clone() *Options { // Scheme is required. // There are two connection types: by tcp socket and by unix socket. // Tcp connection: -// redis://:@:/ +// redis://:@:/ // Unix connection: // unix://:@?db= +// Most Option fields can be set using query parameters, with the following restrictions: +// - field names are mapped using snake-case conversion: to set MaxRetries, use max_retries +// - only scalar type fields are supported (bool, int, time.Duration) +// - for time.Duration fields, values must be a valid input for time.ParseDuration(); +// additionally a plain integer as value (i.e. without unit) is intepreted as seconds +// - to disable a duration field, use value less than or equal to 0; to use the default +// value, leave the value blank or remove the parameter +// - only the last value is interpreted if a parameter is given multiple times +// - fields "network", "addr", "username" and "password" can only be set using other +// URL attributes (scheme, host, userinfo, resp.), query paremeters using these +// names will be treated as unknown parameters +// - unknown parameter names will result in an error +// Examples: +// redis://user:password@localhost:6789/3?dial_timeout=3&db=1&read_timeout=6s&max_retries=2 +// is equivalent to: +// &Options{ +// Network: "tcp", +// Addr: "localhost:6789", +// DB: 1, // path "/3" was overridden by "&db=1" +// DialTimeout: 3 * time.Second, // no time unit = seconds +// ReadTimeout: 6 * time.Second, +// MaxRetries: 2, +// } func ParseURL(redisURL string) (*Options, error) { u, err := url.Parse(redisURL) if err != nil { @@ -216,10 +240,6 @@ func setupTCPConn(u *url.URL) (*Options, error) { o.Username, o.Password = getUserPassword(u) - if len(u.Query()) > 0 { - return nil, errors.New("redis: no options supported") - } - h, p, err := net.SplitHostPort(u.Host) if err != nil { h = u.Host @@ -250,7 +270,7 @@ func setupTCPConn(u *url.URL) (*Options, error) { o.TLSConfig = &tls.Config{ServerName: h} } - return o, nil + return setupConnParams(u, o) } func setupUnixConn(u *url.URL) (*Options, error) { @@ -262,19 +282,122 @@ func setupUnixConn(u *url.URL) (*Options, error) { return nil, errors.New("redis: empty unix socket path") } o.Addr = u.Path - o.Username, o.Password = getUserPassword(u) + return setupConnParams(u, o) +} - dbStr := u.Query().Get("db") - if dbStr == "" { - return o, nil // if database is not set, connect to 0 db. +type queryOptions struct { + q url.Values + err error +} + +func (o *queryOptions) string(name string) string { + vs := o.q[name] + if len(vs) == 0 { + return "" + } + delete(o.q, name) // enable detection of unknown parameters + return vs[len(vs)-1] +} + +func (o *queryOptions) int(name string) int { + s := o.string(name) + if s == "" { + return 0 + } + i, err := strconv.Atoi(s) + if err == nil { + return i + } + if o.err == nil { + o.err = fmt.Errorf("redis: invalid %s number: %s", name, err) + } + return 0 +} + +func (o *queryOptions) duration(name string) time.Duration { + s := o.string(name) + if s == "" { + return 0 + } + // try plain number first + if i, err := strconv.Atoi(s); err == nil { + if i <= 0 { + // disable timeouts + return -1 + } + return time.Duration(i) * time.Second + } + dur, err := time.ParseDuration(s) + if err == nil { + return dur + } + if o.err == nil { + o.err = fmt.Errorf("redis: invalid %s duration: %w", name, err) + } + return 0 +} + +func (o *queryOptions) bool(name string) bool { + switch s := o.string(name); s { + case "true", "1": + return true + case "false", "0", "": + return false + default: + if o.err == nil { + o.err = fmt.Errorf("redis: invalid %s boolean: expected true/false/1/0 or an empty string, got %q", name, s) + } + return false + } +} + +func (o *queryOptions) remaining() []string { + if len(o.q) == 0 { + return nil + } + keys := make([]string, 0, len(o.q)) + for k := range o.q { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +// setupConnParams converts query parameters in u to option value in o. +func setupConnParams(u *url.URL, o *Options) (*Options, error) { + q := queryOptions{q: u.Query()} + + // compat: a future major release may use q.int("db") + if tmp := q.string("db"); tmp != "" { + db, err := strconv.Atoi(tmp) + if err != nil { + return nil, fmt.Errorf("redis: invalid database number: %w", err) + } + o.DB = db } - db, err := strconv.Atoi(dbStr) - if err != nil { - return nil, fmt.Errorf("redis: invalid database number: %w", err) + o.MaxRetries = q.int("max_retries") + o.MinRetryBackoff = q.duration("min_retry_backoff") + o.MaxRetryBackoff = q.duration("max_retry_backoff") + o.DialTimeout = q.duration("dial_timeout") + o.ReadTimeout = q.duration("read_timeout") + o.WriteTimeout = q.duration("write_timeout") + o.PoolFIFO = q.bool("pool_fifo") + o.PoolSize = q.int("pool_size") + o.MinIdleConns = q.int("min_idle_conns") + o.MaxConnAge = q.duration("max_conn_age") + o.PoolTimeout = q.duration("pool_timeout") + o.IdleTimeout = q.duration("idle_timeout") + o.IdleCheckFrequency = q.duration("idle_check_frequency") + if q.err != nil { + return nil, q.err + } + + // any parameters left? + if r := q.remaining(); len(r) > 0 { + return nil, fmt.Errorf("redis: unexpected option: %s", strings.Join(r, ", ")) } - o.DB = db return o, nil } diff --git a/options_test.go b/options_test.go index 3c1218e7..14505239 100644 --- a/options_test.go +++ b/options_test.go @@ -40,6 +40,25 @@ func TestParseURL(t *testing.T) { }, { url: "redis://foo:bar@localhost:123", o: &Options{Addr: "localhost:123", Username: "foo", Password: "bar"}, + }, { + // multiple params + url: "redis://localhost:123/?db=2&read_timeout=2&pool_fifo=true", + o: &Options{Addr: "localhost:123", DB: 2, ReadTimeout: 2 * time.Second, PoolFIFO: true}, + }, { + // special case handling for disabled timeouts + url: "redis://localhost:123/?db=2&idle_timeout=0", + o: &Options{Addr: "localhost:123", DB: 2, IdleTimeout: -1}, + }, { + // negative values disable timeouts as well + url: "redis://localhost:123/?db=2&idle_timeout=-1", + o: &Options{Addr: "localhost:123", DB: 2, IdleTimeout: -1}, + }, { + // absent timeout values will use defaults + url: "redis://localhost:123/?db=2&idle_timeout=", + o: &Options{Addr: "localhost:123", DB: 2, IdleTimeout: 0}, + }, { + url: "redis://localhost:123/?db=2&idle_timeout", // missing "=" at the end + o: &Options{Addr: "localhost:123", DB: 2, IdleTimeout: 0}, }, { url: "unix:///tmp/redis.sock", o: &Options{Addr: "/tmp/redis.sock"}, @@ -50,11 +69,30 @@ func TestParseURL(t *testing.T) { url: "unix://foo:bar@/tmp/redis.sock?db=3", o: &Options{Addr: "/tmp/redis.sock", Username: "foo", Password: "bar", DB: 3}, }, { + // invalid db format url: "unix://foo:bar@/tmp/redis.sock?db=test", err: errors.New(`redis: invalid database number: strconv.Atoi: parsing "test": invalid syntax`), + }, { + // invalid int value + url: "redis://localhost/?pool_size=five", + err: errors.New(`redis: invalid pool_size number: strconv.Atoi: parsing "five": invalid syntax`), + }, { + // invalid bool value + url: "redis://localhost/?pool_fifo=yes", + err: errors.New(`redis: invalid pool_fifo boolean: expected true/false/1/0 or an empty string, got "yes"`), + }, { + // it returns first error + url: "redis://localhost/?db=foo&pool_size=five", + err: errors.New(`redis: invalid database number: strconv.Atoi: parsing "foo": invalid syntax`), }, { url: "redis://localhost/?abc=123", - err: errors.New("redis: no options supported"), + err: errors.New("redis: unexpected option: abc"), + }, { + url: "redis://foo@localhost/?username=bar", + err: errors.New("redis: unexpected option: username"), + }, { + url: "redis://localhost/?wrte_timout=10s&abc=123", + err: errors.New("redis: unexpected option: abc, wrte_timout"), }, { url: "http://google.com", err: errors.New("redis: invalid URL scheme: http"), @@ -98,7 +136,7 @@ func comprareOptions(t *testing.T, actual, expected *Options) { t.Errorf("got %q, want %q", actual.Addr, expected.Addr) } if actual.DB != expected.DB { - t.Errorf("got %q, expected %q", actual.DB, expected.DB) + t.Errorf("DB: got %q, expected %q", actual.DB, expected.DB) } if actual.TLSConfig == nil && expected.TLSConfig != nil { t.Errorf("got nil TLSConfig, expected a TLSConfig") @@ -107,10 +145,49 @@ func comprareOptions(t *testing.T, actual, expected *Options) { t.Errorf("got TLSConfig, expected no TLSConfig") } if actual.Username != expected.Username { - t.Errorf("got %q, expected %q", actual.Username, expected.Username) + t.Errorf("Username: got %q, expected %q", actual.Username, expected.Username) } if actual.Password != expected.Password { - t.Errorf("got %q, expected %q", actual.Password, expected.Password) + t.Errorf("Password: got %q, expected %q", actual.Password, expected.Password) + } + if actual.MaxRetries != expected.MaxRetries { + t.Errorf("MaxRetries: got %v, expected %v", actual.MaxRetries, expected.MaxRetries) + } + if actual.MinRetryBackoff != expected.MinRetryBackoff { + t.Errorf("MinRetryBackoff: got %v, expected %v", actual.MinRetryBackoff, expected.MinRetryBackoff) + } + if actual.MaxRetryBackoff != expected.MaxRetryBackoff { + t.Errorf("MaxRetryBackoff: got %v, expected %v", actual.MaxRetryBackoff, expected.MaxRetryBackoff) + } + if actual.DialTimeout != expected.DialTimeout { + t.Errorf("DialTimeout: got %v, expected %v", actual.DialTimeout, expected.DialTimeout) + } + if actual.ReadTimeout != expected.ReadTimeout { + t.Errorf("ReadTimeout: got %v, expected %v", actual.ReadTimeout, expected.ReadTimeout) + } + if actual.WriteTimeout != expected.WriteTimeout { + t.Errorf("WriteTimeout: got %v, expected %v", actual.WriteTimeout, expected.WriteTimeout) + } + if actual.PoolFIFO != expected.PoolFIFO { + t.Errorf("PoolFIFO: got %v, expected %v", actual.PoolFIFO, expected.PoolFIFO) + } + if actual.PoolSize != expected.PoolSize { + t.Errorf("PoolSize: got %v, expected %v", actual.PoolSize, expected.PoolSize) + } + if actual.MinIdleConns != expected.MinIdleConns { + t.Errorf("MinIdleConns: got %v, expected %v", actual.MinIdleConns, expected.MinIdleConns) + } + if actual.MaxConnAge != expected.MaxConnAge { + t.Errorf("MaxConnAge: got %v, expected %v", actual.MaxConnAge, expected.MaxConnAge) + } + if actual.PoolTimeout != expected.PoolTimeout { + t.Errorf("PoolTimeout: got %v, expected %v", actual.PoolTimeout, expected.PoolTimeout) + } + if actual.IdleTimeout != expected.IdleTimeout { + t.Errorf("IdleTimeout: got %v, expected %v", actual.IdleTimeout, expected.IdleTimeout) + } + if actual.IdleCheckFrequency != expected.IdleCheckFrequency { + t.Errorf("IdleCheckFrequency: got %v, expected %v", actual.IdleCheckFrequency, expected.IdleCheckFrequency) } }