fix list removal and add tests

Author: Kevin Wiesmüller

typed/remove.go

@@ -48,6 +48,7 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) {
 
 	// If list is null, empty, or atomic just return
 	if l == nil || l.Length() == 0 || t.ElementRelationship == schema.Atomic {
+		w.out = l
 		return nil
 	}
 

typed/remove_test.go

@@ -8,7 +8,7 @@ import (
 	"sigs.k8s.io/structured-merge-diff/v3/value"
 )
 
-func TestRemove(t *testing.T) {
+func TestRemoveDeduced(t *testing.T) {
 
 	var cases = []struct {
 		object YAMLObject
@@ -47,17 +47,16 @@ func TestRemove(t *testing.T) {
 			expect: `{"a": "value"}`,
 		},
 		{
+			// this is okay, because in a deduced schema `b` is handled as a map not a struct
 			object: `{"a": "value", "b":{"c":"value"}}`,
 			remove: fieldpath.NewSet(fieldpath.MakePathOrDie("b")),
 			deep:   false,
-			// TODO: this seems to behave differently with a non deduced schema: {"a": "value", "b":{"c":"value"}} which is the reason for this PR
 			expect: `{"a": "value"}`,
 		},
 		{
 			object: `{"a": "value", "b": {"c":"value"}}`,
 			remove: fieldpath.NewSet(fieldpath.MakePathOrDie("b", "c")),
 			deep:   true,
-			// TODO: is a null result expected (insted of a {})
 			expect: `{"a": "value", "b": null}`,
 		},
 		{
@@ -70,21 +69,20 @@ func TestRemove(t *testing.T) {
 			object: `{"a": "value", "b": ["c"]}`,
 			remove: fieldpath.NewSet(fieldpath.MakePathOrDie("b", "c")),
 			deep:   true,
+			// TODO: this should fail
 			expect: `{"a": "value", "b": null}`,
 		},
 		{
 			object: `{"a": "value", "b": ["c", "d"]}`,
 			remove: fieldpath.NewSet(fieldpath.MakePathOrDie("b", "c")),
 			deep:   true,
-			// TODO: is this expected?
-			expect: `{"a": "value", "b": null}`,
+			expect: `{"a": "value", "b": ["c", "d"]}`,
 		},
 		{
 			object: `{"a": "value", "b": [{"c": "value"}, {"d": "value"}]}`,
 			remove: fieldpath.NewSet(fieldpath.MakePathOrDie("b", "c")),
 			deep:   true,
-			// TODO: is this expected?
-			expect: `{"a": "value", "b": null}`,
+			expect: `{"a": "value", "b": [{"c": "value"}, {"d": "value"}]}`,
 		},
 		{
 			object: `{"a": "value", "b": {"c":"value", "d":{"e":"value"}}}`,
@@ -132,7 +130,6 @@ func TestRemove(t *testing.T) {
 			object: `{"a": "value", "b": {"c":"value"}}`,
 			remove: fieldpath.NewSet(fieldpath.MakePathOrDie("b", "c")),
 			deep:   false,
-			// TODO: is a null result expected (insted of a {})
 			expect: `{"a": "value", "b": null}`,
 		},
 		{
@@ -145,21 +142,19 @@ func TestRemove(t *testing.T) {
 			object: `{"a": "value", "b": ["c"]}`,
 			remove: fieldpath.NewSet(fieldpath.MakePathOrDie("b", "c")),
 			deep:   false,
-			expect: `{"a": "value", "b": null}`,
+			expect: `{"a": "value", "b": ["c"]}`,
 		},
 		{
 			object: `{"a": "value", "b": ["c", "d"]}`,
 			remove: fieldpath.NewSet(fieldpath.MakePathOrDie("b", "c")),
 			deep:   false,
-			// TODO: is this expected?
-			expect: `{"a": "value", "b": null}`,
+			expect: `{"a": "value", "b": ["c", "d"]}`,
 		},
 		{
 			object: `{"a": "value", "b": [{"c": "value"}, {"d": "value"}]}`,
 			remove: fieldpath.NewSet(fieldpath.MakePathOrDie("b", "c")),
 			deep:   false,
-			// TODO: is this expected?
-			expect: `{"a": "value", "b": null}`,
+			expect: `{"a": "value", "b": [{"c": "value"}, {"d": "value"}]}`,
 		},
 		{
 			object: `{"a": "value", "b": {"c":"value", "d":{"e":"value"}}}`,
@@ -185,7 +180,11 @@ func TestRemove(t *testing.T) {
 				t.Fatalf("unable to parse/validate object: %v\n%v", err, c.object)
 			}
 
-			expectTyped, err := DeducedParseableType.FromYAML(c.expect)
+			parseable := &ParseableType{
+				Schema:  obj.schema,
+				TypeRef: obj.typeRef,
+			}
+			expectTyped, err := parseable.FromYAML(c.expect)
 			if err != nil {
 				t.Fatalf("unable to parse/validate expected object: %v\n%v", err, c.expect)
 			}
@@ -206,3 +205,119 @@ func TestRemove(t *testing.T) {
 		})
 	}
 }
+func TestRemove(t *testing.T) {
+
+	var cases = []struct {
+		object YAMLObject
+		schema YAMLObject
+		remove *fieldpath.Set
+		deep   bool
+		expect YAMLObject
+	}{
+		{
+			object: `{"a": "value", "b":{"c":"value"}}`,
+			schema: `types:
+- name: type
+  map:
+    fields:
+      - name: a
+        type:
+          scalar: string
+      - name: b
+        type:
+          map:
+            fields:
+            - name: c
+              type:
+                scalar: string
+    elementType:
+      scalar: string
+`,
+			remove: fieldpath.NewSet(fieldpath.MakePathOrDie("b")),
+			deep:   false,
+			expect: `{"a": "value", "b":{"c":"value"}}`,
+		},
+		{
+			object: `{"a": "value", "b":{"c":"value"}}`,
+			schema: `types:
+- name: type
+  map:
+    fields:
+      - name: a
+        type:
+          scalar: string
+      - name: b
+        type:
+          map:
+            fields:
+            - name: c
+              type:
+                scalar: string
+    elementType:
+      scalar: string
+`,
+			remove: fieldpath.NewSet(fieldpath.MakePathOrDie("b")),
+			deep:   true,
+			expect: `{"a": "value"}`,
+		},
+		{
+			object: `{"a": "value", "b": ["c"]}`,
+			schema: `types:
+- name: type
+  map:
+    fields:
+      - name: a
+        type:
+          scalar: string
+      - name: b
+        type:
+          list:
+            elementType:
+              scalar: string
+            elementRelationship: associative
+    elementType:
+      scalar: string
+`,
+			remove: fieldpath.NewSet(fieldpath.MakePathOrDie("b", "c")),
+			deep:   true,
+			expect: `{"a": "value", "b": ["c"]}`,
+		},
+	}
+
+	for i, c := range cases {
+		c := c
+		t.Run(fmt.Sprintf("case-%d", i), func(t *testing.T) {
+			t.Parallel()
+
+			parser, err := NewParser(c.schema)
+			if err != nil {
+				t.Fatalf("unable to parse schema: %v\n%v", err, c.schema)
+			}
+
+			parseable := parser.Type("type")
+			object, err := parseable.FromYAML(c.object)
+			if err != nil {
+				t.Fatalf("unable to parse object: %v\n%v", err, c.object)
+			}
+
+			expectTyped, err := parseable.FromYAML(c.expect)
+			if err != nil {
+				t.Fatalf("unable to parse expected object: %v\n%v", err, c.expect)
+			}
+			expect := expectTyped.AsValue()
+
+			var result value.Value
+			if c.deep {
+				result = object.RemoveDeep(c.remove).AsValue()
+			} else {
+				result = object.RemoveItems(c.remove).AsValue()
+			}
+
+			if !value.Equals(result, expect) {
+				t.Fatalf("unexpected result after remove:\ngot: %v\nexp: %v",
+					value.ToString(result), value.ToString(expect),
+				)
+			}
+		})
+	}
+}