cache/redis: use redisConfig to receive incoming JSON (previously usi…

…ng a map) (#5268) * refactor cache/redis: Use redisConfig to receive incoming JSON (previously using a map). * refactor cache/redis: Use the string type to receive JSON parameters. --------- Co-authored-by: Tan <tanqianheng@gmail.com>
beego · Jul 2, 2023 · efffd35 · efffd35
1 parent fdcf2e9
commit efffd35
Show file tree

Hide file tree

Showing 3 changed files with 154 additions and 37 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@
 - [cache/redis: support skipEmptyPrefix option ](https://github.com/beego/beego/pull/5264)
 - [fix: refactor InsertValue method](https://github.com/beego/beego/pull/5267)
 - [fix: modify InsertOrUpdate method, Remove the isMulti variable and its associated code](https://github.com/beego/beego/pull/5269)
+- [refactor cache/redis: Use redisConfig to receive incoming JSON (previously using a map)](https://github.com/beego/beego/pull/5268)
 
 ## ORM refactoring
 - [introducing internal/models pkg](https://github.com/beego/beego/pull/5238)

diff --git a/client/cache/redis/redis.go b/client/cache/redis/redis.go
@@ -43,8 +43,14 @@ import (
 	"github.com/beego/beego/v2/core/berror"
 )
 
-// DefaultKey defines the collection name of redis for the cache adapter.
-var DefaultKey = "beecacheRedis"
+const (
+	// DefaultKey defines the collection name of redis for the cache adapter.
+	DefaultKey = "beecacheRedis"
+	// defaultMaxIdle defines the default max idle connection number.
+	defaultMaxIdle = 3
+	// defaultTimeout defines the default timeout .
+	defaultTimeout = time.Second * 180
+)
 
 // Cache is Redis cache adapter.
 type Cache struct {
@@ -61,7 +67,8 @@ type Cache struct {
 	// see https://github.com/beego/beego/issues/5248
 	skipEmptyPrefix bool
 
-	// Timeout value (less than the redis server's timeout value)
+	// Timeout value (less than the redis server's timeout value).
+	// Timeout used for idle connection
 	timeout time.Duration
 }
 
@@ -204,60 +211,106 @@ func (rc *Cache) Scan(pattern string) (keys []string, err error) {
 // config: must be in this format {"key":"collection key","conn":"connection info","dbNum":"0", "skipEmptyPrefix":"true"}
 // Cached items in redis are stored forever, no garbage collection happens
 func (rc *Cache) StartAndGC(config string) error {
-	var cf map[string]string
+	err := rc.parseConf(config)
+	if err != nil {
+		return err
+	}
+
+	rc.connectInit()
+
+	c := rc.p.Get()
+	defer func() {
+		_ = c.Close()
+	}()
+
+	// test connection
+	if err = c.Err(); err != nil {
+		return berror.Wrapf(err, cache.InvalidConnection,
+			"can not connect to remote redis server, please check the connection info and network state: %s", config)
+	}
+	return nil
+}
+
+func (rc *Cache) parseConf(config string) error {
+	var cf redisConfig
 	err := json.Unmarshal([]byte(config), &cf)
 	if err != nil {
 		return berror.Wrapf(err, cache.InvalidRedisCacheCfg, "could not unmarshal the config: %s", config)
 	}
 
-	if _, ok := cf["key"]; !ok {
-		cf["key"] = DefaultKey
-	}
-	if _, ok := cf["conn"]; !ok {
-		return berror.Wrapf(err, cache.InvalidRedisCacheCfg, "config missing conn field: %s", config)
+	err = cf.parse()
+	if err != nil {
+		return err
 	}
 
+	rc.dbNum = cf.dbNum
+	rc.key = cf.Key
+	rc.conninfo = cf.Conn
+	rc.password = cf.password
+	rc.maxIdle = cf.maxIdle
+	rc.timeout = cf.timeout
+	rc.skipEmptyPrefix = cf.skipEmptyPrefix
+
+	return nil
+}
+
+type redisConfig struct {
+	DbNum           string `json:"dbNum"`
+	SkipEmptyPrefix string `json:"skipEmptyPrefix"`
+	Key             string `json:"key"`
 	// Format redis://<password>@<host>:<port>
-	cf["conn"] = strings.Replace(cf["conn"], "redis://", "", 1)
-	if i := strings.Index(cf["conn"], "@"); i > -1 {
-		cf["password"] = cf["conn"][0:i]
-		cf["conn"] = cf["conn"][i+1:]
+	Conn       string `json:"conn"`
+	MaxIdle    string `json:"maxIdle"`
+	TimeoutStr string `json:"timeout"`
+
+	dbNum           int
+	skipEmptyPrefix bool
+	maxIdle         int
+	// parse from Conn
+	password string
+	// timeout used for idle connection, default is 180 seconds.
+	timeout time.Duration
+}
+
+// parse parses the config.
+// If the necessary settings have not been set, it will return an error.
+// It will fill the default values if some fields are missing.
+func (cf *redisConfig) parse() error {
+	if cf.Conn == "" {
+		return berror.Error(cache.InvalidRedisCacheCfg, "config missing conn field")
 	}
 
-	if v, ok := cf["dbNum"]; ok {
-		rc.dbNum, _ = strconv.Atoi(v)
+	// Format redis://<password>@<host>:<port>
+	cf.Conn = strings.Replace(cf.Conn, "redis://", "", 1)
+	if i := strings.Index(cf.Conn, "@"); i > -1 {
+		cf.password = cf.Conn[0:i]
+		cf.Conn = cf.Conn[i+1:]
 	}
-	if _, ok := cf["maxIdle"]; !ok {
-		cf["maxIdle"] = "3"
+
+	if cf.Key == "" {
+		cf.Key = DefaultKey
 	}
 
-	if v, ok := cf["skipEmptyPrefix"]; ok {
-		rc.skipEmptyPrefix, _ = strconv.ParseBool(v)
+	if cf.DbNum != "" {
+		cf.dbNum, _ = strconv.Atoi(cf.DbNum)
 	}
 
-	rc.key = cf["key"]
-	rc.conninfo = cf["conn"]
-	rc.password = cf["password"]
-	rc.maxIdle, _ = strconv.Atoi(cf["maxIdle"])
+	if cf.SkipEmptyPrefix != "" {
+		cf.skipEmptyPrefix, _ = strconv.ParseBool(cf.SkipEmptyPrefix)
+	}
 
-	if v, err := time.ParseDuration(cf["timeout"]); err == nil {
-		rc.timeout = v
+	if cf.MaxIdle == "" {
+		cf.maxIdle = defaultMaxIdle
 	} else {
-		rc.timeout = 180 * time.Second
+		cf.maxIdle, _ = strconv.Atoi(cf.MaxIdle)
 	}
 
-	rc.connectInit()
-
-	c := rc.p.Get()
-	defer func() {
-		_ = c.Close()
-	}()
-
-	// test connection
-	if err = c.Err(); err != nil {
-		return berror.Wrapf(err, cache.InvalidConnection,
-			"can not connect to remote redis server, please check the connection info and network state: %s", config)
+	if v, err := time.ParseDuration(cf.TimeoutStr); err == nil {
+		cf.timeout = v
+	} else {
+		cf.timeout = defaultTimeout
 	}
+
 	return nil
 }
 

diff --git a/client/cache/redis/redis_test.go b/client/cache/redis/redis_test.go
@@ -296,3 +296,66 @@ func TestCache_associate(t *testing.T) {
 		})
 	}
 }
+
+func TestCache_parseConf(t *testing.T) {
+	tests := []struct {
+		name string
+
+		configStr string
+
+		wantCache Cache
+		wantErr   error
+	}{
+		{
+			name: "just conn",
+			configStr: `{
+  "conn": "127.0.0.1:6379"
+}`,
+
+			wantCache: Cache{
+				conninfo:        "127.0.0.1:6379",
+				dbNum:           0,
+				key:             DefaultKey,
+				password:        "",
+				maxIdle:         defaultMaxIdle,
+				skipEmptyPrefix: false,
+				timeout:         defaultTimeout,
+			},
+			wantErr: nil,
+		},
+
+		{
+			name: "all",
+			configStr: `{
+  "dbNum": "2",
+  "skipEmptyPrefix": "true",
+  "key": "mykey",
+  "conn": "redis://mypwd@127.0.0.1:6379",
+  "maxIdle": "10",
+  "timeout": "30s"
+}`,
+
+			wantCache: Cache{
+				conninfo:        "127.0.0.1:6379",
+				dbNum:           2,
+				key:             "mykey",
+				password:        "mypwd",
+				maxIdle:         10,
+				skipEmptyPrefix: true,
+				timeout:         time.Second * 30,
+			},
+			wantErr: nil,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := Cache{}
+			err := c.parseConf(tt.configStr)
+			assert.Equal(t, tt.wantErr, err)
+			if err != nil {
+				return
+			}
+			assert.Equal(t, tt.wantCache, c)
+		})
+	}
+}