From c076a8e1bc879d75f0c69770ec59e7c197b7768c Mon Sep 17 00:00:00 2001 From: wrfly Date: Wed, 19 Aug 2026 23:45:56 +0800 Subject: [PATCH] fix: panics on named types, unbounded exponent, ignored "-" tag Parsing built the value with a hard-coded concrete type and assigned it to the field, which panics as soon as the field is not exactly that type: type Level int type Conf struct{ L []Level } // []int is not assignable to []Level type Conf struct{ D *time.Duration } // *int64 is not assignable to *time.Duration Both paths now build the value through the field's own reflect.Type, so named types work like their underlying kind. This replaces the fifteen near identical branches of parseSlice and all of parsePointer with a single setValue helper, and *time.Duration gains the "10s" syntax time.Duration already had. Other fixes: - parseScientific expanded the exponent one byte at a time with no upper bound: PORT=1e1000000 spent ~100s building a one megabyte string that cannot fit in any integer type. It is now bounded and rejects the value right away. "1.5e3" is 1500 instead of the unparsable "1.5000". - `yaml:"-"`, `json:"-"` and `env:"-"` were honoured by List but ignored by Parse, which still applied the default and bound the field to a key literally named "-". They are now skipped by both. - Parse(config) instead of Parse(&config) returned nil after filling in nothing at all. It now returns an error. - A *struct field was never walked into: its defaults and keys were silently dropped and List emitted a bogus flat key for it. It is now an optional section, allocated only when one of its fields is set, with a guard so a self referencing type terminates. - `default:"false"` on a bool overwrote a value already set by the caller, unlike every other kind. - Kinds that cannot be filled from a string (map, array, ...) were ignored without a word; they are now reported and no longer listed. - The default whitespace separator collapses repeated separators, so "a b" is two elements instead of three. - List quotes defaults with strconv.Quote instead of bare quotes. - Get and friends take the prefix Parse was called with, work on a read-only config, and no longer miss named types. - Parse errors are wrapped with %w. - The parser type is exported as ECP, so callers can name it. Tests: TestGetKeyLookupValue replaced globalEcp.LookupValue and never restored it, leaking the mock into every test that ran after it. CI runs vet, race and gofmt over a version matrix; the dead Travis config and badge are gone, and the README example now prints what it claims. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/go.yml | 36 +++- .travis.yml | 7 - README.md | 75 ++++++- ecp.go | 118 +++++++--- ecp_test.go | 9 +- get.go | 90 ++++---- parse.go | 297 ++++--------------------- range.go | 235 ++++++++++++-------- regression_test.go | 452 +++++++++++++++++++++++++++++++++++++++ value.go | 104 +++++++++ 10 files changed, 986 insertions(+), 437 deletions(-) delete mode 100644 .travis.yml create mode 100644 regression_test.go create mode 100644 value.go diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index ca84889..31aeb33 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -11,15 +11,41 @@ on: jobs: - build: + test: runs-on: ubuntu-latest + strategy: + matrix: + go-version: [ "1.19", "1.21", "1.23", "stable" ] + steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Set up Go - uses: actions/setup-go@v3 + uses: actions/setup-go@v5 with: - go-version: 1.19 + go-version: ${{ matrix.go-version }} + + - name: Vet + run: go vet ./... - name: Test - run: go test -v ./... + run: go test -race -coverprofile=coverage.out -covermode=atomic ./... + + lint: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: stable + + - name: Gofmt + run: | + unformatted=$(gofmt -l .) + if [ -n "$unformatted" ]; then + echo "These files need gofmt:" + echo "$unformatted" + exit 1 + fi diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index 75cfde2..0000000 --- a/.travis.yml +++ /dev/null @@ -1,7 +0,0 @@ -language: go -go: - - "1.12" - - "1.13" - -script: - - go test -v -cover \ No newline at end of file diff --git a/README.md b/README.md index f230aa3..b9e3287 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ The environment config keys can be auto generated or set by the `yaml` or `env` The only thing you should do is importing this package, and `Parse` your config. [![Go Report Card](https://goreportcard.com/badge/github.com/wrfly/ecp)](https://goreportcard.com/report/github.com/wrfly/ecp) -[![Build Status](https://travis-ci.org/wrfly/ecp.svg?branch=master)](https://travis-ci.org/wrfly/ecp) +[![Go](https://github.com/wrfly/ecp/actions/workflows/go.yml/badge.svg)](https://github.com/wrfly/ecp/actions/workflows/go.yml) [![GoDoc](https://godoc.org/github.com/wrfly/ecp?status.svg)](https://godoc.org/github.com/wrfly/ecp) [![license](https://img.shields.io/github/license/wrfly/ecp.svg)](https://github.com/wrfly/ecp/blob/master/LICENSE) @@ -44,7 +44,7 @@ func main() { // set some env envs := map[string]string{ "LOGLEVEL": "info", - "PORT": "1234", + "PORT": "1234", } for k, v := range envs { fmt.Printf("export %s=%s\n", k, v) @@ -53,7 +53,7 @@ func main() { fmt.Println() // then parse configuration from environments - if err := ecp.Parse(config, "ECP"); err != nil { + if err := ecp.Parse(config); err != nil { panic(err) } fmt.Printf("new log level: [ %s ], port: [ %d ]\n", @@ -61,8 +61,7 @@ func main() { fmt.Println() // and list all the env keys - envLists := ecp.List(config, "ecp") - for _, k := range envLists { + for _, k := range ecp.List(config) { fmt.Println(k) } } @@ -81,3 +80,69 @@ new log level: [ info ], port: [ 1234 ] LOGLEVEL=debug PORT= ``` + +`Parse` needs a pointer to a struct, `ecp.Parse(config)` on a plain struct +returns an error instead of quietly filling in nothing. + +## Keys + +The key of a field is built from the name of the struct it lives in and the +name of the field itself, upper cased and joined with `_`. A `yaml` or +`json` tag replaces the field name, and an `env` tag replaces the whole key: + +```go +type Conf struct { + LogLevel string // LOGLEVEL + Redis struct { + Host string `yaml:"host"` // REDIS_HOST + Port int `env:"REDIS_PORT"` // REDIS_PORT + } `yaml:"redis"` + Secret string `yaml:"-"` // ignored +} +``` + +Pass a prefix to namespace them: `ecp.Parse(&config, "APP")` reads +`APP_LOGLEVEL` and `APP_REDIS_HOST`. A key coming from an `env` tag is +taken as is, so `REDIS_PORT` stays `REDIS_PORT`. The same prefix goes to +`List` and to the `Get` helpers. + +## Values + +`Parse` sets a field when the environment key exists, otherwise it falls +back to the `default` tag. A default never overwrites a value the caller +already set, an environment value always does. + +Supported types are `string`, `bool`, all sized `int`/`uint` types, +`float32`, `float64`, `time.Duration`, slices of those, and named types +built on top of them. Anything else (a `map`, an array, ...) is reported as +an error rather than silently skipped. + +- **slices** are separated by a space by default, change it with + `e := ecp.New(); e.Advance.SplitChar = ","` +- **durations** accept everything `time.ParseDuration` does, plus `Xd` + for X days: `10s`, `5m`, `6d` +- **integers** also accept `1e3` and `1,000` notation +- **pointers** (`*int`, `*time.Duration`, ...) only get their default when + they are nil, which makes "unset" and "set to the zero value" + distinguishable +- **pointers to a struct** are optional sections: they are walked into like + a plain struct and only allocated when one of their fields is set + +An environment variable set to an empty value is treated as unset, so a +field keeps its default. + +## Advanced + +`ecp.New()` returns a parser whose behaviour can be changed: + +```go +e := ecp.New() +e.BuildKey = func(structure, field string, tag reflect.StructTag) string { ... } +e.LookupValue = func(key string) (string, bool) { ... } +e.Advance.SplitChar = "," +e.Advance.SetValue = func(tag reflect.StructTag, field reflect.Value, val string) bool { ... } +``` + +`LookupValue` is what makes it possible to read from something other than +the environment, and `SetValue` takes over the conversion of a field, +returning true when it handled it. diff --git a/ecp.go b/ecp.go index daf03c3..ce228a1 100644 --- a/ecp.go +++ b/ecp.go @@ -5,88 +5,137 @@ package ecp import ( "fmt" "reflect" + "strconv" "strings" ) -type ecp struct { - BuildKey BuildKeyFunc +// ECP is an environment config parser. Create one with New when the +// default behaviour has to be changed, or use the package level Parse, +// List and Get functions to work with the default one. +type ECP struct { + // BuildKey builds the environment key of a field + BuildKey BuildKeyFunc + // LookupValue returns the value of a key and whether it exists LookupValue LookupValueFunc - Advance advanceConfig + Advance AdvanceConfig } -type advanceConfig struct { +// AdvanceConfig holds the optional knobs of an ECP +type AdvanceConfig struct { SplitChar string // split slice SetValue SetValueFunc } -var globalEcp = &ecp{ - BuildKey: buildKeyFromEnv, - LookupValue: lookupValueFromEnv, - Advance: advanceConfig{ - SplitChar: space, - }, -} +var globalEcp = New() // New ecp object -func New() *ecp { - return &ecp{ +func New() *ECP { + return &ECP{ BuildKey: buildKeyFromEnv, LookupValue: lookupValueFromEnv, - Advance: advanceConfig{ + Advance: AdvanceConfig{ SplitChar: space, }, } } -func (e *ecp) Parse(config interface{}, prefix ...string) error { +// Parse the configuration through environments starting with the +// prefix (or not), see the package level Parse for the details +func (e *ECP) Parse(config interface{}, prefix ...string) error { if len(prefix) == 0 { prefix = []string{""} } - _, err := e.rangeOver(roOption{config, true, prefix[0], ""}) + + // catch the classic Parse(config) instead of Parse(&config): without + // a pointer every field is read-only, so Parse would report success + // after filling in exactly nothing + if value := toValue(config); value.IsValid() && + value.Kind() == reflect.Struct && !value.CanSet() { + return fmt.Errorf("config must be a pointer to a struct, got %s", value.Type()) + } + + _, err := e.rangeOver(roOption{target: config, setDef: true, prefix: prefix[0]}) return err } -func (e *ecp) List(config interface{}, prefix ...string) []string { - list := []string{} - +// List all the config environments, see the package level List for +// the details +func (e *ECP) List(config interface{}, prefix ...string) []string { if len(prefix) == 0 { prefix = []string{""} } - parentName := prefix[0] + return e.list(config, prefix[0], make(map[reflect.Type]bool, 1)) +} + +func (e *ECP) list(config interface{}, parentName string, + visiting map[reflect.Type]bool) []string { + + list := []string{} configValue := toValue(config) if !configValue.IsValid() || configValue.Kind() != reflect.Struct { return list } configType := configValue.Type() + + // stop a self referencing type from recursing forever + if visiting[configType] { + return list + } + visiting[configType] = true + defer delete(visiting, configType) + for index := 0; index < configValue.NumField(); index++ { if !configType.Field(index).IsExported() { continue } all := e.getAll(getAllOpt{configType, configValue, index, parentName}) - if all.parent == "-" || all.key == "" { + if all.key == "" { continue } - switch all.value.Kind() { - case reflect.Struct: + switch { + case all.value.Kind() == reflect.Struct: prefix := e.BuildKey(parentName, all.parent, all.tag) - list = append(list, e.List(all.value, prefix)...) - default: - if strings.Contains(all.defVal, " ") { - all.defVal = fmt.Sprintf(`"%s"`, all.defVal) + list = append(list, e.list(all.value, prefix, visiting)...) + + case isSection(all.value): + // an optional section: list the keys of the pointed-to + // struct, a nil pointer still has all of them + prefix := e.BuildKey(parentName, all.parent, all.tag) + section := all.value + if section.IsNil() { + section = reflect.New(all.value.Type().Elem()) } - list = append(list, fmt.Sprintf("%s=%s", all.key, all.defVal)) + list = append(list, e.list(section.Elem(), prefix, visiting)...) + + case !canSetKind(all.value.Kind()): + // maps, arrays, channels... cannot be filled from a string, + // so listing a key for them would be misleading + continue + + default: + list = append(list, fmt.Sprintf("%s=%s", all.key, quoteValue(all.defVal))) } } return list } -// List function will also fill up the value of the environment key -// it the "default" tag has value +// quoteValue quotes a default value that would not survive a round trip +// through a shell or an env file unquoted +func quoteValue(v string) string { + if strings.ContainsAny(v, " \t\r\n\"'\\`$") { + return strconv.Quote(v) + } + return v +} -// List all the config environments +// List all the config environments. +// +// The value of each key is the one from the "default" tag, empty if the +// field has no default. Fields tagged with `env:"-"`, `yaml:"-"` or +// `json:"-"` are skipped. func List(config interface{}, prefix ...string) []string { return globalEcp.List(config, prefix...) } @@ -109,9 +158,14 @@ func List(config interface{}, prefix ...string) []string { // type config struct { // One string `default:"1"` // Two int `default:"2"` -// Three []string `default:"1,2,3"` +// Three []string `default:"1 2 3"` // } // c := &config{} +// +// Slice values are separated by Advance.SplitChar, a space by default. +// +// config must be a pointer to a struct, otherwise Parse returns an error +// instead of silently doing nothing. func Parse(config interface{}, prefix ...string) error { return globalEcp.Parse(config, prefix...) } diff --git a/ecp_test.go b/ecp_test.go index c899bbb..d01527e 100644 --- a/ecp_test.go +++ b/ecp_test.go @@ -124,6 +124,9 @@ func TestList(t *testing.T) { }) t.Run("with get key", func(t *testing.T) { + // reset get key function + defer func() { globalEcp.BuildKey = buildKeyFromEnv }() + globalEcp.BuildKey = func(parentName, structName string, tag reflect.StructTag) (key string) { return strings.ToLower(parentName) + "." + strings.ToLower(structName) } @@ -131,9 +134,6 @@ func TestList(t *testing.T) { for _, key := range list[18:27] { fmt.Printf("%s\n", key) } - - // reset get key function - globalEcp.BuildKey = buildKeyFromEnv }) } @@ -266,8 +266,11 @@ func TestGetKeyLookupValue(t *testing.T) { } return structName } + // both hooks have to be restored, leaving the lookup mock behind + // makes every test running after this one read from it defer func() { globalEcp.BuildKey = buildKeyFromEnv + globalEcp.LookupValue = lookupValueFromEnv }() globalEcp.LookupValue = func(key string) (value string, exist bool) { diff --git a/get.go b/get.go index 5745c78..cfc636f 100644 --- a/get.go +++ b/get.go @@ -5,8 +5,15 @@ import ( "reflect" ) -func (e *ecp) getValue(config interface{}, keyName string) (reflect.Value, error) { - v, err := e.rangeOver(roOption{config, false, "", keyName}) +// getValue looks up a field by the environment key it is bound to. The +// optional prefix must match the one Parse was called with, since that is +// what the key names are built from. +func (e *ECP) getValue(config interface{}, keyName string, prefix ...string) (reflect.Value, error) { + if len(prefix) == 0 { + prefix = []string{""} + } + + v, err := e.rangeOver(roOption{target: config, find: keyName, prefix: prefix[0]}) if err != nil { return reflect.Value{}, err } @@ -21,8 +28,9 @@ func (e *ecp) getValue(config interface{}, keyName string) (reflect.Value, error return v, nil } -func (e *ecp) Get(config interface{}, keyName string) (interface{}, error) { - v, err := e.getValue(config, keyName) +// Get the value of the keyName in that struct +func (e *ECP) Get(config interface{}, keyName string, prefix ...string) (interface{}, error) { + v, err := e.getValue(config, keyName, prefix...) if err != nil { return nil, err } @@ -30,88 +38,82 @@ func (e *ecp) Get(config interface{}, keyName string) (interface{}, error) { return v.Interface(), nil } -func (e *ecp) GetBool(config interface{}, keyName string) (bool, error) { - v, err := e.getValue(config, keyName) +// GetBool returns bool +func (e *ECP) GetBool(config interface{}, keyName string, prefix ...string) (bool, error) { + v, err := e.getValue(config, keyName, prefix...) if err != nil { return false, err } - if vv, ok := v.Interface().(bool); ok { - return vv, nil + if v.Kind() == reflect.Bool { + return v.Bool(), nil } return false, fmt.Errorf("value is not bool, it's %s", v.Kind()) } -func (e *ecp) GetInt64(config interface{}, keyName string) (int64, error) { - v, err := e.getValue(config, keyName) +// GetInt64 returns int64 +func (e *ECP) GetInt64(config interface{}, keyName string, prefix ...string) (int64, error) { + v, err := e.getValue(config, keyName, prefix...) if err != nil { return -1, err } - i := v.Interface() - - if vv, ok := i.(int); ok { - return int64(vv), nil - } else if vv, ok := i.(int8); ok { - return int64(vv), nil - } else if vv, ok := i.(int16); ok { - return int64(vv), nil - } else if vv, ok := i.(int32); ok { - return int64(vv), nil - } else if vv, ok := i.(int64); ok { - return vv, nil + + switch v.Kind() { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + return v.Int(), nil } return -1, fmt.Errorf("value is %s", v.Kind()) } -func (e *ecp) GetString(config interface{}, keyName string) (string, error) { - v, err := e.getValue(config, keyName) +// GetString returns string +func (e *ECP) GetString(config interface{}, keyName string, prefix ...string) (string, error) { + v, err := e.getValue(config, keyName, prefix...) if err != nil { return "", err } - if vv, ok := v.Interface().(string); ok { - return vv, nil + if v.Kind() == reflect.String { + return v.String(), nil } return "", fmt.Errorf("value is not string, it's %s", v.Kind()) } -func (e *ecp) GetFloat64(config interface{}, keyName string) (float64, error) { - v, err := e.getValue(config, keyName) +// GetFloat64 returns float64 +func (e *ECP) GetFloat64(config interface{}, keyName string, prefix ...string) (float64, error) { + v, err := e.getValue(config, keyName, prefix...) if err != nil { return -1, err } - i := v.Interface() - if vv, ok := i.(float32); ok { - return float64(vv), nil - } - if vv, ok := i.(float64); ok { - return vv, nil + + switch v.Kind() { + case reflect.Float32, reflect.Float64: + return v.Float(), nil } return -1, fmt.Errorf("value is %s", v.Kind()) } // Get the value of the keyName in that struct -func Get(config interface{}, keyName string) (interface{}, error) { - return globalEcp.Get(config, keyName) +func Get(config interface{}, keyName string, prefix ...string) (interface{}, error) { + return globalEcp.Get(config, keyName, prefix...) } // GetBool returns bool -func GetBool(config interface{}, keyName string) (bool, error) { - return globalEcp.GetBool(config, keyName) +func GetBool(config interface{}, keyName string, prefix ...string) (bool, error) { + return globalEcp.GetBool(config, keyName, prefix...) } // GetInt64 returns int64 -func GetInt64(config interface{}, keyName string) (int64, error) { - return globalEcp.GetInt64(config, keyName) +func GetInt64(config interface{}, keyName string, prefix ...string) (int64, error) { + return globalEcp.GetInt64(config, keyName, prefix...) } // GetString returns string -func GetString(config interface{}, keyName string) (string, error) { - return globalEcp.GetString(config, keyName) +func GetString(config interface{}, keyName string, prefix ...string) (string, error) { + return globalEcp.GetString(config, keyName, prefix...) } // GetFloat64 returns float64 -func GetFloat64(config interface{}, keyName string) (float64, error) { - return globalEcp.GetFloat64(config, keyName) +func GetFloat64(config interface{}, keyName string, prefix ...string) (float64, error) { + return globalEcp.GetFloat64(config, keyName, prefix...) } diff --git a/parse.go b/parse.go index 4685629..2893d86 100644 --- a/parse.go +++ b/parse.go @@ -3,17 +3,23 @@ package ecp import ( "fmt" "reflect" - "strconv" "strings" - "time" ) -var duration []time.Duration -var durationType = reflect.TypeOf(duration) +// split cuts a value into slice elements. With the default whitespace +// separator, repeated separators are collapsed, so "a b" yields two +// elements instead of three (one of them empty and unparsable). +func (e *ECP) split(v string) []string { + if strings.TrimSpace(e.Advance.SplitChar) == "" { + return strings.Fields(v) + } + return strings.Split(v, e.Advance.SplitChar) +} -// parseSlice support slice of string, int, int8, int16, int32, int64 -// float32, float64, uint, uint8, uint16, uint32, uint64, bool and time.Duration -func (e *ecp) parseSlice(v string, field reflect.Value) error { +// parseSlice supports slices of string, bool, int, int8, int16, int32, +// int64, uint, uint8, uint16, uint32, uint64, float32, float64 and +// time.Duration, including named types built on top of them. +func (e *ECP) parseSlice(v string, field reflect.Value) error { if v == "" { return nil } @@ -27,265 +33,46 @@ func (e *ecp) parseSlice(v string, field reflect.Value) error { // either space nor commas is perfect, but I think space is better // since it's more natural: fmt.Println([]int{1, 2, 3}) = [1 2 3] - stringSlice := strings.Split(v, e.Advance.SplitChar) // split by space - - field.Set(reflect.MakeSlice(field.Type(), len(stringSlice), cap(stringSlice))) - - kind := field.Type().Elem().Kind() - - switch kind { - case reflect.String: - field.Set(reflect.ValueOf(stringSlice)) - - case reflect.Int: - slice := []int{} - for _, s := range stringSlice { - i, err := strconv.ParseInt(s, 10, 64) - if err != nil { - return err - } - slice = append(slice, int(i)) - } - field.Set(reflect.ValueOf(slice)) - - case reflect.Int8: - slice := []int8{} - for _, s := range stringSlice { - i, err := strconv.ParseInt(s, 10, 8) - if err != nil { - return err - } - slice = append(slice, int8(i)) - } - field.Set(reflect.ValueOf(slice)) - - case reflect.Int16: - slice := []int16{} - for _, s := range stringSlice { - i, err := strconv.ParseInt(s, 10, 16) - if err != nil { - return err - } - slice = append(slice, int16(i)) - } - field.Set(reflect.ValueOf(slice)) - - case reflect.Int32: - slice := []int32{} - for _, s := range stringSlice { - i, err := strconv.ParseInt(s, 10, 32) - if err != nil { - return err - } - slice = append(slice, int32(i)) - } - field.Set(reflect.ValueOf(slice)) - - case reflect.Int64: - if field.Type() == durationType { - timeSlice := make([]time.Duration, 0, len(stringSlice)) - for _, s := range stringSlice { - d, err := parseDuration(s) - if err != nil { - return err - } - timeSlice = append(timeSlice, d) - } - field.Set(reflect.ValueOf(timeSlice)) - } else { - slice := []int64{} - for _, s := range stringSlice { - i, err := strconv.ParseInt(s, 10, 64) - if err != nil { - return err - } - slice = append(slice, i) - } - field.Set(reflect.ValueOf(slice)) - } - - case reflect.Float32: - slice := []float32{} - for _, s := range stringSlice { - i, err := strconv.ParseFloat(s, 32) - if err != nil { - return err - } - slice = append(slice, float32(i)) - } - field.Set(reflect.ValueOf(slice)) - - case reflect.Float64: - slice := []float64{} - for _, s := range stringSlice { - i, err := strconv.ParseFloat(s, 64) - if err != nil { - return err - } - slice = append(slice, float64(i)) - } - field.Set(reflect.ValueOf(slice)) - - case reflect.Uint: - slice := []uint{} - for _, s := range stringSlice { - i, err := strconv.ParseUint(s, 10, 64) - if err != nil { - return err - } - slice = append(slice, uint(i)) - } - field.Set(reflect.ValueOf(slice)) - - case reflect.Uint8: - slice := []uint8{} - for _, s := range stringSlice { - i, err := strconv.ParseUint(s, 10, 8) - if err != nil { - return err - } - slice = append(slice, uint8(i)) - } - field.Set(reflect.ValueOf(slice)) - - case reflect.Uint16: - slice := []uint16{} - for _, s := range stringSlice { - i, err := strconv.ParseUint(s, 10, 16) - if err != nil { - return err - } - slice = append(slice, uint16(i)) - } - field.Set(reflect.ValueOf(slice)) - - case reflect.Uint32: - slice := []uint32{} - for _, s := range stringSlice { - i, err := strconv.ParseUint(s, 10, 32) - if err != nil { - return err - } - slice = append(slice, uint32(i)) - } - field.Set(reflect.ValueOf(slice)) + parts := e.split(v) - case reflect.Uint64: - slice := []uint64{} - for _, s := range stringSlice { - i, err := strconv.ParseUint(s, 10, 64) - if err != nil { - return err - } - slice = append(slice, i) + // build the slice through the field's own type so that a named + // element type ([]Level) stays assignable + slice := reflect.MakeSlice(field.Type(), len(parts), len(parts)) + for i, s := range parts { + if err := setValue(slice.Index(i), s); err != nil { + return err } - field.Set(reflect.ValueOf(slice)) - - case reflect.Bool: - slice := []bool{} - for _, s := range stringSlice { - i, err := strconv.ParseBool(strings.ToLower(s)) - if err != nil { - return err - } - slice = append(slice, i) - } - field.Set(reflect.ValueOf(slice)) - - default: - return fmt.Errorf("unsupported slice kind %s", kind) } + field.Set(slice) return nil } -func (e *ecp) parsePointer(typ reflect.Type, value string) (interface{}, error) { - var rValue interface{} - switch typ.Kind() { - case reflect.String: - rValue = &value - - case reflect.Int, reflect.Int8, reflect.Int16, - reflect.Int32, reflect.Int64: - // parse with the pointer's bit size so an out-of-range value - // errors out instead of being silently truncated - vInt, err := strconv.ParseInt(value, 10, typ.Bits()) - if err != nil { - return nil, err - } - switch typ.Kind() { - case reflect.Int: - parsed := int(vInt) - rValue = &parsed - case reflect.Int8: - parsed := int8(vInt) - rValue = &parsed - case reflect.Int16: - parsed := int16(vInt) - rValue = &parsed - case reflect.Int32: - parsed := int32(vInt) - rValue = &parsed - case reflect.Int64: - rValue = &vInt - } - - case reflect.Uint, reflect.Uint8, reflect.Uint16, - reflect.Uint32, reflect.Uint64: - v, err := strconv.ParseUint(value, 10, typ.Bits()) - if err != nil { - return nil, err - } - switch typ.Kind() { - case reflect.Uint: - parsed := uint(v) - rValue = &parsed - case reflect.Uint8: - parsed := uint8(v) - rValue = &parsed - case reflect.Uint16: - parsed := uint16(v) - rValue = &parsed - case reflect.Uint32: - parsed := uint32(v) - rValue = &parsed - case reflect.Uint64: - rValue = &v - } - - case reflect.Bool: - if b, err := strconv.ParseBool(strings.ToLower(value)); err == nil { - rValue = &b - } else { - return nil, err - } - - case reflect.Float32: - v, err := strconv.ParseFloat(value, 32) - if err != nil { - return nil, err +// setPointer fills a pointer field, allocating the pointed-to value. +// +// The value is built with reflect.New from the field's own element type, +// which keeps named pointer types (*time.Duration, *Level, ...) +// assignable and lets *time.Duration accept the same "10s" syntax as +// time.Duration. +func (e *ECP) setPointer(field reflect.Value, v string) error { + elemType := field.Type().Elem() + pointer := reflect.New(elemType) + + if elemType.Kind() == reflect.Slice { + // parseSlice needs the slice value itself, not the pointer to it + if err := e.parseSlice(v, pointer.Elem()); err != nil { + return err } - x := float32(v) - rValue = &x - - case reflect.Float64: - v, err := strconv.ParseFloat(value, 64) + } else { + v, err := expandNumber(elemType, v) if err != nil { - return nil, err + return err } - rValue = &v - - case reflect.Slice: - newValue := reflect.New(typ) - // parseSlice needs the slice value itself, not the pointer to it - if err := e.parseSlice(value, newValue.Elem()); err != nil { - return rValue, err + if err := setValue(pointer.Elem(), v); err != nil { + return err } - return newValue.Interface(), nil - - default: - return rValue, fmt.Errorf("unsupported pointer kind %s", typ.Kind()) } - return rValue, nil + field.Set(pointer) + return nil } diff --git a/range.go b/range.go index c8afc62..532cc27 100644 --- a/range.go +++ b/range.go @@ -8,6 +8,10 @@ import ( "time" ) +// the expansion of parseScientific is only ever fed to ParseInt/ParseUint, +// so anything beyond 20 digits is out of range for every Go integer type +const maxExponent = 20 + func toValue(config interface{}) reflect.Value { value, ok := config.(reflect.Value) if !ok { @@ -20,6 +24,19 @@ func toValue(config interface{}) reflect.Value { return value } +// isSection reports whether a field is a nested config section, that is a +// struct or a pointer to a struct. Sections are walked into even when no +// value of their own is available. +func isSection(field reflect.Value) bool { + switch field.Kind() { + case reflect.Struct: + return true + case reflect.Ptr: + return field.Type().Elem().Kind() == reflect.Struct + } + return false +} + type getAllOpt struct { typ reflect.Type value reflect.Value @@ -31,11 +48,11 @@ type getAllResult struct { value reflect.Value tag reflect.StructTag parent string // struct name - key string // key name + key string // key name, empty means "ignore this field" defVal string // default value } -func (e *ecp) getAll(opts getAllOpt) getAllResult { +func (e *ECP) getAll(opts getAllOpt) getAllResult { field := opts.typ.Field(opts.index) r := getAllResult{ @@ -56,6 +73,12 @@ func (e *ecp) getAll(opts getAllOpt) getAllResult { } } + // a "-" tag means "ignore this field", the same way encoding/json + // reads it; leaving the key empty makes both Parse and List skip it + if r.parent == "-" || field.Tag.Get("env") == "-" { + return r + } + r.key = e.BuildKey(opts.parent, r.parent, r.tag) return r @@ -67,9 +90,13 @@ type roOption struct { setDef bool // set default value prefix string // prefix, usually the parent struct name find string // lookup some key + // struct types currently being walked, so that a self referencing + // type (type Node struct{ Next *Node }) stops instead of recursing + // until the stack blows up + visiting map[reflect.Type]bool } -func (e *ecp) rangeOver(opts roOption) (reflect.Value, error) { +func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { rValue := toValue(opts.target) if !rValue.IsValid() || rValue.Kind() != reflect.Struct { @@ -77,7 +104,17 @@ func (e *ecp) rangeOver(opts roOption) (reflect.Value, error) { } rType := rValue.Type() + if opts.visiting == nil { + opts.visiting = make(map[reflect.Type]bool, 1) + } + opts.visiting[rType] = true + defer delete(opts.visiting, rType) + for index := 0; index < rValue.NumField(); index++ { + if !rType.Field(index).IsExported() { + continue + } + info := e.getAll(getAllOpt{rType, rValue, index, opts.prefix}) field := info.value structName := info.parent @@ -89,12 +126,14 @@ func (e *ecp) rangeOver(opts roOption) (reflect.Value, error) { continue } + section := isSection(field) + if opts.find != "" { if opts.find == keyName { return field, nil } // skip this field - if field.Kind() != reflect.Struct { + if !section { continue } } @@ -105,115 +144,80 @@ func (e *ecp) rangeOver(opts roOption) (reflect.Value, error) { } if !field.CanAddr() || !field.CanSet() { - continue + // a read-only config can still be searched, just not filled + if opts.find == "" || !section { + continue + } } kind := field.Kind() - if v == "" && kind != reflect.Struct { + if v == "" && !section { continue } // set value via self-defined function - if e.Advance.SetValue != nil && e.Advance.SetValue(info.tag, field, v) { + if opts.find == "" && e.Advance.SetValue != nil && + e.Advance.SetValue(info.tag, field, v) { continue } switch kind { - case reflect.String: - if field.String() != "" && !exist { - continue - } - field.SetString(v) - - case reflect.Float32, reflect.Float64: - if field.Float() != 0 && !exist { - continue - } - parsed, err := strconv.ParseFloat(v, field.Type().Bits()) + case reflect.Struct: + prefix := e.BuildKey(opts.prefix, structName, info.tag) + found, err := e.rangeOver(roOption{ + target: field, + setDef: opts.setDef, + prefix: prefix, + find: opts.find, + visiting: opts.visiting, + }) if err != nil { - return field, fmt.Errorf("convert %s error: %s", keyName, err) + return reflect.Value{}, err } - field.SetFloat(parsed) - - case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - if field.Int() != 0 && !exist { - continue + if opts.find != "" && found.IsValid() { + return found, nil } - // only time.Duration (an int64 based type) fields accept - // duration syntax like "10s" or "1d"; parsing a plain int - // field that way would silently turn "10s" into 1e10 - if field.Type() == reflect.TypeOf(time.Duration(0)) { - d, err := parseDuration(v) + + case reflect.Ptr: + if section { + found, err := e.rangeOverPointer(field, structName, info.tag, opts) if err != nil { - return field, fmt.Errorf("convert %s error: %s", keyName, err) + return reflect.Value{}, err + } + if opts.find != "" && found.IsValid() { + return found, nil } - field.SetInt(int64(d)) continue } - v, err := parseScientific(v) - if err != nil { - return field, fmt.Errorf("convert %s error: %s", keyName, err) - } - // parse with the field's bit size so an out-of-range value - // errors out instead of being silently truncated - parsed, err := strconv.ParseInt(v, 10, field.Type().Bits()) - if err != nil { - return field, fmt.Errorf("convert %s error: %s", keyName, err) - } - field.SetInt(parsed) - - case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - if field.Uint() != 0 && !exist { + // only set the default value to a nil pointer, but still + // allow an existing environment value to overwrite a set one + if !field.IsNil() && !exist { continue } - v, err := parseScientific(v) - if err != nil { - return field, fmt.Errorf("convert %s error: %s", keyName, err) + if err := e.setPointer(field, v); err != nil { + return field, fmt.Errorf("convert %s error: %w", keyName, err) } - parsed, err := strconv.ParseUint(v, 10, field.Type().Bits()) - if err != nil { - return field, fmt.Errorf("convert %s error: %s", keyName, err) - } - field.SetUint(parsed) - - case reflect.Bool: - parsed, err := strconv.ParseBool(strings.ToLower(v)) - if err != nil { - return field, fmt.Errorf("convert %s error: %s", keyName, err) - } - field.SetBool(parsed) case reflect.Slice: if !field.IsNil() && !exist { continue } if err := e.parseSlice(v, field); err != nil { - return field, fmt.Errorf("convert %s error: %s", keyName, err) - } - - case reflect.Struct: - prefix := e.BuildKey(opts.prefix, structName, info.tag) - v, err := e.rangeOver(roOption{field, opts.setDef, prefix, opts.find}) - if err != nil { - return reflect.Value{}, err - } - if opts.find != "" && v.IsValid() { - return v, nil + return field, fmt.Errorf("convert %s error: %w", keyName, err) } - case reflect.Ptr: - // only set the default value to a nil pointer, but still - // allow an existing environment value to overwrite a set one - if !field.IsNil() && !exist { + default: + // a value already set by the caller wins over the default, + // but never over an environment value + if !exist && !field.IsZero() { continue } - // get pointer real kind - value, err := e.parsePointer(field.Type().Elem(), v) + value, err := expandNumber(field.Type(), v) if err != nil { - return field, fmt.Errorf("convert %s error: %s", keyName, err) + return field, fmt.Errorf("convert %s error: %w", keyName, err) } - if value != nil { - field.Set(reflect.ValueOf(value)) + if err := setValue(field, value); err != nil { + return field, fmt.Errorf("convert %s error: %w", keyName, err) } } @@ -221,6 +225,47 @@ func (e *ecp) rangeOver(opts roOption) (reflect.Value, error) { return reflect.Value{}, nil } +// rangeOverPointer walks into a pointer to a struct, that is an optional +// config section. A nil section is filled through a temporary value and +// only allocated when something was actually set, so that an untouched +// optional section stays nil. +func (e *ECP) rangeOverPointer(field reflect.Value, structName string, + tag reflect.StructTag, opts roOption) (reflect.Value, error) { + + elemType := field.Type().Elem() + if opts.visiting[elemType] { + // cyclic type, stop here + return reflect.Value{}, nil + } + + target := field + if field.IsNil() { + if opts.find != "" || !field.CanSet() { + // nothing to read from, and nothing to fill + return reflect.Value{}, nil + } + target = reflect.New(elemType) + } + + found, err := e.rangeOver(roOption{ + target: target.Elem(), + setDef: opts.setDef, + prefix: e.BuildKey(opts.prefix, structName, tag), + find: opts.find, + visiting: opts.visiting, + }) + if err != nil { + return reflect.Value{}, err + } + + if field.IsNil() && !target.Elem().IsZero() { + field.Set(target) + } + + return found, nil +} + +// parseScientific rewrites "1e3" and "1,000" into a plain integer literal func parseScientific(v string) (string, error) { v = strings.ReplaceAll(v, ",", "") @@ -239,15 +284,29 @@ func parseScientific(v string) (string, error) { return "", err } // a negative exponent would be silently ignored by the expansion - // loop below (e.g. "1e-3" -> "1"), which is worse than an error + // below (e.g. "1e-3" -> "1"), which is worse than an error if n < 0 { return "", fmt.Errorf("bad number %s", v) } - result := v[:index] - for i := 0; i < n; i++ { - result += "0" + // without an upper bound, "1e1000000" would spend minutes building a + // one megabyte string that cannot fit in any integer type anyway + if n > maxExponent { + return "", fmt.Errorf("number %s out of range", v) + } + + mantissa := v[:index] + // shift the decimal point instead of blindly appending zeros, so that + // "1.5e3" becomes 1500 instead of the unparsable "1.5000" + if dot := strings.IndexByte(mantissa, '.'); dot != -1 { + decimals := len(mantissa) - dot - 1 + if decimals > n { + return "", fmt.Errorf("number %s is not an integer", v) + } + mantissa = mantissa[:dot] + mantissa[dot+1:] + n -= decimals } - return result, nil + + return mantissa + strings.Repeat("0", n), nil } // parseDuration wrapper func of time.ParseDuration to support `Xd` = `X*24h` @@ -259,7 +318,11 @@ func parseDuration(v string) (time.Duration, error) { if err != nil { return 0, err } - v = fmt.Sprintf("%dh", dayN*24) + hours := int64(dayN) * 24 + if dayN != 0 && hours/int64(dayN) != 24 { + return 0, fmt.Errorf("duration %s out of range", v) + } + v = fmt.Sprintf("%dh", hours) } d, err := time.ParseDuration(v) if err != nil { diff --git a/regression_test.go b/regression_test.go new file mode 100644 index 0000000..573c6f0 --- /dev/null +++ b/regression_test.go @@ -0,0 +1,452 @@ +package ecp + +import ( + "errors" + "os" + "strconv" + "strings" + "testing" + "time" +) + +// regression tests for the bugs fixed in this round + +type ( + level int + name string + duration time.Duration +) + +// named element types used to panic with +// "reflect.Set: value of type []int is not assignable to type []ecp.level" +func TestNamedTypeSlice(t *testing.T) { + c := &struct { + Levels []level `default:"1 2 3"` + Names []name `default:"a b"` + }{} + if err := Parse(c); err != nil { + t.Fatal(err) + } + if len(c.Levels) != 3 || c.Levels[2] != 3 { + t.Errorf("parse []level failed: %v", c.Levels) + } + if len(c.Names) != 2 || c.Names[1] != "b" { + t.Errorf("parse []name failed: %v", c.Names) + } +} + +// pointers to named types used to panic with +// "reflect.Set: value of type *int64 is not assignable to type *time.Duration" +func TestNamedTypePointer(t *testing.T) { + c := &struct { + Dur *time.Duration `env:"REG_DUR"` + Lvl *level `default:"7"` + }{} + withEnv(t, "REG_DUR", "10s") + if err := Parse(c); err != nil { + t.Fatal(err) + } + // *time.Duration accepts the same syntax as time.Duration + if c.Dur == nil || *c.Dur != 10*time.Second { + t.Errorf("parse *time.Duration failed: %v", c.Dur) + } + if c.Lvl == nil || *c.Lvl != 7 { + t.Errorf("parse *level failed: %v", c.Lvl) + } +} + +// "1e1000000" used to spend minutes building a one megabyte string +func TestScientificOutOfRange(t *testing.T) { + done := make(chan error, 1) + go func() { + c := &struct { + N int64 `env:"REG_HUGE"` + }{} + done <- Parse(c) + }() + + os.Setenv("REG_HUGE", "1e1000000") + defer os.Unsetenv("REG_HUGE") + + select { + case err := <-done: + if err == nil { + t.Error("expected an out of range error") + } + case <-time.After(5 * time.Second): + t.Fatal("parsing a huge exponent took too long") + } +} + +// "1.5e3" is 1500, it used to expand to the unparsable "1.5000" +func TestScientificDecimalMantissa(t *testing.T) { + r, err := parseScientific("1.5e3") + if err != nil { + t.Fatal(err) + } + if r != "1500" { + t.Errorf("parse 1.5e3 error, result=%s", r) + } + if _, err := parseScientific("1.5e0"); err == nil { + t.Error("1.5e0 is not an integer, it should be an error") + } +} + +// a "-" tag means "ignore this field", Parse used to honour it in List +// only and still filled the field in +func TestIgnoreTag(t *testing.T) { + c := &struct { + Yaml string `yaml:"-" default:"nope"` + JSON string `json:"-" default:"nope"` + Env string `env:"-" default:"nope"` + }{} + // the bogus keys the ignored fields used to be bound to + withEnv(t, "-", "leaked") + + if err := Parse(c); err != nil { + t.Fatal(err) + } + if c.Yaml != "" || c.JSON != "" || c.Env != "" { + t.Errorf("ignored fields were parsed: %+v", c) + } + if list := List(*c); len(list) != 0 { + t.Errorf("ignored fields were listed: %v", list) + } +} + +// Parse(config) instead of Parse(&config) used to report success after +// filling in exactly nothing +func TestParseRequiresPointer(t *testing.T) { + type conf struct { + S string `default:"x"` + } + if err := Parse(conf{}); err == nil { + t.Error("expected an error when config is not a pointer") + } + c := conf{} + if err := Parse(&c); err != nil || c.S != "x" { + t.Errorf("Parse(&c) failed: %v %+v", err, c) + } +} + +// a *struct is a config section, its fields used to be skipped entirely +func TestPointerSection(t *testing.T) { + type sub struct { + Value string `default:"hey"` + Port int `default:"80"` + } + type conf struct { + P *sub `yaml:"p"` + Empty *sub `yaml:"empty"` + } + + t.Run("nil section gets filled", func(t *testing.T) { + withEnv(t, "P_VALUE", "yoo") + c := &conf{} + if err := Parse(c); err != nil { + t.Fatal(err) + } + if c.P == nil { + t.Fatal("nil section was not filled") + } + if c.P.Value != "yoo" || c.P.Port != 80 { + t.Errorf("section not parsed: %+v", *c.P) + } + }) + + t.Run("existing section is walked into", func(t *testing.T) { + withEnv(t, "P_VALUE", "yoo") + c := &conf{P: &sub{Port: 8080}} + if err := Parse(c); err != nil { + t.Fatal(err) + } + if c.P.Value != "yoo" || c.P.Port != 8080 { + t.Errorf("section not parsed: %+v", *c.P) + } + }) + + t.Run("listed with the section prefix", func(t *testing.T) { + list := strings.Join(List(conf{}), " ") + for _, want := range []string{"P_VALUE=hey", "P_PORT=80", "EMPTY_VALUE=hey"} { + if !strings.Contains(list, want) { + t.Errorf("missing %s in %s", want, list) + } + } + }) +} + +// a self referencing type used to be impossible to describe, walking into +// pointer sections must not recurse until the stack blows up +func TestCyclicType(t *testing.T) { + type node struct { + Name string `default:"n"` + Next *node + } + + done := make(chan struct{}) + go func() { + defer close(done) + c := &node{} + if err := Parse(c); err != nil { + t.Error(err) + } + if c.Name != "n" { + t.Errorf("name not parsed: %+v", c) + } + if c.Next != nil { + t.Errorf("cyclic section should stay nil, got %+v", c.Next) + } + _ = List(node{}) + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("cyclic type did not terminate") + } +} + +// every other kind keeps a value set by the caller, bool used to be reset +// to false by a `default:"false"` tag +func TestBoolDefaultKeepsCallerValue(t *testing.T) { + c := &struct { + Feature bool `env:"REG_FEATURE" default:"false"` + }{Feature: true} + if err := Parse(c); err != nil { + t.Fatal(err) + } + if !c.Feature { + t.Error("default:\"false\" overwrote the value set by the caller") + } + + // an environment value still wins + withEnv(t, "REG_FEATURE", "false") + if err := Parse(c); err != nil { + t.Fatal(err) + } + if c.Feature { + t.Error("the environment value should have won") + } +} + +// kinds that cannot be filled from a string used to be ignored without a +// word, now they are reported and never listed +func TestUnsupportedKinds(t *testing.T) { + c := &struct { + M map[string]string `env:"REG_MAP"` + }{} + withEnv(t, "REG_MAP", "a=1") + if err := Parse(c); err == nil { + t.Error("expected an error for a map field with a value") + } + + arr := &struct { + A [3]int `env:"REG_ARR"` + }{} + withEnv(t, "REG_ARR", "1 2 3") + if err := Parse(arr); err == nil { + t.Error("expected an error for an array field with a value") + } + + if list := List(*c); len(list) != 0 { + t.Errorf("unsupported kinds should not be listed: %v", list) + } +} + +// "a b" used to be split into three elements, the empty one making every +// numeric slice fail to parse +func TestSliceRepeatedSeparators(t *testing.T) { + c := &struct { + S []string `env:"REG_S"` + I []int `env:"REG_I"` + }{} + withEnv(t, "REG_S", " a b ") + withEnv(t, "REG_I", "1 2") + if err := Parse(c); err != nil { + t.Fatal(err) + } + if len(c.S) != 2 || c.S[1] != "b" { + t.Errorf("parse []string failed: %q", c.S) + } + if len(c.I) != 2 || c.I[1] != 2 { + t.Errorf("parse []int failed: %v", c.I) + } +} + +// a default value with a quote in it used to produce a broken listing +func TestListQuoting(t *testing.T) { + list := List(struct { + A string `default:"a \"b\" c"` + B string `default:"plain"` + }{}) + want := []string{`A="a \"b\" c"`, "B=plain"} + for i, w := range want { + if list[i] != w { + t.Errorf("got %s, want %s", list[i], w) + } + } +} + +// Get had no way to reach a config parsed with a prefix +func TestGetWithPrefix(t *testing.T) { + type conf struct { + Port int `default:"8080"` + Sub struct { + Name string `default:"x"` + } + } + c := &conf{} + if err := Parse(c, "APP"); err != nil { + t.Fatal(err) + } + + if v, err := GetInt64(c, "APP_PORT", "APP"); err != nil || v != 8080 { + t.Errorf("Get with prefix failed: %v %v", v, err) + } + if v, err := GetString(c, "APP_SUB_NAME", "APP"); err != nil || v != "x" { + t.Errorf("Get with prefix failed: %q %v", v, err) + } + // and a read-only config can be searched too + if v, err := GetInt64(*c, "PORT"); err != nil || v != 8080 { + t.Errorf("Get on a value config failed: %v %v", v, err) + } +} + +// named types used to fall through the type assertions of the Get helpers +func TestGetNamedType(t *testing.T) { + c := &struct { + Lvl level `default:"5"` + Name name `default:"hey"` + }{} + if err := Parse(c); err != nil { + t.Fatal(err) + } + if v, err := GetInt64(c, "LVL"); err != nil || v != 5 { + t.Errorf("GetInt64 on a named int failed: %v %v", v, err) + } + if v, err := GetString(c, "NAME"); err != nil || v != "hey" { + t.Errorf("GetString on a named string failed: %q %v", v, err) + } +} + +// parse errors are wrapped, so the caller can inspect them +func TestErrorUnwrap(t *testing.T) { + c := &struct { + N int `env:"REG_N"` + }{} + withEnv(t, "REG_N", "not-a-number") + err := Parse(c) + if err == nil { + t.Fatal("expected an error") + } + var numErr *strconv.NumError + if !errors.As(err, &numErr) { + t.Errorf("error is not wrapped: %v", err) + } +} + +// the Get helpers report a clear error when the field has another type +func TestGetTypeMismatch(t *testing.T) { + c := &struct { + S string `default:"x"` + N int `default:"1"` + F float64 `default:"1.5"` + B bool `default:"true"` + }{} + if err := Parse(c); err != nil { + t.Fatal(err) + } + if _, err := GetBool(c, "S"); err == nil { + t.Error("expected an error from GetBool on a string") + } + if _, err := GetInt64(c, "S"); err == nil { + t.Error("expected an error from GetInt64 on a string") + } + if _, err := GetString(c, "N"); err == nil { + t.Error("expected an error from GetString on an int") + } + if _, err := GetFloat64(c, "B"); err == nil { + t.Error("expected an error from GetFloat64 on a bool") + } + if v, err := GetFloat64(c, "F"); err != nil || v != 1.5 { + t.Errorf("GetFloat64 failed: %v %v", v, err) + } + if v, err := GetBool(c, "B"); err != nil || !v { + t.Errorf("GetBool failed: %v %v", v, err) + } + for _, get := range []func() error{ + func() error { _, err := Get(c, "NOPE"); return err }, + func() error { _, err := GetBool(c, "NOPE"); return err }, + func() error { _, err := GetInt64(c, "NOPE"); return err }, + func() error { _, err := GetString(c, "NOPE"); return err }, + func() error { _, err := GetFloat64(c, "NOPE"); return err }, + } { + if err := get(); err == nil { + t.Error("expected a not found error") + } + } +} + +// a custom separator keeps its empty elements, only the default +// whitespace one collapses them +func TestCustomSplitChar(t *testing.T) { + e := New() + e.Advance.SplitChar = "," + c := &struct { + S []string `default:"a,b,,c"` + }{} + if err := e.Parse(c); err != nil { + t.Fatal(err) + } + if len(c.S) != 4 || c.S[3] != "c" { + t.Errorf("custom split char failed: %q", c.S) + } +} + +// a pointer to a slice is filled through the same path as a slice +func TestPointerToSliceErrors(t *testing.T) { + c := &struct { + P *[]int `env:"REG_PSL"` + }{} + withEnv(t, "REG_PSL", "1 nope") + if err := Parse(c); err == nil { + t.Error("expected an error for a bad element in *[]int") + } +} + +// the cycle guard must only stop a type from containing itself, two +// sections of the same type next to each other are perfectly fine +func TestSameTypeSections(t *testing.T) { + type common struct { + Host string `default:"localhost"` + } + type conf struct { + Value common `yaml:"value"` + A *common `yaml:"a"` + B *common `yaml:"b"` + } + + withEnv(t, "A_HOST", "a.local") + withEnv(t, "B_HOST", "b.local") + + c := &conf{} + if err := Parse(c); err != nil { + t.Fatal(err) + } + if c.Value.Host != "localhost" { + t.Errorf("value section: %+v", c.Value) + } + if c.A == nil || c.A.Host != "a.local" { + t.Errorf("section A: %+v", c.A) + } + if c.B == nil || c.B.Host != "b.local" { + t.Errorf("section B: %+v", c.B) + } + + list := strings.Join(List(conf{}), " ") + for _, want := range []string{"VALUE_HOST=localhost", "A_HOST=localhost", "B_HOST=localhost"} { + if !strings.Contains(list, want) { + t.Errorf("missing %s in %s", want, list) + } + } +} diff --git a/value.go b/value.go new file mode 100644 index 0000000..72fd182 --- /dev/null +++ b/value.go @@ -0,0 +1,104 @@ +package ecp + +import ( + "fmt" + "reflect" + "strconv" + "strings" + "time" +) + +var durationType = reflect.TypeOf(time.Duration(0)) + +// setValue sets a single scalar value from its string form. +// +// The value is always assigned through the field's own reflect.Value, so +// named types (type Level int, type Name string, time.Duration, ...) are +// handled like their underlying kind instead of panicking on an +// unassignable concrete type such as []int -> []Level. +func setValue(field reflect.Value, v string) error { + switch field.Kind() { + case reflect.String: + field.SetString(v) + + case reflect.Bool: + b, err := strconv.ParseBool(strings.ToLower(v)) + if err != nil { + return err + } + field.SetBool(b) + + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + // only time.Duration (an int64 based type) accepts duration syntax + // like "10s" or "1d"; parsing a plain int field that way would + // silently turn "10s" into 1e10 + if field.Type() == durationType { + d, err := parseDuration(v) + if err != nil { + return err + } + field.SetInt(int64(d)) + return nil + } + // parse with the field's bit size so an out-of-range value errors + // out instead of being silently truncated + n, err := strconv.ParseInt(v, 10, field.Type().Bits()) + if err != nil { + return err + } + field.SetInt(n) + + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + n, err := strconv.ParseUint(v, 10, field.Type().Bits()) + if err != nil { + return err + } + field.SetUint(n) + + case reflect.Float32, reflect.Float64: + f, err := strconv.ParseFloat(v, field.Type().Bits()) + if err != nil { + return err + } + field.SetFloat(f) + + default: + return fmt.Errorf("unsupported kind %s", field.Kind()) + } + + return nil +} + +// expandNumber rewrites the scientific and thousand-separator notation +// ("1e3", "1,000") into a plain integer literal. Duration fields keep +// their own syntax, and non integer kinds are returned untouched. +// +// It is deliberately not applied to slice elements: there, "1,2" is much +// more likely to be a wrong separator than the number 12. +func expandNumber(typ reflect.Type, v string) (string, error) { + switch typ.Kind() { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + if typ == durationType { + return v, nil + } + return parseScientific(v) + } + return v, nil +} + +// canSetKind reports whether a field of this kind can be filled from a +// string value at all. Unsupported kinds (map, array, chan, ...) are +// reported so that they can be skipped when listing and rejected with a +// clear error when a value is actually provided for them. +func canSetKind(kind reflect.Kind) bool { + switch kind { + case reflect.String, reflect.Bool, + reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, + reflect.Float32, reflect.Float64, + reflect.Slice, reflect.Ptr, reflect.Struct: + return true + } + return false +}