Merge pull request #1427 from bcwaldon/members-serialization

Centralize Members serialization

client/members.go

@@ -23,6 +23,8 @@ import (
 	"net/url"
 	"path"
 	"time"
+
+	"github.com/coreos/etcd/etcdserver/etcdhttp/httptypes"
 )
 
 var (
@@ -51,64 +53,14 @@ func NewMembersAPI(tr *http.Transport, ep string, to time.Duration) (MembersAPI,
 }
 
 type MembersAPI interface {
-	List() ([]Member, error)
-}
-
-type Member struct {
-	ID         uint64
-	Name       string
-	PeerURLs   []url.URL
-	ClientURLs []url.URL
-}
-
-func (m *Member) UnmarshalJSON(data []byte) (err error) {
-	rm := struct {
-		ID         uint64
-		Name       string
-		PeerURLs   []string
-		ClientURLs []string
-	}{}
-
-	if err := json.Unmarshal(data, &rm); err != nil {
-		return err
-	}
-
-	parseURLs := func(strs []string) ([]url.URL, error) {
-		urls := make([]url.URL, len(strs))
-		for i, s := range strs {
-			u, err := url.Parse(s)
-			if err != nil {
-				return nil, err
-			}
-			urls[i] = *u
-		}
-
-		return urls, nil
-	}
-
-	if m.PeerURLs, err = parseURLs(rm.PeerURLs); err != nil {
-		return err
-	}
-
-	if m.ClientURLs, err = parseURLs(rm.ClientURLs); err != nil {
-		return err
-	}
-
-	m.ID = rm.ID
-	m.Name = rm.Name
-
-	return nil
-}
-
-type membersCollection struct {
-	Members []Member
+	List() ([]httptypes.Member, error)
 }
 
 type httpMembersAPI struct {
 	client *httpClient
 }
 
-func (m *httpMembersAPI) List() ([]Member, error) {
+func (m *httpMembersAPI) List() ([]httptypes.Member, error) {
 	httpresp, body, err := m.client.doWithTimeout(&membersAPIActionList{})
 	if err != nil {
 		return nil, err
@@ -116,24 +68,22 @@ func (m *httpMembersAPI) List() ([]Member, error) {
 
 	mResponse := httpMembersAPIResponse{
 		code: httpresp.StatusCode,
-		body: body,
 	}
 
-	if err = mResponse.err(); err != nil {
+	if err := mResponse.err(); err != nil {
 		return nil, err
 	}
 
-	var mCollection membersCollection
-	if err = mResponse.unmarshalBody(&mCollection); err != nil {
+	var mCollection httptypes.MemberCollection
+	if err := json.Unmarshal(body, &mCollection); err != nil {
 		return nil, err
 	}
 
-	return mCollection.Members, nil
+	return []httptypes.Member(mCollection), nil
 }
 
 type httpMembersAPIResponse struct {
 	code int
-	body []byte
 }
 
 func (r *httpMembersAPIResponse) err() (err error) {
@@ -143,10 +93,6 @@ func (r *httpMembersAPIResponse) err() (err error) {
 	return
 }
 
-func (r *httpMembersAPIResponse) unmarshalBody(dst interface{}) (err error) {
-	return json.Unmarshal(r.body, dst)
-}
-
 type membersAPIActionList struct{}
 
 func (l *membersAPIActionList) httpRequest(ep url.URL) *http.Request {

client/members_test.go

@@ -17,10 +17,8 @@
 package client
 
 import (
-	"encoding/json"
 	"net/http"
 	"net/url"
-	"reflect"
 	"testing"
 )
 
@@ -39,130 +37,3 @@ func TestMembersAPIListAction(t *testing.T) {
 		t.Errorf(err.Error())
 	}
 }
-
-func TestMembersAPIUnmarshalMember(t *testing.T) {
-	tests := []struct {
-		body       []byte
-		wantMember Member
-		wantError  bool
-	}{
-		// no URLs, just check ID & Name
-		{
-			body:       []byte(`{"id": 1, "name": "dungarees"}`),
-			wantMember: Member{ID: 1, Name: "dungarees", PeerURLs: []url.URL{}, ClientURLs: []url.URL{}},
-		},
-
-		// both client and peer URLs
-		{
-			body: []byte(`{"peerURLs": ["http://127.0.0.1:4001"], "clientURLs": ["http://127.0.0.1:4001"]}`),
-			wantMember: Member{
-				PeerURLs: []url.URL{
-					{Scheme: "http", Host: "127.0.0.1:4001"},
-				},
-				ClientURLs: []url.URL{
-					{Scheme: "http", Host: "127.0.0.1:4001"},
-				},
-			},
-		},
-
-		// multiple peer URLs
-		{
-			body: []byte(`{"peerURLs": ["http://127.0.0.1:4001", "https://example.com"]}`),
-			wantMember: Member{
-				PeerURLs: []url.URL{
-					{Scheme: "http", Host: "127.0.0.1:4001"},
-					{Scheme: "https", Host: "example.com"},
-				},
-				ClientURLs: []url.URL{},
-			},
-		},
-
-		// multiple client URLs
-		{
-			body: []byte(`{"clientURLs": ["http://127.0.0.1:4001", "https://example.com"]}`),
-			wantMember: Member{
-				PeerURLs: []url.URL{},
-				ClientURLs: []url.URL{
-					{Scheme: "http", Host: "127.0.0.1:4001"},
-					{Scheme: "https", Host: "example.com"},
-				},
-			},
-		},
-
-		// invalid JSON
-		{
-			body:      []byte(`{"peerU`),
-			wantError: true,
-		},
-
-		// valid JSON, invalid URL
-		{
-			body:      []byte(`{"peerURLs": [":"]}`),
-			wantError: true,
-		},
-	}
-
-	for i, tt := range tests {
-		got := Member{}
-		err := json.Unmarshal(tt.body, &got)
-		if tt.wantError != (err != nil) {
-			t.Errorf("#%d: want error %t, got %v", i, tt.wantError, err)
-			continue
-		}
-
-		if !reflect.DeepEqual(tt.wantMember, got) {
-			t.Errorf("#%d: incorrect output: want=%#v, got=%#v", i, tt.wantMember, got)
-		}
-	}
-}
-
-func TestMembersAPIUnmarshalMembers(t *testing.T) {
-	body := []byte(`{"members":[{"id":176869799018424574,"peerURLs":["http://127.0.0.1:7003"],"name":"node3","clientURLs":["http://127.0.0.1:4003"]},{"id":297577273835923749,"peerURLs":["http://127.0.0.1:2380","http://127.0.0.1:7001"],"name":"node1","clientURLs":["http://127.0.0.1:2379","http://127.0.0.1:4001"]},{"id":10666918107976480891,"peerURLs":["http://127.0.0.1:7002"],"name":"node2","clientURLs":["http://127.0.0.1:4002"]}]}`)
-
-	want := membersCollection{
-		Members: []Member{
-			{
-				ID:   176869799018424574,
-				Name: "node3",
-				PeerURLs: []url.URL{
-					{Scheme: "http", Host: "127.0.0.1:7003"},
-				},
-				ClientURLs: []url.URL{
-					{Scheme: "http", Host: "127.0.0.1:4003"},
-				},
-			},
-			{
-				ID:   297577273835923749,
-				Name: "node1",
-				PeerURLs: []url.URL{
-					{Scheme: "http", Host: "127.0.0.1:2380"},
-					{Scheme: "http", Host: "127.0.0.1:7001"},
-				},
-				ClientURLs: []url.URL{
-					{Scheme: "http", Host: "127.0.0.1:2379"},
-					{Scheme: "http", Host: "127.0.0.1:4001"},
-				},
-			},
-			{
-				ID:   10666918107976480891,
-				Name: "node2",
-				PeerURLs: []url.URL{
-					{Scheme: "http", Host: "127.0.0.1:7002"},
-				},
-				ClientURLs: []url.URL{
-					{Scheme: "http", Host: "127.0.0.1:4002"},
-				},
-			},
-		},
-	}
-
-	got := membersCollection{}
-	err := json.Unmarshal(body, &got)
-	if err != nil {
-		t.Fatalf("Unexpected error: %v", err)
-	}
-
-	if !reflect.DeepEqual(want, got) {
-		t.Errorf("Incorrect output: want=%#v, got=%#v", want, got)
-	}
-}

etcdserver/etcdhttp/client.go

@@ -33,6 +33,7 @@ import (
 	"github.com/coreos/etcd/Godeps/_workspace/src/github.com/jonboulle/clockwork"
 	etcdErr "github.com/coreos/etcd/error"
 	"github.com/coreos/etcd/etcdserver"
+	"github.com/coreos/etcd/etcdserver/etcdhttp/httptypes"
 	"github.com/coreos/etcd/etcdserver/etcdserverpb"
 	"github.com/coreos/etcd/pkg/types"
 	"github.com/coreos/etcd/store"
@@ -162,13 +163,9 @@ func (h *adminMembersHandler) ServeHTTP(w http.ResponseWriter, r *http.Request)
 			http.NotFound(w, r)
 			return
 		}
-		ms := struct {
-			Members []*etcdserver.Member `json:"members"`
-		}{
-			Members: h.clusterInfo.Members(),
-		}
+		mc := newMemberCollection(h.clusterInfo.Members())
 		w.Header().Set("Content-Type", "application/json")
-		if err := json.NewEncoder(w).Encode(ms); err != nil {
+		if err := json.NewEncoder(w).Encode(mc); err != nil {
 			log.Printf("etcdhttp: %v", err)
 		}
 	case "POST":
@@ -518,3 +515,23 @@ func getBool(form url.Values, key string) (b bool, err error) {
 	}
 	return
 }
+
+func newMemberCollection(ms []*etcdserver.Member) httptypes.MemberCollection {
+	c := httptypes.MemberCollection(make([]httptypes.Member, len(ms)))
+
+	for i, m := range ms {
+		tm := httptypes.Member{
+			ID:         m.ID,
+			Name:       m.Name,
+			PeerURLs:   make([]string, len(m.PeerURLs)),
+			ClientURLs: make([]string, len(m.ClientURLs)),
+		}
+
+		copy(m.PeerURLs, tm.PeerURLs)
+		copy(m.ClientURLs, tm.ClientURLs)
+
+		c[i] = tm
+	}
+
+	return c
+}

etcdserver/etcdhttp/client_test.go

@@ -561,17 +561,11 @@ func TestServeAdminMembers(t *testing.T) {
 		clusterInfo: cluster,
 	}
 
-	msb, err := json.Marshal(
-		struct {
-			Members []etcdserver.Member `json:"members"`
-		}{
-			Members: []etcdserver.Member{memb1, memb2},
-		},
-	)
+	mcb, err := json.Marshal(newMemberCollection([]*etcdserver.Member{&memb1, &memb2}))
 	if err != nil {
 		t.Fatal(err)
 	}
-	wms := string(msb) + "\n"
+	wmc := string(mcb) + "\n"
 
 	tests := []struct {
 		path  string
@@ -579,7 +573,7 @@ func TestServeAdminMembers(t *testing.T) {
 		wct   string
 		wbody string
 	}{
-		{adminMembersPrefix, http.StatusOK, "application/json", wms},
+		{adminMembersPrefix, http.StatusOK, "application/json", wmc},
 		{path.Join(adminMembersPrefix, "100"), http.StatusNotFound, "text/plain; charset=utf-8", "404 page not found\n"},
 		{path.Join(adminMembersPrefix, "foobar"), http.StatusNotFound, "text/plain; charset=utf-8", "404 page not found\n"},
 	}

etcdserver/etcdhttp/httptypes/doc.go

@@ -0,0 +1,21 @@
+/*
+   Copyright 2014 CoreOS, Inc.
+
+   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 httptypes defines how etcd's HTTP API entities are serialized to and deserialized from JSON.
+*/
+
+package httptypes

etcdserver/etcdhttp/httptypes/member.go

@@ -0,0 +1,58 @@
+/*
+   Copyright 2014 CoreOS, Inc.
+
+   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 httptypes
+
+import (
+	"encoding/json"
+)
+
+type Member struct {
+	ID         uint64   `json:"id"`
+	Name       string   `json:"name"`
+	PeerURLs   []string `json:"peerURLs"`
+	ClientURLs []string `json:"clientURLs"`
+}
+
+type MemberCollection []Member
+
+func (c *MemberCollection) MarshalJSON() ([]byte, error) {
+	d := struct {
+		Members []Member `json:"members"`
+	}{
+		Members: []Member(*c),
+	}
+
+	return json.Marshal(d)
+}
+
+func (c *MemberCollection) UnmarshalJSON(data []byte) error {
+	d := struct {
+		Members []Member
+	}{}
+
+	if err := json.Unmarshal(data, &d); err != nil {
+		return err
+	}
+
+	if d.Members == nil {
+		*c = make([]Member, 0)
+		return nil
+	}
+
+	*c = d.Members
+	return nil
+}

etcdserver/etcdhttp/httptypes/member_test.go

@@ -0,0 +1,157 @@
+/*
+   Copyright 2014 CoreOS, Inc.
+
+   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 httptypes
+
+import (
+	"encoding/json"
+	"reflect"
+	"testing"
+)
+
+func TestMemberUnmarshal(t *testing.T) {
+	tests := []struct {
+		body       []byte
+		wantMember Member
+		wantError  bool
+	}{
+		// no URLs, just check ID & Name
+		{
+			body:       []byte(`{"id": 1, "name": "dungarees"}`),
+			wantMember: Member{ID: 1, Name: "dungarees", PeerURLs: nil, ClientURLs: nil},
+		},
+
+		// both client and peer URLs
+		{
+			body: []byte(`{"peerURLs": ["http://127.0.0.1:4001"], "clientURLs": ["http://127.0.0.1:4001"]}`),
+			wantMember: Member{
+				PeerURLs: []string{
+					"http://127.0.0.1:4001",
+				},
+				ClientURLs: []string{
+					"http://127.0.0.1:4001",
+				},
+			},
+		},
+
+		// multiple peer URLs
+		{
+			body: []byte(`{"peerURLs": ["http://127.0.0.1:4001", "https://example.com"]}`),
+			wantMember: Member{
+				PeerURLs: []string{
+					"http://127.0.0.1:4001",
+					"https://example.com",
+				},
+				ClientURLs: nil,
+			},
+		},
+
+		// multiple client URLs
+		{
+			body: []byte(`{"clientURLs": ["http://127.0.0.1:4001", "https://example.com"]}`),
+			wantMember: Member{
+				PeerURLs: nil,
+				ClientURLs: []string{
+					"http://127.0.0.1:4001",
+					"https://example.com",
+				},
+			},
+		},
+
+		// invalid JSON
+		{
+			body:      []byte(`{"peerU`),
+			wantError: true,
+		},
+	}
+
+	for i, tt := range tests {
+		got := Member{}
+		err := json.Unmarshal(tt.body, &got)
+		if tt.wantError != (err != nil) {
+			t.Errorf("#%d: want error %t, got %v", i, tt.wantError, err)
+			continue
+		}
+
+		if !reflect.DeepEqual(tt.wantMember, got) {
+			t.Errorf("#%d: incorrect output: want=%#v, got=%#v", i, tt.wantMember, got)
+		}
+	}
+}
+
+func TestMemberCollectionUnmarshal(t *testing.T) {
+	tests := []struct {
+		body []byte
+		want MemberCollection
+	}{
+		{
+			body: []byte(`{"members":[]}`),
+			want: MemberCollection([]Member{}),
+		},
+		{
+			body: []byte(`{"members":[{"id":176869799018424574,"peerURLs":["http://127.0.0.1:7003"],"name":"node3","clientURLs":["http://127.0.0.1:4003"]},{"id":297577273835923749,"peerURLs":["http://127.0.0.1:2380","http://127.0.0.1:7001"],"name":"node1","clientURLs":["http://127.0.0.1:2379","http://127.0.0.1:4001"]},{"id":10666918107976480891,"peerURLs":["http://127.0.0.1:7002"],"name":"node2","clientURLs":["http://127.0.0.1:4002"]}]}`),
+			want: MemberCollection(
+				[]Member{
+					{
+						ID:   176869799018424574,
+						Name: "node3",
+						PeerURLs: []string{
+							"http://127.0.0.1:7003",
+						},
+						ClientURLs: []string{
+							"http://127.0.0.1:4003",
+						},
+					},
+					{
+						ID:   297577273835923749,
+						Name: "node1",
+						PeerURLs: []string{
+							"http://127.0.0.1:2380",
+							"http://127.0.0.1:7001",
+						},
+						ClientURLs: []string{
+							"http://127.0.0.1:2379",
+							"http://127.0.0.1:4001",
+						},
+					},
+					{
+						ID:   10666918107976480891,
+						Name: "node2",
+						PeerURLs: []string{
+							"http://127.0.0.1:7002",
+						},
+						ClientURLs: []string{
+							"http://127.0.0.1:4002",
+						},
+					},
+				},
+			),
+		},
+	}
+
+	for i, tt := range tests {
+		var got MemberCollection
+		err := json.Unmarshal(tt.body, &got)
+		if err != nil {
+			t.Errorf("#%d: unexpected error: %v", i, err)
+			continue
+		}
+
+		if !reflect.DeepEqual(tt.want, got) {
+			t.Errorf("#%d: incorrect output: want=%#v, got=%#v", i, tt.want, got)
+		}
+	}
+}

test

@@ -15,7 +15,7 @@ COVER=${COVER:-"-cover"}
 source ./build
 
 # Hack: gofmt ./ will recursively check the .git directory. So use *.go for gofmt.
-TESTABLE_AND_FORMATTABLE="client discovery etcdctl/command etcdserver etcdserver/etcdhttp etcdserver/etcdserverpb integration pkg/flags pkg/transport proxy raft snap store wait wal"
+TESTABLE_AND_FORMATTABLE="client discovery etcdctl/command etcdserver etcdserver/etcdhttp etcdserver/etcdhttp/httptypes etcdserver/etcdserverpb integration pkg/flags pkg/transport proxy raft snap store wait wal"
 TESTABLE="$TESTABLE_AND_FORMATTABLE ./"
 FORMATTABLE="$TESTABLE_AND_FORMATTABLE *.go etcdctl/"