Skip to content

Commit 26434f9

Browse files
author
Chris McDonnell
committed
Migrate to only doing marshalling once
1 parent 1b43f4f commit 26434f9

File tree

3 files changed

+106
-101
lines changed

3 files changed

+106
-101
lines changed

‎pkg/config/app_config.go‎

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,14 @@ func migrateUserConfig(path string, content []byte) ([]byte, error) {
237237

238238
// A pure function helper for testing purposes
239239
func computeMigratedConfig(path string, content []byte) ([]byte, error) {
240-
changedContent := content
240+
var err error
241+
var rootNode yaml.Node
242+
// Track if any of the transformations did anything
243+
anyMigrations := false
244+
err = yaml.Unmarshal(content, &rootNode)
245+
if err != nil {
246+
return nil, fmt.Errorf("failed to parse YAML: %w", err)
247+
}
241248

242249
pathsToReplace := []struct {
243250
oldPath []string
@@ -248,35 +255,47 @@ func computeMigratedConfig(path string, content []byte) ([]byte, error) {
248255
{[]string{"gui", "windowSize"}, "screenMode"},
249256
}
250257

251-
var err error
252258
for _, pathToReplace := range pathsToReplace {
253-
changedContent, err = yaml_utils.RenameYamlKey(changedContent, pathToReplace.oldPath, pathToReplace.newName)
259+
migrated, err := yaml_utils.RenameYamlKey(&rootNode, pathToReplace.oldPath, pathToReplace.newName)
254260
if err != nil {
255261
return nil, fmt.Errorf("Couldn't migrate config file at `%s` for key %s: %s", path, strings.Join(pathToReplace.oldPath, "."), err)
256262
}
263+
anyMigrations = anyMigrations || migrated
257264
}
258265

259-
changedContent, err = changeNullKeybindingsToDisabled(changedContent)
266+
migrated, err := changeNullKeybindingsToDisabled(&rootNode)
260267
if err != nil {
261268
return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
262269
}
270+
anyMigrations = anyMigrations || migrated
263271

264-
changedContent, err = changeElementToSequence(changedContent, []string{"git", "commitPrefix"})
272+
migrated, err = changeElementToSequence(&rootNode, []string{"git", "commitPrefix"})
265273
if err != nil {
266274
return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
267275
}
276+
anyMigrations = anyMigrations || migrated
268277

269-
changedContent, err = changeCommitPrefixesMap(changedContent)
278+
migrated, err = changeCommitPrefixesMap(&rootNode)
270279
if err != nil {
271280
return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
272281
}
282+
anyMigrations = anyMigrations || migrated
283+
273284
// Add more migrations here...
274285

275-
return changedContent, nil
286+
if anyMigrations {
287+
newContent, err := yaml_utils.YamlMarshal(&rootNode)
288+
if err != nil {
289+
return nil, fmt.Errorf("Failed to remarsall!")
290+
}
291+
return newContent, nil
292+
} else {
293+
return content, nil
294+
}
276295
}
277296

278-
func changeNullKeybindingsToDisabled(changedContent []byte) ([]byte, error) {
279-
return yaml_utils.Walk(changedContent, func(node *yaml.Node, path string) bool {
297+
func changeNullKeybindingsToDisabled(rootNode *yaml.Node) (bool, error) {
298+
return yaml_utils.Walk(rootNode, func(node *yaml.Node, path string) bool {
280299
if strings.HasPrefix(path, "keybinding.") && node.Kind == yaml.ScalarNode && node.Tag == "!!null" {
281300
node.Value = "<disabled>"
282301
node.Tag = "!!str"
@@ -286,8 +305,8 @@ func changeNullKeybindingsToDisabled(changedContent []byte) ([]byte, error) {
286305
})
287306
}
288307

289-
func changeElementToSequence(changedContent []byte, path []string) ([]byte, error) {
290-
return yaml_utils.TransformNode(changedContent, path, func(node *yaml.Node) (bool, error) {
308+
func changeElementToSequence(rootNode *yaml.Node, path []string) (bool, error) {
309+
return yaml_utils.TransformNode(rootNode, path, func(node *yaml.Node) (bool, error) {
291310
if node.Kind == yaml.MappingNode {
292311
nodeContentCopy := node.Content
293312
node.Kind = yaml.SequenceNode
@@ -304,8 +323,8 @@ func changeElementToSequence(changedContent []byte, path []string) ([]byte, erro
304323
})
305324
}
306325

307-
func changeCommitPrefixesMap(changedContent []byte) ([]byte, error) {
308-
return yaml_utils.TransformNode(changedContent, []string{"git", "commitPrefixes"}, func(prefixesNode *yaml.Node) (bool, error) {
326+
func changeCommitPrefixesMap(rootNode *yaml.Node) (bool, error) {
327+
return yaml_utils.TransformNode(rootNode, []string{"git", "commitPrefixes"}, func(prefixesNode *yaml.Node) (bool, error) {
309328
changedAnyNodes := false
310329
if prefixesNode.Kind == yaml.MappingNode {
311330
for _, contentNode := range prefixesNode.Content {

‎pkg/utils/yaml_utils/yaml_utils.go‎

Lines changed: 26 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ func UpdateYamlValue(yamlBytes []byte, path []string, value string) ([]byte, err
3535
}
3636

3737
// Convert the updated YAML node back to YAML bytes.
38-
updatedYAMLBytes, err := yamlMarshal(body)
38+
updatedYAMLBytes, err := YamlMarshal(body)
3939
if err != nil {
4040
return nil, fmt.Errorf("failed to convert YAML node to bytes: %w", err)
4141
}
@@ -100,39 +100,25 @@ func lookupKey(node *yaml.Node, key string) (*yaml.Node, *yaml.Node) {
100100
return nil, nil
101101
}
102102

103-
// Walks a yaml document to the specified path, and then applies the transformation to that node.
103+
// Walks a yaml tree to the specified path, and then applies the transformation to that node.
104104
//
105105
// The transform must return true if it made changes to the node.
106106
// If the requested path is not defined in the document, no changes are made to the document.
107107
//
108-
// If no changes are made, the original document is returned.
109-
// If changes are made, a newly marshalled document is returned. (This may result in different indentation for all nodes)
110-
func TransformNode(yamlBytes []byte, path []string, transform func(node *yaml.Node) (bool, error)) ([]byte, error) {
111-
// Parse the YAML file.
112-
var node yaml.Node
113-
err := yaml.Unmarshal(yamlBytes, &node)
114-
if err != nil {
115-
return nil, fmt.Errorf("failed to parse YAML: %w", err)
116-
}
117-
108+
// Returns true if the transform made changes to the node
109+
func TransformNode(rootNode *yaml.Node, path []string, transform func(node *yaml.Node) (bool, error)) (bool, error) {
118110
// Empty document: nothing to do.
119-
if len(node.Content) == 0 {
120-
return yamlBytes, nil
111+
if len(rootNode.Content) == 0 {
112+
return false, nil
121113
}
122114

123-
body := node.Content[0]
115+
body := rootNode.Content[0]
124116

125117
if didTransform, err := transformNode(body, path, transform); err != nil || !didTransform {
126-
return yamlBytes, err
118+
return false, err
127119
}
128120

129-
// Convert the updated YAML node back to YAML bytes.
130-
updatedYAMLBytes, err := yamlMarshal(body)
131-
if err != nil {
132-
return nil, fmt.Errorf("failed to convert YAML node to bytes: %w", err)
133-
}
134-
135-
return updatedYAMLBytes, nil
121+
return true, nil
136122
}
137123

138124
// A recursive function to walk down the tree. See TransformNode for more details.
@@ -149,34 +135,22 @@ func transformNode(node *yaml.Node, path []string, transform func(node *yaml.Nod
149135
return transformNode(valueNode, path[1:], transform)
150136
}
151137

152-
// takes a yaml document in bytes, a path to a key, and a new name for the key.
138+
// Takes the root node of a yaml document, a path to a key, and a new name for the key.
153139
// Will rename the key to the new name if it exists, and do nothing otherwise.
154-
func RenameYamlKey(yamlBytes []byte, path []string, newKey string) ([]byte, error) {
155-
// Parse the YAML file.
156-
var node yaml.Node
157-
err := yaml.Unmarshal(yamlBytes, &node)
158-
if err != nil {
159-
return nil, fmt.Errorf("failed to parse YAML: %w for bytes %s", err, string(yamlBytes))
160-
}
161-
140+
// Returns true if it found the old path and renamed the key
141+
func RenameYamlKey(rootNode *yaml.Node, path []string, newKey string) (bool, error) {
162142
// Empty document: nothing to do.
163-
if len(node.Content) == 0 {
164-
return yamlBytes, nil
143+
if len(rootNode.Content) == 0 {
144+
return false, nil
165145
}
166146

167-
body := node.Content[0]
147+
body := rootNode.Content[0]
168148

169149
if didRename, err := renameYamlKey(body, path, newKey); err != nil || !didRename {
170-
return yamlBytes, err
171-
}
172-
173-
// Convert the updated YAML node back to YAML bytes.
174-
updatedYAMLBytes, err := yamlMarshal(body)
175-
if err != nil {
176-
return nil, fmt.Errorf("failed to convert YAML node to bytes: %w", err)
150+
return false, err
177151
}
178152

179-
return updatedYAMLBytes, nil
153+
return true, nil
180154
}
181155

182156
// Recursive function to rename the YAML key.
@@ -206,34 +180,21 @@ func renameYamlKey(node *yaml.Node, path []string, newKey string) (bool, error)
206180

207181
// Traverses a yaml document, calling the callback function for each node. The
208182
// callback is allowed to modify the node in place, in which case it should
209-
// return true. The function returns the original yaml document if none of the
210-
// callbacks returned true, and the modified document otherwise.
211-
func Walk(yamlBytes []byte, callback func(node *yaml.Node, path string) bool) ([]byte, error) {
212-
// Parse the YAML file.
213-
var node yaml.Node
214-
err := yaml.Unmarshal(yamlBytes, &node)
215-
if err != nil {
216-
return nil, fmt.Errorf("failed to parse YAML: %w", err)
217-
}
218-
183+
// return true. The function returns true if the callback modified the document at all,
184+
// and false otherwise.
185+
func Walk(rootNode *yaml.Node, callback func(node *yaml.Node, path string) bool) (bool, error) {
219186
// Empty document: nothing to do.
220-
if len(node.Content) == 0 {
221-
return yamlBytes, nil
187+
if len(rootNode.Content) == 0 {
188+
return false, nil
222189
}
223190

224-
body := node.Content[0]
191+
body := rootNode.Content[0]
225192

226193
if didChange, err := walk(body, "", callback); err != nil || !didChange {
227-
return yamlBytes, err
194+
return false, err
228195
}
229196

230-
// Convert the updated YAML node back to YAML bytes.
231-
updatedYAMLBytes, err := yamlMarshal(body)
232-
if err != nil {
233-
return nil, fmt.Errorf("failed to convert YAML node to bytes: %w", err)
234-
}
235-
236-
return updatedYAMLBytes, nil
197+
return true, nil
237198
}
238199

239200
func walk(node *yaml.Node, path string, callback func(*yaml.Node, string) bool) (bool, error) {
@@ -275,7 +236,7 @@ func walk(node *yaml.Node, path string, callback func(*yaml.Node, string) bool)
275236
return didChange, nil
276237
}
277238

278-
func yamlMarshal(node *yaml.Node) ([]byte, error) {
239+
func YamlMarshal(node *yaml.Node) ([]byte, error) {
279240
var buffer bytes.Buffer
280241
encoder := yaml.NewEncoder(&buffer)
281242
encoder.SetIndent(2)

‎pkg/utils/yaml_utils/yaml_utils_test.go‎

Lines changed: 48 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -186,14 +186,16 @@ func TestRenameYamlKey(t *testing.T) {
186186

187187
for _, test := range tests {
188188
t.Run(test.name, func(t *testing.T) {
189-
out, actualErr := RenameYamlKey([]byte(test.in), test.path, test.newKey)
189+
node := unmarshalForTest(t, test.in)
190+
_, actualErr := RenameYamlKey(&node, test.path, test.newKey)
190191
if test.expectedErr == "" {
191192
assert.NoError(t, actualErr)
192193
} else {
193194
assert.EqualError(t, actualErr, test.expectedErr)
194195
}
196+
out := marshalForTest(t, &node)
195197

196-
assert.Equal(t, test.expectedOut, string(out))
198+
assert.Equal(t, test.expectedOut, out)
197199
})
198200
}
199201
}
@@ -238,8 +240,9 @@ func TestWalk_paths(t *testing.T) {
238240

239241
for _, test := range tests {
240242
t.Run(test.name, func(t *testing.T) {
243+
node := unmarshalForTest(t, test.document)
241244
paths := []string{}
242-
_, err := Walk([]byte(test.document), func(node *yaml.Node, path string) bool {
245+
_, err := Walk(&node, func(node *yaml.Node, path string) bool {
243246
paths = append(paths, path)
244247
return true
245248
})
@@ -258,10 +261,9 @@ func TestWalk_inPlaceChanges(t *testing.T) {
258261
expectedOut string
259262
}{
260263
{
261-
name: "no change",
262-
in: "x: 5",
263-
callback: func(node *yaml.Node, path string) bool { return false },
264-
expectedOut: "x: 5",
264+
name: "no change",
265+
in: "x: 5",
266+
callback: func(node *yaml.Node, path string) bool { return false },
265267
},
266268
{
267269
name: "change value",
@@ -303,10 +305,15 @@ func TestWalk_inPlaceChanges(t *testing.T) {
303305

304306
for _, test := range tests {
305307
t.Run(test.name, func(t *testing.T) {
306-
result, err := Walk([]byte(test.in), test.callback)
307-
308+
node := unmarshalForTest(t, test.in)
309+
modified, err := Walk(&node, test.callback)
308310
assert.NoError(t, err)
309-
assert.Equal(t, test.expectedOut, string(result))
311+
if test.expectedOut == "" {
312+
assert.False(t, modified, "We should not have modified the element, but we did!")
313+
} else {
314+
result := marshalForTest(t, &node)
315+
assert.Equal(t, test.expectedOut, result)
316+
}
310317
})
311318
}
312319
}
@@ -336,11 +343,10 @@ func TestTransformNode(t *testing.T) {
336343
expectedOut string
337344
}{
338345
{
339-
name: "Path not present",
340-
in: "foo: 1",
341-
path: []string{"bar"},
342-
transform: transformIntValueToString,
343-
expectedOut: "foo: 1",
346+
name: "Path not present",
347+
in: "foo: 1",
348+
path: []string{"bar"},
349+
transform: transformIntValueToString,
344350
},
345351
{
346352
name: "Part of path present",
@@ -349,9 +355,6 @@ foo:
349355
bar: 2`,
350356
path: []string{"foo", "baz"},
351357
transform: transformIntValueToString,
352-
expectedOut: `
353-
foo:
354-
bar: 2`,
355358
},
356359
{
357360
name: "Successfully Transforms to string",
@@ -371,19 +374,41 @@ foo:
371374
bar: "2"`,
372375
path: []string{"foo", "bar"},
373376
transform: transformIntValueToString,
374-
expectedOut: `
375-
foo:
376-
bar: "2"`,
377377
},
378378
}
379379

380380
for _, test := range tests {
381381
t.Run(test.name, func(t *testing.T) {
382-
result, err := TransformNode([]byte(test.in), test.path, test.transform)
382+
node := unmarshalForTest(t, test.in)
383+
modified, err := TransformNode(&node, test.path, test.transform)
383384
if err != nil {
384385
t.Fatal(err)
385386
}
386-
assert.Equal(t, test.expectedOut, string(result))
387+
if test.expectedOut == "" {
388+
assert.False(t, modified, "We should not have modified the element, but we did!")
389+
} else {
390+
result := marshalForTest(t, &node)
391+
assert.Equal(t, test.expectedOut, result)
392+
}
387393
})
388394
}
389395
}
396+
397+
func unmarshalForTest(t *testing.T, input string) yaml.Node {
398+
t.Helper()
399+
var node yaml.Node
400+
err := yaml.Unmarshal([]byte(input), &node)
401+
if err != nil {
402+
t.Fatal(err)
403+
}
404+
return node
405+
}
406+
407+
func marshalForTest(t *testing.T, node *yaml.Node) string {
408+
t.Helper()
409+
result, err := YamlMarshal(node)
410+
if err != nil {
411+
t.Fatal(err)
412+
}
413+
return string(result)
414+
}

0 commit comments

Comments
 (0)