Merge pull request #277 from ofpiyush/client_compat

Add client option for literal URLs

client_options.go

@@ -25,6 +25,7 @@ type ClientOptions struct {
 	Interceptors []Interceptor
 	Hooks        *ClientHooks
 	pathPrefix   *string
+	LiteralURLs  bool
 }
 
 func (opts *ClientOptions) PathPrefix() string {
@@ -130,3 +131,17 @@ func WithClientPathPrefix(prefix string) ClientOption {
 		o.pathPrefix = &prefix
 	}
 }
+
+// WithClientLiteralURLs configures the Twirp client to use the exact values
+// as defined in the proto file for Service and Method names,
+// fixing the issue https://github.com/twitchtv/twirp/issues/244, which is manifested
+// when working with Twirp services implemented other languages (e.g. Python) and the proto file definitions
+// are not properly following the [Protobuf Style Guide](https://developers.google.com/protocol-buffers/docs/style#services).
+// By default (false), Go clients modify the routes by CamelCasing the values. For example,
+// with Service: `haberdasher`, Method: `make_hat`, the URLs generated by Go clients are `Haberdasher/MakeHat`,
+// but with this option enabled (true) the client will properly use `haberdasher/make_hat` instead.
+func WithClientLiteralURLs(b bool) ClientOption {
+	return func(o *ClientOptions) {
+		o.LiteralURLs = b
+	}
+}

client_options_test.go

@@ -112,3 +112,19 @@ func TestWithClientPathPrefix(t *testing.T) {
 		t.Errorf("unexpected value after WithClientPathPrefix, have: %q, want: %q", have, want)
 	}
 }
+
+func TestWithClientLiteralURLs(t *testing.T) {
+	opts := &ClientOptions{}
+
+	WithClientLiteralURLs(true)(opts)
+	if have, want := opts.LiteralURLs, true; have != want {
+		t.Errorf("unexpected value after WithClientLiteralURLs, have: %t, want: %t", have, want)
+		return
+	}
+
+	WithClientLiteralURLs(false)(opts)
+	if have, want := opts.LiteralURLs, false; have != want {
+		t.Errorf("unexpected value after WithClientLiteralURLs, have: %t, want: %t", have, want)
+		return
+	}
+}

internal/twirptest/snake_case_names/compatibility_test.go

@@ -15,9 +15,12 @@ package snake_case_names
 
 import (
 	context "context"
+	"fmt"
 	http "net/http"
 	"net/http/httptest"
 	"testing"
+
+	twirp "github.com/twitchtv/twirp"
 )
 
 type HaberdasherService struct{}
@@ -52,19 +55,21 @@ type compatibilityTestClient struct {
 }
 
 func (t compatibilityTestClient) Do(req *http.Request) (*http.Response, error) {
-	req.URL.Path = "/twirp/twirp.internal.twirptest.snake_case_names.Haberdasher_v1/MakeHat_v1"
+	expectedPath := "/twirp/twirp.internal.twirptest.snake_case_names.Haberdasher_v1/MakeHat_v1"
+	if req.URL.Path != expectedPath {
+		return nil, fmt.Errorf("expected: %s, got: %s", expectedPath, req.URL.Path)
+	}
 	return t.client.Do(req)
 }
 
