From 7d08d1bddb9cd15fc6cd30f60fc6f352b1c00eeb Mon Sep 17 00:00:00 2001 From: Noah Dietz Date: Tue, 28 Jun 2022 13:06:03 -0700 Subject: [PATCH] fix(gengapic): regapic fix missing return statement (#1054) --- internal/gengapic/BUILD.bazel | 1 + internal/gengapic/gengapic.go | 1 + internal/gengapic/genrest.go | 9 +-- internal/gengapic/genrest_test.go | 58 +++++++++++++--- .../gengapic/testdata/rest_HttpBodyRPC.want | 66 +++++++++++++++++++ test.sh | 3 + 6 files changed, 125 insertions(+), 13 deletions(-) create mode 100644 internal/gengapic/testdata/rest_HttpBodyRPC.want diff --git a/internal/gengapic/BUILD.bazel b/internal/gengapic/BUILD.bazel index cdd74123c..8116eebc2 100644 --- a/internal/gengapic/BUILD.bazel +++ b/internal/gengapic/BUILD.bazel @@ -74,6 +74,7 @@ go_test( "@com_github_google_go_cmp//cmp", "@go_googleapis//gapic/metadata:metadata_go_proto", "@go_googleapis//google/api:annotations_go_proto", + "@go_googleapis//google/api:httpbody_go_proto", "@go_googleapis//google/api:serviceconfig_go_proto", "@go_googleapis//google/cloud:extended_operations_go_proto", "@go_googleapis//google/longrunning:longrunning_go_proto", diff --git a/internal/gengapic/gengapic.go b/internal/gengapic/gengapic.go index 6381b6044..4760f02da 100644 --- a/internal/gengapic/gengapic.go +++ b/internal/gengapic/gengapic.go @@ -36,6 +36,7 @@ const ( // protoc puts a dot in front of name, signaling that the name is fully qualified. emptyType = "." + emptyValue lroType = ".google.longrunning.Operation" + httpBodyType = ".google.api.HttpBody" alpha = "alpha" beta = "beta" disableDeadlinesVar = "GOOGLE_API_GO_EXPERIMENTAL_DISABLE_DEFAULT_DEADLINE" diff --git a/internal/gengapic/genrest.go b/internal/gengapic/genrest.go index 9082910dd..8c1147dfb 100644 --- a/internal/gengapic/genrest.go +++ b/internal/gengapic/genrest.go @@ -1045,8 +1045,8 @@ func (g *generator) unaryRESTCall(servName string, m *descriptor.MethodDescripto if err != nil { return err } - outFqn := fmt.Sprintf("%s.%s", g.descInfo.ParentFile[outType].GetPackage(), outType.GetName()) - isHTTPBodyMessage := outFqn == "google.api.HttpBody" + outFqn := fmt.Sprintf(".%s.%s", g.descInfo.ParentFile[outType].GetPackage(), outType.GetName()) + isHTTPBodyMessage := outFqn == httpBodyType // Ignore error because the only possible error would be from looking up // the ImportSpec for the OutputType, which has already happened above. @@ -1133,9 +1133,10 @@ func (g *generator) unaryRESTCall(servName string, m *descriptor.MethodDescripto p("if err := unm.Unmarshal(buf, resp); err != nil {") p(" return maybeUnknownEnum(err)") p("}") - p("") - p("return nil") } + + p("") + p(" return nil") p("}, opts...)") p("if e != nil {") p(" return nil, e") diff --git a/internal/gengapic/genrest_test.go b/internal/gengapic/genrest_test.go index c92bad3fe..2adcf79c2 100644 --- a/internal/gengapic/genrest_test.go +++ b/internal/gengapic/genrest_test.go @@ -26,6 +26,7 @@ import ( "github.com/googleapis/gapic-generator-go/internal/pbinfo" "github.com/googleapis/gapic-generator-go/internal/txtdiff" "google.golang.org/genproto/googleapis/api/annotations" + "google.golang.org/genproto/googleapis/api/httpbody" "google.golang.org/genproto/googleapis/api/serviceconfig" "google.golang.org/genproto/googleapis/cloud/extendedops" "google.golang.org/genproto/googleapis/longrunning" @@ -570,6 +571,26 @@ func TestGenRestMethod(t *testing.T) { Options: lroRPCOpt, } + httpBodyDesc := protodesc.ToDescriptorProto((&httpbody.HttpBody{}).ProtoReflect().Descriptor()) + httpBodyRPCOpt := &descriptor.MethodOptions{} + proto.SetExtension(httpBodyRPCOpt, annotations.E_Http, &annotations.HttpRule{ + Pattern: &annotations.HttpRule_Post{ + Post: "/v1/foo", + }, + Body: "*", + }) + proto.SetExtension(httpBodyRPCOpt, annotations.E_Routing, &annotations.RoutingRule{ + RoutingParameters: []*annotations.RoutingParameter{ + {Field: "other"}, + }, + }) + httpBodyRPC := &descriptor.MethodDescriptorProto{ + Name: proto.String("HttpBodyRPC"), + InputType: proto.String(foofqn), + OutputType: proto.String(httpBodyType), + Options: httpBodyRPCOpt, + } + s := &descriptor.ServiceDescriptorProto{ Name: proto.String("FooService"), } @@ -599,15 +620,16 @@ func TestGenRestMethod(t *testing.T) { }, descInfo: pbinfo.Info{ ParentFile: map[protoiface.MessageV1]*descriptor.FileDescriptorProto{ - op: f, - opS: f, - opRPC: f, - lroRPC: f, - foo: f, - s: f, - pagedFooReq: f, - pagedFooRes: f, - lroDesc: protodesc.ToFileDescriptorProto(longrunning.File_google_longrunning_operations_proto), + op: f, + opS: f, + opRPC: f, + lroRPC: f, + foo: f, + s: f, + pagedFooReq: f, + pagedFooRes: f, + lroDesc: protodesc.ToFileDescriptorProto(longrunning.File_google_longrunning_operations_proto), + httpBodyDesc: protodesc.ToFileDescriptorProto(httpbody.File_google_api_httpbody_proto), }, ParentElement: map[pbinfo.ProtoType]pbinfo.ProtoType{ opRPC: s, @@ -616,6 +638,7 @@ func TestGenRestMethod(t *testing.T) { pagingRPC: s, serverStreamRPC: s, lroRPC: s, + httpBodyRPC: s, nameField: op, sizeField: foo, otherField: foo, @@ -627,6 +650,7 @@ func TestGenRestMethod(t *testing.T) { pagedFooReqFQN: pagedFooReq, pagedFooResFQN: pagedFooRes, lroType: lroDesc, + httpBodyType: httpBodyDesc, }, }, } @@ -718,6 +742,22 @@ func TestGenRestMethod(t *testing.T) { {Name: "longrunningpb", Path: "google.golang.org/genproto/googleapis/longrunning"}: true, }, }, + { + name: "http_body_rpc", + method: httpBodyRPC, + options: &options{}, + imports: map[pbinfo.ImportSpec]bool{ + {Path: "bytes"}: true, + {Path: "fmt"}: true, + {Path: "google.golang.org/protobuf/encoding/protojson"}: true, + {Path: "google.golang.org/api/googleapi"}: true, + {Path: "net/url"}: true, + {Path: "regexp"}: true, + {Path: "strings"}: true, + {Name: "foopb", Path: "google.golang.org/genproto/cloud/foo/v1"}: true, + {Name: "httpbodypb", Path: "google.golang.org/genproto/googleapis/api/httpbody"}: true, + }, + }, } { s.Method = []*descriptor.MethodDescriptorProto{tst.method} g.opts = tst.options diff --git a/internal/gengapic/testdata/rest_HttpBodyRPC.want b/internal/gengapic/testdata/rest_HttpBodyRPC.want new file mode 100644 index 000000000..18ce2e33f --- /dev/null +++ b/internal/gengapic/testdata/rest_HttpBodyRPC.want @@ -0,0 +1,66 @@ +func (c *fooRESTClient) HttpBodyRPC(ctx context.Context, req *foopb.Foo, opts ...gax.CallOption) (*httpbodypb.HttpBody, error) { + m := protojson.MarshalOptions{AllowPartial: true, UseEnumNumbers: true} + jsonReq, err := m.Marshal(req) + if err != nil { + return nil, err + } + + baseUrl, err := url.Parse(c.endpoint) + if err != nil { + return nil, err + } + baseUrl.Path += fmt.Sprintf("/v1/foo") + + // Build HTTP headers from client and context metadata. + routingHeaders := "" + routingHeadersMap := make(map[string]string) + if reg := regexp.MustCompile("(.*)"); reg.MatchString(req.GetOther()) && len(url.QueryEscape(reg.FindStringSubmatch(req.GetOther())[1])) > 0 { + routingHeadersMap["other"] = url.QueryEscape(reg.FindStringSubmatch(req.GetOther())[1]) + } + for headerName, headerValue := range routingHeadersMap { + routingHeaders = fmt.Sprintf("%s%s=%s&", routingHeaders, headerName, headerValue) + } + routingHeaders = strings.TrimSuffix(routingHeaders, "&") + md := metadata.Pairs("x-goog-request-params", routingHeaders) + + headers := buildHeaders(ctx, c.xGoogMetadata, md, metadata.Pairs("Content-Type", "application/json")) + opts = append((*c.CallOptions).HttpBodyRPC[0:len((*c.CallOptions).HttpBodyRPC):len((*c.CallOptions).HttpBodyRPC)], opts...) + resp := &httpbodypb.HttpBody{} + e := gax.Invoke(ctx, func(ctx context.Context, settings gax.CallSettings) error { + if settings.Path != "" { + baseUrl.Path = settings.Path + } + httpReq, err := http.NewRequest("POST", baseUrl.String(), bytes.NewReader(jsonReq)) + if err != nil { + return err + } + httpReq = httpReq.WithContext(ctx) + httpReq.Header = headers + + httpRsp, err := c.httpClient.Do(httpReq) + if err != nil{ + return err + } + defer httpRsp.Body.Close() + + if err = googleapi.CheckResponse(httpRsp); err != nil { + return err + } + + buf, err := ioutil.ReadAll(httpRsp.Body) + if err != nil { + return err + } + + resp.Data = buf + if headers := httpRsp.Header; len(headers["Content-Type"]) > 0 { + resp.ContentType = headers["Content-Type"][0] + } + + return nil + }, opts...) + if e != nil { + return nil, e + } + return resp, nil +} diff --git a/test.sh b/test.sh index ac84bd200..66b831fee 100755 --- a/test.sh +++ b/test.sh @@ -48,6 +48,9 @@ generate --go_gapic_opt 'go-gapic-package=cloud.google.com/go/texttospeech/apiv1 echo "Generating Cloud Storage v2" generate --go_gapic_opt 'go-gapic-package=cloud.google.com/go/storage/internal/apiv2;storage,transport=grpc' $GOOGLEAPIS/google/storage/v2/*.proto +echo "Generating Cloud Retail v2" +generate --go_gapic_opt 'go-gapic-package=cloud.google.com/go/retail/apiv2;retail,transport=rest' $GOOGLEAPIS/google/cloud/retail/v2/*.proto + echo "Generation complete" echo "Running gofmt to check for syntax errors"