Merge pull request #2718 from stefanhaller/improve-yaml-utils

Improve yaml_utils

pkg/utils/formatting_test.go

@@ -79,9 +79,7 @@ func TestGetPadWidths(t *testing.T) {
 
 	for _, test := range tests {
 		output := getPadWidths(test.input)
-		if !assert.EqualValues(t, output, test.expected) {
+		assert.EqualValues(t, output, test.expected)
-			t.Errorf("getPadWidths(%v) = %v, want %v", test.input, output, test.expected)
-		}
 	}
 }
 
@@ -219,8 +217,6 @@ func TestRenderDisplayStrings(t *testing.T) {
 
 	for _, test := range tests {
 		output := RenderDisplayStrings(test.input, test.columnAlignments)
-		if !assert.EqualValues(t, output, test.expected) {
+		assert.EqualValues(t, output, test.expected)
-			t.Errorf("RenderDisplayStrings(%v) = %v, want %v", test.input, output, test.expected)
-		}
 	}
 }

pkg/utils/rebase_todo_test.go

@@ -321,18 +321,12 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
 			actualTodos, actualErr := moveFixupCommitDown(scenario.todos, scenario.originalSha, scenario.fixupSha)
 
 			if scenario.expectedErr == nil {
-				if !assert.NoError(t, actualErr) {
+				assert.NoError(t, actualErr)
-					t.Errorf("Expected no error, got: %v", actualErr)
-				}
 			} else {
-				if !assert.EqualError(t, actualErr, scenario.expectedErr.Error()) {
+				assert.EqualError(t, actualErr, scenario.expectedErr.Error())
-					t.Errorf("Expected err: %v, got: %v", scenario.expectedErr, actualErr)
-				}
 			}
 
-			if !assert.EqualValues(t, actualTodos, scenario.expectedTodos) {
+			assert.EqualValues(t, actualTodos, scenario.expectedTodos)
-				t.Errorf("Expected todos: %v, got: %v", scenario.expectedTodos, actualTodos)
-			}
 		})
 	}
 }

pkg/utils/yaml_utils/yaml_utils.go

@@ -1,13 +1,14 @@
 package yaml_utils
 
 import (
+	"errors"
 	"fmt"
 
 	"gopkg.in/yaml.v3"
 )
 
 // takes a yaml document in bytes, a path to a key, and a value to set. The value must be a scalar.