-// When the proto definition contains service and/or method names with underscores (not following proto naming
-// best practices), Go clients will mistakenly convert routes into it's CamelCased versions, but clients in other
-// languages may keep the literal casing of the routes. This test makes a fake client that would send literal routes
+// This test uses a fake client that checks if the routes are not camel cased,
 // and checks that the generated Go server is still able to handle those routes.
 func TestServiceMethodNamesUnderscores(t *testing.T) {
 	s := httptest.NewServer(NewHaberdasherV1Server(&HaberdasherService{}, nil))
 	defer s.Close()
 
-	client := NewHaberdasherV1ProtobufClient(s.URL, compatibilityTestClient{client: http.DefaultClient})
+	client := NewHaberdasherV1ProtobufClient(s.URL, compatibilityTestClient{client: http.DefaultClient},
+		twirp.WithClientLiteralURLs(true))
 	hat, err := client.MakeHatV1(context.Background(), &MakeHatArgsV1_SizeV1{Inches: 1})
 	if err != nil {
 		t.Fatalf("compatible protobuf client err=%q", err)
@@ -73,4 +78,13 @@ func TestServiceMethodNamesUnderscores(t *testing.T) {
 		t.Errorf("wrong hat size returned")
 	}
 
+	camelCasedClient := NewHaberdasherV1ProtobufClient(s.URL, compatibilityTestClient{client: http.DefaultClient},
+		twirp.WithClientLiteralURLs(false)) // default value, send CamelCased routes
+	_, err = camelCasedClient.MakeHatV1(context.Background(), &MakeHatArgsV1_SizeV1{Inches: 1})
+	if err == nil {
+		t.Fatalf("expected error raised by the compatibilityTestClient because routes are camelcased. Got nil.")
+	}
+	if err.Error() != "twirp error internal: failed to do request: expected: /twirp/twirp.internal.twirptest.snake_case_names.Haberdasher_v1/MakeHat_v1, got: /twirp/twirp.internal.twirptest.snake_case_names.HaberdasherV1/MakeHatV1" {
+		t.Fatalf("expected error to be about the expected path, got err=%q", err)
+	}
 }

protoc-gen-twirp/generator.go

@@ -964,15 +964,18 @@ func (t *twirp) generateClient(name string, file *descriptor.FileDescriptorProto
 	servName := serviceNameCamelCased(service)
 	structName := unexported(servName) + name + "Client"
 	newClientFunc := "New" + servName + name + "Client"
+	servNameLit := serviceNameLiteral(service)
+	servNameCc := servName
 
 	methCnt := strconv.Itoa(len(service.Method))
 	t.P(`type `, structName, ` struct {`)
 	t.P(`  client HTTPClient`)
-	t.P(`  urls   [`, methCnt, `]string`)
+	t.P(`  urls  [`, methCnt, `]string`)
 	t.P(`  interceptor `, t.pkgs["twirp"], `.Interceptor`)
 	t.P(`  opts `, t.pkgs["twirp"], `.ClientOptions`)
 	t.P(`}`)
 	t.P()
+
 	t.P(`// `, newClientFunc, ` creates a `, name, ` client that implements the `, servName, ` interface.`)
 	t.P(`// It communicates using `, name, ` and can be configured with a custom HTTPClient.`)
 	t.P(`func `, newClientFunc, `(baseURL string, client HTTPClient, opts ...`, t.pkgs["twirp"], `.ClientOption) `, servName, ` {`)
@@ -988,13 +991,41 @@ func (t *twirp) generateClient(name string, file *descriptor.FileDescriptorProto
 	if len(service.Method) > 0 {
 		t.P(`  // Build method URLs: <baseURL>[<prefix>]/<package>.<Service>/<Method>`)
 		t.P(`  serviceURL := sanitizeBaseURL(baseURL)`)
-		t.P(`  serviceURL += baseServicePath(clientOpts.PathPrefix(), "`, servPkg, `", "`, servName, `")`)
+		if servNameLit == servNameCc {
+			t.P(`  serviceURL += baseServicePath(clientOpts.PathPrefix(), "`, servPkg, `", "`, servNameCc, `")`)
+		} else { // proto service name is not CamelCased, then it needs to check client option to decide if needs to change case
+			t.P(`  if clientOpts.LiteralURLs {`)
+			t.P(`    serviceURL += baseServicePath(clientOpts.PathPrefix(), "`, servPkg, `", "`, servNameLit, `")`)
+			t.P(`  } else {`)
+			t.P(`    serviceURL += baseServicePath(clientOpts.PathPrefix(), "`, servPkg, `", "`, servNameCc, `")`)
+			t.P(`  }`)
+		}
 	}
 	t.P(`  urls := [`, methCnt, `]string{`)
 	for _, method := range service.Method {
 		t.P(`    serviceURL + "`, methodNameCamelCased(method), `",`)
 	}
 	t.P(`  }`)
+
+	allMethodsCamelCased := true
+	for _, method := range service.Method {
+		methNameLit := methodNameLiteral(method)
+		methNameCc := methodNameCamelCased(method)
+		if methNameCc != methNameLit {
+			allMethodsCamelCased = false
+			break
+		}
+	}
+	if !allMethodsCamelCased {
+		t.P(`  if clientOpts.LiteralURLs {`)
+		t.P(`    urls = [`, methCnt, `]string{`)
+		for _, method := range service.Method {
+			t.P(`    serviceURL + "`, methodNameLiteral(method), `",`)
+		}
+		t.P(`    }`)
+		t.P(`  }`)
+	}
+
 	t.P()
 	t.P(`  return &`, structName, `{`)
 	t.P(`    client: client,`)
@@ -1039,7 +1070,6 @@ func (t *twirp) generateClient(name string, file *descriptor.FileDescriptorProto
 		t.P(`}`)
 		t.P()
 	}
-
 }
 
 func (t *twirp) generateClientHooks() {