From e93a09cda6299ab46f97e2a2d5a226af2352e66d Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Tue, 3 Oct 2023 19:33:53 +0200 Subject: [PATCH 1/4] Automatically format nested objects in YAML files --- internal/builder/dynamic_mappings.go | 3 +- internal/formatter/formatter.go | 2 +- internal/formatter/yaml_formatter.go | 60 +++++++++++++++- internal/formatter/yaml_formatter_test.go | 83 +++++++++++++++++++++++ internal/packages/changelog/yaml.go | 3 +- 5 files changed, 145 insertions(+), 6 deletions(-) create mode 100644 internal/formatter/yaml_formatter_test.go diff --git a/internal/builder/dynamic_mappings.go b/internal/builder/dynamic_mappings.go index 7f0c458f80..da0026e505 100644 --- a/internal/builder/dynamic_mappings.go +++ b/internal/builder/dynamic_mappings.go @@ -240,7 +240,8 @@ func formatResult(result interface{}) ([]byte, error) { if err != nil { return nil, errors.New("failed to encode") } - d, _, err = formatter.YAMLFormatter(d) + yamlFormatter := &formatter.YAMLFormatter{} + d, _, err = yamlFormatter.Format(d) if err != nil { return nil, errors.New("failed to format") } diff --git a/internal/formatter/formatter.go b/internal/formatter/formatter.go index eda77e11bc..ed16e86583 100644 --- a/internal/formatter/formatter.go +++ b/internal/formatter/formatter.go @@ -21,7 +21,7 @@ func newFormatter(specVersion semver.Version, ext string) formatter { case ".json": return JSONFormatterBuilder(specVersion).Format case ".yaml", ".yml": - return YAMLFormatter + return NewYAMLFormatter(specVersion).Format default: return nil } diff --git a/internal/formatter/yaml_formatter.go b/internal/formatter/yaml_formatter.go index 7fbcca9368..f6846b16cb 100644 --- a/internal/formatter/yaml_formatter.go +++ b/internal/formatter/yaml_formatter.go @@ -7,13 +7,24 @@ package formatter import ( "bytes" "fmt" + "strings" + "github.com/Masterminds/semver/v3" "gopkg.in/yaml.v3" ) -// YAMLFormatter function is responsible for formatting the given YAML input. -// The function is exposed, so it can be used by other internal packages. -func YAMLFormatter(content []byte) ([]byte, bool, error) { +// YAMLFormatter is responsible for formatting the given YAML input. +type YAMLFormatter struct { + specVersion semver.Version +} + +func NewYAMLFormatter(specVersion semver.Version) *YAMLFormatter { + return &YAMLFormatter{ + specVersion: specVersion, + } +} + +func (f *YAMLFormatter) Format(content []byte) ([]byte, bool, error) { // yaml.Unmarshal() requires `yaml.Node` to be passed instead of generic `interface{}`. // Otherwise it can't detect any comments and fields are considered as normal map. var node yaml.Node @@ -22,6 +33,10 @@ func YAMLFormatter(content []byte) ([]byte, bool, error) { return nil, false, fmt.Errorf("unmarshalling YAML file failed: %w", err) } + if !f.specVersion.LessThan(semver.MustParse("3.0.0")) { + extendNestedObjects(&node) + } + var b bytes.Buffer encoder := yaml.NewEncoder(&b) encoder.SetIndent(2) @@ -39,3 +54,42 @@ func YAMLFormatter(content []byte) ([]byte, bool, error) { return formatted, string(content) == string(formatted), nil } + +func extendNestedObjects(node *yaml.Node) { + if node.Kind == yaml.MappingNode { + extendMapNode(node) + } + for _, child := range node.Content { + extendNestedObjects(child) + } +} + +func extendMapNode(node *yaml.Node) { + for i := 0; i < len(node.Content); i += 2 { + key := node.Content[i] + value := node.Content[i+1] + + base, rest, found := strings.Cut(key.Value, ".") + + // Insert nested objects only when the key has a dot, and is not quoted. + if found && key.Style == 0 { + newKey := *key + newKey.Value = base + newKey.FootComment = "" + newKey.HeadComment = "" + newKey.LineComment = "" + newChildKey := *key + newChildKey.Value = rest + newNode := *node + newNode.Content = []*yaml.Node{ + &newChildKey, + value, + } + + node.Content[i] = &newKey + node.Content[i+1] = &newNode + } + + extendNestedObjects(node.Content[i+1]) + } +} diff --git a/internal/formatter/yaml_formatter_test.go b/internal/formatter/yaml_formatter_test.go new file mode 100644 index 0000000000..7f86c7b851 --- /dev/null +++ b/internal/formatter/yaml_formatter_test.go @@ -0,0 +1,83 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License; +// you may not use this file except in compliance with the Elastic License. + +package formatter + +import ( + "strconv" + "testing" + + "github.com/Masterminds/semver/v3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestYAMLFormatterNestedObjects(t *testing.T) { + cases := []struct { + doc string + expected string + }{ + { + doc: `foo.bar: 3`, + expected: `foo: + bar: 3 +`, + }, + { + doc: `foo.bar.baz: 3`, + expected: `foo: + bar: + baz: 3 +`, + }, + { + doc: `foo: + bar.baz: 3`, + expected: `foo: + bar: + baz: 3 +`, + }, + { + doc: `foo.bar.baz: 3 +a.b.c: 42`, + expected: `foo: + bar: + baz: 3 +a: + b: + c: 42 +`, + }, + { + doc: `foo.bar.baz: 3 # baz +# Mistery of life and everything else. +a.b.c: 42`, + expected: `foo: + bar: + baz: 3 # baz +a: + b: + # Mistery of life and everything else. + c: 42 +`, + }, + { + doc: `"foo.bar.baz": 3`, + expected: "\"foo.bar.baz\": 3\n", + }, + } + + sv := semver.MustParse("3.0.0") + formatter := NewYAMLFormatter(*sv).Format + + for i, c := range cases { + t.Run(strconv.Itoa(i), func(t *testing.T) { + result, _, err := formatter([]byte(c.doc)) + require.NoError(t, err) + assert.Equal(t, c.expected, string(result)) + }) + } + +} diff --git a/internal/packages/changelog/yaml.go b/internal/packages/changelog/yaml.go index 31f0ec620d..53a8eb36b0 100644 --- a/internal/packages/changelog/yaml.go +++ b/internal/packages/changelog/yaml.go @@ -125,7 +125,8 @@ func formatResult(result interface{}) ([]byte, error) { if err != nil { return nil, errors.New("failed to encode") } - d, _, err = formatter.YAMLFormatter(d) + yamlFormatter := &formatter.YAMLFormatter{} + d, _, err = yamlFormatter.Format(d) if err != nil { return nil, errors.New("failed to format") } From ffed989ff505a5c5f560c867be53ca1cbdbad2db Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Tue, 3 Oct 2023 19:59:21 +0200 Subject: [PATCH 2/4] Update related docs --- docs/howto/update_major_package_spec.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/howto/update_major_package_spec.md b/docs/howto/update_major_package_spec.md index e69ff7e40e..039cfb9cea 100644 --- a/docs/howto/update_major_package_spec.md +++ b/docs/howto/update_major_package_spec.md @@ -42,8 +42,10 @@ setting was included with and withoud dotted notation. This is commonly found in `conditions` or in `elasticsearch` settings. -To solve this, please use nested dotations. So if for example your package has -something like the following: +`elastic-package` `check` and `format` subcommands will try to fix this +automatically. If you are still finding this issue, you will need to fix it +manually. For that, please use nested dotations. So if for example your package +has something like the following: ``` conditions: elastic.subscription: basic From 648d8e003690ca976668c35573a79cfb27c810d4 Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Tue, 3 Oct 2023 23:53:02 +0200 Subject: [PATCH 3/4] More tests and comments in code --- internal/formatter/yaml_formatter.go | 8 ++++++ internal/formatter/yaml_formatter_test.go | 33 +++++++++++++++++++---- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/internal/formatter/yaml_formatter.go b/internal/formatter/yaml_formatter.go index f6846b16cb..2594dbb503 100644 --- a/internal/formatter/yaml_formatter.go +++ b/internal/formatter/yaml_formatter.go @@ -73,23 +73,31 @@ func extendMapNode(node *yaml.Node) { // Insert nested objects only when the key has a dot, and is not quoted. if found && key.Style == 0 { + // Copy key to create the new parent with the first part of the path. newKey := *key newKey.Value = base newKey.FootComment = "" newKey.HeadComment = "" newKey.LineComment = "" + + // Copy key also to create the key of the child value. newChildKey := *key newChildKey.Value = rest + + // Copy the parent node to create the nested object, that contains the new + // child key and the original value. newNode := *node newNode.Content = []*yaml.Node{ &newChildKey, value, } + // Replace current key and value. node.Content[i] = &newKey node.Content[i+1] = &newNode } + // Recurse on the current value. extendNestedObjects(node.Content[i+1]) } } diff --git a/internal/formatter/yaml_formatter_test.go b/internal/formatter/yaml_formatter_test.go index 7f86c7b851..abbe6d4381 100644 --- a/internal/formatter/yaml_formatter_test.go +++ b/internal/formatter/yaml_formatter_test.go @@ -5,7 +5,6 @@ package formatter import ( - "strconv" "testing" "github.com/Masterminds/semver/v3" @@ -15,23 +14,27 @@ import ( func TestYAMLFormatterNestedObjects(t *testing.T) { cases := []struct { + title string doc string expected string }{ { - doc: `foo.bar: 3`, + title: "one-level nested setting", + doc: `foo.bar: 3`, expected: `foo: bar: 3 `, }, { - doc: `foo.bar.baz: 3`, + title: "two-level nested setting", + doc: `foo.bar.baz: 3`, expected: `foo: bar: baz: 3 `, }, { + title: "nested setting at second level", doc: `foo: bar.baz: 3`, expected: `foo: @@ -40,6 +43,7 @@ func TestYAMLFormatterNestedObjects(t *testing.T) { `, }, { + title: "two two-level nested settings", doc: `foo.bar.baz: 3 a.b.c: 42`, expected: `foo: @@ -51,6 +55,7 @@ a: `, }, { + title: "keep comments with the leaf value", doc: `foo.bar.baz: 3 # baz # Mistery of life and everything else. a.b.c: 42`, @@ -64,16 +69,34 @@ a: `, }, { + title: "keep double-quoted keys", doc: `"foo.bar.baz": 3`, expected: "\"foo.bar.baz\": 3\n", }, + { + title: "keep single-quoted keys", + doc: `"foo.bar.baz": 3`, + expected: "\"foo.bar.baz\": 3\n", + }, + { + title: "array of maps", + doc: `foo: + - foo.bar: 1 + - foo.bar: 2`, + expected: `foo: + - foo: + bar: 1 + - foo: + bar: 2 +`, + }, } sv := semver.MustParse("3.0.0") formatter := NewYAMLFormatter(*sv).Format - for i, c := range cases { - t.Run(strconv.Itoa(i), func(t *testing.T) { + for _, c := range cases { + t.Run(c.title, func(t *testing.T) { result, _, err := formatter([]byte(c.doc)) require.NoError(t, err) assert.Equal(t, c.expected, string(result)) From 31b4b604499a126b39a67ecd22dcc5bdff50264e Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Wed, 4 Oct 2023 00:18:29 +0200 Subject: [PATCH 4/4] Keep a single key when multiple objects have the same one --- internal/formatter/yaml_formatter.go | 25 +++++++++++++++++++++++ internal/formatter/yaml_formatter_test.go | 12 +++++++++++ 2 files changed, 37 insertions(+) diff --git a/internal/formatter/yaml_formatter.go b/internal/formatter/yaml_formatter.go index 2594dbb503..28e3d8ab23 100644 --- a/internal/formatter/yaml_formatter.go +++ b/internal/formatter/yaml_formatter.go @@ -100,4 +100,29 @@ func extendMapNode(node *yaml.Node) { // Recurse on the current value. extendNestedObjects(node.Content[i+1]) } + + mergeNodes(node) +} + +// mergeNodes merges the contents of keys with the same name. +func mergeNodes(node *yaml.Node) { + keys := make(map[string]*yaml.Node) + k := 0 + for i := 0; i < len(node.Content); i += 2 { + key := node.Content[i] + value := node.Content[i+1] + + merged, found := keys[key.Value] + if !found { + keys[key.Value] = value + node.Content[k] = key + node.Content[k+1] = value + k += 2 + continue + } + + merged.Content = append(merged.Content, value.Content...) + } + + node.Content = node.Content[:k] } diff --git a/internal/formatter/yaml_formatter_test.go b/internal/formatter/yaml_formatter_test.go index abbe6d4381..d714e2c921 100644 --- a/internal/formatter/yaml_formatter_test.go +++ b/internal/formatter/yaml_formatter_test.go @@ -88,6 +88,18 @@ a: bar: 1 - foo: bar: 2 +`, + }, + { + title: "merge keys", + doc: `es.something: true +es.other.thing: false +es.other.level: 13`, + expected: `es: + something: true + other: + thing: false + level: 13 `, }, }