Skip to content

Commit cd90dbc

Browse files
ChrisJBurnsclaude
ChrisJBurns
and
claude
authored
Fix YAML replacement affecting wrong fields with same value (#22)
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent b702275 commit cd90dbc

2 files changed

Lines changed: 200 additions & 30 deletions

File tree

‎internal/files/yaml.go‎

Lines changed: 60 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,11 @@ func UpdateYAMLFile(cfg VersionFileConfig, currentVersion, newVersion string) er
8282
newValue = newVersionStr
8383
}
8484

85-
// Perform surgical replacement - find and replace only the value
86-
newData, err := surgicalReplace(data, oldValue, newValue)
85+
// Extract the key name from the path for targeted replacement
86+
key := extractKeyFromPath(cfg.Path)
87+
88+
// Perform surgical replacement - find and replace only the value for this specific key
89+
newData, err := surgicalReplace(data, key, oldValue, newValue)
8790
if err != nil {
8891
return fmt.Errorf("replacing value at path %s: %w", cfg.Path, err)
8992
}
@@ -96,84 +99,111 @@ func UpdateYAMLFile(cfg VersionFileConfig, currentVersion, newVersion string) er
9699
return nil
97100
}
98101

102+
// extractKeyFromPath extracts the final key name from a dot-notation path.
103+
// Examples:
104+
//
105+
// "version" -> "version"
106+
// "metadata.version" -> "version"
107+
// "spec.containers[0].image" -> "image"
108+
// "operator.image" -> "image"
109+
func extractKeyFromPath(path string) string {
110+
// Remove any array indices from the path
111+
re := regexp.MustCompile(`\[\d+\]`)
112+
cleanPath := re.ReplaceAllString(path, "")
113+
114+
// Get the last component after splitting by dots
115+
parts := strings.Split(cleanPath, ".")
116+
if len(parts) == 0 {
117+
return path
118+
}
119+
return parts[len(parts)-1]
120+
}
121+
99122
// replacementRule defines a pattern-replacement pair for surgical YAML value replacement.
100123
// Each rule targets a specific quote style or value format in YAML files.
101124
type replacementRule struct {
102125
// name describes what this rule handles (for debugging/documentation)
103126
name string
104-
// pattern returns the regex pattern to match for the given old value
105-
pattern func(oldValue string) string
106-
// replacement returns the replacement string for the given new value
107-
replacement func(newValue string) string
127+
// pattern returns the regex pattern to match for the given key and old value.
128+
// The key is included to ensure we only match the specific YAML key we're updating.
129+
pattern func(key, oldValue string) string
130+
// replacement returns the replacement string for the given key and new value
131+
replacement func(key, newValue string) string
108132
}
109133

