fix(gengapic): regapic GetOperation path fallback logic (#1072)

Author: quartzmo

internal/gengapic/genrest.go

@@ -937,7 +937,7 @@ func (g *generator) lroRESTCall(servName string, m *descriptor.MethodDescriptorP
 	p("  return nil, e")
 	p("}")
 	p("")
-	override := g.getOperationPathOverride()
+	override := g.getOperationPathOverride(g.descInfo.ParentFile[m].GetPackage())
 	p("override := fmt.Sprintf(%q, resp.GetName())", override)
 	p("return &%s{", opWrapperType)
 	p("  lro: longrunning.InternalNewOperation(*c.LROClient, resp),")

internal/gengapic/helpers.go

@@ -122,6 +122,17 @@ func hasMethod(service *descriptor.ServiceDescriptorProto, method string) bool {
 	return false
 }
 
+// getMethod returns the MethodDescriptorProto for the given service RPC and simple method name.
+func getMethod(service *descriptor.ServiceDescriptorProto, method string) *descriptor.MethodDescriptorProto {
+	for _, m := range service.GetMethod() {
+		if m.GetName() == method {
+			return m
+		}
+	}
+
+	return nil
+}
+
 // containsTransport determines if a set of transports contains a specific
 // transport.
 func containsTransport(t []transport, tr transport) bool {

internal/gengapic/lro.go

@@ -75,7 +75,8 @@ func (g *generator) lroCall(servName string, m *descriptor.MethodDescriptorProto
 }
 
 func (g *generator) lroType(servName string, serv *descriptor.ServiceDescriptorProto, m *descriptor.MethodDescriptorProto) error {
-	mFQN := fmt.Sprintf("%s.%s.%s", g.descInfo.ParentFile[serv].GetPackage(), serv.GetName(), m.GetName())
+	protoPkg := g.descInfo.ParentFile[serv].GetPackage()
+	mFQN := fmt.Sprintf("%s.%s.%s", protoPkg, serv.GetName(), m.GetName())
 	lroType := lroTypeName(m.GetName())
 	p := g.printf
 	hasREST := containsTransport(g.opts.transports, rest)
@@ -94,7 +95,7 @@ func (g *generator) lroType(servName string, serv *descriptor.ServiceDescriptorP
 		// TODO(ndietz) this won't work with nested message types in the same package;
 		// migrating to protoreflect will help remove from semantic meaning in the names.
 		if strings.IndexByte(fullName, '.') < 0 {
-			fullName = g.descInfo.ParentFile[serv].GetPackage() + "." + fullName
+			fullName = protoPkg + "." + fullName
 		}
 
 		// When we build a map[name]Type in pbinfo, we prefix names with '.' to signify that they are fully qualified.
@@ -121,7 +122,7 @@ func (g *generator) lroType(servName string, serv *descriptor.ServiceDescriptorP
 		// TODO(ndietz) this won't work with nested message types in the same package;
 		// migrating to protoreflect will help remove from semantic meaning in the names.
 		if strings.IndexByte(fullName, '.') < 0 {
-			fullName = g.descInfo.ParentFile[serv].GetPackage() + "." + fullName
+			fullName = protoPkg + "." + fullName
 		}
 		fullName = "." + fullName
 
@@ -164,7 +165,7 @@ func (g *generator) lroType(servName string, serv *descriptor.ServiceDescriptorP
 				p("")
 			case rest:
 				receiver := lowcaseRestClientName(servName)
-				override := g.getOperationPathOverride()
+				override := g.getOperationPathOverride(protoPkg)
 				p("func (c *%s) %[2]s(name string) *%[2]s {", receiver, lroType)
 				p("  override := fmt.Sprintf(%q, name)", override)
 				p("  return &%s{", lroType)

internal/gengapic/mixins.go

@@ -16,6 +16,7 @@ package gengapic
 
 import (
 	"fmt"
+	"regexp"
 
 	"github.com/golang/protobuf/protoc-gen-go/descriptor"
 	"github.com/googleapis/gapic-generator-go/internal/pbinfo"
@@ -28,6 +29,17 @@ import (
 )
 
 func init() {
+	initMixinFiles()
+}
+
+var apiVersionRegexp = regexp.MustCompile(`v\d+[a-z]*\d*[a-z]*\d*`)
+
+var mixinFiles map[string][]*descriptor.FileDescriptorProto
+
+type mixins map[string][]*descriptor.MethodDescriptorProto
+
+// initMixinFiles allows test code to re-initialize the mixinFiles global.
+func initMixinFiles() {
 	mixinFiles = map[string][]*descriptor.FileDescriptorProto{
 		"google.cloud.location.Locations": {
 			protodesc.ToFileDescriptorProto(location.File_google_cloud_location_locations_proto),
@@ -43,10 +55,6 @@ func init() {
 	}
 }
 
-var mixinFiles map[string][]*descriptor.FileDescriptorProto
-
-type mixins map[string][]*descriptor.MethodDescriptorProto
-
 // collectMixins collects the configured mixin APIs from the Service config and
 // gathers the appropriately configured mixin methods to generate for each.
 func (g *generator) collectMixins() {
@@ -246,9 +254,24 @@ func (g *generator) lookupHTTPOverride(fqn string, f func(h *annotations.HttpRul
 	return ""
 }
 
-func (g *generator) getOperationPathOverride() string {
+// getOperationPathOverride looks up the google.api.http rule for LRO GetOperation
+// and returns the path override. If no value is present, it synthesizes a path
+// using the proto package client version, for example, "/v1/{name=operations/**}".
+func (g *generator) getOperationPathOverride(protoPkg string) string {
 	get := func(h *annotations.HttpRule) string { return h.GetGet() }
 	override := g.lookupHTTPOverride("google.longrunning.Operations.GetOperation", get)
+	if override == "" {
+		// extract httpInfo from "hot loaded" Operations.GetOperation MethodDescriptor
+		// Should be "/v1/{name=operations/**}"
+		file := mixinFiles["google.longrunning.Operations"][0]
+		mdp := getMethod(file.GetService()[0], "GetOperation")
+		getOperationPath := getHTTPInfo(mdp).url
+
+		// extract client version from proto package with global regex
+		// replace version base path in GetOperation path with proto package version segment
+		version := apiVersionRegexp.FindStringSubmatch(protoPkg)
+		override = apiVersionRegexp.ReplaceAllStringFunc(getOperationPath, func(s string) string { return version[0] })
+	}
 	override = httpPatternVarRegex.ReplaceAllStringFunc(override, func(s string) string { return "%s" })
 	return override
 }

internal/gengapic/mixins_test.go

@@ -261,6 +261,52 @@ func TestHasLROMixin(t *testing.T) {
 	}
 }
 
+func TestGetOperationPathOverride(t *testing.T) {
+	for _, tc := range []struct {
+		pkg, want string
+		http      *annotations.Http
+	}{
+		{
+			pkg:  "google.example.library.v1beta2",
+			want: "/v1beta2/%s",
+		},
+		{
+			pkg:  "google.example.library.v1",
+			want: "/v1/%s",
+		},
+		{
+			pkg:  "google.example.library.v3alpha1p1",
+			want: "/v3alpha1p1/%s",
+		},
+		{
+			pkg:  "google.example.library.v2",
+			want: "/v2/%s",
+			http: &annotations.Http{
+				Rules: []*annotations.HttpRule{
+					&annotations.HttpRule{
+						Selector: "google.longrunning.Operations.GetOperation",
+						Pattern: &annotations.HttpRule_Get{
+							Get: "/v2/{operation=projects/*/locations/*/operations/*}",
+						},
+					},
+				},
+			},
+		},
+	} {
+		initMixinFiles()
+		g := generator{
+			comments: make(map[protoiface.MessageV1]string),
+			mixins:   make(mixins),
+			serviceConfig: &serviceconfig.Service{
+				Http: tc.http,
+			},
+		}
+		if got := g.getOperationPathOverride(tc.pkg); !cmp.Equal(got, tc.want) {
+			t.Errorf("TestGetOperationPathOverrideMissing wanted %v but got %v", tc.want, got)
+		}
+	}
+}
+
 // locationMethods is just used for testing.
 func locationMethods() []*descriptor.MethodDescriptorProto {
 	return mixinFiles["google.cloud.location.Locations"][0].GetService()[0].GetMethod()