goccy / go-yaml

YAML support for the Go language
MIT License
1.12k stars 129 forks source link

replacing values with a multi-line string value provides broken yaml format. #447

Open mandelsoft opened 4 months ago

mandelsoft commented 4 months ago

Describe the bug

The multi line string is replaced, but the indentation of the string lines is wrong.

To Reproduce

use yaml document:

data:
  value1: orig1
  value2: orig2

replace $.data.value1 by

line1
line2

The result will be

data:
  value1: |-
  line1
  line2
  value2: orig2

instead of

data:
  value1: |-
    line1
    line2
  value2: orig2

The data is parsed with parser.ParseBytes(data, 0). The replace operation is

file, err := parser.ParseBytes(value, 0)
if err != nil {
    return err
}
p, err := yaml.PathString("$.data.value1")
if err != nil {
    return err
}
return p.ReplaceWithFile(doc, file)

Expected behavior provide correct multi-line result

Screenshots If applicable, add screenshots to help explain your problem.

Version Variables

Additional context

The ast nodes provide some strange column values for string value. Especially there is no column info for follow-up value lines. Therefore, the String() method just uses the original string formatting from the target value, which will for sure be wrong in most cases. When replacing a value, only the column value is adapted for the new value, but this cannot be used to serialize follow-up lines.

Skarlso commented 4 months ago

https://github.com/goccy/go-yaml/issues/292

Also reported two years ago without any resolution. :(

Skarlso commented 4 months ago

I'm attempting to see if I can fix this.

Skarlso commented 4 months ago

Reproduced the issue in path_test.go with:

        {
            path: "$.data.value1",
            dst: `
data:
  value1: orig1
  value2: orig2
`,
            src: `
line1
line2
`,
            expected: `
data:
  value1: |-
    line1
    line2
  value2: orig2
`,
        },

I'll attempt to fix the problem.

Skarlso commented 4 months ago

The AST node replace doesn't care how deep it is so it plain does a replace on the node value resulting in a YAML that doesn't work.

Skarlso commented 4 months ago

Tomorrow I'll try to fix this. I have an idea of tracking the deepness recursively for StringNodes for multi-line text. let's see..

Skarlso commented 4 months ago

so I figured out that the library just expects you to pass in the value you require including the right indentation level.

        {
            path: "$.data.value1.orig1",
            dst: `
data:
  value1:
    orig1: orig1.1
  value2: orig2
`,
            src: `|-
      line1
      line2
`,
            expected: `
data:
  value1:
    orig1: |-
      line1
      line2
  value2: orig2
`,
        },

Passes as a test case.

dee0 commented 4 months ago

Nice find @Skarlso Can something similar be done when the new value of orig1 is an object instead of a multiline string? I ask because when I stumbled on this problem I was seeing the same problem for an object.

Skarlso commented 4 months ago

Do you have an example?

Skarlso commented 4 months ago

I tried replacing objects, but the marshalling is all over the place. There is virtually no consistency in indentation whatsoever. I can't find a pattern or anything that would make it work. :(

Skarlso commented 4 months ago

Okay, this now worked finally:

        {
            path: "$.data.value1.orig1",
            dst: `
data:
  value1:
    orig1: orig1.1
  value2: orig2
`,
            src: `
orig1:
  newValue:
    - value1
    - value2
`,
            expected: `
data:
  value1:
    orig1:
      orig1:
        newValue:
          - value1
          - value2
  value2: orig2
`,
        },