Repository: googleapis/gapic-generator-go
Pull request #1310 (2023-04-18 21:56:48)

fix(internal/gengapic): fix mixin file filter
Fixes filtering out mixin proto files unless generating a mixin gapic explicitly.

The check is now based on if the Go package path for the GAPIC to be generated includes the `iam` `location` or `longrunning` package in it e.g. `cloud.google.com/go/iam/apiv1`. This ensures we can still generate a GAPIC for those mixin services, but prevents them from being accidentally included in a dependent GAPIC.

Refactors some of this code to make it easier to unit test.


Patch:
diff --git a/internal/gengapic/gengapic.go b/internal/gengapic/gengapic.go
index 7b10b45..d51d918 100644
--- a/internal/gengapic/gengapic.go
+++ b/internal/gengapic/gengapic.go
@@ -59,16 +59,7 @@ func Gen(genReq *plugin.CodeGeneratorRequest) (*plugin.CodeGeneratorResponse, er
 		return &g.resp, err
 	}
 
-	var genServs []*descriptor.ServiceDescriptorProto
-	for _, f := range genReq.GetProtoFile() {
-		if !strContains(genReq.GetFileToGenerate(), f.GetName()) {
-			continue
-		}
-		if !g.includeMixinInputFile(f.GetName()) {
-			continue
-		}
-		genServs = append(genServs, f.GetService()...)
-	}
+	genServs := g.collectServices(genReq)
 
 	if len(genServs) == 0 {
 		return &g.resp, nil
@@ -175,6 +166,20 @@ func Gen(genReq *plugin.CodeGeneratorRequest) (*plugin.CodeGeneratorResponse, er
 	return &g.resp, nil
 }
 
+// Collects the proto services to generate GAPICs for from the CodeGeneratorRequest.
+func (g *generator) collectServices(genReq *plugin.CodeGeneratorRequest) (genServs []*descriptor.ServiceDescriptorProto) {
+	for _, f := range genReq.GetProtoFile() {
+		if !strContains(genReq.GetFileToGenerate(), f.GetName()) {
+			continue
+		}
+		if !g.includeMixinInputFile(f.GetName()) {
+			continue
+		}
+		genServs = append(genServs, f.GetService()...)
+	}
+	return
+}
+
 // gen generates client for the given service.
 func (g *generator) gen(serv *descriptor.ServiceDescriptorProto) error {
 	servName := pbinfo.ReduceServName(serv.GetName(), g.opts.pkgName)
diff --git a/internal/gengapic/mixins.go b/internal/gengapic/mixins.go
index fe8b4c2..9b581ab 100644
--- a/internal/gengapic/mixins.go
+++ b/internal/gengapic/mixins.go
@@ -17,6 +17,7 @@ package gengapic
 import (
 	"fmt"
 	"regexp"
+	"strings"
 
 	iam "cloud.google.com/go/iam/apiv1/iampb"
 	longrunning "cloud.google.com/go/longrunning/autogen/longrunningpb"
@@ -227,18 +228,19 @@ func (g *generator) checkIAMPolicyOverrides(servs []*descriptor.ServiceDescripto
 
 // includeMixinInputFile determines if the given proto file name matches
 // a known mixin file and indicates if it should be included in the
-// protos-to-be-generated file set based on if the package is using it for
-// mixins or not.
+// protos-to-be-generated file set based on if the Go package to be generated
+// is for one of the mixin services explicitly or not.
 func (g *generator) includeMixinInputFile(file string) bool {
-	if file == "google/cloud/location/locations.proto" && g.hasLocationMixin() {
+	if strings.HasPrefix(file, "google/cloud/location") && !strings.Contains(g.opts.pkgPath, "location") {
 		return false
 	}
-	if file == "google/iam/v1/iam_policy.proto" && g.hasIAMPolicyMixin() {
+	if strings.HasPrefix(file, "google/iam/v1") && !strings.Contains(g.opts.pkgPath, "iam") {
 		return false
 	}
-	if file == "google/longrunning/operations.proto" && g.hasLROMixin() {
+	if strings.HasPrefix(file, "google/longrunning") && !strings.Contains(g.opts.pkgPath, "longrunning") {
 		return false
 	}
+	// Not a mixin file or generating a mixin GAPIC explicitly so include the file.
 	return true
 }
 


Test patch:
diff --git a/internal/gengapic/gengapic_test.go b/internal/gengapic/gengapic_test.go
index 61a5958..a3d8ec8 100644
--- a/internal/gengapic/gengapic_test.go
+++ b/internal/gengapic/gengapic_test.go
@@ -1235,6 +1235,66 @@ func TestReturnType(t *testing.T) {
 	}
 }
 
+func TestCollectServices(t *testing.T) {
+	libraryServ := &descriptor.ServiceDescriptorProto{
+		Name: proto.String("Library"),
+	}
+	library := &descriptor.FileDescriptorProto{
+		Name: proto.String("google/cloud/library/v1/library.proto"),
+		Options: &descriptor.FileOptions{
+			GoPackage: proto.String("cloud.google.com/go/library/apiv1/librarypb;librarypb"),
+		},
+		Service: []*descriptor.ServiceDescriptorProto{libraryServ},
+	}
+
+	for _, tst := range []struct {
+		name, goPkgPath string
+		toGen           []string
+		fileSet         []*descriptor.FileDescriptorProto
+		want            []*descriptor.ServiceDescriptorProto
+	}{
+		{
+			name:      "simple",
+			goPkgPath: "cloud.google.com/go/library/apiv1",
+			toGen:     []string{library.GetName()},
+			fileSet:   []*descriptor.FileDescriptorProto{library},
+			want:      []*descriptor.ServiceDescriptorProto{libraryServ},
+		},
+		{
+			name:      "ignore-mixins",
+			goPkgPath: "cloud.google.com/go/library/apiv1",
+			toGen:     []string{library.GetName()},
+			fileSet: []*descriptor.FileDescriptorProto{
+				library,
+				mixinFiles["google.longrunning.Operations"][0],
+				mixinFiles["google.iam.v1.IAMPolicy"][0],
+				mixinFiles["google.cloud.location.Locations"][0],
+			},
+			want: []*descriptor.ServiceDescriptorProto{libraryServ},
+		},
+		{
+			name:      "include-iam-mixin",
+			goPkgPath: "cloud.google.com/go/iam/apiv1",
+			toGen:     []string{mixinFiles["google.iam.v1.IAMPolicy"][0].GetName()},
+			fileSet: []*descriptor.FileDescriptorProto{
+				mixinFiles["google.longrunning.Operations"][0],
+				mixinFiles["google.iam.v1.IAMPolicy"][0],
+				mixinFiles["google.cloud.location.Locations"][0],
+			},
+			want: []*descriptor.ServiceDescriptorProto{mixinFiles["google.iam.v1.IAMPolicy"][0].GetService()[0]},
+		},
+	} {
+		g := &generator{opts: &options{pkgPath: tst.goPkgPath}}
+		got := g.collectServices(&pluginpb.CodeGeneratorRequest{
+			FileToGenerate: tst.toGen,
+			ProtoFile:      tst.fileSet,
+		})
+		if diff := cmp.Diff(got, tst.want, cmp.Comparer(proto.Equal)); diff != "" {
+			t.Errorf("%s: got(-),want(+):\n%s", tst.name, diff)
+		}
+	}
+}
+
 func setHTTPOption(o *descriptor.MethodOptions, pattern string) {
 	proto.SetExtension(o, annotations.E_Http, &annotations.HttpRule{
 		Pattern: &annotations.HttpRule_Get{
diff --git a/internal/gengapic/mixins_test.go b/internal/gengapic/mixins_test.go
index 2972d89..ba753f7 100644
--- a/internal/gengapic/mixins_test.go
+++ b/internal/gengapic/mixins_test.go
@@ -307,6 +307,61 @@ func TestGetOperationPathOverride(t *testing.T) {
 	}
 }
 
+func TestIncludeMixinInputFile(t *testing.T) {
+	for _, tst := range []struct {
+		name, file, pkgPath string
+		want                bool
+	}{
+		{
+			name:    "non-mixin",
+			file:    "google/cloud/library/v1/library.proto",
+			pkgPath: "cloud.google.com/go/library/apiv1",
+			want:    true,
+		},
+		{
+			name:    "exclude-iam",
+			file:    "google/iam/v1/iam_policy.proto",
+			pkgPath: "cloud.google.com/go/library/apiv1",
+			want:    false,
+		},
+		{
+			name:    "exclude-locations",
+			file:    "google/cloud/location/locations.proto",
+			pkgPath: "cloud.google.com/go/library/apiv1",
+			want:    false,
+		},
+		{
+			name:    "exclude-longrunning",
+			file:    "google/longrunning/operations.proto",
+			pkgPath: "cloud.google.com/go/library/apiv1",
+			want:    false,
+		},
+		{
+			name:    "include-iam",
+			file:    "google/iam/v1/iam_policy.proto",
+			pkgPath: "cloud.google.com/go/iam/apiv1",
+			want:    true,
+		},
+		{
+			name:    "include-locations",
+			file:    "google/cloud/location/locations.proto",
+			pkgPath: "cloud.google.com/go/locations/apiv1",
+			want:    true,
+		},
+		{
+			name:    "include-longrunning",
+			file:    "google/longrunning/operations.proto",
+			pkgPath: "cloud.google.com/go/longrunning/apiv1",
+			want:    true,
+		},
+	} {
+		g := &generator{opts: &options{pkgPath: tst.pkgPath}}
+		if got := g.includeMixinInputFile(tst.file); got != tst.want {
+			t.Errorf("%s: got %v, want %v", tst.name, got, tst.want)
+		}
+	}
+}
+
 // locationMethods is just used for testing.
 func locationMethods() []*descriptor.MethodDescriptorProto {
 	return mixinFiles["google.cloud.location.Locations"][0].GetService()[0].GetMethod()