Skip to content

Commit

Permalink
gapic: fix rest-only client generation (#675)
Browse files Browse the repository at this point in the history 
bazel: add transport option

bazel: add REST deps to go_gapic_library

gapic: remove protobuf-go v1 from showcase test
  • Loading branch information
@noahdietz
noahdietz committed Jun 23, 2021
1 parent ef4d58c commit 9224977
Show file tree
Hide file tree
Showing 14 changed files with 83 additions and 49 deletions.
1 change: 1 addition & 0 deletions .gitignore
View file
Expand Up @@ -4,6 +4,7 @@ perf.data*
showcase/gen
showcase/gapic-showcase
showcase/showcase_grpc_service_config.json
showcase/compliance_suite.json
gofmt.txt
golint.txt
govet.txt
Expand Down
1 change: 1 addition & 0 deletions Makefile
View file
Expand Up @@ -35,5 +35,6 @@ clean:
rm -rf showcase/gen
rm -f showcase/gapic-showcase
rm -f showcase/showcase_grpc_service_config.json
rm -f showcase/compliance_suite.json
rm -f showcase/showcase_v1beta1.yaml
cd showcase; go mod edit -dropreplace github.com/googleapis/gapic-showcase
6 changes: 5 additions & 1 deletion internal/gengapic/client_init.go
View file
Expand Up @@ -74,11 +74,15 @@ func (g *generator) internalClientIntfInit(serv *descriptor.ServiceDescriptorPro
p("Close() error")
p("setGoogleClientInfo(...string)")
p("Connection() *grpc.ClientConn")
g.imports[pbinfo.ImportSpec{Path: "google.golang.org/grpc"}] = true

// The mixin methods are for manipulating LROs, IAM, and Location.
methods := append(serv.GetMethod(), g.getMixinMethods()...)
// methods := serv.GetMethod()

if len(methods) > 0 {
g.imports[pbinfo.ImportSpec{Path: "context"}] = true
g.imports[pbinfo.ImportSpec{Path: "google.golang.org/api/option"}] = true
}
for _, m := range methods {

inType := g.descInfo.Type[m.GetInputType()]
Expand Down
31 changes: 19 additions & 12 deletions internal/gengapic/client_init_test.go
View file
Expand Up @@ -327,6 +327,7 @@ func TestClientInit(t *testing.T) {
{Path: "context"}: true,
{Path: "google.golang.org/grpc"}: true,
{Path: "google.golang.org/grpc/metadata"}: true,
{Path: "google.golang.org/api/option"}: true,
{Name: "gtransport", Path: "google.golang.org/api/transport/grpc"}: true,
{Name: "iampb", Path: "google.golang.org/genproto/googleapis/iam/v1"}: true,
{Name: "locationpb", Path: "google.golang.org/genproto/googleapis/cloud/location"}: true,
Expand All @@ -343,13 +344,16 @@ func TestClientInit(t *testing.T) {
serv: servPlain,
parameter: proto.String("go-gapic-package=path;mypackage,transport=rest"),
imports: map[pbinfo.ImportSpec]bool{
{Path: "bytes"}: true,
{Path: "fmt"}: true,
{Path: "google.golang.org/api/option/internaloption"}: true,
{Path: "google.golang.org/grpc/metadata"}: true,
{Path: "google.golang.org/protobuf/encoding/protojson"}: true,
{Path: "io/ioutil"}: true,
{Path: "net/http"}: true,
{Path: "bytes"}: true,
{Path: "context"}: true,
{Path: "fmt"}: true,
{Path: "google.golang.org/api/option"}: true,
{Path: "google.golang.org/api/option/internaloption"}: true,
{Path: "google.golang.org/grpc"}: true,
{Path: "google.golang.org/grpc/metadata"}: true,
{Path: "google.golang.org/protobuf/encoding/protojson"}: true,
{Path: "io/ioutil"}: true,
{Path: "net/http"}: true,
{Name: "httptransport", Path: "google.golang.org/api/transport/http"}: true,
},
},
Expand All @@ -359,8 +363,9 @@ func TestClientInit(t *testing.T) {
serv: servPlain,
parameter: proto.String("go-gapic-package=path;mypackage,transport=grpc+rest"),
imports: map[pbinfo.ImportSpec]bool{
{Path: "bytes"}: true,
{Path: "fmt"}: true,
{Path: "bytes"}: true,
{Path: "fmt"}: true,
{Path: "google.golang.org/api/option"}: true,
{Path: "google.golang.org/api/option/internaloption"}: true,
{Path: "google.golang.org/protobuf/encoding/protojson"}: true,
{Path: "io/ioutil"}: true,
Expand All @@ -387,6 +392,7 @@ func TestClientInit(t *testing.T) {
{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,
{Path: "google.golang.org/grpc/metadata"}: true,
},
Expand All @@ -400,9 +406,10 @@ func TestClientInit(t *testing.T) {
{Name: "gtransport", Path: "google.golang.org/api/transport/grpc"}: true,
{Name: "httptransport", Path: "google.golang.org/api/transport/http"}: true,
{Name: "mypackagepb", Path: "github.com/googleapis/mypackage"}: true,
{Path: "bytes"}: true,
{Path: "context"}: true,
{Path: "fmt"}: true,
{Path: "bytes"}: true,
{Path: "context"}: true,
{Path: "fmt"}: true,
{Path: "google.golang.org/api/option"}: true,
{Path: "google.golang.org/api/option/internaloption"}: true,
{Path: "google.golang.org/grpc"}: true,
{Path: "google.golang.org/grpc/metadata"}: true,
Expand Down
3 changes: 2 additions & 1 deletion internal/gengapic/gengapic_test.go
View file
Expand Up @@ -489,7 +489,8 @@ func TestGenLRO(t *testing.T) {
"google.iam.v1.IAMPolicy": iamPolicyMethods(),
}
g.opts = &options{
pkgName: "pkg",
pkgName: "pkg",
transports: []transport{grpc},
}
cpb := &conf.ServiceConfig{
MethodConfig: []*conf.MethodConfig{
Expand Down
16 changes: 12 additions & 4 deletions internal/gengapic/gengrpc.go
View file
Expand Up @@ -27,6 +27,14 @@ import (
"google.golang.org/genproto/googleapis/rpc/code"
)

func lowcaseGRPCClientName(servName string) string {
if servName == "" {
return "gRPCClient"
}

return lowerFirst(servName + "GRPCClient")
}

func (g *generator) genGRPCMethods(serv *descriptor.ServiceDescriptorProto, servName string) error {
g.addMetadataServiceForTransport(serv.GetName(), "grpc", servName)

Expand Down Expand Up @@ -91,7 +99,7 @@ func (g *generator) unaryGRPCCall(servName string, m *descriptor.MethodDescripto

p := g.printf

lowcaseServName := lowerFirst(servName + "GRPCClient")
lowcaseServName := lowcaseGRPCClientName(servName)

p("func (c *%s) %s(ctx context.Context, req *%s.%s, opts ...gax.CallOption) (*%s.%s, error) {",
lowcaseServName, m.GetName(), inSpec.Name, inType.GetName(), outSpec.Name, outType.GetName())
Expand Down Expand Up @@ -136,7 +144,7 @@ func (g *generator) emptyUnaryGRPCCall(servName string, m *descriptor.MethodDesc

p := g.printf

lowcaseServName := lowerFirst(servName + "GRPCClient")
lowcaseServName := lowcaseGRPCClientName(servName)

p("func (c *%s) %s(ctx context.Context, req *%s.%s, opts ...gax.CallOption) error {",
lowcaseServName, m.GetName(), inSpec.Name, inType.GetName())
Expand Down Expand Up @@ -263,7 +271,7 @@ func (g *generator) grpcClientInit(serv *descriptor.ServiceDescriptorProto, serv
p := g.printf

// We DON'T want to export the transport layers.
lowcaseServName := lowerFirst(servName + "GRPCClient")
lowcaseServName := lowcaseGRPCClientName(servName)

p("// %s is a client for interacting with %s over gRPC transport.", lowcaseServName, g.apiName)
p("//")
Expand Down Expand Up @@ -314,7 +322,7 @@ func (g *generator) grpcClientUtilities(serv *descriptor.ServiceDescriptorProto,

clientName := camelToSnake(serv.GetName())
clientName = strings.Replace(clientName, "_", " ", -1)
lowcaseServName := lowerFirst(servName + "GRPCClient")
lowcaseServName := lowcaseGRPCClientName(servName)

// Factory function
p("// New%sClient creates a new %s client based on gRPC.", servName, clientName)
Expand Down
16 changes: 5 additions & 11 deletions internal/gengapic/genrest.go
View file
Expand Up @@ -127,7 +127,7 @@ func (g *generator) restClientUtilities(serv *descriptor.ServiceDescriptorProto,
p("")
// TODO(dovs): make rest default call options
// TODO(dovs): set the LRO client
p(" return &%[1]sClient{internalClient: c, CallOptions: default%[1]sCallOptions()}, nil", servName)
p(" return &%[1]sClient{internalClient: c, CallOptions: &%[1]sCallOptions{}}, nil", servName)
p("}")
p("")

Expand Down Expand Up @@ -494,6 +494,9 @@ func (g *generator) pagingRESTCall(servName string, m *descriptor.MethodDescript
p("return it")
p("}")

g.imports[pbinfo.ImportSpec{Path: "google.golang.org/api/iterator"}] = true
g.imports[pbinfo.ImportSpec{Path: "google.golang.org/protobuf/proto"}] = true

return nil
}

Expand All @@ -520,16 +523,7 @@ func (g *generator) lroRESTCall(servName string, m *descriptor.MethodDescriptorP
p("}")
p("")

p("// %[1]s returns a new %[1]s from a given name.", lroType)
p("// The name must be that of a previously created %s, possibly from a different process.", lroType)
p("func (c *%s) %[2]s(name string) *%[2]s {", lowcaseServName, lroType)
p(" return &%s{", lroType)
// TODO(dovs): return a non-empty and useful object
p(" }")
p("}")
p("")

g.imports[pbinfo.ImportSpec{Name: "longrunningpb", Path: "google.golang.org/genproto/googleapis/longrunning"}] = true
g.imports[pbinfo.ImportSpec{Path: "cloud.google.com/go/longrunning"}] = true

return nil
}
Expand Down
34 changes: 22 additions & 12 deletions internal/gengapic/lro.go
View file
Expand Up @@ -93,8 +93,6 @@ func (g *generator) lroType(servName string, serv *descriptor.ServiceDescriptorP
return fmt.Errorf("rpc %q has google.longrunning.operation_info but is missing option google.longrunning.operation_info.response_type", mFQN)
}

lowcaseServName := lowerFirst(servName + "GRPCClient")

var respType string
{
// eLRO.ResponseType is either fully-qualified or top-level in the same package as the method.
Expand Down Expand Up @@ -154,16 +152,28 @@ func (g *generator) lroType(servName string, serv *descriptor.ServiceDescriptorP

// LRO from name
{
p("// %[1]s returns a new %[1]s from a given name.", lroType)
p("// The name must be that of a previously created %s, possibly from a different process.", lroType)
p("func (c *%s) %[2]s(name string) *%[2]s {", lowcaseServName, lroType)
p(" return &%s{", lroType)
p(" lro: longrunning.InternalNewOperation(*c.LROClient, &longrunningpb.Operation{Name: name}),")
p(" }")
p("}")
p("")

g.imports[pbinfo.ImportSpec{Name: "longrunningpb", Path: "google.golang.org/genproto/googleapis/longrunning"}] = true
lowGRPC := lowcaseGRPCClientName(servName)
lowREST := lowcaseRestClientName(servName)
for _, t := range g.opts.transports {
p("// %[1]s returns a new %[1]s from a given name.", lroType)
p("// The name must be that of a previously created %s, possibly from a different process.", lroType)
switch t {
case grpc:
p("func (c *%s) %[2]s(name string) *%[2]s {", lowGRPC, lroType)
p(" return &%s{", lroType)
p(" lro: longrunning.InternalNewOperation(*c.LROClient, &longrunningpb.Operation{Name: name}),")
p(" }")
// TODO(noahdietz): move this outside of the for-loop when REST starts using it.
g.imports[pbinfo.ImportSpec{Name: "longrunningpb", Path: "google.golang.org/genproto/googleapis/longrunning"}] = true
case rest:
p("func (c *%s) %[2]s(name string) *%[2]s {", lowREST, lroType)
// TODO(dovs): return a non-empty and useful object
p(" return &%s{}", lroType)
}
p("}")

p("")
}
}

// Wait
Expand Down
2 changes: 1 addition & 1 deletion internal/gengapic/testdata/deprecated_client_init.want
View file
Expand Up @@ -161,7 +161,7 @@ func NewRESTClient(ctx context.Context, opts ...option.ClientOption) (*Client, e
}
c.setGoogleClientInfo()

return &Client{internalClient: c, CallOptions: defaultCallOptions()}, nil
return &Client{internalClient: c, CallOptions: &CallOptions{}}, nil
}

// setGoogleClientInfo sets the name and version of the application in
Expand Down
2 changes: 1 addition & 1 deletion internal/gengapic/testdata/empty_client_init.want
View file
Expand Up @@ -155,7 +155,7 @@ func NewRESTClient(ctx context.Context, opts ...option.ClientOption) (*Client, e
}
c.setGoogleClientInfo()

return &Client{internalClient: c, CallOptions: defaultCallOptions()}, nil
return &Client{internalClient: c, CallOptions: &CallOptions{}}, nil
}

// setGoogleClientInfo sets the name and version of the application in
Expand Down
2 changes: 1 addition & 1 deletion internal/gengapic/testdata/foo_rest_client_init.want
View file
Expand Up @@ -99,7 +99,7 @@ func NewFooRESTClient(ctx context.Context, opts ...option.ClientOption) (*FooCli
}
c.setGoogleClientInfo()

return &FooClient{internalClient: c, CallOptions: defaultFooCallOptions()}, nil
return &FooClient{internalClient: c, CallOptions: &FooCallOptions{}}, nil
}

// setGoogleClientInfo sets the name and version of the application in
Expand Down
8 changes: 8 additions & 0 deletions rules_go_gapic/go_gapic.bzl
View file
Expand Up @@ -93,6 +93,7 @@ def go_gapic_library(
samples = [],
sample_only = False,
metadata = False,
transport = ["grpc"],
**kwargs):

file_args = {}
Expand All @@ -110,8 +111,13 @@ def go_gapic_library(
for path in samples:
file_args[path] = "sample"

t = transport[0]
if len(transport) > 1:
t = "+".join(transport)

plugin_args = [
"go-gapic-package={}".format(importpath),
"transport={}".format(t)
]

if release_level:
Expand Down Expand Up @@ -149,12 +155,14 @@ def go_gapic_library(
"@org_golang_google_api//option:go_default_library",
"@org_golang_google_api//option/internaloption:go_default_library",
"@org_golang_google_api//iterator:go_default_library",
"@org_golang_google_api//transport/http:go_default_library",
"@org_golang_google_api//transport/grpc:go_default_library",
"@org_golang_google_grpc//:go_default_library",
"@org_golang_google_grpc//codes:go_default_library",
"@org_golang_google_grpc//metadata:go_default_library",
"@org_golang_google_grpc//status:go_default_library",
"@org_golang_google_protobuf//proto:go_default_library",
"@org_golang_google_protobuf//encoding/protojson:go_default_library",
"@org_golang_google_protobuf//types/known/emptypb:go_default_library",
"@org_golang_google_protobuf//types/known/wrapperspb:go_default_library",
"@org_golang_google_protobuf//types/known/fieldmaskpb:go_default_library",
Expand Down
2 changes: 1 addition & 1 deletion showcase/compliance_test.go
View file
Expand Up @@ -24,12 +24,12 @@ import (
"strings"
"testing"

"github.com/golang/protobuf/proto"
"github.com/google/go-cmp/cmp"
showcase "github.com/googleapis/gapic-showcase/client"
genprotopb "github.com/googleapis/gapic-showcase/server/genproto"
gax "github.com/googleapis/gax-go/v2"
"google.golang.org/protobuf/encoding/protojson"
"google.golang.org/protobuf/proto"
)

var complianceClient *showcase.ComplianceClient
Expand Down
8 changes: 4 additions & 4 deletions test.sh
View file
Expand Up @@ -36,11 +36,11 @@ generate() {
protoc --go_gapic_out "$OUT" -I "$GOOGLEAPIS" $*
}

echo "Generating Cloud KMS v1"
generate --go_gapic_opt 'go-gapic-package=cloud.google.com/go/kms/apiv1;kms,transport=grpc+rest' $GOOGLEAPIS/google/cloud/kms/v1/*.proto
echo "Generating Cloud KMS v1 - gRPC"
generate --go_gapic_opt 'go-gapic-package=cloud.google.com/go/kms/apiv1;kms,transport=grpc' $GOOGLEAPIS/google/cloud/kms/v1/*.proto

echo "Generating Cloud Data Catalog v1beta1"
generate --go_gapic_opt 'go-gapic-package=cloud.google.com/go/datacatalog/apiv1beta1;datacatalog,transport=grpc+rest' $GOOGLEAPIS/google/cloud/datacatalog/v1beta1/*.proto
echo "Generating Cloud Data Catalog v1beta1 - REST"
generate --go_gapic_opt 'go-gapic-package=cloud.google.com/go/datacatalog/apiv1beta1;datacatalog,transport=rest' $GOOGLEAPIS/google/cloud/datacatalog/v1beta1/*.proto

echo "Generating Cloud Text-to-Speech v1 w/gRPC ServiceConfig"
generate --go_gapic_opt 'go-gapic-package=cloud.google.com/go/texttospeech/apiv1;texttospeech,transport=grpc+rest' --go_gapic_opt "grpc-service-config=$GOOGLEAPIS/google/cloud/texttospeech/v1/texttospeech_grpc_service_config.json" $GOOGLEAPIS/google/cloud/texttospeech/v1/*.proto
Expand Down

0 comments on commit 9224977

Please sign in to comment.