From d3e61bc8258ecf5b04064ccdff8b848fff219c59 Mon Sep 17 00:00:00 2001 From: James Palawaga Date: Mon, 3 Oct 2022 11:58:28 -0700 Subject: [PATCH 1/3] Fallback to stringer if a struct is 100% ignored --- hashstructure.go | 46 +++++++++++++--------- hashstructure_test.go | 88 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 17 deletions(-) diff --git a/hashstructure.go b/hashstructure.go index 3dc0eb7..9d102ce 100644 --- a/hashstructure.go +++ b/hashstructure.go @@ -72,29 +72,28 @@ const ( // // Notes on the value: // -// * Unexported fields on structs are ignored and do not affect the +// - Unexported fields on structs are ignored and do not affect the // hash value. // -// * Adding an exported field to a struct with the zero value will change +// - Adding an exported field to a struct with the zero value will change // the hash value. // // For structs, the hashing can be controlled using tags. For example: // -// struct { -// Name string -// UUID string `hash:"ignore"` -// } +// struct { +// Name string +// UUID string `hash:"ignore"` +// } // // The available tag values are: // -// * "ignore" or "-" - The field will be ignored and not affect the hash code. +// - "ignore" or "-" - The field will be ignored and not affect the hash code. // -// * "set" - The field will be treated as a set, where ordering doesn't -// affect the hash code. This only works for slices. -// -// * "string" - The field will be hashed as a string, only works when the -// field implements fmt.Stringer +// - "set" - The field will be treated as a set, where ordering doesn't +// affect the hash code. This only works for slices. // +// - "string" - The field will be hashed as a string, only works when the +// field implements fmt.Stringer func Hash(v interface{}, format Format, opts *HashOptions) (uint64, error) { // Validate our format if format <= formatInvalid || format >= formatMax { @@ -307,23 +306,27 @@ func (w *walker) visit(v reflect.Value, opts *visitOpts) (uint64, error) { } l := v.NumField() + unhashedfields := 0 for i := 0; i < l; i++ { if innerV := v.Field(i); v.CanSet() || t.Field(i).Name != "_" { var f visitFlag fieldType := t.Field(i) if fieldType.PkgPath != "" { + unhashedfields++ // Unexported continue } tag := fieldType.Tag.Get(w.tag) if tag == "ignore" || tag == "-" { + unhashedfields++ // Ignore this field continue } if w.ignorezerovalue { if innerV.IsZero() { + unhashedfields++ continue } } @@ -348,6 +351,7 @@ func (w *walker) visit(v reflect.Value, opts *visitOpts) (uint64, error) { return 0, err } if !incl { + unhashedfields++ continue } } @@ -380,6 +384,14 @@ func (w *walker) visit(v reflect.Value, opts *visitOpts) (uint64, error) { h = hashFinishUnordered(w.h, h) } } + // no fields involved in the hash! try and string instead. + if unhashedfields == l { + if impl, ok := v.Interface().(fmt.Stringer); ok { + w.h.Reset() + _, err := w.h.Write([]byte(impl.String())) + return w.h.Sum64(), err + } + } return h, nil @@ -453,11 +465,11 @@ func hashUpdateUnordered(a, b uint64) uint64 { // hashUpdateUnordered can effectively cancel out a previous change to the hash // result if the same hash value appears later on. For example, consider: // -// hashUpdateUnordered(hashUpdateUnordered("A", "B"), hashUpdateUnordered("A", "C")) = -// H("A") ^ H("B")) ^ (H("A") ^ H("C")) = -// (H("A") ^ H("A")) ^ (H("B") ^ H(C)) = -// H(B) ^ H(C) = -// hashUpdateUnordered(hashUpdateUnordered("Z", "B"), hashUpdateUnordered("Z", "C")) +// hashUpdateUnordered(hashUpdateUnordered("A", "B"), hashUpdateUnordered("A", "C")) = +// H("A") ^ H("B")) ^ (H("A") ^ H("C")) = +// (H("A") ^ H("A")) ^ (H("B") ^ H(C)) = +// H(B) ^ H(C) = +// hashUpdateUnordered(hashUpdateUnordered("Z", "B"), hashUpdateUnordered("Z", "C")) // // hashFinishUnordered "hardens" the result, so that encountering partially // overlapping input data later on in a different context won't cancel out. diff --git a/hashstructure_test.go b/hashstructure_test.go index 7b0034a..650e353 100644 --- a/hashstructure_test.go +++ b/hashstructure_test.go @@ -729,3 +729,91 @@ func (t *testHashablePointer) Hash() (uint64, error) { return 100, nil } + +type Unexported struct { + n int +} + +func (u Unexported) String() string { + return fmt.Sprintf("%d", u.n) +} + +func TestHash_unexported_stringable(t *testing.T) { + cases := []struct { + One, Two interface{} + Match bool + Err string + }{ + { + Unexported{n: 1}, + Unexported{n: 1}, + true, + "", + }, + { + Unexported{n: 1}, + Unexported{n: 2}, + false, + "", + }, + { + []interface{}{Unexported{n: 1}}, + []interface{}{Unexported{n: 1}}, + true, + "", + }, + { + []interface{}{Unexported{n: 1}}, + []interface{}{Unexported{n: 2}}, + false, + "", + }, + { + map[string]interface{}{"v": Unexported{n: 1}}, + map[string]interface{}{"v": Unexported{n: 1}}, + true, + "", + }, + { + map[string]interface{}{"v": Unexported{n: 1}}, + map[string]interface{}{"v": Unexported{n: 2}}, + false, + "", + }, + } + + for i, tc := range cases { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + one, err := Hash(tc.One, testFormat, &HashOptions{}) + if tc.Err != "" { + if err == nil { + t.Fatal("expected error") + } + + if !strings.Contains(err.Error(), tc.Err) { + t.Fatalf("expected error to contain %q, got: %s", tc.Err, err) + } + + return + } + if err != nil { + t.Fatalf("Failed to hash %#v: %s", tc.One, err) + } + + two, err := Hash(tc.Two, testFormat, nil) + if err != nil { + t.Fatalf("Failed to hash %#v: %s", tc.Two, err) + } + + // Zero is always wrong + if one == 0 { + t.Fatalf("zero hash: %#v", tc.One) + } + + // Compare + if (one == two) != tc.Match { + t.Fatalf("bad, expected: %#v\n\n%#v\n\n%#v", tc.Match, tc.One, tc.Two) + } + }) + } +} From 84126762b045413e4aa479f4439a8e001c2509ea Mon Sep 17 00:00:00 2001 From: James Palawaga Date: Mon, 3 Oct 2022 12:42:46 -0700 Subject: [PATCH 2/3] put the new functionality behind an opt --- hashstructure.go | 43 ++++++++++++++++++++++++++----------------- hashstructure_test.go | 6 +++--- 2 files changed, 29 insertions(+), 20 deletions(-) diff --git a/hashstructure.go b/hashstructure.go index 9d102ce..f0e4de9 100644 --- a/hashstructure.go +++ b/hashstructure.go @@ -37,6 +37,10 @@ type HashOptions struct { // precedence (meaning that if the type doesn't implement fmt.Stringer, we // panic) UseStringer bool + + // StringIgnoredStructs will attempt to .String() a struct if all of the + // members of the struct are ignored for the purposes of hashing. + StringIgnoredStructs bool } // Format specifies the hashing process used. Different formats typically @@ -116,25 +120,27 @@ func Hash(v interface{}, format Format, opts *HashOptions) (uint64, error) { // Create our walker and walk the structure w := &walker{ - format: format, - h: opts.Hasher, - tag: opts.TagName, - zeronil: opts.ZeroNil, - ignorezerovalue: opts.IgnoreZeroValue, - sets: opts.SlicesAsSets, - stringer: opts.UseStringer, + format: format, + h: opts.Hasher, + tag: opts.TagName, + zeronil: opts.ZeroNil, + ignorezerovalue: opts.IgnoreZeroValue, + sets: opts.SlicesAsSets, + stringer: opts.UseStringer, + stringignoredstructs: opts.StringIgnoredStructs, } return w.visit(reflect.ValueOf(v), nil) } type walker struct { - format Format - h hash.Hash64 - tag string - zeronil bool - ignorezerovalue bool - sets bool - stringer bool + format Format + h hash.Hash64 + tag string + zeronil bool + ignorezerovalue bool + sets bool + stringer bool + stringignoredstructs bool } type visitOpts struct { @@ -385,11 +391,14 @@ func (w *walker) visit(v reflect.Value, opts *visitOpts) (uint64, error) { } } // no fields involved in the hash! try and string instead. - if unhashedfields == l { - if impl, ok := v.Interface().(fmt.Stringer); ok { + if unhashedfields == l && w.stringignoredstructs { + if impl, ok := parent.(fmt.Stringer); ok { w.h.Reset() _, err := w.h.Write([]byte(impl.String())) - return w.h.Sum64(), err + if err != nil { + return 0, err + } + return w.h.Sum64(), nil } } diff --git a/hashstructure_test.go b/hashstructure_test.go index 650e353..f69b319 100644 --- a/hashstructure_test.go +++ b/hashstructure_test.go @@ -738,7 +738,7 @@ func (u Unexported) String() string { return fmt.Sprintf("%d", u.n) } -func TestHash_unexported_stringable(t *testing.T) { +func TestHash_StringIgnoredStructs(t *testing.T) { cases := []struct { One, Two interface{} Match bool @@ -784,7 +784,7 @@ func TestHash_unexported_stringable(t *testing.T) { for i, tc := range cases { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - one, err := Hash(tc.One, testFormat, &HashOptions{}) + one, err := Hash(tc.One, testFormat, &HashOptions{StringIgnoredStructs: true}) if tc.Err != "" { if err == nil { t.Fatal("expected error") @@ -800,7 +800,7 @@ func TestHash_unexported_stringable(t *testing.T) { t.Fatalf("Failed to hash %#v: %s", tc.One, err) } - two, err := Hash(tc.Two, testFormat, nil) + two, err := Hash(tc.Two, testFormat, &HashOptions{StringIgnoredStructs: true}) if err != nil { t.Fatalf("Failed to hash %#v: %s", tc.Two, err) } From eb8203e2cbc0d7fc83744f02243a82df058161ab Mon Sep 17 00:00:00 2001 From: James Palawaga Date: Mon, 3 Oct 2022 12:58:02 -0700 Subject: [PATCH 3/3] Make use of binaryencoder interface too --- hashstructure.go | 62 +++++++++++++++++++++-------------- hashstructure_test.go | 76 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 97 insertions(+), 41 deletions(-) diff --git a/hashstructure.go b/hashstructure.go index f0e4de9..88a410e 100644 --- a/hashstructure.go +++ b/hashstructure.go @@ -1,6 +1,7 @@ package hashstructure import ( + "encoding" "encoding/binary" "fmt" "hash" @@ -38,9 +39,10 @@ type HashOptions struct { // panic) UseStringer bool - // StringIgnoredStructs will attempt to .String() a struct if all of the - // members of the struct are ignored for the purposes of hashing. - StringIgnoredStructs bool + // UnhashedStructFallback will attempt to make use of the BinaryEncoder and + // Stringer interfaces (in that order) to hash structs that contain no + // exported fields. + UnhashedStructFallback bool } // Format specifies the hashing process used. Different formats typically @@ -120,27 +122,27 @@ func Hash(v interface{}, format Format, opts *HashOptions) (uint64, error) { // Create our walker and walk the structure w := &walker{ - format: format, - h: opts.Hasher, - tag: opts.TagName, - zeronil: opts.ZeroNil, - ignorezerovalue: opts.IgnoreZeroValue, - sets: opts.SlicesAsSets, - stringer: opts.UseStringer, - stringignoredstructs: opts.StringIgnoredStructs, + format: format, + h: opts.Hasher, + tag: opts.TagName, + zeronil: opts.ZeroNil, + ignorezerovalue: opts.IgnoreZeroValue, + sets: opts.SlicesAsSets, + stringer: opts.UseStringer, + unhashedstructfallback: opts.UnhashedStructFallback, } return w.visit(reflect.ValueOf(v), nil) } type walker struct { - format Format - h hash.Hash64 - tag string - zeronil bool - ignorezerovalue bool - sets bool - stringer bool - stringignoredstructs bool + format Format + h hash.Hash64 + tag string + zeronil bool + ignorezerovalue bool + sets bool + stringer bool + unhashedstructfallback bool } type visitOpts struct { @@ -390,16 +392,26 @@ func (w *walker) visit(v reflect.Value, opts *visitOpts) (uint64, error) { h = hashFinishUnordered(w.h, h) } } - // no fields involved in the hash! try and string instead. - if unhashedfields == l && w.stringignoredstructs { - if impl, ok := parent.(fmt.Stringer); ok { - w.h.Reset() - _, err := w.h.Write([]byte(impl.String())) + // no fields involved in the hash! try binary and string instead. + if unhashedfields == l && w.unhashedstructfallback { + var data []byte + if impl, ok := parent.(encoding.BinaryMarshaler); ok { + data, err = impl.MarshalBinary() if err != nil { return 0, err } - return w.h.Sum64(), nil } + + if impl, ok := parent.(fmt.Stringer); ok { + data = []byte(impl.String()) + } + + w.h.Reset() + _, err := w.h.Write(data) + if err != nil { + return 0, err + } + return w.h.Sum64(), nil } return h, nil diff --git a/hashstructure_test.go b/hashstructure_test.go index f69b319..2da2358 100644 --- a/hashstructure_test.go +++ b/hashstructure_test.go @@ -730,14 +730,22 @@ func (t *testHashablePointer) Hash() (uint64, error) { return 100, nil } -type Unexported struct { +type UnexportedStringer struct { n int } -func (u Unexported) String() string { +func (u UnexportedStringer) String() string { return fmt.Sprintf("%d", u.n) } +type UnexportedBinaryer struct { + n int +} + +func (u UnexportedBinaryer) MarshalBinary() (data []byte, err error) { + return []byte(fmt.Sprintf("%d", u.n)), nil +} + func TestHash_StringIgnoredStructs(t *testing.T) { cases := []struct { One, Two interface{} @@ -745,38 +753,74 @@ func TestHash_StringIgnoredStructs(t *testing.T) { Err string }{ { - Unexported{n: 1}, - Unexported{n: 1}, + UnexportedStringer{n: 1}, + UnexportedStringer{n: 1}, + true, + "", + }, + { + UnexportedStringer{n: 1}, + UnexportedStringer{n: 2}, + false, + "", + }, + { + []interface{}{UnexportedStringer{n: 1}}, + []interface{}{UnexportedStringer{n: 1}}, + true, + "", + }, + { + []interface{}{UnexportedStringer{n: 1}}, + []interface{}{UnexportedStringer{n: 2}}, + false, + "", + }, + { + map[string]interface{}{"v": UnexportedStringer{n: 1}}, + map[string]interface{}{"v": UnexportedStringer{n: 1}}, + true, + "", + }, + { + map[string]interface{}{"v": UnexportedStringer{n: 1}}, + map[string]interface{}{"v": UnexportedStringer{n: 2}}, + false, + "", + }, + { + UnexportedBinaryer{n: 1}, + UnexportedBinaryer{n: 1}, true, "", }, { - Unexported{n: 1}, - Unexported{n: 2}, + UnexportedBinaryer{n: 1}, + UnexportedBinaryer{n: 2}, false, "", }, { - []interface{}{Unexported{n: 1}}, - []interface{}{Unexported{n: 1}}, + []interface{}{UnexportedBinaryer{n: 1}}, + []interface{}{UnexportedBinaryer{n: 1}}, true, "", }, { - []interface{}{Unexported{n: 1}}, - []interface{}{Unexported{n: 2}}, + []interface{}{UnexportedBinaryer{n: 1}}, + []interface{}{UnexportedBinaryer{n: 2}}, false, "", }, { - map[string]interface{}{"v": Unexported{n: 1}}, - map[string]interface{}{"v": Unexported{n: 1}}, + map[string]interface{}{"v": UnexportedBinaryer{n: 1}}, + map[string]interface{}{"v": UnexportedBinaryer{n: 1}}, true, "", }, { - map[string]interface{}{"v": Unexported{n: 1}}, - map[string]interface{}{"v": Unexported{n: 2}}, + map[string]interface{}{"v": UnexportedBinaryer{n: 1}}, + map[string]interface{}{"v": UnexportedBinaryer{n: 2}}, false, "", }, @@ -784,7 +828,7 @@ func TestHash_StringIgnoredStructs(t *testing.T) { for i, tc := range cases { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - one, err := Hash(tc.One, testFormat, &HashOptions{StringIgnoredStructs: true}) + one, err := Hash(tc.One, testFormat, &HashOptions{UnhashedStructFallback: true}) if tc.Err != "" { if err == nil { t.Fatal("expected error") @@ -800,7 +844,7 @@ func TestHash_StringIgnoredStructs(t *testing.T) { t.Fatalf("Failed to hash %#v: %s", tc.One, err) } - two, err := Hash(tc.Two, testFormat, &HashOptions{StringIgnoredStructs: true}) + two, err := Hash(tc.Two, testFormat, &HashOptions{UnhashedStructFallback: true}) if err != nil { t.Fatalf("Failed to hash %#v: %s", tc.Two, err) }