Repository: zeromicro/go-zero
Pull request #2947 (2023-03-02 01:57:51)

feat: detect conf overwritten fields


Patch:
diff --git a/core/conf/config.go b/core/conf/config.go
index 002a4d91..ccc9e8ee 100644
--- a/core/conf/config.go
+++ b/core/conf/config.go
@@ -20,8 +20,10 @@ var loaders = map[string]func([]byte, any) error{
 	".yml":  LoadFromYamlBytes,
 }
 
+// children and mapField should not be both filled.
+// named fields and map cannot be bound to the same field name.
 type fieldInfo struct {
-	children map[string]fieldInfo
+	children map[string]*fieldInfo
 	mapField *fieldInfo
 }
 
@@ -57,13 +59,17 @@ func LoadConfig(file string, v any, opts ...Option) error {
 
 // LoadFromJsonBytes loads config into v from content json bytes.
 func LoadFromJsonBytes(content []byte, v any) error {
+	info, err := buildFieldsInfo(reflect.TypeOf(v))
+	if err != nil {
+		return err
+	}
+
 	var m map[string]any
 	if err := jsonx.Unmarshal(content, &m); err != nil {
 		return err
 	}
 
-	finfo := buildFieldsInfo(reflect.TypeOf(v))
-	lowerCaseKeyMap := toLowerCaseKeyMap(m, finfo)
+	lowerCaseKeyMap := toLowerCaseKeyMap(m, info)
 
 	return mapping.UnmarshalJsonMap(lowerCaseKeyMap, v, mapping.WithCanonicalKeyFunc(toLowerCase))
 }
@@ -107,19 +113,63 @@ func MustLoad(path string, v any, opts ...Option) {
 	}
 }
 