110134
// replacementRules defines all the rules for surgical YAML value replacement.
111135
// Rules are tried in order; the first matching rule is applied.
136+
// Each pattern includes the key name to ensure we only replace the value for the
137+
// specific YAML key being updated, not other keys with the same value.
112138
var replacementRules = []replacementRule{
113139
{
114140
// Handles double-quoted values: key: "value"
115141
name: "double-quoted",
116-
pattern: func(oldValue string) string {
117-
return fmt.Sprintf(`"(%s)"`, regexp.QuoteMeta(oldValue))
142+
pattern: func(key, oldValue string) string {
143+
return fmt.Sprintf(`(%s:\s*)"(%s)"`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue))
118144
},
119-
replacement: func(newValue string) string {
120-
return fmt.Sprintf(`"%s"`, newValue)
145+
replacement: func(_, newValue string) string {
146+
// Use ${1} syntax to avoid ambiguity when newValue starts with a digit
147+
return fmt.Sprintf(`${1}"%s"`, newValue)
121148
},
122149
},
123150
{
124151
// Handles single-quoted values: key: 'value'
125152
name: "single-quoted",
126-
pattern: func(oldValue string) string {
127-
return fmt.Sprintf(`'(%s)'`, regexp.QuoteMeta(oldValue))
153+
pattern: func(key, oldValue string) string {
154+
return fmt.Sprintf(`(%s:\s*)'(%s)'`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue))
128155
},
129-
replacement: func(newValue string) string {
130-
return fmt.Sprintf(`'%s'`, newValue)
156+
replacement: func(_, newValue string) string {
157+
return fmt.Sprintf(`${1}'%s'`, newValue)
131158
},
132159
},
133160
{
134161
// Handles unquoted values at end of line: key: value\n
135162
name: "unquoted-eol",
136-
pattern: func(oldValue string) string {
137-
return fmt.Sprintf(`: (%s)(\s*)$`, regexp.QuoteMeta(oldValue))
163+
pattern: func(key, oldValue string) string {
164+
return fmt.Sprintf(`(%s:\s*)(%s)(\s*)$`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue))
138165
},
139-
replacement: func(newValue string) string {
140-
return fmt.Sprintf(`: %s$2`, newValue)
166+
replacement: func(_, newValue string) string {
167+
return fmt.Sprintf(`${1}%s${3}`, newValue)
141168
},
142169
},
143170
{
144171
// Handles unquoted values followed by inline comment: key: value # comment
145172
name: "unquoted-with-comment",
146-
pattern: func(oldValue string) string {
147-
return fmt.Sprintf(`: (%s)(\s*#)`, regexp.QuoteMeta(oldValue))
173+
pattern: func(key, oldValue string) string {
174+
return fmt.Sprintf(`(%s:\s*)(%s)(\s*#)`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue))
148175
},
149-
replacement: func(newValue string) string {
150-
return fmt.Sprintf(`: %s$2`, newValue)
176+
replacement: func(_, newValue string) string {
177+
return fmt.Sprintf(`${1}%s${3}`, newValue)
151178
},
152179
},
153180
}
154181

155182
// surgicalReplace performs a targeted replacement of a YAML value while preserving
156-
// the original formatting (quotes, whitespace, etc.)
157-
func surgicalReplace(data []byte, oldValue, newValue string) ([]byte, error) {
183+
// the original formatting (quotes, whitespace, etc.). The key parameter ensures
184+
// we only replace the value for the specific YAML key being updated.
185+
func surgicalReplace(data []byte, key, oldValue, newValue string) ([]byte, error) {
158186
content := string(data)
159187

160188
// Try each replacement rule in order; use the first one that matches
161189
for _, rule := range replacementRules {
162-
pattern := rule.pattern(oldValue)
190+
pattern := rule.pattern(key, oldValue)
163191
re := regexp.MustCompile(`(?m)` + pattern)
164192
if re.MatchString(content) {
165-
result := re.ReplaceAllString(content, rule.replacement(newValue))
193+
result := re.ReplaceAllString(content, rule.replacement(key, newValue))
166194
return []byte(result), nil
167195
}
168196
}
169197

170-
// Fallback: simple string replacement if no pattern matched
171-
if strings.Contains(content, oldValue) {
172-
result := strings.Replace(content, oldValue, newValue, 1)
198+
// Fallback: key-aware simple string replacement if no pattern matched
199+
// Look for "key: oldValue" or "key:oldValue" patterns
200+
keyPattern := regexp.MustCompile(fmt.Sprintf(`(%s:\s*)%s`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue)))
201+
if keyPattern.MatchString(content) {
202+
result := keyPattern.ReplaceAllString(content, fmt.Sprintf(`${1}%s`, newValue))
173203
return []byte(result), nil
174204
}
175205

176-
return nil, fmt.Errorf("could not find value %q to replace", oldValue)
206+
return nil, fmt.Errorf("could not find value %q for key %q to replace", oldValue, key)
177207
}
178208

179209
// findEmbeddedVersion looks for a version pattern in the value and returns it if found.

‎internal/files/yaml_test.go‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,34 @@ func TestConvertToYAMLPath(t *testing.T) {
251251
}
252252
}
253253

254+
func TestExtractKeyFromPath(t *testing.T) {
255+
t.Parallel()
256+
257+
tests := []struct {
258+
input string
259+
want string
260+
}{
261+
{"version", "version"},
262+
{"appVersion", "appVersion"},
263+
{"metadata.version", "version"},
264+
{"spec.template.spec.image.tag", "tag"},
265+
{"operator.image", "image"},
266+
{"containers[0].image", "image"},
267+
{"spec.containers[0].image", "image"},
268+
{"data[0]", "data"},
269+
}
270+
271+
for _, tt := range tests {
272+
t.Run(tt.input, func(t *testing.T) {
273+
t.Parallel()
274+
got := extractKeyFromPath(tt.input)
275+
if got != tt.want {
276+
t.Errorf("extractKeyFromPath(%q) = %q, want %q", tt.input, got, tt.want)
277+
}
278+
})
279+
}
280+
}
281+
254282
func TestUpdateYAMLFile_InvalidPath(t *testing.T) {
255283
t.Parallel()
256284

@@ -607,3 +635,115 @@ app:
607635
})
608636
}
609637
}
638+
639+
// TestUpdateYAMLFile_SameValueDifferentKeys tests that updating one field does not
640+
// accidentally modify another field with the same value but different formatting.
641+
// This was a bug where updating "version: 0.9.0" would also modify "appVersion: "0.9.0""
642+
// because the replacement was not key-aware.
643+
func TestUpdateYAMLFile_SameValueDifferentKeys(t *testing.T) {
644+
t.Parallel()
645+
646+
tests := []struct {
647+
name string
648+
input string
649+
config VersionFileConfig
650+
currentVersion string
651+
newVersion string
652+
wantContains []string
653+
wantNotContain []string
654+
}{
655+
{
656+
name: "update unquoted version without affecting quoted appVersion",
657+
input: `apiVersion: v2
658+
name: test-chart
659+
version: 0.9.0
660+
appVersion: "0.9.0"
661+
`,
662+
config: VersionFileConfig{Path: "version"},
663+
currentVersion: "0.9.0",
664+
newVersion: "0.9.1",
665+
wantContains: []string{
666+
"version: 0.9.1",
667+
`appVersion: "0.9.0"`, // appVersion should NOT be changed
668+
},
669+
wantNotContain: []string{
670+
`appVersion: "0.9.1"`, // should NOT happen
671+
},
672+
},
673+
{
674+
name: "update quoted appVersion without affecting unquoted version",
675+
input: `apiVersion: v2
676+
name: test-chart
677+
version: 0.9.0
678+
appVersion: "0.9.0"
679+
`,
680+
config: VersionFileConfig{Path: "appVersion"},
681+
currentVersion: "0.9.0",
682+
newVersion: "0.9.1",
683+
wantContains: []string{
684+
"version: 0.9.0", // version should NOT be changed
685+
`appVersion: "0.9.1"`,
686+
},
687+
wantNotContain: []string{
688+
"version: 0.9.1", // should NOT happen
689+
},
690+
},
691+
{
692+
name: "sequential updates to different keys with same value",
693+
input: `apiVersion: v2
694+
name: operator-crds
695+
version: 0.9.0
696+
appVersion: "0.9.0"
697+
`,
698+
config: VersionFileConfig{Path: "version"},
699+
currentVersion: "0.9.0",
700+
newVersion: "0.9.1",
701+
wantContains: []string{
702+
"version: 0.9.1",
703+
`appVersion: "0.9.0"`,
704+
},
705+
},
706+
{
707+
name: "different keys same value - nested paths",
708+
input: `chart:
709+
version: 0.9.0
710+
appVersion: "0.9.0"
711+
description: A test chart
712+
`,
713+
config: VersionFileConfig{Path: "chart.version"},
714+
currentVersion: "0.9.0",
715+
newVersion: "0.9.1",
716+
wantContains: []string{
717+
"version: 0.9.1",
718+
`appVersion: "0.9.0"`, // appVersion should NOT be changed
719+
},
720+
},
721+
}
722+
723+
for _, tt := range tests {
724+
t.Run(tt.name, func(t *testing.T) {
725+
t.Parallel()
726+
727+
tmpPath := createTempFile(t, tt.input, "test*.yaml")
728+
cfg := tt.config
729+
cfg.File = tmpPath
730+
731+
err := UpdateYAMLFile(cfg, tt.currentVersion, tt.newVersion)
732+
if err != nil {
733+
t.Fatalf("UpdateYAMLFile() error = %v", err)
734+
}
735+
736+
got := readTempFile(t, tmpPath)
737+
for _, want := range tt.wantContains {
738+
if !strings.Contains(got, want) {
739+
t.Errorf("UpdateYAMLFile() should contain %q, got:\n%s", want, got)
740+
}
741+
}
742+
for _, notWant := range tt.wantNotContain {
743+
if strings.Contains(got, notWant) {
744+
t.Errorf("UpdateYAMLFile() should NOT contain %q, got:\n%s", notWant, got)
745+
}
746+
}
747+
})
748+
}
749+
}

0 commit comments

Comments
 (0)