From b97c08148542ab894662f5a634a26c80eef460ce Mon Sep 17 00:00:00 2001 From: Joe Chen Date: Wed, 11 Nov 2026 05:11:52 +0000 Subject: [PATCH 1/2] Make Section.Key stop adding missing keys Section.Key returns a key that reads as empty for a name the section doesn't have, and SetValue, SetComment, AddShadow or AddNestedValue adds it. Until then, reads and changes through it follow any key of that name the section gains, so two such keys for one name stay in step. Reading a missing key no longer writes to the file, so HasKey, Keys and Write don't see it, a raw section read for a key stays writable, and concurrent reads are safe with Locking(false). --- docs/migrate-v1-to-v2.md | 4 +++ key.go | 59 ++++++++++++++++++++++++++------ section.go | 21 ++++++++---- section_test.go | 72 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 138 insertions(+), 18 deletions(-) diff --git a/docs/migrate-v1-to-v2.md b/docs/migrate-v1-to-v2.md index ae0eb30..5592cef 100644 --- a/docs/migrate-v1-to-v2.md +++ b/docs/migrate-v1-to-v2.md @@ -21,6 +21,10 @@ port := sec.Key("PORT").MustInt(3000) sec.Key("PORT").SetValue(strconv.Itoa(port)) ``` +### `Section.Key` no longer adds missing keys + +In v1, `sec.Key("PORT")` added an empty `PORT` key when the section had none, so `HasKey`, `Keys` and `Write` saw it after a mere read. In v2 it returns a key that reads as empty, and only `SetValue`, `SetComment`, `AddShadow` or `AddNestedValue` adds it. `sec.Key(name).SetValue(v)` works as before. Reading a missing key no longer writes to the file, so concurrent reads are safe with `Locking(false)`. + ### `Reload` and `Append` - `Reload` replaces the contents with what the sources hold now. v1 merged them in, so removed keys stayed and shadow values piled up. `*Section` and `*Key` values you hold see the new values. On error, nothing changes. diff --git a/key.go b/key.go index ea96a7c..f768961 100644 --- a/key.go +++ b/key.go @@ -23,6 +23,9 @@ type Key struct { shadows []string isAutoIncrement bool isBooleanType bool + // missing is set on a key Section.Key returned for a name its section + // doesn't have. The key isn't in the section until a change adds it. + missing bool nestedValues []string } @@ -36,6 +39,30 @@ func newKey(s *Section, name, val string) *Key { } } +// current returns the key k reads and changes. That is k, unless k is +// missing and its section has since gained a key of its name. The caller +// holds the lock. +func (k *Key) current() *Key { + if k.missing { + if key := k.s.byName[k.name]; key != nil { + return key + } + } + return k +} + +// attach adds k to its section if it's missing, and returns the key to +// change. The caller holds the write lock. +func (k *Key) attach() *Key { + c := k.current() + if c == k && k.missing { + k.missing = false + k.s.keys = append(k.s.keys, k) + k.s.byName[k.name] = k + } + return c +} + func (k *Key) addShadow(val string) error { if k.isAutoIncrement || k.isBooleanType { return errors.New("cannot add shadow to auto-increment or boolean key") @@ -61,7 +88,7 @@ func (k *Key) AddShadow(val string) error { } k.s.f.maybeLock() defer k.s.f.maybeUnlock() - return k.addShadow(val) + return k.attach().addShadow(val) } func (k *Key) addNestedValue(val string) error { @@ -80,7 +107,7 @@ func (k *Key) AddNestedValue(val string) error { } k.s.f.maybeLock() defer k.s.f.maybeUnlock() - return k.addNestedValue(val) + return k.attach().addNestedValue(val) } // ValueMapper represents a mapping function for values, e.g., os.ExpandEnv @@ -96,7 +123,7 @@ func (k *Key) Name() string { func (k *Key) Comment() string { k.s.f.maybeRLock() defer k.s.f.maybeRUnlock() - return k.comment + return k.current().comment } // SetComment sets the comment lines written before the key. Lines without a @@ -104,19 +131,27 @@ func (k *Key) Comment() string { func (k *Key) SetComment(comment string) { k.s.f.maybeLock() defer k.s.f.maybeUnlock() - k.comment = comment + k.attach().comment = comment } // Value returns raw value of key for performance purpose. func (k *Key) Value() string { + _, v := k.currentValue() + return v +} + +// currentValue returns the current key of k and its raw value. +func (k *Key) currentValue() (*Key, string) { f := k.s.f if !f.locking { - return k.value + c := k.current() + return c, c.value } f.mu.RLock() - v := k.value + c := k.current() + v := c.value f.mu.RUnlock() - return v + return c, v } // Values returns raw values of key and its shadows if any. Empty @@ -125,6 +160,7 @@ func (k *Key) Value() string { func (k *Key) Values() []string { k.s.f.maybeRLock() defer k.s.f.maybeRUnlock() + k = k.current() keepEmpty := k.s.f.options.PreserveEmptyShadowValues vals := make([]string, 0, len(k.shadows)+1) if keepEmpty || k.value != "" { @@ -143,7 +179,7 @@ func (k *Key) Values() []string { func (k *Key) IsBoolean() bool { k.s.f.maybeRLock() defer k.s.f.maybeRUnlock() - return k.isBooleanType + return k.current().isBooleanType } // NestedValues returns nested values stored in the key. @@ -151,7 +187,7 @@ func (k *Key) IsBoolean() bool { func (k *Key) NestedValues() []string { k.s.f.maybeRLock() defer k.s.f.maybeRUnlock() - return k.nestedValues + return k.current().nestedValues } // expandValue takes a raw value and transforms to its final string. @@ -215,7 +251,8 @@ func (k *Key) expand(val string, depth int, e *expansion) string { // String returns string representation of value. func (k *Key) String() string { - return k.expandValue(k.Value()) + c, v := k.currentValue() + return c.expandValue(v) } // Validate accepts a validate function which can @@ -504,7 +541,7 @@ func (k *Key) StringsWithShadows(delim string) []string { func (k *Key) SetValue(v string) { k.s.f.maybeLock() defer k.s.f.maybeUnlock() - k.value = v + k.attach().value = v } // Scalar is a type a value can be parsed as, as struct mapping parses diff --git a/section.go b/section.go index 3ed2c37..8461f53 100644 --- a/section.go +++ b/section.go @@ -171,16 +171,23 @@ func (s *Section) HasValue(value string) bool { return false } -// Key assumes named Key exists in section and returns a zero-value when not. +// Key returns the key with the given name, looking in parent sections too +// as GetKey does. When there is none, it returns a key that reads as empty +// without adding it to the section. SetValue, SetComment, AddShadow or +// AddNestedValue adds it. func (s *Section) Key(name string) *Key { - key, err := s.GetKey(name) - if err != nil { - if key, err = s.NewKey(name, ""); err != nil { - // An empty name can't be added, so return a key that isn't. - return newKey(s, name, "") - } + if key, err := s.GetKey(name); err == nil { + return key + } + key := newKey(s, name, "") + if name == "" { + // An empty name can't be added. return key } + if s.f.options.Insensitive || s.f.options.InsensitiveKeys { + key.name = strings.ToLower(name) + } + key.missing = true return key } diff --git a/section_test.go b/section_test.go index 89b2f53..97f717a 100644 --- a/section_test.go +++ b/section_test.go @@ -1,6 +1,7 @@ package ini import ( + "bytes" "testing" "github.com/stretchr/testify/assert" @@ -187,6 +188,77 @@ func TestSection_Key(t *testing.T) { assert.Equal(t, "18", k.Value()) }) }) + + t.Run("reading a missing key doesn't add it", func(t *testing.T) { + const data = "[app]\nname = demo\n\n[raw]\nbody\n" + f, err := Load([]byte(data), RawSections("raw")) + require.NoError(t, err) + app := f.Section("app") + + k := app.Key("port") + assert.Equal(t, "port", k.Name()) + assert.Equal(t, 8080, k.MustInt(8080)) + assert.Empty(t, k.String()) + assert.Empty(t, k.Values()) + assert.Empty(t, k.Comment()) + assert.Empty(t, k.NestedValues()) + assert.False(t, k.IsBoolean()) + _ = f.Section("raw").Key("x").String() + + assert.False(t, app.HasKey("port")) + assert.Equal(t, []string{"name"}, app.KeyNames()) + var buf bytes.Buffer + _, err = f.WriteTo(&buf) + require.NoError(t, err, "a raw section read for a key stays writable") + assert.Equal(t, data, buf.String()) + }) + + t.Run("changing a missing key adds it", func(t *testing.T) { + f, err := Load([]byte("[s]\n"), AllowShadows(true), AllowNestedValues(true)) + require.NoError(t, err) + sec := f.Section("s") + + sec.Key("a").SetValue("1") + sec.Key("b").SetComment("note") + require.NoError(t, sec.Key("c").AddShadow("x")) + require.NoError(t, sec.Key("d").AddNestedValue("n = 1")) + assert.Equal(t, []string{"a", "b", "c", "d"}, sec.KeyNames()) + assert.Equal(t, "1", sec.Key("a").String()) + + k := sec.Key("e") + k.SetValue("2") + k.SetValue("3") + assert.Same(t, k, sec.Key("e"), "the key added is the one returned") + assert.Equal(t, []string{"a", "b", "c", "d", "e"}, sec.KeyNames()) + }) + + t.Run("a missing key follows the key its section gains", func(t *testing.T) { + f, err := Load([]byte("[s]\n"), CaseInsensitiveKeys(true)) + require.NoError(t, err) + sec := f.Section("s") + + k1, k2 := sec.Key("Port"), sec.Key("port") + assert.Equal(t, "port", k1.Name()) + k1.SetValue("1") + assert.Equal(t, "1", k2.String()) + k2.SetValue("2") + assert.Equal(t, "2", k1.String()) + assert.Equal(t, []string{"port"}, sec.KeyNames()) + + k3 := sec.Key("host") + _, err = sec.NewKey("host", "example.com") + require.NoError(t, err) + assert.Equal(t, "example.com", k3.String()) + k3.SetValue("localhost") + assert.Equal(t, "localhost", sec.Key("host").String()) + assert.Equal(t, []string{"port", "host"}, sec.KeyNames()) + }) + + t.Run("an empty name is never added", func(t *testing.T) { + f := Empty() + f.Section("").Key("").SetValue("v") + assert.Empty(t, f.Section("").KeyNames()) + }) } func TestSection_Keys(t *testing.T) { From 42df5a4dbf614c52e053240445335ea50db8a5af Mon Sep 17 00:00:00 2001 From: Joe Chen Date: Wed, 11 Nov 2026 10:07:36 -0500 Subject: [PATCH 2/2] Expand StringsWithShadows of a missing key with the key it follows A missing key returned by Section.Key expanded its values with itself, so a reference to its own name, once the section gained the key, was expanded twice. Also narrow the docs: the migration guide no longer claims concurrent reads are safe with Locking(false), which Section still breaks by adding missing sections. AddShadow and AddNestedValue add a key only when the file allows them, and a missing key isn't added once its section gains a key of its name. --- docs/migrate-v1-to-v2.md | 2 +- key.go | 16 ++++++++++++---- section.go | 5 +++-- section_test.go | 11 +++++++++++ 4 files changed, 27 insertions(+), 7 deletions(-) diff --git a/docs/migrate-v1-to-v2.md b/docs/migrate-v1-to-v2.md index 5592cef..15ff2ec 100644 --- a/docs/migrate-v1-to-v2.md +++ b/docs/migrate-v1-to-v2.md @@ -23,7 +23,7 @@ sec.Key("PORT").SetValue(strconv.Itoa(port)) ### `Section.Key` no longer adds missing keys -In v1, `sec.Key("PORT")` added an empty `PORT` key when the section had none, so `HasKey`, `Keys` and `Write` saw it after a mere read. In v2 it returns a key that reads as empty, and only `SetValue`, `SetComment`, `AddShadow` or `AddNestedValue` adds it. `sec.Key(name).SetValue(v)` works as before. Reading a missing key no longer writes to the file, so concurrent reads are safe with `Locking(false)`. +In v1, `sec.Key("PORT")` added an empty `PORT` key when the section had none, so `HasKey`, `Keys` and `Write` saw it after a mere read. In v2 it returns a key that reads as empty, and only `SetValue` or `SetComment` adds it, or `AddShadow` and `AddNestedValue` when the file allows shadows or nested values. `sec.Key(name).SetValue(v)` works as before. ### `Reload` and `Append` diff --git a/key.go b/key.go index f768961..a901d9e 100644 --- a/key.go +++ b/key.go @@ -25,6 +25,8 @@ type Key struct { isBooleanType bool // missing is set on a key Section.Key returned for a name its section // doesn't have. The key isn't in the section until a change adds it. + // If the section gains a key of its name first, it reads and changes + // that key instead, and is never added. missing bool nestedValues []string @@ -51,8 +53,9 @@ func (k *Key) current() *Key { return k } -// attach adds k to its section if it's missing, and returns the key to -// change. The caller holds the write lock. +// attach adds k to its section if it's missing and the section still has no +// key of its name, and returns the key to change: k, or the key of its name +// the section gained. The caller holds the write lock. func (k *Key) attach() *Key { c := k.current() if c == k && k.missing { @@ -521,7 +524,12 @@ func splitList(str, delim string) []string { // StringsWithShadows returns list of string divided by given delimiter. // Shadows will also be appended if any. func (k *Key) StringsWithShadows(delim string) []string { - vals := k.Values() + // Expand with the key the values come from, so that a reference to its + // own name is looked up in the default section. + k.s.f.maybeRLock() + c := k.current() + k.s.f.maybeRUnlock() + vals := c.Values() results := make([]string, 0, len(vals)*2) for i := range vals { if len(vals) == 0 { @@ -532,7 +540,7 @@ func (k *Key) StringsWithShadows(delim string) []string { } for i := range results { - results[i] = k.expandValue(strings.TrimSpace(results[i])) + results[i] = c.expandValue(strings.TrimSpace(results[i])) } return results } diff --git a/section.go b/section.go index 8461f53..20646c2 100644 --- a/section.go +++ b/section.go @@ -173,8 +173,9 @@ func (s *Section) HasValue(value string) bool { // Key returns the key with the given name, looking in parent sections too // as GetKey does. When there is none, it returns a key that reads as empty -// without adding it to the section. SetValue, SetComment, AddShadow or -// AddNestedValue adds it. +// without adding it to the section. SetValue or SetComment adds it, and so +// do AddShadow and AddNestedValue when the file allows shadows or nested +// values. func (s *Section) Key(name string) *Key { if key, err := s.GetKey(name); err == nil { return key diff --git a/section_test.go b/section_test.go index 97f717a..abb2120 100644 --- a/section_test.go +++ b/section_test.go @@ -254,6 +254,17 @@ func TestSection_Key(t *testing.T) { assert.Equal(t, []string{"port", "host"}, sec.KeyNames()) }) + t.Run("a missing key expands as the key it follows", func(t *testing.T) { + f, err := Load([]byte("path = /usr\n[s]\n")) + require.NoError(t, err) + sec := f.Section("s") + k := sec.Key("path") + _, err = sec.NewKey("path", "%(path)s/bin") + require.NoError(t, err) + assert.Equal(t, "/usr/bin", k.String()) + assert.Equal(t, []string{"/usr/bin"}, k.StringsWithShadows(",")) + }) + t.Run("an empty name is never added", func(t *testing.T) { f := Empty() f.Section("").Key("").SetValue("v")