Merge pull request #60 from jennybuckley/remove-disowned

Remove removed items from maps and lists on apply

Author: k8s-ci-robot

fieldpath/set.go

@@ -154,6 +154,16 @@ func (s *Set) iteratePrefix(prefix Path, f func(Path)) {
 	s.Children.iteratePrefix(prefix, f)
 }
 
+// WithPrefix returns the subset of paths which begin with the given prefix,
+// with the prefix not included.
+func (s *Set) WithPrefix(pe PathElement) *Set {
+	subset, ok := s.Children.Get(pe)
+	if !ok {
+		return NewSet()
+	}
+	return subset
+}
+
 // setNode is a pair of PathElement / Set, for the purpose of expressing
 // nested set membership.
 type setNode struct {

merge/set_test.go

@@ -304,19 +304,6 @@ func TestUpdateSet(t *testing.T) {
 				},
 			},
 		},
-	}
-
-	for name, test := range tests {
-		t.Run(name, func(t *testing.T) {
-			if err := test.Test(setFieldsParser); err != nil {
-				t.Fatal(err)
-			}
-		})
-	}
-}
-
-func TestUpdateSetBroken(t *testing.T) {
-	tests := map[string]TestCase{
 		"apply_twice_remove": {
 			Ops: []Operation{
 				Apply{
@@ -359,8 +346,8 @@ func TestUpdateSetBroken(t *testing.T) {
 
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
-			if test.Test(setFieldsParser) == nil {
-				t.Fatalf("Broken test passed.")
+			if err := test.Test(setFieldsParser); err != nil {
+				t.Fatal(err)
 			}
 		})
 	}

merge/update.go

@@ -90,6 +90,12 @@ func (s *Updater) update(oldObject, newObject typed.TypedValue, version fieldpat
 		}
 	}
 
+	if _, ok := managers[workflow]; !ok {
+		managers[workflow] = &fieldpath.VersionedSet{
+			Set: fieldpath.NewSet(),
+		}
+	}
+
 	return managers, nil
 }
 
@@ -108,11 +114,6 @@ func (s *Updater) Update(liveObject, newObject typed.TypedValue, version fieldpa
 	if err != nil {
 		return fieldpath.ManagedFields{}, fmt.Errorf("failed to compare live and new objects: %v", err)
 	}
-	if _, ok := managers[manager]; !ok {
-		managers[manager] = &fieldpath.VersionedSet{
-			Set: fieldpath.NewSet(),
-		}
-	}
 	managers[manager].Set = managers[manager].Set.Union(compare.Modified).Union(compare.Added).Difference(compare.Removed)
 	managers[manager].APIVersion = version
 	if managers[manager].Set.Empty() {
@@ -133,9 +134,10 @@ func (s *Updater) Apply(liveObject, configObject typed.TypedValue, version field
 	if err != nil {
 		return nil, fieldpath.ManagedFields{}, err
 	}
-
-	// TODO: Remove unconflicting removed fields
-
+	newObject, err = s.removeDisownedItems(newObject, configObject, managers[manager])
+	if err != nil {
+		return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to remove fields: %v", err)
+	}
 	set, err := configObject.ToFieldSet()
 	if err != nil {
 		return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to get field set: %v", err)
@@ -149,3 +151,15 @@ func (s *Updater) Apply(liveObject, configObject typed.TypedValue, version field
 	}
 	return newObject, managers, nil
 }
+
+func (s *Updater) removeDisownedItems(merged, applied typed.TypedValue, lastSet *fieldpath.VersionedSet) (typed.TypedValue, error) {
+	convertedApplied, err := s.Converter.Convert(applied, lastSet.APIVersion)
+	if err != nil {
+		return nil, fmt.Errorf("failed to convert applied config to last applied version: %v", err)
+	}
+	appliedSet, err := convertedApplied.ToFieldSet()
+	if err != nil {
+		return nil, fmt.Errorf("failed to create field set from applied config in last applied version: %v", err)
+	}
+	return merged.RemoveItems(lastSet.Set.Difference(appliedSet)), nil
+}

typed/deduced.go

@@ -19,8 +19,8 @@ package typed
 import (
 	"reflect"
 
-	"sigs.k8s.io/structured-merge-diff/value"
 	"sigs.k8s.io/structured-merge-diff/fieldpath"
+	"sigs.k8s.io/structured-merge-diff/value"
 )
 
 // deducedTypedValue holds a value and guesses what it is and what to
@@ -130,7 +130,7 @@ func added(lhs, rhs value.Value, path fieldpath.Path, set *fieldpath.Set) {
 	} else if lhs.MapValue == nil && rhs.MapValue != nil {
 		// From leaf to map, add leaf fields of map.
 		fieldsetDeduced(rhs, path, set)
-	} else if lhs.MapValue != nil  && rhs.MapValue == nil {
+	} else if lhs.MapValue != nil && rhs.MapValue == nil {
 		// Went from map to field, add field.
 		set.Insert(path)
 	} else {
@@ -150,7 +150,7 @@ func added(lhs, rhs value.Value, path fieldpath.Path, set *fieldpath.Set) {
 }
 
 func modified(lhs, rhs value.Value, path fieldpath.Path, set *fieldpath.Set) {
-	if lhs.MapValue == nil && rhs.MapValue == nil  {
+	if lhs.MapValue == nil && rhs.MapValue == nil {
 		if !reflect.DeepEqual(lhs, rhs) {
 			set.Insert(path)
 		}
@@ -170,3 +170,9 @@ func modified(lhs, rhs value.Value, path fieldpath.Path, set *fieldpath.Set) {
 		}
 	}
 }
+
+// RemoveItems does nothing because all lists in a deducedTypedValue are considered atomic,
+// and there are no maps because it is indistinguishable from a struct.
+func (dv deducedTypedValue) RemoveItems(_ *fieldpath.Set) TypedValue {
+	return dv
+}

typed/remove.go

@@ -0,0 +1,119 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package typed
+
+import (
+	"sigs.k8s.io/structured-merge-diff/fieldpath"
+	"sigs.k8s.io/structured-merge-diff/schema"
+	"sigs.k8s.io/structured-merge-diff/value"
+)
+
+type removingWalker struct {
+	value    *value.Value
+	schema   *schema.Schema
+	toRemove *fieldpath.Set
+}
+
+func removeItemsWithSchema(value *value.Value, toRemove *fieldpath.Set, schema *schema.Schema, typeRef schema.TypeRef) {
+	w := &removingWalker{
+		value:    value,
+		schema:   schema,
+		toRemove: toRemove,
+	}
+	resolveSchema(schema, typeRef, w)
+}
+
+// doLeaf should be called on leaves before descending into children, if there
+// will be a descent. It modifies w.inLeaf.
+func (w *removingWalker) doLeaf() ValidationErrors { return nil }
+
+func (w *removingWalker) doScalar(t schema.Scalar) ValidationErrors { return nil }
+
+func (w *removingWalker) doStruct(t schema.Struct) ValidationErrors {
+	s := w.value.MapValue
+
+	// If struct is null, empty, or atomic just return
+	if s == nil || len(s.Items) == 0 || t.ElementRelationship == schema.Atomic {
+		return nil
+	}
+
+	fieldTypes := map[string]schema.TypeRef{}
+	for _, structField := range t.Fields {
+		fieldTypes[structField.Name] = structField.Type
+	}
+
+	for i, _ := range s.Items {
+		item := s.Items[i]
+		pe := fieldpath.PathElement{FieldName: &item.Name}
+		if subset := w.toRemove.WithPrefix(pe); !subset.Empty() {
+			removeItemsWithSchema(&s.Items[i].Value, subset, w.schema, fieldTypes[item.Name])
+		}
+	}
+	return nil
+}
+
+func (w *removingWalker) doList(t schema.List) (errs ValidationErrors) {
+	l := w.value.ListValue
+
+	// If list is null, empty, or atomic just return
+	if l == nil || len(l.Items) == 0 || t.ElementRelationship == schema.Atomic {
+		return nil
+	}
+
+	newItems := []value.Value{}
+	for i, _ := range l.Items {
+		item := l.Items[i]
+		// Ignore error because we have already validated this list
+		pe, _ := listItemToPathElement(t, i, item)
+		path, _ := fieldpath.MakePath(pe)
+		if w.toRemove.Has(path) {
+			continue
+		}
+		if subset := w.toRemove.WithPrefix(pe); !subset.Empty() {
+			removeItemsWithSchema(&l.Items[i], subset, w.schema, t.ElementType)
+		}
+		newItems = append(newItems, l.Items[i])
+	}
+	l.Items = newItems
+	return nil
+}
+
+func (w *removingWalker) doMap(t schema.Map) ValidationErrors {
+	m := w.value.MapValue
+
+	// If map is null, empty, or atomic just return
+	if m == nil || len(m.Items) == 0 || t.ElementRelationship == schema.Atomic {
+		return nil
+	}
+
+	newMap := &value.Map{}
+	for i, _ := range m.Items {
+		item := m.Items[i]
+		pe := fieldpath.PathElement{FieldName: &item.Name}
+		path, _ := fieldpath.MakePath(pe)
+		if w.toRemove.Has(path) {
+			continue
+		}
+		if subset := w.toRemove.WithPrefix(pe); !subset.Empty() {
+			removeItemsWithSchema(&m.Items[i].Value, subset, w.schema, t.ElementType)
+		}
+		newMap.Set(item.Name, m.Items[i].Value)
+	}
+	w.value.MapValue = newMap
+	return nil
+}
+
+func (*removingWalker) doUntyped(_ schema.Untyped) ValidationErrors { return nil }
+
+func (*removingWalker) errorf(_ string, _ ...interface{}) ValidationErrors { return nil }

typed/typed.go

@@ -53,6 +53,8 @@ type TypedValue interface {
 	// match), or an error will be returned. Validation errors will be returned if
 	// the objects don't conform to the schema.
 	Compare(rhs TypedValue) (c *Comparison, err error)
+	// RemoveItems removes each provided list or map item from the value.
+	RemoveItems(items *fieldpath.Set) TypedValue
 }
 
 // AsTyped accepts a value and a type and returns a TypedValue. 'v' must have
@@ -159,6 +161,12 @@ func (tv typedValue) Compare(rhs TypedValue) (c *Comparison, err error) {
 	return c, nil
 }
 
+// RemoveItems removes each provided list or map item from the value.
+func (tv typedValue) RemoveItems(items *fieldpath.Set) TypedValue {
+	removeItemsWithSchema(&tv.value, items, tv.schema, tv.typeRef)
+	return tv
+}
+
 func merge(lhs, rhs typedValue, rule, postRule mergeRule) (TypedValue, error) {
 	if lhs.schema != rhs.schema {
 		return nil, errorFormatter{}.