From e82e8b4e893610289dffd6a4b3e368a09ce41e7a Mon Sep 17 00:00:00 2001 From: Mikhail Mazurskiy Date: Tue, 3 Apr 2018 22:13:29 +1000 Subject: [PATCH 1/4] Make UnstructuredContent return contents without mutating the source --- pkg/printers/internalversion/describe_test.go | 4 +-- .../pkg/apis/meta/v1/unstructured/BUILD | 1 + .../apis/meta/v1/unstructured/unstructured.go | 2 +- .../meta/v1/unstructured/unstructured_list.go | 20 +++++++------ .../meta/v1/unstructured/unstructured_test.go | 30 +++++++++++++++++++ .../apimachinery/pkg/runtime/converter.go | 3 +- .../apimachinery/pkg/runtime/interfaces.go | 4 +-- .../apimachinery/pkg/runtime/testing/types.go | 2 +- 8 files changed, 49 insertions(+), 17 deletions(-) create mode 100644 staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go diff --git a/pkg/printers/internalversion/describe_test.go b/pkg/printers/internalversion/describe_test.go index a49b812d9fc..487325415e2 100644 --- a/pkg/printers/internalversion/describe_test.go +++ b/pkg/printers/internalversion/describe_test.go @@ -2133,14 +2133,14 @@ URL: http://localhost "name": "MyName", "namespace": "MyNamespace", "creationTimestamp": "2017-04-01T00:00:00Z", - "resourceVersion": 123, + "resourceVersion": int64(123), "uid": "00000000-0000-0000-0000-000000000001", "dummy3": "present", }, "items": []interface{}{ map[string]interface{}{ "itemBool": true, - "itemInt": 42, + "itemInt": int64(42), }, }, "url": "http://localhost", diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/BUILD b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/BUILD index 22c1acee07d..5f55c32d94e 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/BUILD +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/BUILD @@ -11,6 +11,7 @@ go_test( srcs = [ "helpers_test.go", "unstructured_list_test.go", + "unstructured_test.go", ], embed = [":go_default_library"], deps = [ diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go index 2a13330490a..b56437955d0 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go @@ -82,7 +82,7 @@ func (obj *Unstructured) EachListItem(fn func(runtime.Object) error) error { func (obj *Unstructured) UnstructuredContent() map[string]interface{} { if obj.Object == nil { - obj.Object = make(map[string]interface{}) + return make(map[string]interface{}) } return obj.Object } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go index 57d78a09dcc..fbcb386ee47 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go @@ -53,19 +53,21 @@ func (u *UnstructuredList) EachListItem(fn func(runtime.Object) error) error { } // UnstructuredContent returns a map contain an overlay of the Items field onto -// the Object field. Items always overwrites overlay. Changing "items" in the -// returned object will affect items in the underlying Items field, but changing -// the "items" slice itself will have no effect. -// TODO: expose SetUnstructuredContent on runtime.Unstructured that allows -// items to be changed. +// the Object field. Items always overwrites overlay. func (u *UnstructuredList) UnstructuredContent() map[string]interface{} { - out := u.Object - if out == nil { - out = make(map[string]interface{}) + out := make(map[string]interface{}, len(u.Object)+1) + + // shallow copy every property but items + for k, v := range u.Object { + if k == "items" { + continue + } + out[k] = v } + items := make([]interface{}, len(u.Items)) for i, item := range u.Items { - items[i] = item.Object + items[i] = item.UnstructuredContent() } out["items"] = items return out diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go new file mode 100644 index 00000000000..4021bedca09 --- /dev/null +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go @@ -0,0 +1,30 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package unstructured + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNilUnstructuredContent(t *testing.T) { + var u Unstructured + content := u.UnstructuredContent() + expContent := make(map[string]interface{}) + assert.EqualValues(t, expContent, content) +} diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go b/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go index f6f7c10de62..429857e88e9 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go @@ -411,8 +411,7 @@ func (c *unstructuredConverter) ToUnstructured(obj interface{}) (map[string]inte var u map[string]interface{} var err error if unstr, ok := obj.(Unstructured); ok { - // UnstructuredContent() mutates the object so we need to make a copy first - u = unstr.DeepCopyObject().(Unstructured).UnstructuredContent() + u = unstr.UnstructuredContent() } else { t := reflect.TypeOf(obj) value := reflect.ValueOf(obj) diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go b/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go index 9d00f1650e9..cfe819e0a28 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go @@ -234,9 +234,9 @@ type Object interface { // to JSON allowed. type Unstructured interface { Object - // UnstructuredContent returns a non-nil, mutable map of the contents of this object. Values may be + // UnstructuredContent returns a non-nil map with this object's contents. Values may be // []interface{}, map[string]interface{}, or any primitive type. Contents are typically serialized to - // and from JSON. + // and from JSON. SetUnstructuredContent should be used to mutate the contents. UnstructuredContent() map[string]interface{} // SetUnstructuredContent updates the object content to match the provided map. SetUnstructuredContent(map[string]interface{}) diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/testing/types.go b/staging/src/k8s.io/apimachinery/pkg/runtime/testing/types.go index 0694e701122..b8d67061d4b 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/testing/types.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/testing/types.go @@ -263,7 +263,7 @@ func (obj *Unstructured) EachListItem(fn func(runtime.Object) error) error { func (obj *Unstructured) UnstructuredContent() map[string]interface{} { if obj.Object == nil { - obj.Object = make(map[string]interface{}) + return make(map[string]interface{}) } return obj.Object } From d5fdac399c45d94df4203d4d0934d1e9989f9ec1 Mon Sep 17 00:00:00 2001 From: Mikhail Mazurskiy Date: Wed, 4 Apr 2018 20:54:53 +1000 Subject: [PATCH 2/4] Remove check for items --- .../pkg/apis/meta/v1/unstructured/unstructured_list.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go index fbcb386ee47..bf3fd023f4d 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go @@ -57,11 +57,8 @@ func (u *UnstructuredList) EachListItem(fn func(runtime.Object) error) error { func (u *UnstructuredList) UnstructuredContent() map[string]interface{} { out := make(map[string]interface{}, len(u.Object)+1) - // shallow copy every property but items + // shallow copy every property for k, v := range u.Object { - if k == "items" { - continue - } out[k] = v } From 53e8fd04ec39034b28d3a13920548e9761f6ac70 Mon Sep 17 00:00:00 2001 From: Mikhail Mazurskiy Date: Wed, 4 Apr 2018 22:27:21 +1000 Subject: [PATCH 3/4] Support typed nils; test empty Unstructured is not mutated --- pkg/printers/internalversion/describe_test.go | 4 ++-- .../apis/meta/v1/unstructured/unstructured_test.go | 4 +++- .../k8s.io/apimachinery/pkg/runtime/converter.go | 13 ++++++++++++- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/pkg/printers/internalversion/describe_test.go b/pkg/printers/internalversion/describe_test.go index 487325415e2..a49b812d9fc 100644 --- a/pkg/printers/internalversion/describe_test.go +++ b/pkg/printers/internalversion/describe_test.go @@ -2133,14 +2133,14 @@ URL: http://localhost "name": "MyName", "namespace": "MyNamespace", "creationTimestamp": "2017-04-01T00:00:00Z", - "resourceVersion": int64(123), + "resourceVersion": 123, "uid": "00000000-0000-0000-0000-000000000001", "dummy3": "present", }, "items": []interface{}{ map[string]interface{}{ "itemBool": true, - "itemInt": int64(42), + "itemInt": 42, }, }, "url": "http://localhost", diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go index 4021bedca09..cbcbbcef34b 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go @@ -1,5 +1,5 @@ /* -Copyright 2017 The Kubernetes Authors. +Copyright 2018 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -24,7 +24,9 @@ import ( func TestNilUnstructuredContent(t *testing.T) { var u Unstructured + uCopy := u.DeepCopy() content := u.UnstructuredContent() expContent := make(map[string]interface{}) assert.EqualValues(t, expContent, content) + assert.Equal(t, uCopy, &u) } diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go b/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go index 429857e88e9..66125dbef23 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go @@ -446,20 +446,31 @@ func DeepCopyJSON(x map[string]interface{}) map[string]interface{} { // DeepCopyJSONValue deep copies the passed value, assuming it is a valid JSON representation i.e. only contains // types produced by json.Unmarshal(). func DeepCopyJSONValue(x interface{}) interface{} { + if x == nil { + return nil + } switch x := x.(type) { case map[string]interface{}: + if x == nil { + // Typed nil - an interface{} that contains a type map[string]interface{} with a value of nil + return x + } clone := make(map[string]interface{}, len(x)) for k, v := range x { clone[k] = DeepCopyJSONValue(v) } return clone case []interface{}: + if x == nil { + // Typed nil - an interface{} that contains a type []interface{} with a value of nil + return x + } clone := make([]interface{}, len(x)) for i, v := range x { clone[i] = DeepCopyJSONValue(v) } return clone - case string, int64, bool, float64, nil, encodingjson.Number: + case string, int64, bool, float64, encodingjson.Number: return x default: panic(fmt.Errorf("cannot deep copy %T", x)) From 1fcd199cf7366a15f0e94f2892cb5b2cb5c27efa Mon Sep 17 00:00:00 2001 From: Mikhail Mazurskiy Date: Thu, 5 Apr 2018 20:40:20 +1000 Subject: [PATCH 4/4] Put nil back into switch --- staging/src/k8s.io/apimachinery/pkg/runtime/converter.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go b/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go index 66125dbef23..91920535d36 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go @@ -446,9 +446,6 @@ func DeepCopyJSON(x map[string]interface{}) map[string]interface{} { // DeepCopyJSONValue deep copies the passed value, assuming it is a valid JSON representation i.e. only contains // types produced by json.Unmarshal(). func DeepCopyJSONValue(x interface{}) interface{} { - if x == nil { - return nil - } switch x := x.(type) { case map[string]interface{}: if x == nil { @@ -470,7 +467,7 @@ func DeepCopyJSONValue(x interface{}) interface{} { clone[i] = DeepCopyJSONValue(v) } return clone - case string, int64, bool, float64, encodingjson.Number: + case string, int64, bool, float64, nil, encodingjson.Number: return x default: panic(fmt.Errorf("cannot deep copy %T", x))