Merge pull request #76 from tongjingran/67

optimize statement and encapsulate logic of #67

cmd/profile.go

@@ -38,10 +38,10 @@ goc profile
 # Get coverage counter from specified register center, the result output to specified file.
 goc profile --center=http://192.168.1.1:8080 --output=./coverage.cov
 
-# Get coverage counter of several specified services. You can get all available service names from command 'goc list'. Use 'service' and 'address' flag at the same time is illegal.
+# Get coverage counter of several specified services. You can get all available service names from command 'goc list'. Use 'service' and 'address' flag at the same time may cause ambiguity, please use them separately.
 goc profile --service=service1,service2,service3
 
-# Get coverage counter of several specified addresses. You can get all available addresses from command 'goc list'. Use 'service' and 'address' flag at the same time is illegal.
+# Get coverage counter of several specified addresses. You can get all available addresses from command 'goc list'. Use 'service' and 'address' flag at the same time may cause ambiguity, please use them separately.
 goc profile --address=address1,address2,address3
 
 # Force to get the coverage counter of all the available services you want.
@@ -81,8 +81,8 @@ var addrList []string
 
 func init() {
 	profileCmd.Flags().StringVarP(&output, "output", "o", "", "download cover profile")
-	profileCmd.Flags().StringSliceVarP(&svrList, "service", "", nil, "get the cover profile of these services, you can get all available service names from command `goc list`, use this flag and 'address' flag at the same time is illegal.")
+	profileCmd.Flags().StringSliceVarP(&svrList, "service", "", nil, "get the cover profile of these services, you can get all available service names from command `goc list`, use this flag and 'address' flag at the same time may cause ambiguity, please use them separately.")
-	profileCmd.Flags().StringSliceVarP(&addrList, "address", "", nil, "get the cover profile of these addresses, you can get all available addresses from command `goc list`, use this flag and 'service' flag at the same time is illegal.")
+	profileCmd.Flags().StringSliceVarP(&addrList, "address", "", nil, "get the cover profile of these addresses, you can get all available addresses from command `goc list`, use this flag and 'service' flag at the same time may cause ambiguity, please use them separately.")
 	profileCmd.Flags().BoolVarP(&force, "force", "f", false, "force to get the coverage counter of all the available services you want")
 	addBasicFlags(profileCmd.Flags())
 	rootCmd.AddCommand(profileCmd)

pkg/cover/client.go

@@ -92,7 +92,7 @@ func (c *client) ListServices() ([]byte, error) {
 func (c *client) Profile(param ProfileParam) ([]byte, error) {
 	u := fmt.Sprintf("%s%s?force=%s", c.Host, CoverProfileAPI, strconv.FormatBool(param.Force))
 	if len(param.Service) != 0 && len(param.Address) != 0 {
-		return nil, fmt.Errorf("use 'service' and 'address' flag at the same time is illegal")
+		return nil, fmt.Errorf("use 'service' flag and 'address' flag at the same time may cause ambiguity, please use them separately")
 	}
 
 	for _, svr := range param.Service {

pkg/cover/client_test.go

@@ -68,7 +68,7 @@ func TestClientAction(t *testing.T) {
 		{
 			service: Service{Name: "serviceOK", Address: profileSuccessMockSvr.URL},
 			param:   ProfileParam{Force: false, Service: []string{"serviceOK"}, Address: []string{profileSuccessMockSvr.URL}},
-			res:     "use 'service' and 'address' flag at the same time is illegal",
+			res:     "use 'service' flag and 'address' flag at the same time may cause ambiguity, please use them separately",
 		},
 		{
 			service: Service{Name: "serviceOK", Address: profileSuccessMockSvr.URL},
@@ -94,11 +94,6 @@ func TestClientAction(t *testing.T) {
 			service: Service{Name: "serviceErr", Address: profileErrMockSvr.URL},
 			res:     "bad mode line: error",
 		},
-		{
-			service: Service{Name: "serviceErr", Address: profileErrMockSvr.URL},
-			param:   ProfileParam{Force: true},
-			res:     "no profiles",
-		},
 		{
 			service: Service{Name: "serviceNotExist", Address: "http://172.0.0.2:7777"},
 			res:     "connection refused",

pkg/cover/server.go

@@ -138,20 +138,21 @@ func profile(c *gin.Context) {
 		c.JSON(http.StatusExpectationFailed, gin.H{"error": "invalid param"})
 		return
 	}
-	svrList := c.QueryArray("service")
+	serviceList := removeDuplicateElement(c.QueryArray("service"))
-	addrList := c.QueryArray("address")
+	addressList := removeDuplicateElement(c.QueryArray("address"))
-	svrsAll := DefaultStore.GetAll()
+	allInfos := DefaultStore.GetAll()
-	svrsUnderTest, err := getSvrUnderTest(svrList, addrList, force, svrsAll)
+	filterAddrList, err := filterAddrs(serviceList, addressList, force, allInfos)
 	if err != nil {
 		c.JSON(http.StatusExpectationFailed, gin.H{"error": err.Error()})
+		return
 	}
 
 	var mergedProfiles = make([][]*cover.Profile, 0)
-	for _, svrs := range svrsUnderTest {
+	for _, addr := range filterAddrList {
-		for _, addr := range svrs {
 		pp, err := NewWorker(addr).Profile(ProfileParam{})
 		if err != nil {
 			if force {
+				log.Warnf("get profile from [%s] failed, error: %s", addr, err.Error())
 				continue
 			}
 			c.JSON(http.StatusExpectationFailed, gin.H{"error": err.Error()})
@@ -159,15 +160,11 @@ func profile(c *gin.Context) {
 		}
 		profile, err := convertProfile(pp)
 		if err != nil {
-				if force {
-					continue
-				}
 			c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
 			return
 		}
 		mergedProfiles = append(mergedProfiles, profile)
 	}
-	}
 
 	if len(mergedProfiles) == 0 {
 		c.JSON(http.StatusOK, "no profiles")
@@ -235,48 +232,53 @@ func contains(arr []string, str string) bool {
 	return false
 }
 
-// getSvrUnderTest get service map by service and address list
+// filterAddrs filter address list by given service and address list
-func getSvrUnderTest(svrList, addrList []string, force bool, svrsAll map[string][]string) (svrsUnderTest map[string][]string, err error) {
+func filterAddrs(serviceList, addressList []string, force bool, allInfos map[string][]string) (filterAddrList []string, err error) {
-	svrsUnderTest = map[string][]string{}
+	addressAll := []string{}
-	if len(svrList) != 0 && len(addrList) != 0 {
+	for _, addr := range allInfos {
-		return nil, fmt.Errorf("use this flag and 'address' flag at the same time is illegal")
+		addressAll = append(addressAll, addr...)
+	}
+	if len(serviceList) != 0 && len(addressList) != 0 {
+		return nil, fmt.Errorf("use 'service' flag and 'address' flag at the same time may cause ambiguity, please use them separately")
 	}
-	// Return all servers when all param is nil
-	if len(svrList) == 0 && len(addrList) == 0 {
-		return svrsAll, nil
-	} else {
 	// Add matched services to map
-		if len(svrList) != 0 {
+	for _, name := range serviceList {
-			for _, name := range svrList {
+		if addr, ok := allInfos[name]; ok {
-				if addr, ok := svrsAll[name]; ok {
+			filterAddrList = append(filterAddrList, addr...)
-					svrsUnderTest[name] = addr
 			continue // jump to match the next service
 		}
 		if !force {
 			return nil, fmt.Errorf("service [%s] not found", name)
 		}
-			}
+		log.Warnf("service [%s] not found", name)
 	}
 	// Add matched addresses to map
-		if len(addrList) != 0 {
+	for _, addr := range addressList {
-		I:
+		if contains(addressAll, addr) {
-			for _, addr := range addrList {
+			filterAddrList = append(filterAddrList, addr)
-				for svr, addrs := range svrsAll {
+			continue
-					if contains(svrsUnderTest[svr], addr) {
-						continue I // The address is duplicate, jump over
-					}
-					for _, a := range addrs {
-						if a == addr {
-							svrsUnderTest[svr] = append(svrsUnderTest[svr], a)
-							continue I // jump to match the next address
-						}
-					}
 		}
 		if !force {
 			return nil, fmt.Errorf("address [%s] not found", addr)
 		}
+		log.Warnf("address [%s] not found", addr)
 	}
+	if len(addressList) == 0 && len(serviceList) == 0 {
+		filterAddrList = addressAll
 	}
-	}
+	// Return all servers when all param is nil
-	return svrsUnderTest, nil
+	return filterAddrList, nil
+}
+
+// removeDuplicateElement remove duplicate element in slice
+func removeDuplicateElement(addrs []string) []string {
+	result := make([]string, 0, len(addrs))
+	temp := map[string]struct{}{}
+	for _, item := range addrs {
+		if _, ok := temp[item]; !ok {
+			temp[item] = struct{}{}
+			result = append(result, item)
+		}
+	}
+	return result
 }

pkg/cover/server_test.go

@@ -10,51 +10,69 @@ func TestContains(t *testing.T) {
 	assert.Equal(t, contains([]string{"a", "b"}, "c"), false)
 }
 
-func TestGetSvrUnderTest(t *testing.T) {
+func TestFilterAddrs(t *testing.T) {
 	svrAll := map[string][]string{
 		"service1": {"http://127.0.0.1:7777", "http://127.0.0.1:8888"},
 		"service2": {"http://127.0.0.1:9999"},
 	}
+	addrAll := []string{}
+	for _, addr := range svrAll {
+		addrAll = append(addrAll, addr...)
+	}
 	items := []struct {
 		svrList  []string
 		addrList []string
 		force    bool
 		err      string
-		svrRes   map[string][]string
+		addrRes  []string
 	}{
 		{
 			svrList:  []string{"service1"},
 			addrList: []string{"http://127.0.0.1:7777"},
-			err:      "use this flag and 'address' flag at the same time is illegal",
+			err:      "use 'service' flag and 'address' flag at the same time may cause ambiguity, please use them separately",
 		},
 		{
-			svrRes: svrAll,
+			addrRes: addrAll,
 		},
 		{
 			svrList: []string{"service1", "unknown"},
 			err:     "service [unknown] not found",
 		},
 		{
-			svrList: []string{"service1", "service1", "service2", "unknown"},
+			svrList: []string{"service1", "service2", "unknown"},
+			force:   true,
+			addrRes: addrAll,
+		},
+		{
+			svrList: []string{"unknown"},
 			force:   true,
-			svrRes:  svrAll,
 		},
 		{
 			addrList: []string{"http://127.0.0.1:7777", "http://127.0.0.2:7777"},
 			err:      "address [http://127.0.0.2:7777] not found",
 		},
 		{
-			addrList: []string{"http://127.0.0.1:7777", "http://127.0.0.1:7777", "http://127.0.0.1:9999", "http://127.0.0.2:7777"},
+			addrList: []string{"http://127.0.0.1:7777", "http://127.0.0.1:9999", "http://127.0.0.2:7777"},
 			force:    true,
-			svrRes:   map[string][]string{"service1": {"http://127.0.0.1:7777"}, "service2": {"http://127.0.0.1:9999"}},
+			addrRes:  []string{"http://127.0.0.1:7777", "http://127.0.0.1:9999"},
 		},
 	}
 	for _, item := range items {
-		svrs, err := getSvrUnderTest(item.svrList, item.addrList, item.force, svrAll)
+		addrs, err := filterAddrs(item.svrList, item.addrList, item.force, svrAll)
 		if err != nil {
 			assert.Equal(t, err.Error(), item.err)
 		} else {
-			assert.Equal(t, svrs, item.svrRes)
+			if len(addrs) == 0 {
+				assert.Equal(t, addrs, item.addrRes)
+			}
+			for _, a := range addrs {
+				assert.Contains(t, item.addrRes, a)
+			}
 		}
 	}
 }
+
+func TestRemoveDuplicateElement(t *testing.T) {
+	strArr := []string{"a", "a", "b"}
+	assert.Equal(t, removeDuplicateElement(strArr), []string{"a", "b"})
+}