From ab6b2707a132405e07fe89b72a9f70ee927dec56 Mon Sep 17 00:00:00 2001 From: Zaq? Wiedmann Date: Tue, 17 May 2022 19:30:09 -0700 Subject: [PATCH 1/3] fix: gate transformers on valid non-nil destinations This builds on #203 which attempted to provide a more flexible gating to running transformers. However upon testing #203 in my own environment, I ran into the first panic listed below. There are a variety of errors that can happen when trying to run reflection on zero values: 2 just from my testing of this PR ``` panic: reflect: call of reflect.Value.Type on zero Value panic: reflect: call of reflect.Value.FieldByName on zero Value ``` The panic specifically calls out zero values, but it's actual a more specific set of values which is covered by `reflect.IsValid`. I attempted to replace the check with `reflect.IsZero`, which ends up being too restrictive and breaks existing tests. I also attempted to solely use `reflect.IsZero` which is not restrictive enough and cause the later panic above in the tests. Thus I arrived on the combination in this PR which seems to strike the "right" balance. --- merge.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/merge.go b/merge.go index 8c2a8fc..8b4e2f4 100644 --- a/merge.go +++ b/merge.go @@ -79,7 +79,7 @@ func deepMerge(dst, src reflect.Value, visited map[uintptr]*visit, depth int, co visited[h] = &visit{addr, typ, seen} } - if config.Transformers != nil && !isEmptyValue(dst) { + if config.Transformers != nil && !isReflectNil(dst) && dst.IsValid() { if fn := config.Transformers.Transformer(dst.Type()); fn != nil { err = fn(dst, src) return From 4bed36ec55e461d22a3a5d17c1525693d9587cc8 Mon Sep 17 00:00:00 2001 From: Zaq? Wiedmann Date: Tue, 17 May 2022 19:59:24 -0700 Subject: [PATCH 2/3] add test for keeping zero values with transformer --- pr211_test.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 pr211_test.go diff --git a/pr211_test.go b/pr211_test.go new file mode 100644 index 0000000..09b87e9 --- /dev/null +++ b/pr211_test.go @@ -0,0 +1,36 @@ +package mergo_test + +import ( + "reflect" + "testing" + + "github.com/imdario/mergo" +) + +func TestMergeWithTransformerZeroValue(t *testing.T) { + // This test specifically tests that a transformer can be used to + // prevent overwriting a zero value (in this case a bool). This would fail prior to #211 + type fooWithBoolPtr struct { + b *bool + } + var Bool = func(b bool) *bool { return &b } + a := fooWithBoolPtr{b: Bool(false)} + b := fooWithBoolPtr{b: Bool(true)} + + if err := mergo.Merge(&a, &b, mergo.WithTransformers(&transformer{ + m: map[reflect.Type]func(dst, src reflect.Value) error{ + reflect.TypeOf(Bool(false)): func(dst, src reflect.Value) error { + if dst.CanSet() && dst.IsNil() { + dst.Set(src) + } + return nil + }, + }, + })); err != nil { + t.Error(err) + } + + if *a.b != false { + t.Errorf("b not merged in properly: a.b(%v) != expected(%v)", a.b, false) + } +} From 8109749b3ef7378913953135f986c307d5f159a9 Mon Sep 17 00:00:00 2001 From: Zaq? Wiedmann Date: Tue, 17 May 2022 21:14:57 -0700 Subject: [PATCH 3/3] add test for deepMerge panic --- pr211_2_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 pr211_2_test.go diff --git a/pr211_2_test.go b/pr211_2_test.go new file mode 100644 index 0000000..df4c575 --- /dev/null +++ b/pr211_2_test.go @@ -0,0 +1,25 @@ +package mergo + +import ( + "reflect" + "testing" + "time" +) + +type transformer struct { +} + +func (s *transformer) Transformer(t reflect.Type) func(dst, src reflect.Value) error { + return nil +} + +func Test_deepMergeTransformerInvalidDestination(t *testing.T) { + foo := time.Time{} + src := reflect.ValueOf(foo) + deepMerge(reflect.Value{}, src, make(map[uintptr]*visit), 0, &Config{ + Transformers: &transformer{}, + }) + // this test is intentionally not asserting on anything, it's sole + // purpose to verify deepMerge doesn't panic when a transformer is + // passed and the destination is invalid. +}