Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ordered-Marshal doesn't give the correct order and in certain cases duplicates the path after using SetPath #450

Closed
heyaWorld opened this issue Oct 19, 2020 · 5 comments · Fixed by #539
Labels
bug Issues describing a bug in go-toml.

Comments

@heyaWorld
Copy link

Describe the bug
NewEncoder(w io.Writer).Order(OrderPreserve).Encode(v interface{}) doesn't return the correct ordered output and in certain cases duplicates the path set by (*Tree).SetPath()

To Reproduce

    config := `
	[[inputs.cpu]]
	    collect_cpu_time = false
	
        [[inputs.apache]]
	     urls = []
	    [inputs.apache.tags]
		foo="bar"

      [[inputs.disk]]`

	// Load config, output is *Tree
	tree, _ := toml.Load(config)
	node := tree.GetPath([]string{"inputs"}).(*toml.Tree)
	for _, key := range node.Keys() {
		tree.SetPath([]string{"inputs", key, "tags", "computer"}, "mycomputerName")
		tree.SetPath([]string{"inputs", key, "tags", "origin"}, "telegraf/apache")
	}
	fmt.Println(tree)
	fmt.Println("******************************\n")

	// Convert to bytes to preserve the order of the original config
	var result bytes.Buffer
	if err := toml.NewEncoder(&result).Order(toml.OrderPreserve).Encode(tree); err != nil {
		fmt.Println(err)
	}
	fmt.Println("************original ordered toml with tags************")
	fmt.Println(string(result.Bytes()))

Expected behavior
[inputs.cpu.tags] should not be duplicate.
"collect_cpu_time = false" is missing under the inputs.cpu table.

Original Ordered output after running the above code:

[inputs]

  [[inputs.cpu]]

    [inputs.cpu.tags]
      computer = "mycomputerName"
      origin = "telegraf/apache"

    [inputs.cpu.tags]
      computer = "mycomputerName"
      origin = "telegraf/apache"

  [[inputs.apache]]
    urls = []

    [inputs.apache.tags]
      foo = "bar"
      computer = "mycomputerName"
      origin = "telegraf/apache"

  [[inputs.disk]]

    [inputs.disk.tags]
      computer = "mycomputerName"
      origin = "telegraf/apache"

Expected ordered output:

`[inputs]

  [[inputs.cpu]]
    collect_cpu_time = false
    [inputs.cpu.tags]
      computer = "mycomputerName"
      origin = "telegraf/apache"

  [[inputs.apache]]
    urls = []

    [inputs.apache.tags]
      foo = "bar"
      computer = "mycomputerName"
      origin = "telegraf/apache"

  [[inputs.disk]]

    [inputs.disk.tags]
      computer = "mycomputerName"
      origin = "telegraf/apache"`

Another example(If we change the config variable in the reproduce step to this):

 config := `
	[[inputs.apache]]
	     urls = []
	     [inputs.apache.tags]
		  foo="bar"

        [[inputs.disk]]

        [[inputs.cpu]]
	     collect_cpu_time = false`

Original Ordered output after running the above code:

[inputs]

  [[inputs.apache]]
    urls = []

    [inputs.apache.tags]
      foo = "bar"
      computer = "mycomputerName"
      origin = "telegraf/apache"

  [[inputs.disk]]

    [inputs.disk.tags]
      computer = "mycomputerName"
      origin = "telegraf/apache"

  [[inputs.cpu]]

    [inputs.cpu.tags]
      computer = "mycomputerName"
      origin = "telegraf/apache"
    collect_cpu_time = false

Expected ordered output:

[inputs]

  [[inputs.apache]]
    urls = []

    [inputs.apache.tags]
      foo = "bar"
      computer = "mycomputerName"
      origin = "telegraf/apache"

  [[inputs.disk]]

    [inputs.disk.tags]
      computer = "mycomputerName"
      origin = "telegraf/apache"

  [[inputs.cpu]]
    collect_cpu_time = false
    [inputs.cpu.tags]
      computer = "mycomputerName"
      origin = "telegraf/apache"

collect_cpu_time = false should come before the [inputs.cpu.tags] Also compared to example 1, there is no duplication in this case.

Versions

  • go-toml: version (v1.8.1)
  • go: version go1.14 windows/amd64
  • operating system: windows

Additional context

@heyaWorld heyaWorld changed the title Ordered-Marshal doesn't give the correct order and in certain cases duplicates the value after using SetPath Ordered-Marshal doesn't give the correct order and in certain cases duplicates the path after using SetPath Oct 19, 2020
nikoksr added a commit to proji-rocks/proji that referenced this issue Nov 25, 2020
There's currently a bug (pelletier/go-toml#450) in the toml library
proji is using and its affecting our package export. In some cases the
name field of a package is missing and sometimes its duplicate. The
problem is backtrackable to the order command which makes it so packages
exports always look the same and in their intended order. Until the fix
is out we'll just take out the order command. Export works fine; just
isn't ordered anymore as originally wanted.
@pelletier pelletier added the bug Issues describing a bug in go-toml. label Jan 7, 2021
@pelletier
Copy link
Owner

Thank you for reporting this! Definitely seems wrong.

@heyaWorld
Copy link
Author

Hey, I think I have a fix for this, I am just refactoring the code and testing it once more. My code will also fix this issue:

#445

@fiws
Copy link

fiws commented Apr 9, 2021

here is a much simpler example that reproduces the duplication bug: https://play.golang.org/p/FN1Jc33D4PF

not sure if this is the same issue. happy to open another one if it's not the case.

The problem seems to be related to the string map in my example. it works if i remove replace that with a string for example.

fiws added a commit to minepkg/minepkg that referenced this issue Apr 9, 2021
Encoding has serious problems that cause keys beeing duplicated
or overwritten:
pelletier/go-toml#450
@pelletier
Copy link
Owner

pelletier commented Apr 11, 2021

@fiws thank you for the simplified repro! I likely won't try to fix it in v1 given all my time is focused on v2. Fortunately toml.OrderPreserve is probably not going to be ported over given that v2's default behavior is to respect the struct field order.

Felixoid added a commit to Felixoid/go-toml that referenced this issue May 11, 2021
@Felixoid
Copy link
Contributor

Hey dear @heyaWorld and @fiws, can you try if #539 fixes your problems?

pelletier pushed a commit that referenced this issue May 11, 2021
fiws added a commit to minepkg/minepkg that referenced this issue May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues describing a bug in go-toml.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants