From 5c579348a5011a74d5ee952f054614f45bbbb841 Mon Sep 17 00:00:00 2001
From: Aliaksandr Mianzhynski <amenzhinsky@gmail.com>
Date: Fri, 12 Jun 2020 19:40:15 +0300
Subject: [PATCH] Add default method config support

---
 clientconn.go          | 12 +++++++-----
 clientconn_test.go     | 23 +++++++++++++++++++++++
 service_config.go      | 33 ++++++++++++++++++++++++++++-----
 service_config_test.go | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 10 deletions(-)

diff --git a/clientconn.go b/clientconn.go
index ef327e8af4f7..6c36e0c2ba52 100644
--- a/clientconn.go
+++ b/clientconn.go
@@ -871,12 +871,14 @@ func (cc *ClientConn) GetMethodConfig(method string) MethodConfig {
 	if cc.sc == nil {
 		return MethodConfig{}
 	}
-	m, ok := cc.sc.Methods[method]
-	if !ok {
-		i := strings.LastIndex(method, "/")
-		m = cc.sc.Methods[method[:i+1]]
+	if m, ok := cc.sc.Methods[method]; ok {
+		return m
 	}
-	return m
+	i := strings.LastIndex(method, "/")
+	if m, ok := cc.sc.Methods[method[:i+1]]; ok {
+		return m
+	}
+	return cc.sc.Methods[""]
 }
 
 func (cc *ClientConn) healthCheckConfig() *healthCheckConfig {
diff --git a/clientconn_test.go b/clientconn_test.go
index 524b9736c1a0..939d4e001b1e 100644
--- a/clientconn_test.go
+++ b/clientconn_test.go
@@ -782,6 +782,29 @@ func (s) TestDisableServiceConfigOption(t *testing.T) {
 	}
 }
 
+func (s) TestMethodConfigDefaultService(t *testing.T) {
+	addr := "nonexist:///non.existent"
+	cc, err := Dial(addr, WithInsecure(), WithDefaultServiceConfig(`{
+  "methodConfig": [{
+    "name": [
+      {
+        "service": ""
+      }
+    ],
+    "waitForReady": true
+  }]
+}`))
+	if err != nil {
+		t.Fatalf("Dial(%s, _) = _, %v, want _, <nil>", addr, err)
+	}
+	defer cc.Close()
+
+	m := cc.GetMethodConfig("/foo/Bar")
+	if m.WaitForReady == nil {
+		t.Fatalf("want: method (%q) config to fallback to the default service", "/foo/Bar")
+	}
+}
+
 func (s) TestGetClientConnTarget(t *testing.T) {
 	addr := "nonexist:///non.existent"
 	cc, err := Dial(addr, WithInsecure())
diff --git a/service_config.go b/service_config.go
index 3132a66cd68c..0831d95bd427 100644
--- a/service_config.go
+++ b/service_config.go
@@ -20,6 +20,7 @@ package grpc
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"reflect"
 	"strconv"
@@ -229,15 +230,22 @@ type jsonName struct {
 	Method  *string
 }
 
-func (j jsonName) generatePath() (string, bool) {
+func (j jsonName) generatePath() (string, bool, error) {
 	if j.Service == nil {
-		return "", false
+		return "", false, nil
+	}
+	if *j.Service == "" {
+		// when 'service' is empty 'method' must be empty as well
+		if j.Method != nil && *j.Method != "" {
+			return "", false, errors.New("cannot combine empty 'service' and non-empty 'method'")
+		}
+		return "", true, nil
 	}
 	res := "/" + *j.Service + "/"
 	if j.Method != nil {
 		res += *j.Method
 	}
-	return res, true
+	return res, true, nil
 }
 
 // TODO(lyuxuan): delete this struct after cleaning up old service config implementation.
@@ -289,6 +297,8 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult {
 	if rsc.MethodConfig == nil {
 		return &serviceconfig.ParseResult{Config: &sc}
 	}
+
+	paths := map[string]struct{}{}
 	for _, m := range *rsc.MethodConfig {
 		if m.Name == nil {
 			continue
@@ -321,8 +331,21 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult {
 				mc.MaxRespSize = newInt(int(*m.MaxResponseMessageBytes))
 			}
 		}
-		for _, n := range *m.Name {
-			if path, valid := n.generatePath(); valid {
+		for i, n := range *m.Name {
+			path, valid, err := n.generatePath()
+			if err != nil {
+				grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to methodConfig[%d]: %v", js, i, err)
+				return &serviceconfig.ParseResult{Err: err}
+			}
+
+			if _, ok := paths[path]; ok {
+				err = errors.New("duplicated name")
+				grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to methodConfig[%d]: %v", js, i, err)
+				return &serviceconfig.ParseResult{Err: err}
+			}
+			paths[path] = struct{}{}
+
+			if valid {
 				sc.Methods[path] = mc
 			}
 		}
diff --git a/service_config_test.go b/service_config_test.go
index ec9f56db2c71..2b48d18ca782 100644
--- a/service_config_test.go
+++ b/service_config_test.go
@@ -373,6 +373,38 @@ func (s) TestParseMsgSize(t *testing.T) {
 	runParseTests(t, testcases)
 }
 
+func (s) TestParseMethodConfigEmptyServiceAndNonEmptyMethod(t *testing.T) {
+	runParseTests(t, []parseTestCase{{
+		`{
+  "methodConfig": [{
+    "name": [
+      {
+        "service": "",
+        "method": "Bar"
+      }
+    ],
+    "waitForReady": true
+  }]
+}`, nil, true,
+	}})
+}
+
+func (s) TestParseMethodConfigDuplicatedName(t *testing.T) {
+	runParseTests(t, []parseTestCase{
+		{
+			`{
+  "methodConfig": [{
+    "name": [
+      {"service": "foo"},
+      {"service": "foo"}
+    ],
+    "waitForReady": true
+  }]
+}`, nil, true,
+		},
+	})
+}
+
 func (s) TestParseDuration(t *testing.T) {
 	testCases := []struct {
 		s    *string