From 9b5d9c70fc45c0fe317fdf98fe34422dbc9f2c05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arda=20G=C3=BC=C3=A7l=C3=BC?= Date: Sun, 23 Jan 2022 11:39:02 +0300 Subject: [PATCH 1/2] Discard null values in complex objects in strategic patch In strategic patch, if key does not exist in original, value of this new key is not distilled by discarding the null values. That brings about in key creation step, null values are also patched. However, during the update process of this key in subsequent patches, these null values are deleted and this hurts the idempotency of strategic patch. This PR adds discard mechanism for null values if key does not exist in original. It traverses all nested objects. --- .../pkg/util/strategicpatch/patch.go | 35 +++++- .../pkg/util/strategicpatch/patch_test.go | 100 ++++++++++++++++++ 2 files changed, 133 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go index 1aedaa1563f..e0406baf02a 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go @@ -1330,7 +1330,11 @@ func mergeMap(original, patch map[string]interface{}, schema LookupPatchMeta, me if !ok { if !isDeleteList { // If it's not in the original document, just take the patch value. - original[k] = patchV + if mergeOptions.IgnoreUnmatchedNulls { + original[k] = discardNullValuesFromPatch(patchV) + } else { + original[k] = patchV + } } continue } @@ -1339,7 +1343,11 @@ func mergeMap(original, patch map[string]interface{}, schema LookupPatchMeta, me patchType := reflect.TypeOf(patchV) if originalType != patchType { if !isDeleteList { - original[k] = patchV + if mergeOptions.IgnoreUnmatchedNulls { + original[k] = discardNullValuesFromPatch(patchV) + } else { + original[k] = patchV + } } continue } @@ -1375,6 +1383,29 @@ func mergeMap(original, patch map[string]interface{}, schema LookupPatchMeta, me return original, nil } +// discardNullValuesFromPatch discards all null values from patch. +// It traverses for all slices and map types to discard all nulls. +func discardNullValuesFromPatch(patchV interface{}) interface{} { + switch patchV.(type) { + case map[string]interface{}: + for k, v := range patchV.(map[string]interface{}) { + if v == nil { + delete(patchV.(map[string]interface{}), k) + } else { + discardNullValuesFromPatch(v) + } + } + case []interface{}: + patchS := patchV.([]interface{}) + for i, v := range patchV.([]interface{}) { + patchS[i] = discardNullValuesFromPatch(v) + } + return patchS + } + + return patchV +} + // mergeMapHandler handles how to merge `patchV` whose key is `key` with `original` respecting // fieldPatchStrategy and mergeOptions. func mergeMapHandler(original, patch interface{}, schema LookupPatchMeta, diff --git a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go index 5fe78133cb3..f895f234c58 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go @@ -6713,6 +6713,106 @@ func TestUnknownField(t *testing.T) { ExpectedThreeWay: `{}`, ExpectedThreeWayResult: `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`, }, + "no diff even if modified null": { + Original: `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`, + Current: `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`, + Modified: `{"array":[1,2,3],"complex":{"nested":true},"complex_nullable":{"key":null},"name":"foo","scalar":true}`, + + ExpectedTwoWay: `{"complex_nullable":{"key":null}}`, + ExpectedTwoWayResult: `{"array":[1,2,3],"complex":{"nested":true},"complex_nullable":{},"name":"foo","scalar":true}`, + ExpectedThreeWay: `{"complex_nullable":{"key":null}}`, + ExpectedThreeWayResult: `{"array":[1,2,3],"complex":{"nested":true},"complex_nullable":{},"name":"foo","scalar":true}`, + }, + "discard nulls in nested and adds not nulls": { + Original: `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`, + Current: `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`, + Modified: `{"array":[1,2,3],"complex":{"nested":true},"complex_nullable":{"key":{"keynotnull":"value","keynull":null}},"name":"foo","scalar":true}`, + + ExpectedTwoWay: `{"complex_nullable":{"key":{"keynotnull":"value","keynull":null}}}`, + ExpectedTwoWayResult: `{"array":[1,2,3],"complex":{"nested":true},"complex_nullable":{"key":{"keynotnull":"value"}},"name":"foo","scalar":true}`, + ExpectedThreeWay: `{"complex_nullable":{"key":{"keynotnull":"value","keynull":null}}}`, + ExpectedThreeWayResult: `{"array":[1,2,3],"complex":{"nested":true},"complex_nullable":{"key":{"keynotnull":"value"}},"name":"foo","scalar":true}`, + }, + "discard if modified all nulls": { + Original: `{}`, + Current: `{}`, + Modified: `{"complex":{"nested":null}}`, + + ExpectedTwoWay: `{"complex":{"nested":null}}`, + ExpectedTwoWayResult: `{"complex":{}}`, + ExpectedThreeWay: `{"complex":{"nested":null}}`, + ExpectedThreeWayResult: `{"complex":{}}`, + }, + "add only not nulls": { + Original: `{}`, + Current: `{}`, + Modified: `{"complex":{"nested":null,"nested2":"foo"}}`, + + ExpectedTwoWay: `{"complex":{"nested":null,"nested2":"foo"}}`, + ExpectedTwoWayResult: `{"complex":{"nested2":"foo"}}`, + ExpectedThreeWay: `{"complex":{"nested":null,"nested2":"foo"}}`, + ExpectedThreeWayResult: `{"complex":{"nested2":"foo"}}`, + }, + "null values in original are preserved": { + Original: `{"thing":null}`, + Current: `{"thing":null}`, + Modified: `{"nested":{"value":5},"thing":null}`, + + ExpectedTwoWay: `{"nested":{"value":5}}`, + ExpectedTwoWayResult: `{"nested":{"value":5},"thing":null}`, + ExpectedThreeWay: `{"nested":{"value":5}}`, + ExpectedThreeWayResult: `{"nested":{"value":5},"thing":null}`, + }, + "nested null values in original are preserved": { + Original: `{"complex":{"key":null},"thing":null}`, + Current: `{"complex":{"key":null},"thing":null}`, + Modified: `{"complex":{"key":null},"nested":{"value":5},"thing":null}`, + + ExpectedTwoWay: `{"nested":{"value":5}}`, + ExpectedTwoWayResult: `{"complex":{"key":null},"nested":{"value":5},"thing":null}`, + ExpectedThreeWay: `{"nested":{"value":5}}`, + ExpectedThreeWayResult: `{"complex":{"key":null},"nested":{"value":5},"thing":null}`, + }, + "add empty slices": { + Original: `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`, + Current: `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`, + Modified: `{"array":[1,2,3],"complex":{"nested":true},"complex_nullable":[],"name":"foo","scalar":true}`, + + ExpectedTwoWay: `{"complex_nullable":[]}`, + ExpectedTwoWayResult: `{"array":[1,2,3],"complex":{"nested":true},"complex_nullable":[],"name":"foo","scalar":true}`, + ExpectedThreeWay: `{"complex_nullable":[]}`, + ExpectedThreeWayResult: `{"array":[1,2,3],"complex":{"nested":true},"complex_nullable":[],"name":"foo","scalar":true}`, + }, + "filter nulls from nested slices": { + Original: `{}`, + Current: `{}`, + Modified: `{"complex_nullable":[{"inner_one":{"key_one":"foo","key_two":null}}]}`, + + ExpectedTwoWay: `{"complex_nullable":[{"inner_one":{"key_one":"foo","key_two":null}}]}`, + ExpectedTwoWayResult: `{"complex_nullable":[{"inner_one":{"key_one":"foo"}}]}`, + ExpectedThreeWay: `{"complex_nullable":[{"inner_one":{"key_one":"foo","key_two":null}}]}`, + ExpectedThreeWayResult: `{"complex_nullable":[{"inner_one":{"key_one":"foo"}}]}`, + }, + "filter if slice is all empty": { + Original: `{}`, + Current: `{}`, + Modified: `{"complex_nullable":[{"inner_one":{"key_one":null,"key_two":null}}]}`, + + ExpectedTwoWay: `{"complex_nullable":[{"inner_one":{"key_one":null,"key_two":null}}]}`, + ExpectedTwoWayResult: `{"complex_nullable":[{"inner_one":{}}]}`, + ExpectedThreeWay: `{"complex_nullable":[{"inner_one":{"key_one":null,"key_two":null}}]}`, + ExpectedThreeWayResult: `{"complex_nullable":[{"inner_one":{}}]}`, + }, + "not filter nulls from non-associative slice": { + Original: `{}`, + Current: `{}`, + Modified: `{"complex_nullable":["key1",null,"key2"]}`, + + ExpectedTwoWay: `{"complex_nullable":["key1",null,"key2"]}`, + ExpectedTwoWayResult: `{"complex_nullable":["key1",null,"key2"]}`, + ExpectedThreeWay: `{"complex_nullable":["key1",null,"key2"]}`, + ExpectedThreeWayResult: `{"complex_nullable":["key1",null,"key2"]}`, + }, "added only": { Original: `{"name":"foo"}`, Current: `{"name":"foo"}`, From 0ee00ba10438a9a010d10fcc4e2301dc05fe7b68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arda=20G=C3=BC=C3=A7l=C3=BC?= Date: Wed, 16 Feb 2022 19:43:35 +0300 Subject: [PATCH 2/2] Simplify casting in discardNullValuesFromPatch --- .../pkg/util/strategicpatch/patch.go | 30 ++++++++----------- 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go index e0406baf02a..6fb36973213 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go @@ -1331,10 +1331,9 @@ func mergeMap(original, patch map[string]interface{}, schema LookupPatchMeta, me if !isDeleteList { // If it's not in the original document, just take the patch value. if mergeOptions.IgnoreUnmatchedNulls { - original[k] = discardNullValuesFromPatch(patchV) - } else { - original[k] = patchV + discardNullValuesFromPatch(patchV) } + original[k] = patchV } continue } @@ -1344,10 +1343,9 @@ func mergeMap(original, patch map[string]interface{}, schema LookupPatchMeta, me if originalType != patchType { if !isDeleteList { if mergeOptions.IgnoreUnmatchedNulls { - original[k] = discardNullValuesFromPatch(patchV) - } else { - original[k] = patchV + discardNullValuesFromPatch(patchV) } + original[k] = patchV } continue } @@ -1383,27 +1381,23 @@ func mergeMap(original, patch map[string]interface{}, schema LookupPatchMeta, me return original, nil } -// discardNullValuesFromPatch discards all null values from patch. -// It traverses for all slices and map types to discard all nulls. -func discardNullValuesFromPatch(patchV interface{}) interface{} { - switch patchV.(type) { +// discardNullValuesFromPatch discards all null property values from patch. +// It traverses all slices and map types. +func discardNullValuesFromPatch(patchV interface{}) { + switch patchV := patchV.(type) { case map[string]interface{}: - for k, v := range patchV.(map[string]interface{}) { + for k, v := range patchV { if v == nil { - delete(patchV.(map[string]interface{}), k) + delete(patchV, k) } else { discardNullValuesFromPatch(v) } } case []interface{}: - patchS := patchV.([]interface{}) - for i, v := range patchV.([]interface{}) { - patchS[i] = discardNullValuesFromPatch(v) + for _, v := range patchV { + discardNullValuesFromPatch(v) } - return patchS } - - return patchV } // mergeMapHandler handles how to merge `patchV` whose key is `key` with `original` respecting