From 4c8f8dda500cba4081057c6a5bdf5eee4abad54a Mon Sep 17 00:00:00 2001 From: wrfly Date: Thu, 20 Aug 2026 00:18:15 +0800 Subject: [PATCH] fix: honour a chosen separator, allocate explicitly-zero sections Three bugs found while auditing v0.3.0. A separator the caller chose was silently replaced. v0.3.0 collapsed repeated separators by routing the value through strings.Fields whenever the separator looked like whitespace, so SplitChar = "\t" split on spaces too: "a b\tc d" became four elements instead of two. v0.2.5 got this right. Only the default separator collapses repeats now, and it does so by dropping empty elements rather than re-splitting, which also stops a tab inside an element from being treated as a separator. An optional section was allocated based on the result being non-zero, which cannot tell "nothing was said about this section" from "every value in it is zero". PORT=0 left the section nil, and List advertised keys that Get could not read back. The walk now reports whether a field was actually assigned, from the environment or from a default tag, and the section is allocated on that. A value that merely contains an "e" was blamed on the fragment after it: PORT=not-a-number reported `strconv.Atoi: parsing "r"` and PORT=hello reported `parsing "llo"`. A non-numeric exponent means the value is not scientific notation at all, so it is handed back unchanged and reported whole. This one predates v0.3.0. Also documents that the scientific and thousand-separator notation is deliberately not applied to slice elements, where "1,2" is far more likely to be the wrong separator than the number 12. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 12 +++++-- parse.go | 30 ++++++++++++---- range.go | 38 +++++++++++++++++--- regression_test.go | 90 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index b9e3287..ddd643a 100644 --- a/README.md +++ b/README.md @@ -118,15 +118,21 @@ 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 = ","` + `e := ecp.New(); e.Advance.SplitChar = ","`. The default separator + collapses repeats, so `a b` is two elements; a separator you choose is + taken literally, empty elements and all - **durations** accept everything `time.ParseDuration` does, plus `Xd` for X days: `10s`, `5m`, `6d` -- **integers** also accept `1e3` and `1,000` notation +- **integers** also accept `1e3` and `1,000` notation. Slice elements do + not: there `1,2` is far more likely to be the wrong separator than the + number 12, so it is reported instead of quietly parsed - **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 + a plain struct and only allocated when one of their fields is actually + set, whether from the environment or from a `default` tag. A section + nothing was said about stays nil An environment variable set to an empty value is treated as unset, so a field keeps its default. diff --git a/parse.go b/parse.go index 2893d86..24619cd 100644 --- a/parse.go +++ b/parse.go @@ -6,14 +6,32 @@ import ( "strings" ) -// 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). +// split cuts a value into slice elements. +// +// Only the default separator collapses repeats, so "a b" yields two +// elements instead of three (one of them empty and unparsable). Any +// separator the caller chose is taken literally, including a tab or a +// newline, which a "is it whitespace" test used to swallow. func (e *ECP) split(v string) []string { - if strings.TrimSpace(e.Advance.SplitChar) == "" { - return strings.Fields(v) + sep := e.Advance.SplitChar + if sep == "" { + // an empty separator would make strings.Split cut between every + // rune, which is never what the caller meant + sep = space + } + + parts := strings.Split(v, sep) + if sep != space { + return parts + } + + collapsed := parts[:0] + for _, p := range parts { + if p != "" { + collapsed = append(collapsed, p) + } } - return strings.Split(v, e.Advance.SplitChar) + return collapsed } // parseSlice supports slices of string, bool, int, int8, int16, int32, diff --git a/range.go b/range.go index 532cc27..72c0067 100644 --- a/range.go +++ b/range.go @@ -94,6 +94,19 @@ type roOption struct { // type (type Node struct{ Next *Node }) stops instead of recursing // until the stack blows up visiting map[reflect.Type]bool + // filled reports whether any field was actually assigned. An + // optional section needs to know that, since a section filled + // entirely with zero values (PORT=0) is still a section that was + // asked for, and testing the result for zero cannot tell the two + // apart. + filled *bool +} + +// markFilled records that a field was assigned during this walk +func (o roOption) markFilled() { + if o.filled != nil { + *o.filled = true + } } func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { @@ -158,6 +171,7 @@ func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { // set value via self-defined function if opts.find == "" && e.Advance.SetValue != nil && e.Advance.SetValue(info.tag, field, v) { + opts.markFilled() continue } @@ -170,6 +184,7 @@ func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { prefix: prefix, find: opts.find, visiting: opts.visiting, + filled: opts.filled, }) if err != nil { return reflect.Value{}, err @@ -197,6 +212,7 @@ func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { if err := e.setPointer(field, v); err != nil { return field, fmt.Errorf("convert %s error: %w", keyName, err) } + opts.markFilled() case reflect.Slice: if !field.IsNil() && !exist { @@ -205,6 +221,7 @@ func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { if err := e.parseSlice(v, field); err != nil { return field, fmt.Errorf("convert %s error: %w", keyName, err) } + opts.markFilled() default: // a value already set by the caller wins over the default, @@ -219,6 +236,7 @@ func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { if err := setValue(field, value); err != nil { return field, fmt.Errorf("convert %s error: %w", keyName, err) } + opts.markFilled() } } @@ -227,8 +245,9 @@ func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { // 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. +// only allocated when one of its fields was actually assigned, so that an +// untouched optional section stays nil while a section explicitly asked +// for is allocated even when every value in it is zero. func (e *ECP) rangeOverPointer(field reflect.Value, structName string, tag reflect.StructTag, opts roOption) (reflect.Value, error) { @@ -247,20 +266,27 @@ func (e *ECP) rangeOverPointer(field reflect.Value, structName string, target = reflect.New(elemType) } + filled := false found, err := e.rangeOver(roOption{ target: target.Elem(), setDef: opts.setDef, prefix: e.BuildKey(opts.prefix, structName, tag), find: opts.find, visiting: opts.visiting, + filled: &filled, }) if err != nil { return reflect.Value{}, err } - if field.IsNil() && !target.Elem().IsZero() { + if !filled { + return found, nil + } + if field.IsNil() { field.Set(target) } + // an allocated or updated section fills the struct holding it + opts.markFilled() return found, nil } @@ -281,7 +307,11 @@ func parseScientific(v string) (string, error) { } n, err := strconv.Atoi(v[index+1:]) if err != nil { - return "", err + // not scientific notation at all, just a value that happens to + // contain an "e". Handing it back unchanged lets the caller + // report it as a whole ("hello"), instead of blaming the + // fragment after the e ("llo"). + return v, nil } // a negative exponent would be silently ignored by the expansion // below (e.g. "1e-3" -> "1"), which is worse than an error diff --git a/regression_test.go b/regression_test.go index 573c6f0..2255064 100644 --- a/regression_test.go +++ b/regression_test.go @@ -450,3 +450,93 @@ func TestSameTypeSections(t *testing.T) { } } } + +// a separator the caller chose is taken literally; only the default one +// collapses repeats. A "is the separator whitespace" test used to route +// a deliberate tab through strings.Fields and split on spaces too. +func TestCustomWhitespaceSplitChar(t *testing.T) { + for _, tc := range []struct { + sep string + in string + want []string + }{ + {"\t", "a b\tc d", []string{"a b", "c d"}}, + {"\n", "a b\nc d", []string{"a b", "c d"}}, + {",", "a b,c d", []string{"a b", "c d"}}, + {" ", " a b ", []string{"a", "b"}}, // the default still collapses + {"", "a b", []string{"a", "b"}}, // unset falls back to the default + } { + e := New() + e.Advance.SplitChar = tc.sep + c := &struct { + S []string `env:"SPLIT_S"` + }{} + os.Setenv("SPLIT_S", tc.in) + err := e.Parse(c) + os.Unsetenv("SPLIT_S") + if err != nil { + t.Errorf("sep %q: %v", tc.sep, err) + continue + } + if strings.Join(c.S, "|") != strings.Join(tc.want, "|") { + t.Errorf("sep %q: got %q, want %q", tc.sep, c.S, tc.want) + } + } +} + +// a section is allocated when one of its fields was assigned, even if +// every value in it is zero. Testing the result for zero instead used to +// leave the section nil for an explicit PORT=0. +func TestPointerSectionExplicitZero(t *testing.T) { + type sub struct { + Port int + Name string + } + type conf struct { + P *sub `yaml:"p"` + Empty *sub `yaml:"empty"` + } + + withEnv(t, "P_PORT", "0") + c := &conf{} + if err := Parse(c); err != nil { + t.Fatal(err) + } + if c.P == nil { + t.Error("a section with an explicit value must be allocated") + } else if c.P.Port != 0 { + t.Errorf("port: %d", c.P.Port) + } + // and a section nothing was said about stays nil + if c.Empty != nil { + t.Errorf("untouched section should stay nil, got %+v", c.Empty) + } + + // every key List advertises has to be readable back after Parse + for _, item := range List(conf{}) { + key := strings.Split(item, "=")[0] + if _, err := Get(c, key); err != nil && strings.HasPrefix(key, "P_") { + t.Errorf("listed key %s is not gettable: %v", key, err) + } + } +} + +// a value that merely contains an "e" is reported as a whole, it used to +// be blamed on the fragment after the e +func TestNonNumericValueErrorMessage(t *testing.T) { + for _, in := range []string{"not-a-number", "hello"} { + c := &struct { + N int `env:"MSG_N"` + }{} + os.Setenv("MSG_N", in) + err := Parse(c) + os.Unsetenv("MSG_N") + if err == nil { + t.Errorf("%q should not parse", in) + continue + } + if !strings.Contains(err.Error(), in) { + t.Errorf("error for %q does not mention it: %v", in, err) + } + } +}