Repository: googleapis/gapic-generator-go
Pull request #1397 (2023-09-21 20:52:07)

fix(internal/gengapic): add workaround for operation collision
We have a service that is not yet released that uses the same RPC name across two services in the same proto package. Our Operation wrapper identifiers are not scoped to a service so this causes issues when generated a client that does this. For now this is for one client, which may change before GA, so a hacky solution is fine. But we should keep this in the back of our minds should we ever v2 our libraries this is something we would want to avoid.


Patch:
diff --git a/internal/gengapic/BUILD.bazel b/internal/gengapic/BUILD.bazel
index 380e18a..427ba60 100644
--- a/internal/gengapic/BUILD.bazel
+++ b/internal/gengapic/BUILD.bazel
@@ -99,6 +99,7 @@ go_test(
         "@org_golang_google_protobuf//encoding/protojson",
         "@org_golang_google_protobuf//proto",
         "@org_golang_google_protobuf//reflect/protodesc",
+        "@org_golang_google_protobuf//reflect/protoreflect",
         "@org_golang_google_protobuf//runtime/protoiface",
         "@org_golang_google_protobuf//types/descriptorpb",
         "@org_golang_google_protobuf//types/known/apipb",
diff --git a/internal/gengapic/client_init.go b/internal/gengapic/client_init.go
index ceb936e..8f35c61 100644
--- a/internal/gengapic/client_init.go
+++ b/internal/gengapic/client_init.go
@@ -116,9 +116,9 @@ func (g *generator) internalClientIntfInit(serv *descriptor.ServiceDescriptorPro
 		case g.isLRO(m):
 			// Unary call where the return type is a wrapper of
 			// longrunning.Operation and more precise types
-			lroType := lroTypeName(m.GetName())
+			lroType := lroTypeName(m)
 			p("%s(context.Context, *%s.%s, ...gax.CallOption) (*%s, error)",
-				m.GetName(), inSpec.Name, inType.GetName(), lroTypeName(m.GetName()))
+				m.GetName(), inSpec.Name, inType.GetName(), lroType)
 			p("%[1]s(name string) *%[1]s", lroType)
 
 		case m.GetClientStreaming():
@@ -258,7 +258,7 @@ func (g *generator) genClientWrapperMethod(m *descriptor.MethodDescriptorProto,
 
 	if g.isLRO(m) {
 		reqTyp := fmt.Sprintf("%s.%s", inSpec.Name, inType.GetName())
-		lroType := lroTypeName(m.GetName())
+		lroType := lroTypeName(m)
 		p("func (c *%s) %s(ctx context.Context, req *%s, opts ...gax.CallOption) (*%s, error) {",
 			clientTypeName, m.GetName(), reqTyp, lroType)
 		p("    return c.internalClient.%s(ctx, req, opts...)", m.GetName())
diff --git a/internal/gengapic/genrest.go b/internal/gengapic/genrest.go
index 938aa12..a442bac 100644
--- a/internal/gengapic/genrest.go
+++ b/internal/gengapic/genrest.go
@@ -909,7 +909,7 @@ func (g *generator) lroRESTCall(servName string, m *descriptor.MethodDescriptorP
 	}
 	g.imports[outSpec] = true
 
-	opWrapperType := lroTypeName(m.GetName())
+	opWrapperType := lroTypeName(m)
 	p("func (c *%s) %s(ctx context.Context, req *%s.%s, opts ...gax.CallOption) (*%s, error) {",
 		lowcaseServName, m.GetName(), inSpec.Name, inType.GetName(), opWrapperType)
 
diff --git a/internal/gengapic/lro.go b/internal/gengapic/lro.go
index de223ca..c0876a5 100644
--- a/internal/gengapic/lro.go
+++ b/internal/gengapic/lro.go
@@ -21,6 +21,7 @@ import (
 	longrunning "cloud.google.com/go/longrunning/autogen/longrunningpb"
 	"github.com/golang/protobuf/protoc-gen-go/descriptor"
 	"github.com/googleapis/gapic-generator-go/internal/pbinfo"
+	"google.golang.org/genproto/googleapis/api/annotations"
 	"google.golang.org/protobuf/proto"
 )
 
@@ -38,7 +39,7 @@ func (g *generator) lroCall(servName string, m *descriptor.MethodDescriptorProto
 		return err
 	}
 
-	lroType := lroTypeName(m.GetName())
+	lroType := lroTypeName(m)
 	p := g.printf
 
 	lowcaseServName := lowerFirst(servName + "GRPCClient")
@@ -74,7 +75,7 @@ func (g *generator) lroCall(servName string, m *descriptor.MethodDescriptorProto
 func (g *generator) lroType(servName string, serv *descriptor.ServiceDescriptorProto, m *descriptor.MethodDescriptorProto) error {
 	protoPkg := g.descInfo.ParentFile[serv].GetPackage()
 	mFQN := fmt.Sprintf("%s.%s.%s", protoPkg, serv.GetName(), m.GetName())
-	lroType := lroTypeName(m.GetName())
+	lroType := lroTypeName(m)
 	p := g.printf
 	hasREST := containsTransport(g.opts.transports, rest)
 
@@ -280,6 +281,17 @@ func (g *generator) lroType(servName string, serv *descriptor.ServiceDescriptorP
 	return nil
 }
 
-func lroTypeName(methodName string) string {
-	return methodName + "Operation"
+func lroTypeName(m *descriptor.MethodDescriptorProto) string {
+	// This whole if block is a hack to workaround a operation handler namespace
+	// collision. We should remove this in the future if the design is fixed for
+	// the v1 api. This is for aiplatform.featureregistryservice.createfeature.
+	if eHTTP, ok := proto.GetExtension(m.GetOptions(), annotations.E_Http).(*annotations.HttpRule); ok && eHTTP != nil && eHTTP.Pattern != nil {
+		switch t := eHTTP.Pattern.(type) {
+		case *annotations.HttpRule_Post:
+			if t.Post == "/v1beta1/{parent=projects/*/locations/*/featureGroups/*}/features" {
+				return m.GetName() + "RegistryOperation"
+			}
+		}
+	}
+	return m.GetName() + "Operation"
 }


Test patch:
diff --git a/internal/gengapic/client_init_test.go b/internal/gengapic/client_init_test.go
index 8f096d3..9b10a86 100644
--- a/internal/gengapic/client_init_test.go
+++ b/internal/gengapic/client_init_test.go
@@ -33,6 +33,7 @@ import (
 	code "google.golang.org/genproto/googleapis/rpc/code"
 	"google.golang.org/protobuf/encoding/protojson"
 	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/reflect/protoreflect"
 	"google.golang.org/protobuf/runtime/protoiface"
 	"google.golang.org/protobuf/types/known/apipb"
 	duration "google.golang.org/protobuf/types/known/durationpb"
@@ -361,7 +362,6 @@ func TestClientInit(t *testing.T) {
 	}
 
 	customOpOpts := &descriptor.MethodOptions{}
-	proto.SetExtension(customOpOpts, extendedops.E_OperationService, opS.GetName())
 	servCustomOp := &descriptor.ServiceDescriptorProto{
 		Name: proto.String("Foo"),
 		Method: []*descriptor.MethodDescriptorProto{
@@ -390,6 +390,7 @@ func TestClientInit(t *testing.T) {
 		parameter    *string
 		imports      map[pbinfo.ImportSpec]bool
 		wantNumSnps  int
+		setExt       func() (protoreflect.ExtensionType, interface{})
 	}{
 		{
 			tstName: "foo_client_init",
@@ -498,8 +499,50 @@ func TestClientInit(t *testing.T) {
 				{Name: "httptransport", Path: "google.golang.org/api/transport/http"}: true,
 			},
 			wantNumSnps: 1,
+			setExt: func() (protoreflect.ExtensionType, interface{}) {
+				return extendedops.E_OperationService, opS.GetName()
+			},
+		},
+		{
+			tstName: "lro_client_conflict",
+			mixins: mixins{
+				"google.longrunning.Operations": operationsMethods(),
+			},
+			servName:  "Foo",
+			serv:      servLRO,
+			parameter: proto.String("go-gapic-package=path;mypackage"),
+			imports: map[pbinfo.ImportSpec]bool{
+				{Name: "gtransport", Path: "google.golang.org/api/transport/grpc"}:                     true,
+				{Name: "longrunningpb", Path: "cloud.google.com/go/longrunning/autogen/longrunningpb"}: true,
+				{Name: "lroauto", Path: "cloud.google.com/go/longrunning/autogen"}:                     true,
+				{Name: "mypackagepb", Path: "github.com/googleapis/mypackage"}:                         true,
+				{Path: "context"}:                      true,
+				{Path: "google.golang.org/api/option"}: true,
+				{Path: "google.golang.org/grpc"}:       true,
+			},
+			wantNumSnps: 6,
+			setExt: func() (protoreflect.ExtensionType, interface{}) {
+				return annotations.E_Http, &annotations.HttpRule{
+					Pattern: &annotations.HttpRule_Post{
+						Post: "/v1beta1/{parent=projects/*/locations/*/featureGroups/*}/features",
+					},
+				}
+			},
 		},
 	} {
+		setExt := tst.setExt
+		if setExt == nil {
+			setExt = func() (protoreflect.ExtensionType, interface{}) {
+				return annotations.E_Http, &annotations.HttpRule{
+					Pattern: &annotations.HttpRule_Get{
+						Get: "/zip",
+					},
+				}
+			}
+		}
+		ext, value := setExt()
+		proto.SetExtension(tst.serv.Method[0].GetOptions(), ext, value)
+
 		fds := append(mixinDescriptors(), &descriptor.FileDescriptorProto{
 			Package: proto.String("mypackage"),
 			Options: &descriptor.FileOptions{
diff --git a/internal/gengapic/testdata/lro_client_conflict.want b/internal/gengapic/testdata/lro_client_conflict.want
new file mode 100644
index 0000000..d46e0ac
--- /dev/null
+++ b/internal/gengapic/testdata/lro_client_conflict.want
@@ -0,0 +1,178 @@
+// internalFooClient is an interface that defines the methods available from Awesome Foo API.
+type internalFooClient interface {
+	Close() error
+	setGoogleClientInfo(...string)
+	Connection() *grpc.ClientConn
+	Zip(context.Context, *mypackagepb.Bar, ...gax.CallOption) (*ZipRegistryOperation, error)
+	ZipRegistryOperation(name string) *ZipRegistryOperation
+	ListOperations(context.Context, *longrunningpb.ListOperationsRequest, ...gax.CallOption) *OperationIterator
+	GetOperation(context.Context, *longrunningpb.GetOperationRequest, ...gax.CallOption) (*longrunningpb.Operation, error)
+	DeleteOperation(context.Context, *longrunningpb.DeleteOperationRequest, ...gax.CallOption) error
+	CancelOperation(context.Context, *longrunningpb.CancelOperationRequest, ...gax.CallOption) error
+	WaitOperation(context.Context, *longrunningpb.WaitOperationRequest, ...gax.CallOption) (*longrunningpb.Operation, error)
+}
+
+// FooClient is a client for interacting with Awesome Foo API.
+// Methods, except Close, may be called concurrently. However, fields must not be modified concurrently with method calls.
+//
+// Foo service does stuff.
+type FooClient struct {
+	// The internal transport-dependent client.
+	internalClient internalFooClient
+
+	// The call options for this service.
+	CallOptions *FooCallOptions
+
+	// LROClient is used internally to handle long-running operations.
+	// It is exposed so that its CallOptions can be modified if required.
+	// Users should not Close this client.
+	LROClient *lroauto.OperationsClient
+
+}
+
+// Wrapper methods routed to the internal client.
+
+// Close closes the connection to the API service. The user should invoke this when
+// the client is no longer required.
+func (c *FooClient) Close() error {
+	return c.internalClient.Close()
+}
+
+// setGoogleClientInfo sets the name and version of the application in
+// the `x-goog-api-client` header passed on each request. Intended for
+// use by Google-written clients.
+func (c *FooClient) setGoogleClientInfo(keyval ...string) {
+	c.internalClient.setGoogleClientInfo(keyval...)
+}
+
+// Connection returns a connection to the API service.
+//
+// Deprecated: Connections are now pooled so this method does not always
+// return the same resource.
+func (c *FooClient) Connection() *grpc.ClientConn {
+	return c.internalClient.Connection()
+}
+
+// Zip does some stuff.
+func (c *FooClient) Zip(ctx context.Context, req *mypackagepb.Bar, opts ...gax.CallOption) (*ZipRegistryOperation, error) {
+	return c.internalClient.Zip(ctx, req, opts...)
+}
+
+// ZipRegistryOperation returns a new ZipRegistryOperation from a given name.
+// The name must be that of a previously created ZipRegistryOperation, possibly from a different process.
+func (c *FooClient) ZipRegistryOperation(name string) *ZipRegistryOperation {
+	return c.internalClient.ZipRegistryOperation(name)
+}
+
+func (c *FooClient) ListOperations(ctx context.Context, req *longrunningpb.ListOperationsRequest, opts ...gax.CallOption) *OperationIterator {
+	return c.internalClient.ListOperations(ctx, req, opts...)
+}
+
+func (c *FooClient) GetOperation(ctx context.Context, req *longrunningpb.GetOperationRequest, opts ...gax.CallOption) (*longrunningpb.Operation, error) {
+	return c.internalClient.GetOperation(ctx, req, opts...)
+}
+
+func (c *FooClient) DeleteOperation(ctx context.Context, req *longrunningpb.DeleteOperationRequest, opts ...gax.CallOption) error {
+	return c.internalClient.DeleteOperation(ctx, req, opts...)
+}
+
+func (c *FooClient) CancelOperation(ctx context.Context, req *longrunningpb.CancelOperationRequest, opts ...gax.CallOption) error {
+	return c.internalClient.CancelOperation(ctx, req, opts...)
+}
+
+func (c *FooClient) WaitOperation(ctx context.Context, req *longrunningpb.WaitOperationRequest, opts ...gax.CallOption) (*longrunningpb.Operation, error) {
+	return c.internalClient.WaitOperation(ctx, req, opts...)
+}
+
+// fooGRPCClient is a client for interacting with Awesome Foo API over gRPC transport.
+//
+// Methods, except Close, may be called concurrently. However, fields must not be modified concurrently with method calls.
+type fooGRPCClient struct {
+	// Connection pool of gRPC connections to the service.
+	connPool gtransport.ConnPool
+
+	// Points back to the CallOptions field of the containing FooClient
+	CallOptions **FooCallOptions
+
+	// The gRPC API client.
+	fooClient mypackagepb.FooClient
+
+	// LROClient is used internally to handle long-running operations.
+	// It is exposed so that its CallOptions can be modified if required.
+	// Users should not Close this client.
+	LROClient **lroauto.OperationsClient
+
+	operationsClient longrunningpb.OperationsClient
+
+	// The x-goog-* metadata to be sent with each request.
+	xGoogHeaders []string
+}
+
+// NewFooClient creates a new foo client based on gRPC.
+// The returned client must be Closed when it is done being used to clean up its underlying connections.
+//
+// Foo service does stuff.
+func NewFooClient(ctx context.Context, opts ...option.ClientOption) (*FooClient, error) {
+	clientOpts := defaultFooGRPCClientOptions()
+	if newFooClientHook != nil {
+		hookOpts, err := newFooClientHook(ctx, clientHookParams{})
+		if err != nil {
+			return nil, err
+		}
+		clientOpts = append(clientOpts, hookOpts...)
+	}
+
+	connPool, err := gtransport.DialPool(ctx, append(clientOpts, opts...)...)
+	if err != nil {
+		return nil, err
+	}
+	client := FooClient{CallOptions: defaultFooCallOptions()}
+
+	c := &fooGRPCClient{
+		connPool:    connPool,
+		fooClient: mypackagepb.NewFooClient(connPool),
+		CallOptions: &client.CallOptions,
+		operationsClient: longrunningpb.NewOperationsClient(connPool),
+
+	}
+	c.setGoogleClientInfo()
+
+	client.internalClient = c
+
+	client.LROClient, err = lroauto.NewOperationsClient(ctx, gtransport.WithConnPool(connPool))
+	if err != nil {
+		// This error "should not happen", since we are just reusing old connection pool
+		// and never actually need to dial.
+		// If this does happen, we could leak connp. However, we cannot close conn:
+		// If the user invoked the constructor with option.WithGRPCConn,
+		// we would close a connection that's still in use.
+		// TODO: investigate error conditions.
+		return nil, err
+	}
+	c.LROClient = &client.LROClient
+	return &client, nil
+}
+
+// Connection returns a connection to the API service.
+//
+// Deprecated: Connections are now pooled so this method does not always
+// return the same resource.
+func (c *fooGRPCClient) Connection() *grpc.ClientConn {
+	return c.connPool.Conn()
+}
+
+// setGoogleClientInfo sets the name and version of the application in
+// the `x-goog-api-client` header passed on each request. Intended for
+// use by Google-written clients.
+func (c *fooGRPCClient) setGoogleClientInfo(keyval ...string) {
+	kv := append([]string{"gl-go", gax.GoVersion}, keyval...)
+	kv = append(kv, "gapic", getVersionClient(), "gax", gax.Version, "grpc", grpc.Version)
+	c.xGoogHeaders = []string{"x-goog-api-client", gax.XGoogHeader(kv...)}
+}
+
+// Close closes the connection to the API service. The user should invoke this when
+// the client is no longer required.
+func (c *fooGRPCClient) Close() error {
+	return c.connPool.Close()
+}
+