Non-string type config with default value can't work

[jeffcaiz]

What happened:

It seems there is no way to apply default value to non-string type of config variable. It will always consider as a string and complain about wrong type like below.

panic: proto: (line 1:20): invalid value for bool type: "true"

What you expected to happen:

For booltype: ${SOME_VAR:true}, it should works. If I say 'true' and it will be consider a boolean true, not panic with wrong type

How to reproduce it (as minimally and precisely as possible):

1. kratos v2.1.0 installed
2. kratos new showbug
3. Replace conf.proto with attached
4. Replace config.yaml with attached
5. make config
6. make build
7. ./bin/showbug -conf ./configs/config.yaml

Anything else we need to know?:

Environment:

• Kratos version (use kratos -v): v2.1.0
• Go version (use go version): v1.17.1 linux
• OS (e.g: cat /etc/os-release): WSL2 on Windows 10
• Others:

Attachment

config.yaml

server:
  http:
    addr: 0.0.0.0:8000
    timeout: 1s
  grpc:
    addr: 0.0.0.0:9000
    timeout: 1s
data:
  database:
    driver: mysql
    source: root:root@tcp(127.0.0.1:3306)/test
  redis:
    addr: 127.0.0.1:6379
    read_timeout: 0.2s
    write_timeout: 0.2s
booltype: "${SOME_VAR:true}"

conf.proto

syntax = "proto3";
package kratos.api;

option go_package = "showbug/internal/conf;conf";

import "google/protobuf/duration.proto";

message Bootstrap {
  Server server = 1;
  Data data = 2;
  bool booltype = 3;
}

message Server {
  message HTTP {
    string network = 1;
    string addr = 2;
    google.protobuf.Duration timeout = 3;
  }
  message GRPC {
    string network = 1;
    string addr = 2;
    google.protobuf.Duration timeout = 3;
  }
  HTTP http = 1;
  GRPC grpc = 2;
}

message Data {
  message Database {
    string driver = 1;
    string source = 2;
  }
  message Redis {
    string network = 1;
    string addr = 2;
    google.protobuf.Duration read_timeout = 3;
    google.protobuf.Duration write_timeout = 4;
  }
  Database database = 1;
  Redis redis = 2;
}

[kagaya85]

Hi @jeffcaiz, I think it is a bug, because of we store all config value as a string type in a map[string]interface{} struct, when we try to load config value to our pb struct, we use json.Marshal and json.Unmarshal as a intermediate step:

// config.go:136
func (c *config) Scan(v interface{}) error {
	data, err := c.reader.Source()
	if err != nil {
		return err
	}
	return unmarshalJSON(data, v)
}

therefore, the true value will be convert to "true", and it's not a standard boolean type in json.

we will consider fixing it as soon as possible, if you have any other suggestion, welcome to discuss

[jeffcaiz]

Hi @kagaya85 ,

Thanks for your reply. Further check on the source shows that it's PKG yaml.v3's job to deal with true/false convertion to bool type in YAML format. If we write xxx: true, it will convert to bool; while xxx: "true", will be string.

I believe we can make some change to the the defaultResolver, treat true,false as boolean, valid numberic string as int and floating. If the user really just want a string, they should quote with single quotation mark.

[jeffcaiz]

Dirty fix snippet:

// V2.1.1
// Kratos的配置解码器其实有点问题
// 提了Issue，官方也认了。自己动手修复一下
// https://github.com/go-kratos/kratos/issues/1565