-func UpdateYaml(yamlBytes []byte, path []string, value string) ([]byte, error) {
+func UpdateYamlValue(yamlBytes []byte, path []string, value string) ([]byte, error) {
 	// Parse the YAML file.
 	var node yaml.Node
 	err := yaml.Unmarshal(yamlBytes, &node)
@@ -15,9 +16,22 @@ func UpdateYaml(yamlBytes []byte, path []string, value string) ([]byte, error) {
 		return nil, fmt.Errorf("failed to parse YAML: %w", err)
 	}
 
+	// Empty document: need to create the top-level map ourselves
+	if len(node.Content) == 0 {
+		node.Content = append(node.Content, &yaml.Node{
+			Kind: yaml.MappingNode,
+		})
+	}
+
 	body := node.Content[0]
 
-	updateYamlNode(body, path, value)
+	if body.Kind != yaml.MappingNode {
+		return yamlBytes, errors.New("yaml document is not a dictionary")
+	}
+
+	if didChange, err := updateYamlNode(body, path, value); err != nil || !didChange {
+		return yamlBytes, err
+	}
 
 	// Convert the updated YAML node back to YAML bytes.
 	updatedYAMLBytes, err := yaml.Marshal(body)
@@ -29,26 +43,113 @@ func UpdateYaml(yamlBytes []byte, path []string, value string) ([]byte, error) {
 }
 
 // Recursive function to update the YAML node.
-func updateYamlNode(node *yaml.Node, path []string, value string) {
+func updateYamlNode(node *yaml.Node, path []string, value string) (bool, error) {
 	if len(path) == 0 {
-		node.Value = value
+		if node.Kind != yaml.ScalarNode {
-		return
+			return false, errors.New("yaml node is not a scalar")
+		}
+		if node.Value != value {
+			node.Value = value
+			return true, nil
+		}
+		return false, nil
+	}
+
+	if node.Kind != yaml.MappingNode {
+		return false, errors.New("yaml node in path is not a dictionary")
 	}
 
 	key := path[0]
-	for i := 0; i < len(node.Content)-1; i += 2 {
+	if _, valueNode := lookupKey(node, key); valueNode != nil {
-		if node.Content[i].Value == key {
+		return updateYamlNode(valueNode, path[1:], value)
-			updateYamlNode(node.Content[i+1], path[1:], value)
-			return
-		}
 	}
 
 	// if the key doesn't exist, we'll add it
+
+	// at end of path: add the new key, done
+	if len(path) == 1 {
+		node.Content = append(node.Content, &yaml.Node{
+			Kind:  yaml.ScalarNode,
+			Value: key,
+		}, &yaml.Node{
+			Kind:  yaml.ScalarNode,
+			Value: value,
+		})
+		return true, nil
+	}
+
+	// otherwise, create the missing intermediate node and continue
+	newNode := &yaml.Node{
+		Kind: yaml.MappingNode,
+	}
 	node.Content = append(node.Content, &yaml.Node{
 		Kind:  yaml.ScalarNode,
 		Value: key,
-	}, &yaml.Node{
+	}, newNode)
-		Kind:  yaml.ScalarNode,
+	return updateYamlNode(newNode, path[1:], value)
-		Value: value,
+}
-	})
+
+func lookupKey(node *yaml.Node, key string) (*yaml.Node, *yaml.Node) {
+	for i := 0; i < len(node.Content)-1; i += 2 {
+		if node.Content[i].Value == key {
+			return node.Content[i], node.Content[i+1]
+		}
+	}
+
+	return nil, nil
+}
+
+// takes a yaml document in bytes, a path to a key, and a new name for the key.
+// Will rename the key to the new name if it exists, and do nothing otherwise.
+func RenameYamlKey(yamlBytes []byte, path []string, newKey string) ([]byte, error) {
+	// Parse the YAML file.
+	var node yaml.Node
+	err := yaml.Unmarshal(yamlBytes, &node)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse YAML: %w", err)
+	}
+
+	// Empty document: nothing to do.
+	if len(node.Content) == 0 {
+		return yamlBytes, nil
+	}
+
+	body := node.Content[0]
+
+	if didRename, err := renameYamlKey(body, path, newKey); err != nil || !didRename {
+		return yamlBytes, err
+	}
+
+	// Convert the updated YAML node back to YAML bytes.
+	updatedYAMLBytes, err := yaml.Marshal(body)
+	if err != nil {
+		return nil, fmt.Errorf("failed to convert YAML node to bytes: %w", err)
+	}
+
+	return updatedYAMLBytes, nil
+}
+
+// Recursive function to rename the YAML key.
+func renameYamlKey(node *yaml.Node, path []string, newKey string) (bool, error) {
+	if node.Kind != yaml.MappingNode {
+		return false, errors.New("yaml node in path is not a dictionary")
+	}
+
+	keyNode, valueNode := lookupKey(node, path[0])
+	if keyNode == nil {
+		return false, nil
+	}
+
+	// end of path reached: rename key
+	if len(path) == 1 {
+		// Check that new key doesn't exist yet
+		if newKeyNode, _ := lookupKey(node, newKey); newKeyNode != nil {
+			return false, fmt.Errorf("new key `%s' already exists", newKey)
+		}
+
+		keyNode.Value = newKey
+		return true, nil
+	}
+
+	return renameYamlKey(valueNode, path[1:], newKey)
 }

pkg/utils/yaml_utils/yaml_utils_test.go

@@ -1,8 +1,12 @@
 package yaml_utils
 
-import "testing"
+import (
+	"testing"
 
-func TestUpdateYaml(t *testing.T) {
+	"github.com/stretchr/testify/assert"
+)
+
+func TestUpdateYamlValue(t *testing.T) {
 	tests := []struct {
 		name        string
 		in          string
@@ -27,6 +31,14 @@ func TestUpdateYaml(t *testing.T) {
 			expectedOut: "foo: bar\nfoo2: baz\n",
 			expectedErr: "",
 		},
+		{
+			name:        "add new key and value when document was empty",
+			in:          "",
+			path:        []string{"foo"},
+			value:       "bar",
+			expectedOut: "foo: bar\n",
+			expectedErr: "",
+		},
 		{
 			name:        "preserve inline comment",
 			in:          "foo: bar # my comment\n",
@@ -44,21 +56,146 @@ func TestUpdateYaml(t *testing.T) {
 			expectedOut: "foo:\n    bar: qux\n",
 			expectedErr: "",
 		},
+		{
+			name:        "nested where parents doesn't exist yet",
+			in:          "",
+			path:        []string{"foo", "bar", "baz"},
+			value:       "qux",
+			expectedOut: "foo:\n    bar:\n        baz: qux\n",
+			expectedErr: "",
+		},
+		{
+			name:        "don't rewrite file if value didn't change",
+			in:          "foo:\n  bar: baz\n",
+			path:        []string{"foo", "bar"},
+			value:       "baz",
+			expectedOut: "foo:\n  bar: baz\n",
+			expectedErr: "",
+		},
+
+		// Error cases
+		{
+			name:        "existing document is not a dictionary",
+			in:          "42\n",
+			path:        []string{"foo"},
+			value:       "bar",
+			expectedOut: "42\n",
+			expectedErr: "yaml document is not a dictionary",
+		},
+		{
+			name:        "trying to update a note that is not a scalar",
+			in:          "foo: [1, 2, 3]\n",
+			path:        []string{"foo"},
+			value:       "bar",
+			expectedOut: "foo: [1, 2, 3]\n",
+			expectedErr: "yaml node is not a scalar",
+		},
+		{
+			name:        "not all path elements are dictionaries",
+			in:          "foo:\n  bar: [1, 2, 3]\n",
+			path:        []string{"foo", "bar", "baz"},
+			value:       "qux",
+			expectedOut: "foo:\n  bar: [1, 2, 3]\n",
+			expectedErr: "yaml node in path is not a dictionary",
+		},
 	}
 
 	for _, test := range tests {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
-			out, err := UpdateYaml([]byte(test.in), test.path, test.value)
+			out, actualErr := UpdateYamlValue([]byte(test.in), test.path, test.value)
-			if test.expectedErr != "" {
+			if test.expectedErr == "" {
-				if err == nil {
+				assert.NoError(t, actualErr)
-					t.Errorf("expected error %q but got none", test.expectedErr)
+			} else {
-				}
+				assert.EqualError(t, actualErr, test.expectedErr)
-			} else if err != nil {
-				t.Errorf("unexpected error: %v", err)
-			} else if string(out) != test.expectedOut {
-				t.Errorf("expected %q but got %q", test.expectedOut, string(out))
 			}
+
+			assert.Equal(t, test.expectedOut, string(out))
+		})
+	}
+}
+
+func TestRenameYamlKey(t *testing.T) {
+	tests := []struct {
+		name        string
+		in          string
+		path        []string
+		newKey      string
+		expectedOut string
+		expectedErr string
+	}{
+		{
+			name:        "rename key",
+			in:          "foo: 5\n",
+			path:        []string{"foo"},
+			newKey:      "bar",
+			expectedOut: "bar: 5\n",
+			expectedErr: "",
+		},
+		{
+			name:   "rename key, nested",
+			in:     "foo:\n  bar: 5\n",
+			path:   []string{"foo", "bar"},
+			newKey: "baz",
+			// indentation is not preserved. See https://github.com/go-yaml/yaml/issues/899
+			expectedOut: "foo:\n    baz: 5\n",
+			expectedErr: "",
+		},
+		{
+			name:   "rename non-scalar key",
+			in:     "foo:\n  bar: 5\n",
+			path:   []string{"foo"},
+			newKey: "qux",
+			// indentation is not preserved. See https://github.com/go-yaml/yaml/issues/899
+			expectedOut: "qux:\n    bar: 5\n",
+			expectedErr: "",
+		},
+		{
+			name:        "don't rewrite file if value didn't change",
+			in:          "foo:\n  bar: 5\n",
+			path:        []string{"nonExistingKey"},
+			newKey:      "qux",
+			expectedOut: "foo:\n  bar: 5\n",
+			expectedErr: "",
+		},
+
+		// Error cases
+		{
+			name:        "existing document is not a dictionary",
+			in:          "42\n",
+			path:        []string{"foo"},
+			newKey:      "bar",
+			expectedOut: "42\n",
+			expectedErr: "yaml node in path is not a dictionary",
+		},
+		{
+			name:        "not all path elements are dictionaries",
+			in:          "foo:\n  bar: [1, 2, 3]\n",
+			path:        []string{"foo", "bar", "baz"},
+			newKey:      "qux",
+			expectedOut: "foo:\n  bar: [1, 2, 3]\n",
+			expectedErr: "yaml node in path is not a dictionary",
+		},
+		{
+			name:        "new key exists",
+			in:          "foo: 5\nbar: 7\n",
+			path:        []string{"foo"},
+			newKey:      "bar",
+			expectedOut: "foo: 5\nbar: 7\n",
+			expectedErr: "new key `bar' already exists",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			out, actualErr := RenameYamlKey([]byte(test.in), test.path, test.newKey)
+			if test.expectedErr == "" {
+				assert.NoError(t, actualErr)
+			} else {
+				assert.EqualError(t, actualErr, test.expectedErr)
+			}
+
+			assert.Equal(t, test.expectedOut, string(out))
 		})
 	}
 }