Merge is broken for structs with slices

[2opremio]

At some point after 6633656 (the version currently vendored by kubernetes, which works fine), merging broke for structs containing byteslices.

Here's a simplificed repro

package main

import (
        "fmt"
        "github.com/imdario/mergo"
)

type Foo struct {
        Str    string
        Bslice []byte
}

func main() {
        dest := Foo{Str: "a"}

        toMerge := Foo{
                Str:    "b",
                Bslice: []byte{1, 2},
        }

        mergo.Merge(&dest, toMerge)

        fmt.Println(dest)
        // Should print
        // {"b" [1,2]}
        // However, it prints
        // {"a" [1,2]}
}

[2opremio]

For context on the importance of this bug, it completely breaks the configuration merging in kubernetes.

[2opremio]

git bisect reveals that d304790 introduced the problem

d304790b2ed594794496464fadd89d2bb266600a is the first bad commit
commit d304790b2ed594794496464fadd89d2bb266600a
Author: Dario Castañé <[email protected]>
Date:   Mon Apr 6 23:32:18 2015 +0200

    Merge working as designed (don't override non zero values on destination).

:100644 100644 f565137df7aafc5b43848d8fc86872811c157d2c f2d0c387c2c24c10a2522576ee46ef8d3602115b M      merge.go
:100644 100644 220b9357258d796df2ab6fd08acabdfaa755ba40 877401c9edb7c3659b38398ed91dbec14f8dc763 M      mergo_test.go

Which suggests that the change was intentional but unfortunately completely breaks kubernetes' usage of Merge.

[darccio]

I'm going to check this as soon as possible, although I'm sorry that won't be today.

The change you point was intentional. Here is why:

Mergo is intended to assign only zero value fields on destination with source value. Since April 6th it works like this. Before it didn't work properly, causing some random overwrites. After some issues and PRs I found it didn't merge as I designed it. Thanks to #8 overwriting functions were added and the wrong behavior was clearly detected.

If you were using Mergo before April 6th 2015, please check your project works as intended after updating your local copy with go get -u github.com/imdario/mergo. I apologize for any issue caused by its previous behavior and any future bug that Mergo could cause (I hope it won't!) in existing projects after the change (release 0.2.0).

[darccio]

First of all, sorry. Second, I will take care of this this next week.

[mandrean]

@imdario Status?

[darccio]

I finally got time to read this issue carefully (as they are usually tricky because Mergo isn't so simple as it looks!).

I wrote a test to offer a solution for this:

package mergo

import (
        "testing"
)

type Foo struct {
        Str    string
        Bslice []byte
}

func TestIssue33Merge(t *testing.T) {
        dest := Foo{Str: "a"}
        toMerge := Foo{
                Str:    "b",
                Bslice: []byte{1, 2},
        }
        if err := Merge(&dest, toMerge); err != nil {
                t.Errorf("Error while merging: %s", err)
        }
        // Merge doesn't overwrite an attribute if in destination it doesn't have a zero value.
        // In this case, Str isn't a zero value string.
        if dest.Str != "a" {
                t.Errorf("dest.Str should have not been override as it has a non-zero value: dest.Str(%v) != 'a'", dest.Str)
        }
        // If we want to override, we must use MergeWithOverwrite or Merge using WithOverride.
        if err := Merge(&dest, toMerge, WithOverride); err != nil {
                t.Errorf("Error while merging: %s", err)
        }
        if dest.Str != toMerge.Str {
                t.Errorf("dest.Str should have been override: dest.Str(%v) != toMerge.Str(%v)", dest.Str, toMerge.Str)
        }
}

I hope @2opremio and Kubernetes' crew can use the last release available (0.3.0 as we speak).

[2opremio]

I hope @2opremio and Kubernetes' crew can use the last release available (0.3.0 as we speak).

I since moved to other endeavors, but thanks for the effort!