Merge pull request #2350 from smarterclayton/util_encodejson_proven_harmful

util.EncodeJSON proven harmful, remove it everywhere

pkg/cloudprovider/controller/minioncontroller_test.go

@@ -30,7 +30,7 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
-func newMinionList(count int) api.MinionList {
+func newMinionList(count int) *api.MinionList {
 	minions := []api.Minion{}
 	for i := 0; i < count; i++ {
 		minions = append(minions, api.Minion{
@@ -39,7 +39,7 @@ func newMinionList(count int) api.MinionList {
 			},
 		})
 	}
-	return api.MinionList{
+	return &api.MinionList{
 		Items: minions,
 	}
 }
@@ -52,7 +52,7 @@ type serverResponse struct {
 func makeTestServer(t *testing.T, minionResponse serverResponse) (*httptest.Server, *util.FakeHandler) {
 	fakeMinionHandler := util.FakeHandler{
 		StatusCode:   minionResponse.statusCode,
-		ResponseBody: util.EncodeJSON(minionResponse.obj),
+		ResponseBody: runtime.EncodeOrDie(testapi.Codec(), minionResponse.obj.(runtime.Object)),
 	}
 	mux := http.NewServeMux()
 	mux.Handle("/api/"+testapi.Version()+"/minions", &fakeMinionHandler)

pkg/controller/replication_controller_test.go

@@ -284,10 +284,10 @@ func TestSynchonize(t *testing.T) {
 			Node: &etcd.Node{
 				Nodes: []*etcd.Node{
 					{
-						Value: util.EncodeJSON(controllerSpec1),
+						Value: runtime.EncodeOrDie(testapi.Codec(), &controllerSpec1),
 					},
 					{
-						Value: util.EncodeJSON(controllerSpec2),
+						Value: runtime.EncodeOrDie(testapi.Codec(), &controllerSpec2),
 					},
 				},
 			},

pkg/kubelet/server_test.go

@@ -30,7 +30,6 @@ import (
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 	"github.com/google/cadvisor/info"
 )
 
@@ -97,6 +96,15 @@ func newServerTest() *serverTestFramework {
 	return fw
 }
 
+// encodeJSON returns obj marshalled as a JSON string, panicing on any errors
+func encodeJSON(obj interface{}) string {
+	data, err := json.Marshal(obj)
+	if err != nil {
+		panic(err)
+	}
+	return string(data)
+}
+
 func readResp(resp *http.Response) (string, error) {
 	defer resp.Body.Close()
 	body, err := ioutil.ReadAll(resp.Body)
@@ -124,7 +132,7 @@ func TestContainer(t *testing.T) {
 			},
 		},
 	}
-	body := bytes.NewBuffer([]byte(util.EncodeJSON(expected[0]))) // Only send a single ContainerManifest
+	body := bytes.NewBuffer([]byte(encodeJSON(expected[0]))) // Only send a single ContainerManifest
 	resp, err := http.Post(fw.testHTTPServer.URL+"/container", "application/json", body)
 	if err != nil {
 		t.Errorf("Post returned: %v", err)
@@ -199,7 +207,7 @@ func TestContainers(t *testing.T) {
 			},
 		},
 	}
-	body := bytes.NewBuffer([]byte(util.EncodeJSON(expected)))
+	body := bytes.NewBuffer([]byte(encodeJSON(expected)))
 	resp, err := http.Post(fw.testHTTPServer.URL+"/containers", "application/json", body)
 	if err != nil {
 		t.Errorf("Post returned: %v", err)

pkg/service/endpoints_controller_test.go

@@ -30,7 +30,7 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
-func newPodList(count int) api.PodList {
+func newPodList(count int) *api.PodList {
 	pods := []api.Pod{}
 	for i := 0; i < count; i++ {
 		pods = append(pods, api.Pod{
@@ -54,7 +54,7 @@ func newPodList(count int) api.PodList {
 			},
 		})
 	}
-	return api.PodList{
+	return &api.PodList{
 		TypeMeta: api.TypeMeta{APIVersion: testapi.Version(), Kind: "PodList"},
 		Items:    pods,
 	}
@@ -170,15 +170,15 @@ type serverResponse struct {
 func makeTestServer(t *testing.T, podResponse serverResponse, serviceResponse serverResponse, endpointsResponse serverResponse) (*httptest.Server, *util.FakeHandler) {
 	fakePodHandler := util.FakeHandler{
 		StatusCode:   podResponse.statusCode,
-		ResponseBody: util.EncodeJSON(podResponse.obj),
+		ResponseBody: runtime.EncodeOrDie(testapi.Codec(), podResponse.obj.(runtime.Object)),
 	}
 	fakeServiceHandler := util.FakeHandler{
 		StatusCode:   serviceResponse.statusCode,
-		ResponseBody: util.EncodeJSON(serviceResponse.obj),
+		ResponseBody: runtime.EncodeOrDie(testapi.Codec(), serviceResponse.obj.(runtime.Object)),
 	}
 	fakeEndpointsHandler := util.FakeHandler{
 		StatusCode:   endpointsResponse.statusCode,
-		ResponseBody: util.EncodeJSON(endpointsResponse.obj),
+		ResponseBody: runtime.EncodeOrDie(testapi.Codec(), endpointsResponse.obj.(runtime.Object)),
 	}
 	mux := http.NewServeMux()
 	mux.Handle("/api/"+testapi.Version()+"/pods", &fakePodHandler)
@@ -195,8 +195,8 @@ func makeTestServer(t *testing.T, podResponse serverResponse, serviceResponse se
 func TestSyncEndpointsEmpty(t *testing.T) {
 	testServer, _ := makeTestServer(t,
 		serverResponse{http.StatusOK, newPodList(0)},
-		serverResponse{http.StatusOK, api.ServiceList{}},
+		serverResponse{http.StatusOK, &api.ServiceList{}},
-		serverResponse{http.StatusOK, api.Endpoints{}})
+		serverResponse{http.StatusOK, &api.Endpoints{}})
 	defer testServer.Close()
 	client := client.NewOrDie(&client.Config{Host: testServer.URL, Version: testapi.Version()})
 	endpoints := NewEndpointController(client)
@@ -208,8 +208,8 @@ func TestSyncEndpointsEmpty(t *testing.T) {
 func TestSyncEndpointsError(t *testing.T) {
 	testServer, _ := makeTestServer(t,
 		serverResponse{http.StatusOK, newPodList(0)},
-		serverResponse{http.StatusInternalServerError, api.ServiceList{}},
+		serverResponse{http.StatusInternalServerError, &api.ServiceList{}},
-		serverResponse{http.StatusOK, api.Endpoints{}})
+		serverResponse{http.StatusOK, &api.Endpoints{}})
 	defer testServer.Close()
 	client := client.NewOrDie(&client.Config{Host: testServer.URL, Version: testapi.Version()})
 	endpoints := NewEndpointController(client)
@@ -233,8 +233,8 @@ func TestSyncEndpointsItemsPreexisting(t *testing.T) {
 	}
 	testServer, endpointsHandler := makeTestServer(t,
 		serverResponse{http.StatusOK, newPodList(1)},
-		serverResponse{http.StatusOK, serviceList},
+		serverResponse{http.StatusOK, &serviceList},
-		serverResponse{http.StatusOK, api.Endpoints{
+		serverResponse{http.StatusOK, &api.Endpoints{
 			ObjectMeta: api.ObjectMeta{
 				Name:            "foo",
 				ResourceVersion: "1",
@@ -272,8 +272,8 @@ func TestSyncEndpointsItemsPreexistingIdentical(t *testing.T) {
 	}
 	testServer, endpointsHandler := makeTestServer(t,
 		serverResponse{http.StatusOK, newPodList(1)},
-		serverResponse{http.StatusOK, serviceList},
+		serverResponse{http.StatusOK, &serviceList},
-		serverResponse{http.StatusOK, api.Endpoints{
+		serverResponse{http.StatusOK, &api.Endpoints{
 			ObjectMeta: api.ObjectMeta{
 				ResourceVersion: "1",
 			},
@@ -303,8 +303,8 @@ func TestSyncEndpointsItems(t *testing.T) {
 	}
 	testServer, endpointsHandler := makeTestServer(t,
 		serverResponse{http.StatusOK, newPodList(1)},
-		serverResponse{http.StatusOK, serviceList},
+		serverResponse{http.StatusOK, &serviceList},
-		serverResponse{http.StatusOK, api.Endpoints{}})
+		serverResponse{http.StatusOK, &api.Endpoints{}})
 	defer testServer.Close()
 	client := client.NewOrDie(&client.Config{Host: testServer.URL, Version: testapi.Version()})
 	endpoints := NewEndpointController(client)
@@ -333,9 +333,9 @@ func TestSyncEndpointsPodError(t *testing.T) {
 		},
 	}
 	testServer, _ := makeTestServer(t,
-		serverResponse{http.StatusInternalServerError, api.PodList{}},
+		serverResponse{http.StatusInternalServerError, &api.PodList{}},
-		serverResponse{http.StatusOK, serviceList},
+		serverResponse{http.StatusOK, &serviceList},
-		serverResponse{http.StatusOK, api.Endpoints{}})
+		serverResponse{http.StatusOK, &api.Endpoints{}})
 	defer testServer.Close()
 	client := client.NewOrDie(&client.Config{Host: testServer.URL, Version: testapi.Version()})
 	endpoints := NewEndpointController(client)

pkg/tools/etcd_tools_test.go

@@ -24,10 +24,9 @@ import (
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/testapi"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 	"github.com/coreos/go-etcd/etcd"
 )
 
@@ -100,7 +99,7 @@ func TestExtractToList(t *testing.T) {
 	}
 
 	var got api.PodList
-	helper := EtcdHelper{fakeClient, latest.Codec, versioner}
+	helper := EtcdHelper{fakeClient, testapi.Codec(), versioner}
 	err := helper.ExtractToList("/some/key", &got)
 	if err != nil {
 		t.Errorf("Unexpected error %v", err)
@@ -151,7 +150,7 @@ func TestExtractToListAcrossDirectories(t *testing.T) {
 	}
 
 	var got api.PodList
-	helper := EtcdHelper{fakeClient, latest.Codec, versioner}
+	helper := EtcdHelper{fakeClient, testapi.Codec(), versioner}
 	err := helper.ExtractToList("/some/key", &got)
 	if err != nil {
 		t.Errorf("Unexpected error %v", err)
@@ -198,7 +197,7 @@ func TestExtractToListExcludesDirectories(t *testing.T) {
 	}
 
 	var got api.PodList
-	helper := EtcdHelper{fakeClient, latest.Codec, versioner}
+	helper := EtcdHelper{fakeClient, testapi.Codec(), versioner}
 	err := helper.ExtractToList("/some/key", &got)
 	if err != nil {
 		t.Errorf("Unexpected error %v", err)
@@ -211,8 +210,8 @@ func TestExtractToListExcludesDirectories(t *testing.T) {
 func TestExtractObj(t *testing.T) {
 	fakeClient := NewFakeEtcdClient(t)
 	expect := api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}}
-	fakeClient.Set("/some/key", util.EncodeJSON(expect), 0)
+	fakeClient.Set("/some/key", runtime.EncodeOrDie(testapi.Codec(), &expect), 0)
-	helper := EtcdHelper{fakeClient, latest.Codec, versioner}
+	helper := EtcdHelper{fakeClient, testapi.Codec(), versioner}
 	var got api.Pod
 	err := helper.ExtractObj("/some/key", &got, false)
 	if err != nil {
@@ -266,12 +265,12 @@ func TestExtractObjNotFoundErr(t *testing.T) {
 func TestCreateObj(t *testing.T) {
 	obj := &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}}
 	fakeClient := NewFakeEtcdClient(t)
-	helper := EtcdHelper{fakeClient, latest.Codec, versioner}
+	helper := EtcdHelper{fakeClient, testapi.Codec(), versioner}
 	err := helper.CreateObj("/some/key", obj, 5)
 	if err != nil {
 		t.Errorf("Unexpected error %#v", err)
 	}
-	data, err := latest.Codec.Encode(obj)
+	data, err := testapi.Codec().Encode(obj)
 	if err != nil {
 		t.Errorf("Unexpected error %#v", err)
 	}
@@ -287,12 +286,12 @@ func TestCreateObj(t *testing.T) {
 func TestSetObj(t *testing.T) {
 	obj := &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}}
 	fakeClient := NewFakeEtcdClient(t)
-	helper := EtcdHelper{fakeClient, latest.Codec, versioner}
+	helper := EtcdHelper{fakeClient, testapi.Codec(), versioner}
 	err := helper.SetObj("/some/key", obj)
 	if err != nil {
 		t.Errorf("Unexpected error %#v", err)
 	}
-	data, err := latest.Codec.Encode(obj)
+	data, err := testapi.Codec().Encode(obj)
 	if err != nil {
 		t.Errorf("Unexpected error %#v", err)
 	}
@@ -310,18 +309,18 @@ func TestSetObjWithVersion(t *testing.T) {
 	fakeClient.Data["/some/key"] = EtcdResponseWithError{
 		R: &etcd.Response{
 			Node: &etcd.Node{
-				Value:         runtime.EncodeOrDie(latest.Codec, obj),
+				Value:         runtime.EncodeOrDie(testapi.Codec(), obj),
 				ModifiedIndex: 1,
 			},
 		},
 	}
 
-	helper := EtcdHelper{fakeClient, latest.Codec, versioner}
+	helper := EtcdHelper{fakeClient, testapi.Codec(), versioner}
 	err := helper.SetObj("/some/key", obj)
 	if err != nil {
 		t.Fatalf("Unexpected error %#v", err)
 	}
-	data, err := latest.Codec.Encode(obj)
+	data, err := testapi.Codec().Encode(obj)
 	if err != nil {
 		t.Fatalf("Unexpected error %#v", err)
 	}
@@ -335,12 +334,12 @@ func TestSetObjWithVersion(t *testing.T) {
 func TestSetObjWithoutResourceVersioner(t *testing.T) {
 	obj := &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}}
 	fakeClient := NewFakeEtcdClient(t)
-	helper := EtcdHelper{fakeClient, latest.Codec, nil}
+	helper := EtcdHelper{fakeClient, testapi.Codec(), nil}
 	err := helper.SetObj("/some/key", obj)
 	if err != nil {
 		t.Errorf("Unexpected error %#v", err)
 	}
-	data, err := latest.Codec.Encode(obj)
+	data, err := testapi.Codec().Encode(obj)
 	if err != nil {
 		t.Errorf("Unexpected error %#v", err)
 	}

pkg/util/util.go

@@ -60,12 +60,6 @@ func Forever(f func(), period time.Duration) {
 	}
 }
 
-// EncodeJSON returns obj marshalled as a JSON string, ignoring any errors.
-func EncodeJSON(obj interface{}) string {
-	data, _ := json.Marshal(obj)
-	return string(data)
-}
-
 // IntOrString is a type that can hold an int or a string.  When used in
 // JSON or YAML marshalling and unmarshalling, it produces or consumes the
 // inner type.  This allows you to have, for example, a JSON field that can

pkg/util/util_test.go

@@ -24,39 +24,6 @@ import (
 	"gopkg.in/v1/yaml"
 )
 
-type FakeTypeMeta struct {
-	ID string
-}
-type FakePod struct {
-	FakeTypeMeta `json:",inline" yaml:",inline"`
-	Labels       map[string]string
-	Int          int
-	Str          string
-}
-
-func TestEncodeJSON(t *testing.T) {
-	pod := FakePod{
-		FakeTypeMeta: FakeTypeMeta{ID: "foo"},
-		Labels: map[string]string{
-			"foo": "bar",
-			"baz": "blah",
-		},
-		Int: -6,
-		Str: "a string",
-	}
-
-	body := EncodeJSON(pod)
-
-	expectedBody, err := json.Marshal(pod)
-	if err != nil {
-		t.Errorf("unexpected error: %v", err)
-	}
-
-	if string(expectedBody) != body {
-		t.Errorf("JSON doesn't match.  Expected %s, saw %s", expectedBody, body)
-	}
-}
-
 func TestHandleCrash(t *testing.T) {
 	count := 0
 	expect := 10