Skip to content

Specifying {} to remove nested fields should yield {}, not nil #259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Specifying {} to remove nested fields should yield {}, not nil #259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions typed/remove.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import (
"sigs.k8s.io/structured-merge-diff/v4/value"
)

var REMOVEKEEPEMPTYCOLLECTIONS = false

type removingWalker struct {
value value.Value
out interface{}
Expand Down Expand Up @@ -58,6 +60,9 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) {
defer w.allocator.Free(l)
// If list is null or empty just return
if l == nil || l.Length() == 0 {
if REMOVEKEEPEMPTYCOLLECTIONS {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we treat separately empty and nil here? Is that part of the problem?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change wasn't directly part of the problem I was trying to solve but I think something like this will be needed to have a consistent solution. The behavior of this right now always results in a nil list even if you specify []

w.out = w.value.Unstructured()
}
return nil
}

Expand All @@ -66,6 +71,10 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) {
if t.ElementRelationship == schema.Atomic {
if w.shouldExtract {
w.out = w.value.Unstructured()
} else if !w.toRemove.Has(fieldpath.Path{}) && REMOVEKEEPEMPTYCOLLECTIONS {
// If the atomic list itself wasn't being removed, then it
// is being set to empty list (we only get here if a prefix of this fieldPath is in toRemove)
w.out = []interface{}{}
}
return nil
}
Expand Down Expand Up @@ -97,7 +106,7 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) {
}
newItems = append(newItems, item.Unstructured())
}
if len(newItems) > 0 {
if len(newItems) > 0 || REMOVEKEEPEMPTYCOLLECTIONS {
w.out = newItems
}
return nil
Expand All @@ -113,6 +122,9 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors {
}
// If map is null or empty just return
if m == nil || m.Empty() {
if REMOVEKEEPEMPTYCOLLECTIONS {
w.out = w.value
}
return nil
}

Expand All @@ -121,6 +133,10 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors {
if t.ElementRelationship == schema.Atomic {
if w.shouldExtract {
w.out = w.value.Unstructured()
} else if !w.toRemove.Has(fieldpath.Path{}) && REMOVEKEEPEMPTYCOLLECTIONS {
// If the atomic map itself wasn't being removed, then it
// is being set to empty map (we only get here if a prefix of this fieldPath is in toRemove)
w.out = map[string]interface{}{}
}
return nil
}
Expand Down Expand Up @@ -158,7 +174,7 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors {
newMap[k] = val.Unstructured()
return true
})
if len(newMap) > 0 {
if len(newMap) > 0 || REMOVEKEEPEMPTYCOLLECTIONS {
w.out = newMap
}
return nil
Expand Down
141 changes: 141 additions & 0 deletions typed/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
package typed_test

import (
"testing"

"sigs.k8s.io/structured-merge-diff/v4/internal/fixture"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't the other tests usually importing everything from fixture?

"sigs.k8s.io/structured-merge-diff/v4/typed"
)

var updateParser = func() fixture.Parser {
parser, err := typed.NewParser(`
types:
- name: nestedOptionalFields
map:
fields:
- name: nestedList
type:
list:
elementRelationship: associative
keys:
- name
elementType:
map:
fields:
- name: name
type:
scalar: string
- name: value
type:
scalar: numeric
- name: nestedMap
type:
map:
elementType:
scalar: numeric
- name: nested
type:
map:
fields:
- name: numeric
type:
scalar: numeric
- name: string
type:
scalar: string
`)
if err != nil {
panic(err)
}
return fixture.SameVersionParser{T: parser.Type("nestedOptionalFields")}
}()

func TestUpdate(t *testing.T) {
tests := map[string]fixture.TestCase{
"delete_nested_fields_struct": {
Ops: []fixture.Operation{
fixture.Apply{
Manager: "default",
APIVersion: "v1",
Object: `
nested:
numeric: 1
string: my string
`,
},
fixture.Apply{
Manager: "default",
APIVersion: "v1",
Object: `
nested: {}
`,
},
},
APIVersion: `v1`,
Object: `{nested: {}}`,
},
"delete_nested_fields_list": {
Ops: []fixture.Operation{
fixture.Apply{
Manager: "default",
APIVersion: "v1",
Object: `
nestedList:
- name: first
- name: second
`,
},
fixture.Apply{
Manager: "default",
APIVersion: "v1",
Object: `
nestedList: []
`,
},
},
APIVersion: `v1`,
Object: `{nestedList: []}`,
},
"delete_nested_fields_map": {
Ops: []fixture.Operation{
fixture.Apply{
Manager: "default",
APIVersion: "v1",
Object: `
nestedMap:
first: 1
second: 2

`,
},
fixture.Apply{
Manager: "default",
APIVersion: "v1",
Object: `
nestedMap: {}
`,
},
},
APIVersion: `v1`,
Object: `{nestedMap: {}}`,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so I guess now I'm confused about:

  1. What happens if we apply null instead.
  2. What happens if you have further level of nesting (i.e. you're apply {} when you had {'nested': {'numeric':1}} (I'm guessing the same but would love to see)
  3. How do you entirely remove the field? If null keeps null and {} keep it to empty, then how do you remove it altogether?

Copy link
Member Author

@alexzielenski alexzielenski May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add cases for 1 & 2

Re: 3. If you wanted to make {nestedMap: { myField: value }} into {} you would just apply {}. If you applied {} then nestedMap field is included in the toRemove set. If you had applied {nestedMap: {}} then only nestedMap.myField is in the toRemove set, so nestedMap would be retained yielding {nestedMap: {}}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the null case naively I would expect it to keep null if you specified null.

}

for name, tc := range tests {
tc2 := tc
t.Run(name, func(t *testing.T) {
typed.REMOVEKEEPEMPTYCOLLECTIONS = true
if err := tc.Test(updateParser); err != nil {
t.Fatal(err)
}
})

t.Run(name+"Nil", func(t *testing.T) {
typed.REMOVEKEEPEMPTYCOLLECTIONS = false
if err := tc2.Test(updateParser); err != nil {
t.Fatal(err)
}
})
}

}