Skip to content

Commit

Permalink
fix(internal/gengapic): add workaround for operation collision (#1397)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
codyoss committed Sep 21, 2023
1 parent 845a130 commit edb3b8f
Show file tree
Hide file tree
Showing 6 changed files with 243 additions and 9 deletions.
1 change: 1 addition & 0 deletions internal/gengapic/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
6 changes: 3 additions & 3 deletions internal/gengapic/client_init.go
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand Down Expand Up @@ -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())
Expand Down
45 changes: 44 additions & 1 deletion internal/gengapic/client_init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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{
Expand Down
2 changes: 1 addition & 1 deletion internal/gengapic/genrest.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
20 changes: 16 additions & 4 deletions internal/gengapic/lro.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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")
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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"
}
178 changes: 178 additions & 0 deletions internal/gengapic/testdata/lro_client_conflict.want
Original file line number Diff line number Diff line change
@@ -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()
}

0 comments on commit edb3b8f

Please sign in to comment.