From 9c093d14f9d7f957583706ca9342e4cc60b304ea Mon Sep 17 00:00:00 2001 From: Peter Jiang Date: Thu, 2 Oct 2025 17:09:34 -0700 Subject: [PATCH 1/3] fix: RemoveItems should preserve empty list and map Signed-off-by: Peter Jiang --- typed/remove.go | 34 ++++++++++++++++++++++++++++++++-- typed/remove_test.go | 8 ++++---- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/typed/remove.go b/typed/remove.go index 78ba6f5..c1c7a51 100644 --- a/typed/remove.go +++ b/typed/remove.go @@ -75,6 +75,7 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) { } var newItems []interface{} + hadMatches := false iter := l.RangeUsing(w.allocator) defer w.allocator.Free(iter) for iter.Next() { @@ -98,12 +99,26 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) { continue } if isPrefixMatch { + // Removing nested items WITHIN this list item - preserve if it becomes empty + hadMatches = true + wasMap := item.IsMap() + wasList := item.IsList() item = removeItemsWithSchema(item, w.toRemove.WithPrefix(pe), w.schema, t.ElementType, w.shouldExtract) + // If recursive call returned null but we're removing items within (not the item itself), + // preserve the empty container structure + if item.IsNull() && !w.shouldExtract { + if wasMap { + item = value.NewValueInterface(map[string]interface{}{}) + } else if wasList { + item = value.NewValueInterface([]interface{}{}) + } + } } newItems = append(newItems, item.Unstructured()) } } - if len(newItems) > 0 { + // Preserve empty lists (non-nil) instead of converting to null when items were matched and removed + if len(newItems) > 0 || (hadMatches && !w.shouldExtract) { w.out = newItems } return nil @@ -141,6 +156,7 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { } newMap := map[string]interface{}{} + hadMatches := false m.Iterate(func(k string, val value.Value) bool { pe := fieldpath.PathElement{FieldName: &k} path, _ := fieldpath.MakePath(pe) @@ -151,6 +167,7 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { // save values on the path when we shouldExtract // but ignore them when we are removing (i.e. !w.shouldExtract) if w.toRemove.Has(path) { + // Exact match: removing this field itself, not items within it if w.shouldExtract { newMap[k] = removeItemsWithSchema(val, w.toRemove, w.schema, fieldType, w.shouldExtract).Unstructured() @@ -158,7 +175,19 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { return true } if subset := w.toRemove.WithPrefix(pe); !subset.Empty() { + hadMatches = true + wasMap := val.IsMap() + wasList := val.IsList() val = removeItemsWithSchema(val, subset, w.schema, fieldType, w.shouldExtract) + // If recursive call returned null but we're removing items within (not the field itself), + // preserve the empty container structure + if val.IsNull() && !w.shouldExtract { + if wasMap { + val = value.NewValueInterface(map[string]interface{}{}) + } else if wasList { + val = value.NewValueInterface([]interface{}{}) + } + } } else { // don't save values not on the path when we shouldExtract. if w.shouldExtract { @@ -168,7 +197,8 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { newMap[k] = val.Unstructured() return true }) - if len(newMap) > 0 { + // Preserve empty maps (non-nil) instead of converting to null when items were matched and removed + if len(newMap) > 0 || (hadMatches && !w.shouldExtract) { w.out = newMap } return nil diff --git a/typed/remove_test.go b/typed/remove_test.go index df59c16..7685065 100644 --- a/typed/remove_test.go +++ b/typed/remove_test.go @@ -281,7 +281,7 @@ var removeCases = []removeTestCase{{ quadruplets: []removeQuadruplet{{ `{"setBool":[false]}`, _NS(_P("setBool", _V(false))), - `{"setBool":null}`, + `{"setBool":[]}`, `{"setBool":[false]}`, }, { `{"setBool":[false]}`, @@ -671,7 +671,7 @@ var removeCases = []removeTestCase{{ _NS( _P("mapOfMapsRecursive", "a"), ), - `{"mapOfMapsRecursive"}`, + `{"mapOfMapsRecursive":{}}`, `{"mapOfMapsRecursive": {"a":null}}`, }, { // second-level map @@ -679,7 +679,7 @@ var removeCases = []removeTestCase{{ _NS( _P("mapOfMapsRecursive", "a", "b"), ), - `{"mapOfMapsRecursive":{"a":null}}`, + `{"mapOfMapsRecursive":{"a":{}}}`, `{"mapOfMapsRecursive": {"a":{"b":null}}}`, }, { // third-level map @@ -687,7 +687,7 @@ var removeCases = []removeTestCase{{ _NS( _P("mapOfMapsRecursive", "a", "b", "c"), ), - `{"mapOfMapsRecursive":{"a":{"b":null}}}`, + `{"mapOfMapsRecursive":{"a":{"b":{}}}}`, `{"mapOfMapsRecursive": {"a":{"b":{"c":null}}}}`, }, { // empty list From 9138e08e899f3ef992a82589f7ee549da2761c59 Mon Sep 17 00:00:00 2001 From: Peter Jiang Date: Thu, 2 Oct 2025 17:16:08 -0700 Subject: [PATCH 2/3] comments Signed-off-by: Peter Jiang --- typed/remove.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/typed/remove.go b/typed/remove.go index c1c7a51..0db1734 100644 --- a/typed/remove.go +++ b/typed/remove.go @@ -99,12 +99,12 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) { continue } if isPrefixMatch { - // Removing nested items WITHIN this list item - preserve if it becomes empty + // Removing nested items within this list item and preserve if it becomes empty hadMatches = true wasMap := item.IsMap() wasList := item.IsList() item = removeItemsWithSchema(item, w.toRemove.WithPrefix(pe), w.schema, t.ElementType, w.shouldExtract) - // If recursive call returned null but we're removing items within (not the item itself), + // If item returned null but we're removing items within the structure(not the item itself), // preserve the empty container structure if item.IsNull() && !w.shouldExtract { if wasMap { @@ -167,7 +167,6 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { // save values on the path when we shouldExtract // but ignore them when we are removing (i.e. !w.shouldExtract) if w.toRemove.Has(path) { - // Exact match: removing this field itself, not items within it if w.shouldExtract { newMap[k] = removeItemsWithSchema(val, w.toRemove, w.schema, fieldType, w.shouldExtract).Unstructured() @@ -179,7 +178,7 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { wasMap := val.IsMap() wasList := val.IsList() val = removeItemsWithSchema(val, subset, w.schema, fieldType, w.shouldExtract) - // If recursive call returned null but we're removing items within (not the field itself), + // If val returned null but we're removing items within the structure (not the field itself), // preserve the empty container structure if val.IsNull() && !w.shouldExtract { if wasMap { From c760b1d1f8f9f08d3282a7b28c3f1ffc38a9469c Mon Sep 17 00:00:00 2001 From: Peter Jiang Date: Fri, 3 Oct 2025 14:54:08 -0700 Subject: [PATCH 3/3] fix merge test for new behavior Signed-off-by: Peter Jiang --- merge/multiple_appliers_test.go | 30 +++++++++++++++--------------- merge/nested_test.go | 4 ++-- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/merge/multiple_appliers_test.go b/merge/multiple_appliers_test.go index 7fad7b1..48f54cd 100644 --- a/merge/multiple_appliers_test.go +++ b/merge/multiple_appliers_test.go @@ -1061,14 +1061,14 @@ func TestMultipleAppliersNestedType(t *testing.T) { }, }, Object: ` - mapOfMapsRecursive: - a: - c: - d: - e: - f: - g: - `, + mapOfMapsRecursive: + a: {} + c: + d: + e: + f: + g: + `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ "apply-one": fieldpath.NewVersionedSet( @@ -1187,13 +1187,13 @@ func TestMultipleAppliersDeducedType(t *testing.T) { }, }, Object: ` - a: - c: - d: - e: - f: - g: - `, + a: {} + c: + d: + e: + f: + g: + `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ "apply-two": fieldpath.NewVersionedSet( diff --git a/merge/nested_test.go b/merge/nested_test.go index ffdb07b..2da4d4c 100644 --- a/merge/nested_test.go +++ b/merge/nested_test.go @@ -554,8 +554,8 @@ func TestUpdateNestedType(t *testing.T) { }, }, Object: ` - struct: - `, + struct: {} + `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ "default": fieldpath.NewVersionedSet(