add extended remove method for TypedValue

to support recursive removal of fields

Author: Kevin Wiesmüller

fieldpath/element.go

@@ -47,6 +47,16 @@ type PathElement struct {
 	Index *int
 }
 
+// NewValuePathElement creates a PathElement from the provided value.Value
+func NewValuePathElement(v value.Value) PathElement {
+	return PathElement{Value: &v}
+}
+
+// NewIndexPathElement creates a PathElement with the given index
+func NewIndexPathElement(index int) PathElement {
+	return PathElement{Index: &index}
+}
+
 // Less provides an order for path elements.
 func (e PathElement) Less(rhs PathElement) bool {
 	return e.Compare(rhs) < 0

go.mod

@@ -1,12 +1,13 @@
 module sigs.k8s.io/structured-merge-diff/v3
 
-require gopkg.in/yaml.v2 v2.2.1
+require gopkg.in/yaml.v2 v2.2.2
 
 require (
 	github.com/google/gofuzz v1.0.0
 	github.com/json-iterator/go v1.1.6
 	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
 	github.com/modern-go/reflect2 v1.0.1 // indirect
+	github.com/stretchr/testify v1.5.0 // indirect
 )
 
 go 1.13

go.sum

@@ -10,12 +10,11 @@ github.com/modern-go/reflect2 v1.0.1 h1:9f412s+6RmYXLWZSEzVVgPGK7C2PphHj5RJrvfx9
 github.com/modern-go/reflect2 v1.0.1/go.mod h1:bx2lNnkwVCuqBIxFjflWJWanXIb3RllmbCylyMrvgv0=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
+github.com/stretchr/objx v0.1.0 h1:4G4v2dO3VZwixGIRoQ5Lfboy6nUhCyYzaqnIAPPhYs4=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
-github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q=
-github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
+github.com/stretchr/testify v1.5.0 h1:DMOzIV76tmoDNE9pX6RSN0aDtCYeCg5VueieJaAo1uw=
+github.com/stretchr/testify v1.5.0/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
-gopkg.in/yaml.v2 v2.2.1 h1:mUhvW9EsL+naU5Q3cakzfE91YhliOondGd6ZrsDBHQE=
-gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
-sigs.k8s.io/structured-merge-diff v1.0.1 h1:LOs1LZWMsz1xs77Phr/pkB4LFaavH7IVq/3+WTN9XTA=
-sigs.k8s.io/structured-merge-diff v1.0.1/go.mod h1:IIgPezJWb76P0hotTxzDbWsMYB8APh18qZnxkomBpxA=
+gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw=
+gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=

internal/fixture/state.go

@@ -130,7 +130,7 @@ func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion,
 
 // Update the current state with the passed in object
 func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, manager string) error {
-	tv, err := s.Parser.Type(string(version)).FromYAML(FixTabsOrDie(obj))
+	tv, err := s.parseConfig(version, obj)
 	if err != nil {
 		return err
 	}
@@ -159,7 +159,7 @@ func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion,
 
 // Apply the passed in object to the current state
 func (s *State) Apply(obj typed.YAMLObject, version fieldpath.APIVersion, manager string, force bool) error {
-	tv, err := s.Parser.Type(string(version)).FromYAML(FixTabsOrDie(obj))
+	tv, err := s.parseConfig(version, obj)
 	if err != nil {
 		return err
 	}
@@ -184,6 +184,10 @@ func (s *State) CompareLive(obj typed.YAMLObject, version fieldpath.APIVersion)
 	return live.Compare(tv)
 }
 
+func (s *State) parseConfig(apiVersion fieldpath.APIVersion, obj typed.YAMLObject) (*typed.TypedValue, error) {
+	return s.Parser.Type(string(apiVersion)).FromYAML(FixTabsOrDie(obj))
+}
+
 // dummyConverter doesn't convert, it just returns the same exact object, as long as a version is provided.
 type dummyConverter struct{}
 
@@ -471,12 +475,12 @@ func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) e
 
 	// If LastObject was specified, compare it with LiveState
 	if tc.Object != typed.YAMLObject("") {
-		comparison, err := state.CompareLive(tc.Object, tc.APIVersion)
-		if err != nil {
-			return fmt.Errorf("failed to compare live with config: %v", err)
+		expect := ExpectState{
+			APIVersion: tc.APIVersion, Object: tc.Object,
 		}
-		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 err := expect.run(&state); err != nil {
+			return err
 		}
 	}
 
@@ -505,3 +509,44 @@ func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) e
 
 	return nil
 }
+
+// PrintState is an Operation printing the current state to help with debugging tests
+type PrintState struct{}
+
+var _ Operation = PrintState{}
+
+func (op PrintState) run(s *State) error {
+	fmt.Println(value.ToString(s.Live.AsValue()))
+	return nil
+}
+
+func (op PrintState) preprocess(_ Parser) (Operation, error) {
+	return op, nil
+}
+
+// ExpectState is an Operation comparing the current state to the defined config to help with debugging tests
+type ExpectState struct {
+	APIVersion fieldpath.APIVersion
+	Object     typed.YAMLObject
+}
+
+var _ Operation = ExpectState{}
+
+func (op ExpectState) run(state *State) error {
+	comparison, err := state.CompareLive(op.Object, op.APIVersion)
+	if err != nil {
+		return fmt.Errorf("failed to compare live with config: %v", err)
+	}
+	if !comparison.IsSame() {
+		config, err := state.parseConfig(op.APIVersion, op.Object)
+		if err != nil {
+			return fmt.Errorf("failed to parse config: %w", err)
+		}
+		return fmt.Errorf("expected live and config to be the same:\n%v\nConfig: %v\nLive:   %v\n", comparison, value.ToString(config.AsValue()), value.ToString(state.Live.AsValue()))
+	}
+	return nil
+}
+
+func (op ExpectState) preprocess(parser Parser) (Operation, error) {
+	return op, nil
+}