-func addOrMergeFields(info fieldInfo, key string, child fieldInfo) {
+func addOrMergeFields(info *fieldInfo, key string, child *fieldInfo) error {
 	if prev, ok := info.children[key]; ok {
-		// merge fields
-		for k, v := range child.children {
-			prev.children[k] = v
+		if child.mapField != nil {
+			return newDupKeyError(key)
+		}
+
+		if err := mergeFields(prev, key, child.children); err != nil {
+			return err
 		}
-		prev.mapField = child.mapField
 	} else {
 		info.children[key] = child
 	}
+
+	return nil
+}
+
+func buildAnonymousFieldInfo(info *fieldInfo, lowerCaseName string, ft reflect.Type) error {
+	switch ft.Kind() {
+	case reflect.Struct:
+		fields, err := buildFieldsInfo(ft)
+		if err != nil {
+			return err
+		}
+
+		for k, v := range fields.children {
+			if err = addOrMergeFields(info, k, v); err != nil {
+				return err
+			}
+		}
+	case reflect.Map:
+		elemField, err := buildFieldsInfo(mapping.Deref(ft.Elem()))
+		if err != nil {
+			return err
+		}
+
+		if _, ok := info.children[lowerCaseName]; ok {
+			return newDupKeyError(lowerCaseName)
+		}
+
+		info.children[lowerCaseName] = &fieldInfo{
+			children: make(map[string]*fieldInfo),
+			mapField: elemField,
+		}
+	default:
+		if _, ok := info.children[lowerCaseName]; ok {
+			return newDupKeyError(lowerCaseName)
+		}
+
+		info.children[lowerCaseName] = &fieldInfo{
+			children: make(map[string]*fieldInfo),
+		}
+	}
+
+	return nil
 }
 
-func buildFieldsInfo(tp reflect.Type) fieldInfo {
+func buildFieldsInfo(tp reflect.Type) (*fieldInfo, error) {
 	tp = mapping.Deref(tp)
 
 	switch tp.Kind() {
@@ -127,14 +177,53 @@ func buildFieldsInfo(tp reflect.Type) fieldInfo {
 		return buildStructFieldsInfo(tp)
 	case reflect.Array, reflect.Slice:
 		return buildFieldsInfo(mapping.Deref(tp.Elem()))
+	case reflect.Chan, reflect.Func:
+		return nil, fmt.Errorf("unsupported type: %s", tp.Kind())
 	default:
-		return fieldInfo{}
+		return &fieldInfo{
+			children: make(map[string]*fieldInfo),
+		}, nil
 	}
 }
 
-func buildStructFieldsInfo(tp reflect.Type) fieldInfo {
-	info := fieldInfo{
-		children: make(map[string]fieldInfo),
+func buildNamedFieldInfo(info *fieldInfo, lowerCaseName string, ft reflect.Type) error {
+	var finfo *fieldInfo
+	var err error
+
+	switch ft.Kind() {
+	case reflect.Struct:
+		finfo, err = buildFieldsInfo(ft)
+		if err != nil {
+			return err
+		}
+	case reflect.Array, reflect.Slice:
+		finfo, err = buildFieldsInfo(ft.Elem())
+		if err != nil {
+			return err
+		}
+	case reflect.Map:
+		elemInfo, err := buildFieldsInfo(mapping.Deref(ft.Elem()))
+		if err != nil {
+			return err
+		}
+
+		finfo = &fieldInfo{
+			children: make(map[string]*fieldInfo),
+			mapField: elemInfo,
+		}
+	default:
+		finfo, err = buildFieldsInfo(ft)
+		if err != nil {
+			return err
+		}
+	}
+
+	return addOrMergeFields(info, lowerCaseName, finfo)
+}
+
+func buildStructFieldsInfo(tp reflect.Type) (*fieldInfo, error) {
+	info := &fieldInfo{
+		children: make(map[string]*fieldInfo),
 	}
 
 	for i := 0; i < tp.NumField(); i++ {
@@ -144,48 +233,39 @@ func buildStructFieldsInfo(tp reflect.Type) fieldInfo {
 		ft := mapping.Deref(field.Type)
 		// flatten anonymous fields
 		if field.Anonymous {
-			switch ft.Kind() {
-			case reflect.Struct:
-				fields := buildFieldsInfo(ft)
-				for k, v := range fields.children {
-					addOrMergeFields(info, k, v)
-				}
-				info.mapField = fields.mapField
-			case reflect.Map:
-				elemField := buildFieldsInfo(mapping.Deref(ft.Elem()))
-				info.children[lowerCaseName] = fieldInfo{
-					mapField: &elemField,
-				}
-			default:
-				info.children[lowerCaseName] = fieldInfo{
-					children: make(map[string]fieldInfo),
-				}
+			if err := buildAnonymousFieldInfo(info, lowerCaseName, ft); err != nil {
+				return nil, err
 			}
-			continue
+		} else if err := buildNamedFieldInfo(info, lowerCaseName, ft); err != nil {
+			return nil, err
 		}
+	}
+
+	return info, nil
+}
+
+func mergeFields(prev *fieldInfo, key string, children map[string]*fieldInfo) error {
+	if len(prev.children) == 0 || len(children) == 0 {
+		return newDupKeyError(key)
+	}
 
-		var finfo fieldInfo
-		switch ft.Kind() {
-		case reflect.Struct:
-			finfo = buildFieldsInfo(ft)
-		case reflect.Array, reflect.Slice:
-			finfo = buildFieldsInfo(ft.Elem())
-		case reflect.Map:
-			elemInfo := buildFieldsInfo(mapping.Deref(ft.Elem()))
-			finfo.mapField = &elemInfo
+	// merge fields
+	for k, v := range children {
+		if _, ok := prev.children[k]; ok {
+			return newDupKeyError(k)
 		}
 
-		addOrMergeFields(info, lowerCaseName, finfo)
+		prev.children[k] = v
 	}
 
-	return info
+	return nil
 }
 
 func toLowerCase(s string) string {
 	return strings.ToLower(s)
 }
 
-func toLowerCaseInterface(v any, info fieldInfo) any {
+func toLowerCaseInterface(v any, info *fieldInfo) any {
 	switch vv := v.(type) {
 	case map[string]any:
 		return toLowerCaseKeyMap(vv, info)
@@ -200,7 +280,7 @@ func toLowerCaseInterface(v any, info fieldInfo) any {
 	}
 }
 
-func toLowerCaseKeyMap(m map[string]any, info fieldInfo) map[string]any {
+func toLowerCaseKeyMap(m map[string]any, info *fieldInfo) map[string]any {
 	res := make(map[string]any)
 
 	for k, v := range m {
@@ -214,7 +294,7 @@ func toLowerCaseKeyMap(m map[string]any, info fieldInfo) map[string]any {
 		if ti, ok = info.children[lk]; ok {
 			res[lk] = toLowerCaseInterface(v, ti)
 		} else if info.mapField != nil {
-			res[k] = toLowerCaseInterface(v, *info.mapField)
+			res[k] = toLowerCaseInterface(v, info.mapField)
 		} else {
 			res[k] = v
 		}
@@ -222,3 +302,15 @@ func toLowerCaseKeyMap(m map[string]any, info fieldInfo) map[string]any {
 
 	return res
 }
+
+type dupKeyError struct {
+	key string
+}
+
+func newDupKeyError(key string) dupKeyError {
+	return dupKeyError{key: key}
+}
+
+func (e dupKeyError) Error() string {
+	return fmt.Sprintf("duplicated key %s", e.key)
+}
diff --git a/core/logx/config.go b/core/logx/config.go
index 8f0398fd..1bd2317d 100644
--- a/core/logx/config.go
+++ b/core/logx/config.go
@@ -2,17 +2,34 @@ package logx
 
 // A LogConf is a logging config.
 type LogConf struct {
-	ServiceName         string `json:",optional"`
-	Mode                string `json:",default=console,options=[console,file,volume]"`
-	Encoding            string `json:",default=json,options=[json,plain]"`
-	TimeFormat          string `json:",optional"`
-	Path                string `json:",default=logs"`
-	Level               string `json:",default=info,options=[debug,info,error,severe]"`
-	MaxContentLength    uint32 `json:",optional"`
-	Compress            bool   `json:",optional"`
-	Stat                bool   `json:",default=true"`
-	KeepDays            int    `json:",optional"`
-	StackCooldownMillis int    `json:",default=100"`
+	// ServiceName represents the service name.
+	ServiceName string `json:",optional"`
+	// Mode represents the logging mode, default is `console`.
+	// console: log to console.
+	// file: log to file.
+	// volume: used in k8s, prepend the hostname to the log file name.
+	Mode string `json:",default=console,options=[console,file,volume]"`
+	// Encoding represents the encoding type, default is `json`.
+	// json: json encoding.
+	// plain: plain text encoding, typically used in development.
+	Encoding string `json:",default=json,options=[json,plain]"`
+	// TimeFormat represents the time format, default is `2006-01-02T15:04:05.000Z07:00`.
+	TimeFormat string `json:",optional"`
+	// Path represents the log file path, default is `logs`.
+	Path string `json:",default=logs"`
+	// Level represents the log level, default is `info`.
+	Level string `json:",default=info,options=[debug,info,error,severe]"`
+	// MaxContentLength represents the max content bytes, default is no limit.
+	MaxContentLength uint32 `json:",optional"`
+	// Compress represents whether to compress the log file, default is `false`.
+	Compress bool `json:",optional"`
+	// Stdout represents whether to log statistics, default is `true`.
+	Stat bool `json:",default=true"`
+	// KeepDays represents how many days the log files will be kept. Default to keep all files.
+	// Only take effect when Mode is `file` or `volume`, both work when Rotation is `daily` or `size`.
+	KeepDays int `json:",optional"`
+	// StackCooldownMillis represents the cooldown time for stack logging, default is 100ms.
+	StackCooldownMillis int `json:",default=100"`
 	// MaxBackups represents how many backup log files will be kept. 0 means all files will be kept forever.
 	// Only take effect when RotationRuleType is `size`.
 	// Even thougth `MaxBackups` sets 0, log files will still be removed


Test patch:
diff --git a/core/conf/config_test.go b/core/conf/config_test.go
index e92152e7..7ab3689f 100644
--- a/core/conf/config_test.go
+++ b/core/conf/config_test.go
@@ -9,6 +9,8 @@ import (
 	"github.com/zeromicro/go-zero/core/hash"
 )
 
+var dupErr dupKeyError
+
 func TestLoadConfig_notExists(t *testing.T) {
 	assert.NotNil(t, Load("not_a_file", nil))
 }
@@ -413,11 +415,7 @@ func TestLoadFromYamlItemOverlay(t *testing.T) {
 `)
 
 	var c TestConfig
-	if assert.NoError(t, LoadFromYamlBytes(input, &c)) {
-		assert.Equal(t, "localhost", c.Redis.Host)
-		assert.Equal(t, 6379, c.Redis.Port)
-		assert.Equal(t, "test", c.Server.Redis.Key)
-	}
+	assert.ErrorAs(t, LoadFromYamlBytes(input, &c), &dupErr)
 }
 
 func TestLoadFromYamlItemOverlayReverse(t *testing.T) {
@@ -449,11 +447,7 @@ func TestLoadFromYamlItemOverlayReverse(t *testing.T) {
 `)
 
 	var c TestConfig
-	if assert.NoError(t, LoadFromYamlBytes(input, &c)) {
-		assert.Equal(t, "localhost", c.Redis.Host)
-		assert.Equal(t, 6379, c.Redis.Port)
-		assert.Equal(t, "test", c.Redis.Key)
-	}
+	assert.ErrorAs(t, LoadFromYamlBytes(input, &c), &dupErr)
 }
 
 func TestLoadFromYamlItemOverlayWithMap(t *testing.T) {
@@ -485,11 +479,7 @@ func TestLoadFromYamlItemOverlayWithMap(t *testing.T) {
 `)
 
 	var c TestConfig
-	if assert.NoError(t, LoadFromYamlBytes(input, &c)) {
-		assert.Equal(t, "localhost", c.Server.Redis.Host)
-		assert.Equal(t, 6379, c.Server.Redis.Port)
-		assert.Equal(t, "test", c.Server.Redis.Key)
-	}
+	assert.ErrorAs(t, LoadFromYamlBytes(input, &c), &dupErr)
 }
 
 func TestUnmarshalJsonBytesMap(t *testing.T) {
@@ -616,6 +606,422 @@ func TestUnmarshalJsonBytesWithMapTypeValueOfStruct(t *testing.T) {
 	}
 }
 
+func Test_FieldOverwrite(t *testing.T) {
+	t.Run("normal", func(t *testing.T) {
+		type Base struct {
+			Name string
+		}
+
+		type St1 struct {
+			Base
+			Name2 string
+		}
+
+		type St2 struct {
+			Base
+			Name2 string
+		}
+
+		type St3 struct {
+			*Base
+			Name2 string
+		}
+
+		type St4 struct {
+			*Base
+			Name2 *string
+		}
+
+		validate := func(val any) {
+			input := []byte(`{"Name": "hello", "Name2": "world"}`)
+			assert.NoError(t, LoadFromJsonBytes(input, val))
+		}
+
+		validate(&St1{})
+		validate(&St2{})
+		validate(&St3{})
+		validate(&St4{})
+	})
+
+	t.Run("Inherit Override", func(t *testing.T) {
+		type Base struct {
+			Name string
+		}
+
+		type St1 struct {
+			Base
+			Name string
+		}
+
+		type St2 struct {
+			Base
+			Name int
+		}
+
+		type St3 struct {
+			*Base
+			Name int
+		}
+
+		type St4 struct {
+			*Base
+			Name *string
+		}
+
+		validate := func(val any) {
+			input := []byte(`{"Name": "hello"}`)
+			err := LoadFromJsonBytes(input, val)
+			assert.ErrorAs(t, err, &dupErr)
+			assert.Equal(t, newDupKeyError("name").Error(), err.Error())
+		}
+
+		validate(&St1{})
+		validate(&St2{})
+		validate(&St3{})
+		validate(&St4{})
+	})
+
+	t.Run("Inherit more", func(t *testing.T) {
+		type Base1 struct {
+			Name string
+		}
+
+		type St0 struct {
+			Base1
+			Name string
+		}
+
+		type St1 struct {
+			St0
+			Name string
+		}
+
+		type St2 struct {
+			St0
+			Name int
+		}
+
+		type St3 struct {
+			*St0
+			Name int
+		}
+
+		type St4 struct {
+			*St0
+			Name *int
+		}
+
+		validate := func(val any) {
+			input := []byte(`{"Name": "hello"}`)
+			err := LoadFromJsonBytes(input, val)
+			assert.ErrorAs(t, err, &dupErr)
+			assert.Equal(t, newDupKeyError("name").Error(), err.Error())
+		}
+
+		validate(&St0{})
+		validate(&St1{})
+		validate(&St2{})
+		validate(&St3{})
+		validate(&St4{})
+	})
+}
+
+func TestFieldOverwriteComplicated(t *testing.T) {
+	t.Run("double maps", func(t *testing.T) {
+		type (
+			Base1 struct {
+				Values map[string]string
+			}
+			Base2 struct {
+				Values map[string]string
+			}
+			Config struct {
+				Base1
+				Base2
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Values": {"Key": "Value"}}`)
+		assert.ErrorAs(t, LoadFromJsonBytes(input, &c), &dupErr)
+	})
+
+	t.Run("merge children", func(t *testing.T) {
+		type (
+			Inner1 struct {
+				Name string
+			}
+			Inner2 struct {
+				Age int
+			}
+			Base1 struct {
+				Inner Inner1
+			}
+			Base2 struct {
+				Inner Inner2
+			}
+			Config struct {
+				Base1
+				Base2
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Inner": {"Name": "foo", "Age": 10}}`)
+		if assert.NoError(t, LoadFromJsonBytes(input, &c)) {
+			assert.Equal(t, "foo", c.Base1.Inner.Name)
+			assert.Equal(t, 10, c.Base2.Inner.Age)
+		}
+	})
+
+	t.Run("overwritten maps", func(t *testing.T) {
+		type (
+			Inner struct {
+				Map map[string]string
+			}
+			Config struct {
+				Map map[string]string
+				Inner
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Inner": {"Map": {"Key": "Value"}}}`)
+		assert.ErrorAs(t, LoadFromJsonBytes(input, &c), &dupErr)
+	})
+
+	t.Run("overwritten nested maps", func(t *testing.T) {
+		type (
+			Inner struct {
+				Map map[string]string
+			}
+			Middle1 struct {
+				Map map[string]string
+				Inner
+			}
+			Middle2 struct {
+				Map map[string]string
+				Inner
+			}
+			Config struct {
+				Middle1
+				Middle2
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Middle1": {"Inner": {"Map": {"Key": "Value"}}}}`)
+		assert.ErrorAs(t, LoadFromJsonBytes(input, &c), &dupErr)
+	})
+
+	t.Run("overwritten outer/inner maps", func(t *testing.T) {
+		type (
+			Inner struct {
+				Map map[string]string
+			}
+			Middle struct {
+				Inner
+				Map map[string]string
+			}
+			Config struct {
+				Middle
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Middle": {"Inner": {"Map": {"Key": "Value"}}}}`)
+		assert.ErrorAs(t, LoadFromJsonBytes(input, &c), &dupErr)
+	})
+
+	t.Run("overwritten anonymous maps", func(t *testing.T) {
+		type (
+			Inner struct {
+				Map map[string]string
+			}
+			Middle struct {
+				Inner
+				Map map[string]string
+			}
+			Elem   map[string]Middle
+			Config struct {
+				Elem
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Elem": {"Key": {"Inner": {"Map": {"Key": "Value"}}}}}`)
+		assert.ErrorAs(t, LoadFromJsonBytes(input, &c), &dupErr)
+	})
+
+	t.Run("overwritten primitive and map", func(t *testing.T) {
+		type (
+			Inner struct {
+				Value string
+			}
+			Elem  map[string]Inner
+			Named struct {
+				Elem string
+			}
+			Config struct {
+				Named
+				Elem
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Elem": {"Key": {"Value": "Value"}}}`)
+		assert.ErrorAs(t, LoadFromJsonBytes(input, &c), &dupErr)
+	})
+
+	t.Run("overwritten map and slice", func(t *testing.T) {
+		type (
+			Inner struct {
+				Value string
+			}
+			Elem  []Inner
+			Named struct {
+				Elem string
+			}
+			Config struct {
+				Named
+				Elem
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Elem": {"Key": {"Value": "Value"}}}`)
+		assert.ErrorAs(t, LoadFromJsonBytes(input, &c), &dupErr)
+	})
+
+	t.Run("overwritten map and string", func(t *testing.T) {
+		type (
+			Elem  string
+			Named struct {
+				Elem string
+			}
+			Config struct {
+				Named
+				Elem
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Elem": {"Key": {"Value": "Value"}}}`)
+		assert.ErrorAs(t, LoadFromJsonBytes(input, &c), &dupErr)
+	})
+}
+
+func TestLoadNamedFieldOverwritten(t *testing.T) {
+	t.Run("overwritten named struct", func(t *testing.T) {
+		type (
+			Elem  string
+			Named struct {
+				Elem string
+			}
+			Base struct {
+				Named
+				Elem
+			}
+			Config struct {
+				Val Base
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Val": {"Elem": {"Key": {"Value": "Value"}}}}`)
+		assert.ErrorAs(t, LoadFromJsonBytes(input, &c), &dupErr)
+	})
+
+	t.Run("overwritten named []struct", func(t *testing.T) {
+		type (
+			Elem  string
+			Named struct {
+				Elem string
+			}
+			Base struct {
+				Named
+				Elem
+			}
+			Config struct {
+				Vals []Base
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Vals": [{"Elem": {"Key": {"Value": "Value"}}}]}`)
+		assert.ErrorAs(t, LoadFromJsonBytes(input, &c), &dupErr)
+	})
+
+	t.Run("overwritten named map[string]struct", func(t *testing.T) {
+		type (
+			Elem  string
+			Named struct {
+				Elem string
+			}
+			Base struct {
+				Named
+				Elem
+			}
+			Config struct {
+				Vals map[string]Base
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Vals": {"Key": {"Elem": {"Key": {"Value": "Value"}}}}}`)
+		assert.ErrorAs(t, LoadFromJsonBytes(input, &c), &dupErr)
+	})
+
+	t.Run("overwritten named *struct", func(t *testing.T) {
+		type (
+			Elem  string
+			Named struct {
+				Elem string
+			}
+			Base struct {
+				Named
+				Elem
+			}
+			Config struct {
+				Vals *Base
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Vals": [{"Elem": {"Key": {"Value": "Value"}}}]}`)
+		assert.ErrorAs(t, LoadFromJsonBytes(input, &c), &dupErr)
+	})
+
+	t.Run("overwritten named struct", func(t *testing.T) {
+		type (
+			Named struct {
+				Elem string
+			}
+			Base struct {
+				Named
+				Elem Named
+			}
+			Config struct {
+				Val Base
+			}
+		)
+
+		var c Config
+		input := []byte(`{"Val": {"Elem": "Value"}}`)
+		assert.ErrorAs(t, LoadFromJsonBytes(input, &c), &dupErr)
+	})
+
+	t.Run("overwritten named struct", func(t *testing.T) {
+		type Config struct {
+			Val chan int
+		}
+
+		var c Config
+		input := []byte(`{"Val": 1}`)
+		assert.Error(t, LoadFromJsonBytes(input, &c))
+	})
+}
+
 func createTempFile(ext, text string) (string, error) {
 	tmpfile, err := os.CreateTemp(os.TempDir(), hash.Md5Hex([]byte(text))+"*"+ext)
 	if err != nil {