func CustomResolver(input map[string]interface{}) error {
	mapper := func(name string) string {
		args := strings.SplitN(strings.TrimSpace(name), ":", 2) //nolint:gomnd
		if v, has := readValue(input, args[0]); has {
			s, _ := v.String()
			return s
		} else if len(args) > 1 { // default value
			return args[1]
		}
		return ""
	}

	var resolve func(map[string]interface{}) error
	resolve = func(sub map[string]interface{}) error {
		for k, v := range sub {
			switch vt := v.(type) {
			case string:
				vs := expand(vt, mapper)

				// 如果被单引号括住，去掉单引号，保留为string
				if vst := strings.Trim(vs, "'"); len(vst) == len(vs)-1 {
					sub[k] = vst
				} else if vs == "true" || vs == "false" {
					// 如果是true/false，转为boolean。其他形式我们不支持
					vb, _ := strconv.ParseBool(vs)
					sub[k] = vb
				} else if vi, err := strconv.ParseInt(vs, 0, 32); err == nil {
					// 如果可以转整数，转
					sub[k] = vi
				} else if vf, err := strconv.ParseFloat(vs, 32); err == nil {
					// 如果可以转浮点，转
					sub[k] = vf
				} else {
					// 保留原来
					sub[k] = vs
				}

			case map[string]interface{}:
				if err := resolve(vt); err != nil {
					return err
				}
			case []interface{}:
				for i, iface := range vt {
					switch it := iface.(type) {
					case string:
						vt[i] = expand(it, mapper)
					case map[string]interface{}:
						if err := resolve(it); err != nil {
							return err
						}
					}
				}
				sub[k] = vt
			}
		}
		return nil
	}
	return resolve(input)
}


// =============================================
// Copy from kratos and make no change

func expand(s string, mapping func(string) string) string {
	r := regexp.MustCompile(`\${(.*?)}`)
	re := r.FindAllStringSubmatch(s, -1)
	for _, i := range re {
		if len(i) == 2 { //nolint:gomnd
			s = strings.ReplaceAll(s, i[0], mapping(i[1]))
		}
	}
	return s
}

type atomicValue struct {
	atomic.Value
}

type ValueLite interface {
	String() (string, error)
	Store(interface{})
	Load() interface{}
}

func (v *atomicValue) String() (string, error) {
	switch val := v.Load().(type) {
	case string:
		return val, nil
	case bool, int, int32, int64, float64:
		return fmt.Sprint(val), nil
	case []byte:
		return string(val), nil
	default:
		if s, ok := val.(fmt.Stringer); ok {
			return s.String(), nil
		}
	}
	return "", fmt.Errorf("type assert to %v failed", reflect.TypeOf(v.Load()))
}

// readValue read Value in given map[string]interface{}
// by the given path, will return false if not found.
func readValue(values map[string]interface{}, path string) (ValueLite, bool) {
	var (
		next = values
		keys = strings.Split(path, ".")
		last = len(keys) - 1
	)
	for idx, key := range keys {
		value, ok := next[key]
		if !ok {
			return nil, false
		}
		if idx == last {
			av := &atomicValue{}
			av.Store(value)
			return av, true
		}
		switch vm := value.(type) {
		case map[string]interface{}:
			next = vm
		default:
			return nil, false
		}
	}
	return nil, false
}

To use:

c := config.New(
		config.WithSource(
			file.NewSource(flagconf),
		),
		config.WithResolver(CustomResolver), // Add this
	)

[kagaya85]

If the user really just want a string, they should quote with single quotation mark.

I am glad to see CustomResolver solve this problem temporarily. We can consider about it, but it may increase more complexity when using config file. IMO, we can handle it in some way when Scan to a struct, in stead of using json.Marshal

I prefer decide value's type when READ it from map, rather than WRITE it into map, and this is what kratos config do now (by using Value interface, see config/value.go).

what do you think, PTAL @ymh199478 @tonybase

[ymh199478]

Current default value analyze is performed after Unmarshal, adding type handling code will produce side effects anyway.

In the other words，in yaml boolean type is

 y|Y|yes|Yes|YES|n|N|no|No|NO
|true|True|TRUE|false|False|FALSE
|on|On|ON|off|Off|OFF

but in json boolean type is

true | false

Parsing during Scan can solve this problem. But this will introduce more complexity.

[zjx-ERROR]

#2134