Repository: googleapis/gapic-generator-go
Pull request #1530 (2024-05-30 17:56:50)

chore(internal/gengapic): refactor checkIAMPolicyOverrides
At the moment checkIAMPolicyOverrides both checks if IAMPolicyOverrides is needed and sets the value of g.hasIAMPolicyOverrides which is unexpected.

Rename checkIAMPolicyOverrides to containsIAMPolicyOverrides, and return a boolean value to be set outside the function.


Patch:
diff --git a/internal/gengapic/gengapic.go b/internal/gengapic/gengapic.go
index 251c74a..48bfa1b 100644
--- a/internal/gengapic/gengapic.go
+++ b/internal/gengapic/gengapic.go
@@ -64,7 +64,9 @@ func Gen(genReq *pluginpb.CodeGeneratorRequest) (*pluginpb.CodeGeneratorResponse
 		return &g.resp, nil
 	}
 
-	g.checkIAMPolicyOverrides(genServs)
+	if g.containsIAMPolicyOverrides(genServs) {
+		g.hasIAMPolicyOverrides = true
+	}
 
 	if g.serviceConfig != nil {
 		g.apiName = g.serviceConfig.GetTitle()
diff --git a/internal/gengapic/mixins.go b/internal/gengapic/mixins.go
index ba47e37..4a835e9 100644
--- a/internal/gengapic/mixins.go
+++ b/internal/gengapic/mixins.go
@@ -206,24 +206,24 @@ func (g *generator) hasLocationMixin() bool {
 	return len(g.mixins["google.cloud.location.Locations"]) > 0 && len(g.serviceConfig.GetApis()) > 1
 }
 
-// checkIAMPolicyOverrides determines if any of the given services define an
+// containsIAMPolicyOverrides determines if any of the given services define an
 // IAMPolicy RPC and sets the hasIAMpolicyOverrides generator flag if so. If set
 // to true, the IAMPolicy mixin will not be generated on any service client. This
 // is for backwards compatibility with existing IAMPolicy redefinitions.
-func (g *generator) checkIAMPolicyOverrides(servs []*descriptorpb.ServiceDescriptorProto) {
+func (g *generator) containsIAMPolicyOverrides(servs []*descriptorpb.ServiceDescriptorProto) bool {
 	iam, hasMixin := g.mixins["google.iam.v1.IAMPolicy"]
 	if !hasMixin {
-		return
+		return false
 	}
 
 	for _, s := range servs {
 		for _, iamMethod := range iam {
 			if hasMethod(s, iamMethod.GetName()) {
-				g.hasIAMPolicyOverrides = true
-				return
+				return true
 			}
 		}
 	}
+	return false
 }
 
 // includeMixinInputFile determines if the given proto file name matches


Test patch:
diff --git a/internal/gengapic/mixins_test.go b/internal/gengapic/mixins_test.go
index 064a07e..d49ca53 100644
--- a/internal/gengapic/mixins_test.go
+++ b/internal/gengapic/mixins_test.go
@@ -164,7 +164,7 @@ func TestHasIAMPolicyMixin(t *testing.T) {
 	}
 }
 
-func TestCheckIAMPolicyOverrides(t *testing.T) {
+func TestContainsIAMPolicyOverrides(t *testing.T) {
 	g := &generator{
 		mixins: make(mixins),
 	}
@@ -181,18 +181,14 @@ func TestCheckIAMPolicyOverrides(t *testing.T) {
 		},
 	}
 	servs := []*descriptorpb.ServiceDescriptorProto{serv, other}
-	var want bool
-	g.checkIAMPolicyOverrides(servs)
-	if got := g.hasIAMPolicyOverrides; !cmp.Equal(got, want) {
-		t.Errorf("TestCheckIAMPolicyOverrides wanted %v but got %v", want, got)
+	if g.containsIAMPolicyOverrides(servs) {
+		t.Errorf("TestContainsIAMPolicyOverrides = true; want = false")
 	}
 
-	want = true
 	g.mixins["google.iam.v1.IAMPolicy"] = iamPolicyMethods()
 	serv.Method = append(serv.Method, &descriptorpb.MethodDescriptorProto{Name: proto.String("GetIamPolicy")})
-	g.checkIAMPolicyOverrides(servs)
-	if got := g.hasIAMPolicyOverrides; !cmp.Equal(got, want) {
-		t.Errorf("TestCheckIAMPolicyOverrides wanted %v but got %v", want, got)
+	if !g.containsIAMPolicyOverrides(servs) {
+		t.Errorf("TestContainsIAMPolicyOverrides = false; want = true")
 	}
 }