Skip to content
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

Jump to bottom

Make UnstructuredContent return contents without mutating the source #62063

Merged
merged 4 commits into from
Apr 6, 2018
Merged

Make UnstructuredContent return contents without mutating the source #62063

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e82e8b4
Make UnstructuredContent return contents without mutating the source
ash2k Apr 3, 2018
d5fdac3
Remove check for items
ash2k Apr 4, 2018
53e8fd0
Support typed nils; test empty Unstructured is not mutated
ash2k Apr 4, 2018
1fcd199
Put nil back into switch
ash2k Apr 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/printers/internalversion/describe_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
4 changes: 3 additions & 1 deletion staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check that the actual object stays the same? Make a deep copy before and then compare afterwards.

assert.Equal(t, uCopy, &u)
}
13 changes: 12 additions & 1 deletion staging/src/k8s.io/apimachinery/pkg/runtime/converter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this really exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, without this change (and below for completeness) the newly added check in the test (deep copy and compare) was failing. https://play.golang.org/p/KASoa2DeTnp

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 will change the output - this code used to produce non-nil maps and slices for typed nils but now will produce typed nils. JSON encoding will change - typed nils as null in JSON (https://play.golang.org/p/Uz2ajLEQx9A).

I think this change is a good one - it makes the deep copy more precise.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should never see typed nils somewhere deep inside IMO. On the toplevel maybe.

Compare https://play.golang.org/p/R45uKKqDsm6

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we should not see them, but they exist. I'm fine with removing this fix from this PR if that's what you think is best. However I think that way deep copy is more correct. It just mirrors the input precisely. If there are no typed nils, we don't introduce them here. If they create a problem somewhere then the place that "spawns" them should be fixed. Or, even better, a place that does not handle them well could be made more robust.

Also, I'm not sure what exactly you wanted me to compare? typed and untyped nils are both marshaled into null, yes :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Have we ever seen this "cannot deep copy" panic? You say that they show up. Do you know where?

Copy link
Member Author

Choose a reason for hiding this comment

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

Panic? Current code does not cause any panics AFAIK. Are you talking about failing test without these changes? It does not panic, it just fails because objects are not equal. They are not equal because the deep copied Object field ("expected" in the test) is an empty map and the original is `nil.

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)
}
--- FAIL: TestNilUnstructuredContent (0.00s)
	Error Trace:	unstructured_test.go:31
	Error:      	Not equal: 
	            	expected: &unstructured.Unstructured{Object:map[string]interface {}{}}
	            	actual: &unstructured.Unstructured{Object:map[string]interface {}(nil)}
	            	
	            	Diff:
	            	--- Expected
	            	+++ Actual
	            	@@ -1,4 +1,3 @@
	            	 (*unstructured.Unstructured)({
	            	- Object: (map[string]interface {}) {
	            	- }
	            	+ Object: (map[string]interface {}) <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:
Copy link
Contributor

Choose a reason for hiding this comment

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

why not keep the nil and remove the if clause above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. My IntelliJ is complaining that nil is not a valid type but Go compiler is happy 🤷‍♂️ It actually works fine https://play.golang.org/p/PBIyhFgWn8H
I'll revert this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't feel strong about it. Was just curious.

return x
default:
panic(fmt.Errorf("cannot deep copy %T", x))
Expand Down