From 7c4733f2c84171a99d9821baee43941c8515a549 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Mon, 4 Oct 2021 18:07:33 -0700 Subject: [PATCH] review comments - strip the leading "/" unconditionally in parseTarget() - do not strip the leading "/" from endpoint in DialContext() since this is done in parseTarget() - move the resolver building to inside the if in parseTargetAndFindResolver() - doc comments in resolver.go --- clientconn.go | 18 +++++++++--------- clientconn_parsed_target_test.go | 6 +++--- resolver/resolver.go | 6 +++--- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/clientconn.go b/clientconn.go index 89f43edfc83a..150c8fcd096c 100644 --- a/clientconn.go +++ b/clientconn.go @@ -259,7 +259,7 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * // scheme and empty hostname (":port"). // TODO: Define an optional interface on the resolver builder to return the // authority given the user's dial target. - cc.authority = strings.TrimPrefix(cc.parsedTarget.Endpoint, "/") + cc.authority = cc.parsedTarget.Endpoint if strings.HasPrefix(cc.authority, ":") { cc.authority = "localhost" + cc.authority } @@ -1630,16 +1630,18 @@ func (cc *ClientConn) connectionError() error { func (cc *ClientConn) parseTargetAndFindResolver() (resolver.Builder, error) { channelz.Infof(logger, cc.channelzID, "original dial target is: %q", cc.target) + + var rb resolver.Builder parsedTarget, err := parseTarget(cc.target) if err != nil { channelz.Infof(logger, cc.channelzID, "dial target %q parse failed: %v", cc.target, err) } else { channelz.Infof(logger, cc.channelzID, "parsed dial target is: %+v", parsedTarget) - } - rb := cc.getResolver(parsedTarget.Scheme) - if rb != nil { - cc.parsedTarget = parsedTarget - return rb, nil + rb = cc.getResolver(parsedTarget.Scheme) + if rb != nil { + cc.parsedTarget = parsedTarget + return rb, nil + } } // We are here because the user's dial target did not contain a scheme or @@ -1687,9 +1689,7 @@ func parseTarget(target string) (resolver.Target, error) { if endpoint == "" { endpoint = u.Opaque } - if strings.HasPrefix(target, fmt.Sprintf("%s://", u.Scheme)) { - endpoint = strings.TrimPrefix(endpoint, "/") - } + endpoint = strings.TrimPrefix(endpoint, "/") return resolver.Target{ Scheme: u.Scheme, Authority: u.Host, diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index 45059f7c457a..423c9d7c53fe 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -69,7 +69,7 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { {target: "unix-abstract:a b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a b"}}, {target: "unix-abstract:a:b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a:b"}}, {target: "unix-abstract:a-b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a-b"}}, - {target: "unix-abstract:/ a///://::!@#$%25^&*()b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/ a///://::!@"}}, + {target: "unix-abstract:/ a///://::!@#$%25^&*()b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: " a///://::!@"}}, {target: "unix-abstract:passthrough:abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "passthrough:abc"}}, {target: "unix-abstract:unix:///abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "unix:///abc"}}, {target: "unix-abstract:///a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c"}}, @@ -77,7 +77,7 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { {target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "unix:///a/b/c"}}, // Cases for `scheme:absolute-path`. - {target: "dns:/a/b/c", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "/a/b/c"}}, + {target: "dns:/a/b/c", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "a/b/c"}}, {target: "unregistered:/a/b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unregistered:/a/b/c"}}, } @@ -130,7 +130,7 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { }, { target: "unix:/a/b/c", - wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}, + wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"}, wantDialerAddress: "unix:///a/b/c", }, { diff --git a/resolver/resolver.go b/resolver/resolver.go index 0ae636fd976a..3b0730e9ea37 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -217,11 +217,11 @@ type ClientConn interface { // Examples: // // - "dns://some_authority/foo.bar" -// Target{Scheme: "dns", Authority: "some_authority", Endpoint: "/foo.bar"} +// Target{Scheme: "dns", Authority: "some_authority", Endpoint: "foo.bar"} // - "foo.bar" -// Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "/foo.bar"} +// Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "foo.bar"} // - "unknown_scheme://authority/endpoint" -// Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "/unknown_scheme://authority/endpoint"} +// Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "unknown_scheme://authority/endpoint"} type Target struct { Scheme string Authority string