Merge pull request #237 from apelisse/proper-comparison

Do not use Compare for Equality checks

Author: k8s-ci-robot

go.mod

@@ -3,6 +3,7 @@ module sigs.k8s.io/structured-merge-diff/v4
 require gopkg.in/yaml.v2 v2.2.8
 
 require (
+	github.com/google/go-cmp v0.5.9 // indirect
 	github.com/google/gofuzz v1.0.0
 	github.com/json-iterator/go v1.1.12
 	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect

go.sum

@@ -1,6 +1,8 @@
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
+github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
+github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
 github.com/google/gofuzz v1.0.0 h1:A8PeW59pxE9IoFRqBp37U+mSNaQoZ46F1f0f863XSXw=
 github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
 github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM=

internal/fixture/state.go

@@ -20,10 +20,11 @@ import (
 	"bytes"
 	"fmt"
 
+	"github.com/google/go-cmp/cmp"
+
 	"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
 	"sigs.k8s.io/structured-merge-diff/v4/merge"
 	"sigs.k8s.io/structured-merge-diff/v4/typed"
-	"sigs.k8s.io/structured-merge-diff/v4/value"
 )
 
 // For the sake of tests, a parser is something that can retrieve a
@@ -168,20 +169,47 @@ func (s *State) Apply(obj typed.YAMLObject, version fieldpath.APIVersion, manage
 
 // CompareLive takes a YAML string and returns the comparison with the
 // current live object or an error.
-func (s *State) CompareLive(obj typed.YAMLObject, version fieldpath.APIVersion) (*typed.Comparison, error) {
+func (s *State) CompareLive(obj typed.YAMLObject, version fieldpath.APIVersion) (string, error) {
 	obj = FixTabsOrDie(obj)
 	if err := s.checkInit(version); err != nil {
-		return nil, err
+		return "", err
 	}
 	tv, err := s.Parser.Type(string(version)).FromYAML(obj)
 	if err != nil {
-		return nil, err
+		return "", err
 	}
 	live, err := s.Updater.Converter.Convert(s.Live, version)
 	if err != nil {
-		return nil, err
+		return "", err
+	}
+	tvu := convertMapAnyToMapString(tv.AsValue().Unstructured())
+	liveu := convertMapAnyToMapString(live.AsValue().Unstructured())
+	return cmp.Diff(tvu, liveu), nil
+}
+
+func convertMapAnyToMapString(obj interface{}) interface{} {
+	switch m := obj.(type) {
+	case map[string]interface{}:
+		out := map[string]interface{}{}
+		for key, value := range m {
+			out[key] = convertMapAnyToMapString(value)
+		}
+		return out
+	case map[interface{}]interface{}:
+		out := map[string]interface{}{}
+		for key, value := range m {
+			out[key.(string)] = convertMapAnyToMapString(value)
+		}
+		return out
+	case []interface{}:
+		out := []interface{}{}
+		for _, value := range m {
+			out = append(out, convertMapAnyToMapString(value))
+		}
+		return out
+	default:
+		return obj
 	}
-	return live.Compare(tv)
 }
 
 // dummyConverter doesn't convert, it just returns the same exact object, as long as a version is provided.
@@ -509,6 +537,9 @@ type TestCase struct {
 	Managed fieldpath.ManagedFields
 	// Set to true if the test case needs the union behavior enabled.
 	RequiresUnions bool
+	// ReportInputOnNoop if we don't want to compare the output and
+	// always return it.
+	ReturnInputOnNoop bool
 	// IgnoredFields containing the set to ignore for every version
 	IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set
 }
@@ -541,13 +572,16 @@ func (tc TestCase) PreprocessOperations(parser Parser) error {
 // TestWithConverter once and reset the benchmark, to make sure the test case
 // actually passes..
 func (tc TestCase) BenchWithConverter(parser Parser, converter merge.Converter) error {
+	updaterBuilder := merge.UpdaterBuilder{
+		Converter:         converter,
+		IgnoredFields:     tc.IgnoredFields,
+		ReturnInputOnNoop: tc.ReturnInputOnNoop,
+		EnableUnions:      tc.RequiresUnions,
+	}
 	state := State{
-		Updater: &merge.Updater{Converter: converter, IgnoredFields: tc.IgnoredFields},
+		Updater: updaterBuilder.BuildUpdater(),
 		Parser:  parser,
 	}
-	if tc.RequiresUnions {
-		state.Updater.EnableUnionFeature()
-	}
 	// We currently don't have any test that converts, we can take
 	// care of that later.
 	for i, ops := range tc.Ops {
@@ -561,13 +595,16 @@ func (tc TestCase) BenchWithConverter(parser Parser, converter merge.Converter)
 
 // TestWithConverter runs the test-case using the given parser and converter.
 func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) error {
+	updaterBuilder := merge.UpdaterBuilder{
+		Converter:         converter,
+		IgnoredFields:     tc.IgnoredFields,
+		ReturnInputOnNoop: tc.ReturnInputOnNoop,
+		EnableUnions:      tc.RequiresUnions,
+	}
 	state := State{
-		Updater: &merge.Updater{Converter: converter, IgnoredFields: tc.IgnoredFields},
+		Updater: updaterBuilder.BuildUpdater(),
 		Parser:  parser,
 	}
-	if tc.RequiresUnions {
-		state.Updater.EnableUnionFeature()
-	}
 	for i, ops := range tc.Ops {
 		err := ops.run(&state)
 		if err != nil {
@@ -581,8 +618,8 @@ func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) e
 		if err != nil {
 			return fmt.Errorf("failed to compare live with config: %v", err)
 		}
-		if !comparison.IsSame() {
-			return fmt.Errorf("expected live and config to be the same:\n%v\nConfig: %v\n", comparison, value.ToString(state.Live.AsValue()))
+		if comparison != "" {
+			return fmt.Errorf("expected live and config to be the same:\n%v\n", comparison)
 		}
 	}
 

merge/obsolete_versions_test.go

@@ -127,7 +127,7 @@ func TestApplyObsoleteVersion(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Failed to compare live object: %v", err)
 	}
-	if !comparison.IsSame() {
+	if comparison != "" {
 		t.Fatalf("Unexpected object:\n%v", comparison)
 	}
 }

merge/real_test.go

@@ -108,8 +108,9 @@ func BenchmarkOperations(b *testing.B) {
 		b.Run(bench.name, func(b *testing.B) {
 			obj := typed.YAMLObject(read(testdata(bench.filename)))
 			tests := []struct {
-				name string
-				ops  []Operation
+				name              string
+				returnInputonNoop bool
+				ops               []Operation
 			}{
 				{
 					name: "Create",
@@ -146,6 +147,22 @@ func BenchmarkOperations(b *testing.B) {
 						},
 					},
 				},
+				{
+					name:              "ApplyTwiceNoCompare",
+					returnInputonNoop: true,
+					ops: []Operation{
+						Apply{
+							Manager:    "controller",
+							APIVersion: "v1",
+							Object:     obj,
+						},
+						Apply{
+							Manager:    "other-controller",
+							APIVersion: "v1",
+							Object:     obj,
+						},
+					},
+				},
 				{
 					name: "Update",
 					ops: []Operation{
@@ -180,7 +197,8 @@ func BenchmarkOperations(b *testing.B) {
 			for _, test := range tests {
 				b.Run(test.name, func(b *testing.B) {
 					tc := TestCase{
-						Ops: test.ops,
+						Ops:               test.ops,
+						ReturnInputOnNoop: test.returnInputonNoop,
 					}
 					p := SameVersionParser{T: bench.parseType}
 					tc.PreprocessOperations(p)

merge/set_test.go

@@ -124,11 +124,11 @@ func TestUpdateSet(t *testing.T) {
 			Object: `
 				list:
 				- a
-				- aprime
 				- b
+				- aprime
 				- c
-				- cprime
 				- d
+				- cprime
 			`,
 			APIVersion: "v1",
 			Managed: fieldpath.ManagedFields{
@@ -189,11 +189,11 @@ func TestUpdateSet(t *testing.T) {
 			Object: `
 				list:
 				- a
-				- aprime
 				- b
+				- aprime
 				- c
-				- cprime
 				- d
+				- cprime
 			`,
 			APIVersion: "v1",
 			Managed: fieldpath.ManagedFields{

merge/update.go

@@ -18,6 +18,7 @@ import (
 
 	"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
 	"sigs.k8s.io/structured-merge-diff/v4/typed"
+	"sigs.k8s.io/structured-merge-diff/v4/value"
 )
 
 // Converter is an interface to the conversion logic. The converter
@@ -27,17 +28,50 @@ type Converter interface {
 	IsMissingVersionError(error) bool
 }
 
+// UpdateBuilder allows you to create a new Updater by exposing all of
+// the options and setting them once.
+type UpdaterBuilder struct {
+	Converter     Converter
+	IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set
+
+	EnableUnions bool
+
+	// Stop comparing the new object with old object after applying.
+	// This was initially used to avoid spurious etcd update, but
+	// since that's vastly inefficient, we've come-up with a better
+	// way of doing that. Create this flag to stop it.
+	// Comparing has become more expensive too now that we're not using
+	// `Compare` but `value.Equals` so this gives an option to avoid it.
+	ReturnInputOnNoop bool
+}
+
+func (u *UpdaterBuilder) BuildUpdater() *Updater {
+	return &Updater{
+		Converter:         u.Converter,
+		IgnoredFields:     u.IgnoredFields,
+		enableUnions:      u.EnableUnions,
+		returnInputOnNoop: u.ReturnInputOnNoop,
+	}
+}
+
 // Updater is the object used to compute updated FieldSets and also
 // merge the object on Apply.
 type Updater struct {
-	Converter     Converter
+	// Deprecated: This will eventually become private.
+	Converter Converter
+
+	// Deprecated: This will eventually become private.
 	IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set
 
 	enableUnions bool
+
+	returnInputOnNoop bool
 }
 
 // EnableUnionFeature turns on union handling. It is disabled by default until the
 // feature is complete.
+//
+// Deprecated: Use the builder instead.
 func (s *Updater) EnableUnionFeature() {
 	s.enableUnions = true
 }
@@ -157,8 +191,7 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp
 
 // Apply should be called when Apply is run, given the current object as
 // well as the configuration that is applied. This will merge the object
-// and return it. If the object hasn't changed, nil is returned (the
-// managers can still have changed though).
+// and return it.
 func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string, force bool) (*typed.TypedValue, fieldpath.ManagedFields, error) {
 	var err error
 	managers, err = s.reconcileManagedFieldsWithSchemaChanges(liveObject, managers)
@@ -200,11 +233,11 @@ func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fiel
 	if err != nil {
 		return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to prune fields: %v", err)
 	}
-	managers, compare, err := s.update(liveObject, newObject, version, managers, manager, force)
+	managers, _, err = s.update(liveObject, newObject, version, managers, manager, force)
 	if err != nil {
 		return nil, fieldpath.ManagedFields{}, err
 	}
-	if compare.IsSame() {
+	if !s.returnInputOnNoop && value.EqualsUsing(value.NewFreelistAllocator(), liveObject.AsValue(), newObject.AsValue()) {
 		newObject = nil
 	}
 	return newObject, managers, nil