merge/multiple_appliers_test.go

@@ -670,7 +670,7 @@ func TestMultipleAppliersNestedType(t *testing.T) {
 			},
 			Object: `
 				mapOfMapsRecursive:
-				  a:
+				  a: {}
 				  c:
 				    d:
 				      e:
@@ -788,7 +788,7 @@ func TestMultipleAppliersDeducedType(t *testing.T) {
 				},
 			},
 			Object: `
-				a:
+				a: {}
 				c:
 				  d:
 				    e:

merge/update.go

@@ -213,6 +213,7 @@ func (s *Updater) prune(merged *typed.TypedValue, managers fieldpath.ManagedFiel
 		return nil, fmt.Errorf("failed to convert merged object to last applied version: %v", err)
 	}
 
+	// pruned is the value *after* pruning
 	pruned := convertedMerged.RemoveItems(lastSet.Set())
 	pruned, err = s.addBackOwnedItems(convertedMerged, pruned, managers, applyingManager)
 	if err != nil {
@@ -261,7 +262,11 @@ func (s *Updater) addBackOwnedItems(merged, pruned *typed.TypedValue, managedFie
 		if err != nil {
 			return nil, fmt.Errorf("failed to create field set from pruned object at version %v: %v", version, err)
 		}
-		pruned = merged.RemoveItems(mergedSet.Difference(prunedSet.Union(managed)))
+
+		newOrManaged := prunedSet.Union(managed)
+		oldAndUnmanaged := mergedSet.Difference(newOrManaged)
+		// a value containing only new or previously managed fields
+		pruned = merged.RemoveItems(oldAndUnmanaged)
 	}
 	return pruned, nil
 }
@@ -285,5 +290,9 @@ func (s *Updater) addBackDanglingItems(merged, pruned *typed.TypedValue, lastSet
 	if err != nil {
 		return nil, fmt.Errorf("failed to create field set from merged object in last applied version: %v", err)
 	}
-	return merged.RemoveItems(mergedSet.Difference(prunedSet).Intersection(lastSet.Set())), nil
+
+	oldOrUnmanaged := mergedSet.Difference(prunedSet)
+	onlyOld := oldOrUnmanaged.Intersection(lastSet.Set())
+
+	return merged.RemoveItems(onlyOld), nil
 }

typed/remove.go

@@ -25,14 +25,16 @@ type removingWalker struct {
 	schema    *schema.Schema
 	toRemove  *fieldpath.Set
 	allocator value.Allocator
+	onlyItems bool
 }
 
-func removeItemsWithSchema(val value.Value, toRemove *fieldpath.Set, schema *schema.Schema, typeRef schema.TypeRef) value.Value {
+func removeWithSchema(val value.Value, toRemove *fieldpath.Set, schema *schema.Schema, typeRef schema.TypeRef, onlyItems bool) value.Value {
 	w := &removingWalker{
 		value:     val,
 		schema:    schema,
 		toRemove:  toRemove,
 		allocator: value.NewFreelistAllocator(),
+		onlyItems: onlyItems,
 	}
 	resolveSchema(schema, typeRef, val, w)
 	return value.NewValueInterface(w.out)
@@ -46,8 +48,10 @@ func (w *removingWalker) doScalar(t *schema.Scalar) ValidationErrors {
 func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) {
 	l := w.value.AsListUsing(w.allocator)
 	defer w.allocator.Free(l)
-	// If list is null, empty, or atomic just return
-	if l == nil || l.Length() == 0 || t.ElementRelationship == schema.Atomic {
+
+	// If list is null or empty just return
+	if l == nil || l.Length() == 0 {
+		w.out = w.value.Unstructured()
 		return nil
 	}
 
@@ -63,13 +67,11 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) {
 			continue
 		}
 		if subset := w.toRemove.WithPrefix(pe); !subset.Empty() {
-			item = removeItemsWithSchema(item, subset, w.schema, t.ElementType)
+			item = removeWithSchema(item, subset, w.schema, t.ElementType, w.onlyItems)
 		}
 		newItems = append(newItems, item.Unstructured())
 	}
-	if len(newItems) > 0 {
-		w.out = newItems
-	}
+	w.out = newItems
 	return nil
 }
 
@@ -78,8 +80,9 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors {
 	if m != nil {
 		defer w.allocator.Free(m)
 	}
-	// If map is null, empty, or atomic just return
-	if m == nil || m.Empty() || t.ElementRelationship == schema.Atomic {
+	// If map is null or empty just return
+	if m == nil || m.Empty() {
+		w.out = w.value.Unstructured()
 		return nil
 	}
 
@@ -92,6 +95,11 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors {
 	m.Iterate(func(k string, val value.Value) bool {
 		pe := fieldpath.PathElement{FieldName: &k}
 		path, _ := fieldpath.MakePath(pe)
+
+		if !w.onlyItems && w.toRemove.Has(path) {
+			return true
+		}
+
 		fieldType := t.ElementType
 		if ft, ok := fieldTypes[k]; ok {
 			fieldType = ft
@@ -101,13 +109,11 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors {
 			}
 		}
 		if subset := w.toRemove.WithPrefix(pe); !subset.Empty() {
-			val = removeItemsWithSchema(val, subset, w.schema, fieldType)
+			val = removeWithSchema(val, subset, w.schema, fieldType, w.onlyItems)
 		}
 		newMap[k] = val.Unstructured()
 		return true
 	})
-	if len(newMap) > 0 {
-		w.out = newMap
-	}
+	w.out = newMap
 	return nil
 }