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

alexzielenski · 2024-05-23T14:05:30Z

Consider old object:

{
  requiredField: {
      optionalField1: 1
      optionalFIeld2: 2
   }
}

And patch, all with a single owner

{
    requiredField: {}
}

I expect this to yield:

{
    requiredField: {}
}

But today it yields:

{
    requiredField: nil
}

Which results in an error on k8s since nil is not an acceptable value for this requiredField

k8s-ci-robot · 2024-05-23T14:05:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alexzielenski
Once this PR has been reviewed and has the lgtm label, please assign apelisse for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alexzielenski · 2024-05-23T14:05:38Z

/cc @apelisse

k8s-ci-robot · 2024-05-23T14:06:04Z

@alexzielenski: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-structured-merge-diff-test	`d7c26a3`	link	true	`/test pull-structured-merge-diff-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

apelisse · 2024-05-23T16:24:46Z

Can we take the opportunity to summarize the various use-cases here that we've found so far? I'm getting a bit lost by the different possibilities and the differences they have. I suspect we should create a table for what happens when nil/empty is sent in various cases because there's a lot of confusion here.

apelisse · 2024-05-23T16:35:09Z

typed/remove.go

@@ -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 {


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

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 []

apelisse · 2024-05-23T16:36:17Z

typed/update_test.go

+import (
+	"testing"
+
+	"sigs.k8s.io/structured-merge-diff/v4/internal/fixture"


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

apelisse · 2024-05-23T16:41:13Z

typed/update_test.go

+			},
+			APIVersion: `v1`,
+			Object:     `{nestedMap: {}}`,
+		},


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

What happens if we apply null instead.

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)

How do you entirely remove the field? If null keeps null and {} keep it to empty, then how do you remove it altogether?

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: {}}

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

apelisse · 2024-05-23T16:42:43Z

One last thing, I suspect lists and maps should behave the same. Struct, I'm not entirely sure. maybe they should?

alexzielenski · 2024-05-23T16:50:14Z

One last thing, I suspect lists and maps should behave the same. Struct, I'm not entirely sure. maybe they should?

This bug also affects structs. There are valid CRDs that can be constructed with defined object fields where {} is valid value users should be able to set. Today they cannot

ybettan · 2024-07-02T18:32:51Z

Hello.

Is this fix planned to be merged soon? We currently have to bump our API cause we need to add the +nullable kubebuilder annotation to a bunch of API object and adding a new mutating webhook is just breaking any pointers/slices in the API objects.

Ref: kubernetes-sigs/kernel-module-management#826

k8s-triage-robot · 2024-09-30T19:11:26Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-10-30T19:50:53Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

ybettan · 2024-11-12T10:50:27Z

/remove-lifecycle rotten

POC

d7c26a3

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 23, 2024

k8s-ci-robot requested review from apelisse and jpbetz May 23, 2024 14:05

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 23, 2024

apelisse reviewed May 23, 2024

View reviewed changes

This was referenced May 23, 2024

Invalid value: "null" when field is empty in configuration and SSA is used to apply kubernetes/kubernetes#104694

Closed

'spec in body must be of type object: "null"' when removing a field using SSA kubernetes/kubernetes#117447

Open

alexzielenski mentioned this pull request Jun 21, 2024

Fix non-semantic apply requests to ignore empty maps kubernetes/kubernetes#125317

Merged

ybettan mentioned this pull request Jul 2, 2024

Defaulting to the latest tag if no tag was specified. kubernetes-sigs/kernel-module-management#826

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 30, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 30, 2024

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alexzielenski commented May 23, 2024

k8s-ci-robot commented May 23, 2024

alexzielenski commented May 23, 2024

k8s-ci-robot commented May 23, 2024

apelisse commented May 23, 2024 •

edited

Loading

apelisse May 23, 2024

alexzielenski May 23, 2024

apelisse May 23, 2024

apelisse May 23, 2024

alexzielenski May 23, 2024 •

edited

Loading

alexzielenski May 23, 2024

apelisse commented May 23, 2024

alexzielenski commented May 23, 2024

ybettan commented Jul 2, 2024

k8s-triage-robot commented Sep 30, 2024

k8s-triage-robot commented Oct 30, 2024

ybettan commented Nov 12, 2024

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

Are you sure you want to change the base?

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

Conversation

alexzielenski commented May 23, 2024

k8s-ci-robot commented May 23, 2024

alexzielenski commented May 23, 2024

k8s-ci-robot commented May 23, 2024

apelisse commented May 23, 2024 • edited Loading

apelisse May 23, 2024

Choose a reason for hiding this comment

alexzielenski May 23, 2024

Choose a reason for hiding this comment

apelisse May 23, 2024

Choose a reason for hiding this comment

apelisse May 23, 2024

Choose a reason for hiding this comment

alexzielenski May 23, 2024 • edited Loading

Choose a reason for hiding this comment

alexzielenski May 23, 2024

Choose a reason for hiding this comment

apelisse commented May 23, 2024

alexzielenski commented May 23, 2024

ybettan commented Jul 2, 2024

k8s-triage-robot commented Sep 30, 2024

k8s-triage-robot commented Oct 30, 2024

ybettan commented Nov 12, 2024

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

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

apelisse commented May 23, 2024 •

edited

Loading

alexzielenski May 23, 2024 •

edited

Loading