Repository: googleapis/gapic-generator-go
Pull request #1520 (2024-05-15 14:06:42)

chore(internal/gengapic): refactor g.init to newGenerator
At the moment, the `g.init` modifies a type `generator`. In many cases, `g` is empty and populated by `g.init`. It is generally more idiomatic to use a `new<thing>` function to construct a new instance of a type, so g.init is replaced with a `newGenerator` function to follow that pattern.

Additionally, errors were not captured in tests for some calls to `g.init`, which are now handled.


Patch:
diff --git a/internal/gengapic/generator.go b/internal/gengapic/generator.go
index e5fbfe7..b8554f5 100644
--- a/internal/gengapic/generator.go
+++ b/internal/gengapic/generator.go
@@ -90,27 +90,28 @@ type generator struct {
 	customOpServices map[*descriptorpb.ServiceDescriptorProto]*descriptorpb.ServiceDescriptorProto
 }
 
-func (g *generator) init(req *pluginpb.CodeGeneratorRequest) error {
-	g.metadata = &metadata.GapicMetadata{
-		Schema:   "1.0",
-		Language: "go",
-		Comment:  "This file maps proto services/RPCs to the corresponding library clients/methods.",
-		Services: make(map[string]*metadata.GapicMetadata_ServiceForTransport),
-	}
-
-	g.mixins = make(mixins)
-	g.comments = map[protoiface.MessageV1]string{}
-	g.imports = map[pbinfo.ImportSpec]bool{}
-	g.customOpServices = map[*descriptorpb.ServiceDescriptorProto]*descriptorpb.ServiceDescriptorProto{}
-	g.aux = &auxTypes{
-		iters:           map[string]*iterType{},
-		methodToWrapper: map[*descriptorpb.MethodDescriptorProto]operationWrapper{},
-		opWrappers:      map[string]operationWrapper{},
+func newGenerator(req *pluginpb.CodeGeneratorRequest) (*generator, error) {
+	g := &generator{
+		metadata: &metadata.GapicMetadata{
+			Schema:   "1.0",
+			Language: "go",
+			Comment:  "This file maps proto services/RPCs to the corresponding library clients/methods.",
+			Services: make(map[string]*metadata.GapicMetadata_ServiceForTransport),
+		},
+		mixins:           make(mixins),
+		comments:         map[protoiface.MessageV1]string{},
+		imports:          map[pbinfo.ImportSpec]bool{},
+		customOpServices: map[*descriptorpb.ServiceDescriptorProto]*descriptorpb.ServiceDescriptorProto{},
+		aux: &auxTypes{
+			iters:           map[string]*iterType{},
+			methodToWrapper: map[*descriptorpb.MethodDescriptorProto]operationWrapper{},
+			opWrappers:      map[string]operationWrapper{},
+		},
 	}
 
 	opts, err := parseOptions(req.Parameter)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	files := req.GetProtoFile()
 	files = append(files, wellKnownTypeFiles...)
@@ -118,24 +119,24 @@ func (g *generator) init(req *pluginpb.CodeGeneratorRequest) error {
 	if opts.serviceConfigPath != "" {
 		y, err := os.ReadFile(opts.serviceConfigPath)
 		if err != nil {
-			return fmt.Errorf("error reading service config: %v", err)
+			return nil, fmt.Errorf("error reading service config: %v", err)
 		}
 
 		j, err := yaml.YAMLToJSON(y)
 		if err != nil {
-			return fmt.Errorf("error converting YAML to JSON: %v", err)
+			return nil, fmt.Errorf("error converting YAML to JSON: %v", err)
 		}
 
 		cfg := &serviceconfig.Service{}
 		if err := (protojson.UnmarshalOptions{DiscardUnknown: true}).Unmarshal(j, cfg); err != nil {
-			return fmt.Errorf("error unmarshaling service config: %v", err)
+			return nil, fmt.Errorf("error unmarshaling service config: %v", err)
 		}
 		g.serviceConfig = cfg
 
 		// An API Service Config will always have a `name` so if it is not populated,
 		// it's an invalid config.
 		if g.serviceConfig.GetName() == "" {
-			return fmt.Errorf("invalid API service config file %q", opts.serviceConfigPath)
+			return nil, fmt.Errorf("invalid API service config file %q", opts.serviceConfigPath)
 		}
 
 		g.collectMixins()
@@ -144,13 +145,13 @@ func (g *generator) init(req *pluginpb.CodeGeneratorRequest) error {
 	if opts.grpcConfPath != "" {
 		f, err := os.Open(opts.grpcConfPath)
 		if err != nil {
-			return fmt.Errorf("error opening gRPC service config: %v", err)
+			return nil, fmt.Errorf("error opening gRPC service config: %v", err)
 		}
 		defer f.Close()
 
 		g.grpcConf, err = conf.New(f)
 		if err != nil {
-			return fmt.Errorf("error parsing gPRC service config: %v", err)
+			return nil, fmt.Errorf("error parsing gPRC service config: %v", err)
 		}
 	}
 	g.opts = opts
@@ -184,7 +185,7 @@ func (g *generator) init(req *pluginpb.CodeGeneratorRequest) error {
 		}
 	}
 
-	return nil
+	return g, nil
 }
 
 // printf formatted-prints to sb, using the print syntax from fmt package.
diff --git a/internal/gengapic/gengapic.go b/internal/gengapic/gengapic.go
index ec1bee5..251c74a 100644
--- a/internal/gengapic/gengapic.go
+++ b/internal/gengapic/gengapic.go
@@ -53,8 +53,8 @@ var headerParamRegexp = regexp.MustCompile(`{([_.a-z0-9]+)`)
 
 // Gen is the entry point for GAPIC generation via the protoc pluginpb.
 func Gen(genReq *pluginpb.CodeGeneratorRequest) (*pluginpb.CodeGeneratorResponse, error) {
-	var g generator
-	if err := g.init(genReq); err != nil {
+	g, err := newGenerator(genReq)
+	if err != nil {
 		return &g.resp, err
 	}
 
@@ -127,8 +127,7 @@ func Gen(genReq *pluginpb.CodeGeneratorRequest) (*pluginpb.CodeGeneratorResponse
 			g.opts.transports = transports
 		}
 	}
-	err := g.genAndCommitSnippetMetadata(protoPkg)
-	if err != nil {
+	if err := g.genAndCommitSnippetMetadata(protoPkg); err != nil {
 		return &g.resp, err
 	}
 	g.reset()


Test patch:
diff --git a/internal/gengapic/client_init_test.go b/internal/gengapic/client_init_test.go
index a93155f..74209f2 100644
--- a/internal/gengapic/client_init_test.go
+++ b/internal/gengapic/client_init_test.go
@@ -313,9 +313,6 @@ func TestServiceDoc(t *testing.T) {
 }
 
 func TestClientInit(t *testing.T) {
-	var g generator
-	g.apiName = "Awesome Foo API"
-	g.imports = map[pbinfo.ImportSpec]bool{}
 
 	cop := &descriptorpb.DescriptorProto{
 		Name: proto.String("Operation"),
@@ -550,7 +547,11 @@ func TestClientInit(t *testing.T) {
 				Parameter: tst.parameter,
 				ProtoFile: fds,
 			}
-			g.init(&request)
+			g, err := newGenerator(&request)
+			if err != nil {
+				t.Fatal(err)
+			}
+			g.apiName = "Awesome Foo API"
 			g.comments = map[protoiface.MessageV1]string{
 				tst.serv:                "Foo service does stuff.",
 				tst.serv.GetMethod()[0]: "Does some stuff.",
diff --git a/internal/gengapic/gengapic_test.go b/internal/gengapic/gengapic_test.go
index 8ad42df..421a4ec 100644
--- a/internal/gengapic/gengapic_test.go
+++ b/internal/gengapic/gengapic_test.go
@@ -1113,8 +1113,7 @@ func TestGRPCStubCall(t *testing.T) {
 			},
 		},
 	}
-	var g generator
-	err := g.init(&pluginpb.CodeGeneratorRequest{
+	g, err := newGenerator(&pluginpb.CodeGeneratorRequest{
 		ProtoFile: []*descriptorpb.FileDescriptorProto{foo},
 		Parameter: proto.String("go-gapic-package=cloud.google.com/go/foo/apiv1;foo"),
 	})
@@ -1137,6 +1136,9 @@ func TestGRPCStubCall(t *testing.T) {
 			in:   getBar,
 		},
 	} {
+		if err != nil {
+			t.Fatal(err)
+		}
 		got := g.grpcStubCall(tst.in)
 		if diff := cmp.Diff(got, tst.want); diff != "" {
 			t.Errorf("TestGRPCStubCall(%s) got(-),want(+):\n%s", tst.name, diff)
diff --git a/internal/gengapic/genrest_test.go b/internal/gengapic/genrest_test.go
index c88c10e..0875694 100644
--- a/internal/gengapic/genrest_test.go
+++ b/internal/gengapic/genrest_test.go
@@ -37,7 +37,7 @@ import (
 
 // Note: the fields parameter contains the names of _all_ the request message's fields,
 // not just those that are path or query params.
-func setupMethod(g *generator, url, body string, fields []string) (*descriptorpb.MethodDescriptorProto, error) {
+func setupMethod(url, body string, fields []string) (*pluginpb.CodeGeneratorRequest, *descriptorpb.MethodDescriptorProto, error) {
 	msg := &descriptorpb.DescriptorProto{
 		Name: proto.String("IdentifyRequest"),
 	}
@@ -79,21 +79,14 @@ func setupMethod(g *generator, url, body string, fields []string) (*descriptorpb
 			MessageType: []*descriptorpb.DescriptorProto{msg},
 		},
 	}
-	req := pluginpb.CodeGeneratorRequest{
+	req := &pluginpb.CodeGeneratorRequest{
 		Parameter: proto.String("go-gapic-package=path;mypackage,transport=rest"),
 		ProtoFile: fds,
 	}
-	g.init(&req)
-
-	return mthd, nil
+	return req, mthd, nil
 }
 
 func TestPathParams(t *testing.T) {
-	var g generator
-	g.apiName = "Awesome Mollusc API"
-	g.imports = map[pbinfo.ImportSpec]bool{}
-	g.opts = &options{transports: []transport{rest}}
-
 	for _, tst := range []struct {
 		name     string
 		body     string
@@ -160,7 +153,14 @@ func TestPathParams(t *testing.T) {
 			},
 		},
 	} {
-		mthd, err := setupMethod(&g, tst.url, tst.body, tst.fields)
+		req, mthd, err := setupMethod(tst.url, tst.body, tst.fields)
+		if err != nil {
+			t.Fatal(err)
+		}
+		g, err := newGenerator(req)
+		if err != nil {
+			t.Fatal(err)
+		}
 		if err != nil {
 			t.Errorf("test %s setup got error: %s", tst.name, err.Error())
 		}
@@ -173,10 +173,6 @@ func TestPathParams(t *testing.T) {
 }
 
 func TestQueryParams(t *testing.T) {
-	var g generator
-	g.apiName = "Awesome Mollusc API"
-	g.imports = map[pbinfo.ImportSpec]bool{}
-	g.opts = &options{transports: []transport{rest}}
 	for _, tst := range []struct {
 		name     string
 		body     string
@@ -223,11 +219,14 @@ func TestQueryParams(t *testing.T) {
 			},
 		},
 	} {
-		mthd, err := setupMethod(&g, tst.url, tst.body, tst.fields)
+		req, mthd, err := setupMethod(tst.url, tst.body, tst.fields)
 		if err != nil {
 			t.Errorf("test %s setup got error: %s", tst.name, err.Error())
 		}
-
+		g, err := newGenerator(req)
+		if err != nil {
+			t.Fatal(err)
+		}
 		actual := g.queryParams(mthd)
 		if diff := cmp.Diff(actual, tst.expected, cmp.Comparer(proto.Equal)); diff != "" {
 			t.Errorf("test %s, got(-),want(+):\n%s", tst.name, diff)
@@ -236,11 +235,6 @@ func TestQueryParams(t *testing.T) {
 }
 
 func TestLeafFields(t *testing.T) {
-	var g generator
-	g.apiName = "Awesome Mollusc API"
-	g.imports = map[pbinfo.ImportSpec]bool{}
-	g.opts = &options{transports: []transport{rest}}
-
 	basicMsg := &descriptorpb.DescriptorProto{
 		Name: proto.String("Clam"),
 		Field: []*descriptorpb.FieldDescriptorProto{
@@ -367,7 +361,10 @@ func TestLeafFields(t *testing.T) {
 		Parameter: proto.String("go-gapic-package=path;mypackage,transport=rest"),
 		ProtoFile: []*descriptorpb.FileDescriptorProto{file},
 	}
-	g.init(&req)
+	g, err := newGenerator(&req)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	for _, tst := range []struct {
 		name           string
diff --git a/internal/gengapic/paging_test.go b/internal/gengapic/paging_test.go
index 8f56b8e..32a2112 100644
--- a/internal/gengapic/paging_test.go
+++ b/internal/gengapic/paging_test.go
@@ -118,12 +118,6 @@ func TestIterTypeOf(t *testing.T) {
 }
 
 func TestPagingField(t *testing.T) {
-	g := generator{
-		apiName: "Awesome API",
-		imports: map[pbinfo.ImportSpec]bool{},
-		opts:    &options{transports: []transport{rest}},
-	}
-
 	// The predicate is simple:
 	// * No streaming in the method
 	// * Request has a int32 page_size field XOR a int32 max_results field
@@ -412,7 +406,11 @@ func TestPagingField(t *testing.T) {
 		Parameter: proto.String("go-gapic-package=path;mypackage,transport=rest"),
 		ProtoFile: []*descriptorpb.FileDescriptorProto{file},
 	}
-	g.init(&req)
+	g, err := newGenerator(&req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	g.opts = &options{transports: []transport{rest}}
 
 	for _, tst := range []struct {
 		mthd      *descriptorpb.MethodDescriptorProto
diff --git a/internal/testing/sample/sample.go b/internal/testing/sample/sample.go
index 5732a7d..c74f550 100644
--- a/internal/testing/sample/sample.go
+++ b/internal/testing/sample/sample.go
@@ -102,6 +102,34 @@ const (
 	// https://github.com/googleapis/googleapis/blob/f7df662a24c56ecaab79cb7d808fed4d2bb4981d/google/cloud/secretmanager/v1/resources.proto#L40
 	Resource = "Secret"
 
+	// ListMethod is the name of the method for listing resources.
+	//
+	// Example:
+	// https://pkg.go.dev/cloud.google.com/go/secretmanager/apiv1/secretmanagerpb#ListSecrets
+	// https://github.com/googleapis/googleapis/blob/f7df662a24c56ecaab79cb7d808fed4d2bb4981d/google/cloud/secretmanager/v1/service.proto#L52
+	ListMethod = "ListSecrets"
+
+	// ListRequest is the name of the request for listing resources.
+	//
+	// A ListRequest often contains `page_size` and `page_token` fields (see
+	// https://aip.dev/158).
+	//
+	// Example:
+	// https://pkg.go.dev/cloud.google.com/go/secretmanager/apiv1/secretmanagerpb#ListSecretsRequest
+	// https://github.com/googleapis/googleapis/blob/f7df662a24c56ecaab79cb7d808fed4d2bb4981d/google/cloud/secretmanager/v1/service.proto#L274
+	ListRequest = "ListSecretsRequest"
+
+	// ListResponse is the name of the resource returned by a List request.
+	//
+	// A ListResponse message often contains the Resource, a `next_page_token`
+	// and sometimes a `total_size` field (https://aip.dev/158).
+	// type and pattern (see https://aip.dev/4231#resource-messages).
+	//
+	// Example:
+	// https://pkg.go.dev/cloud.google.com/go/secretmanager/apiv1/secretmanagerpb#ListSecretsResponse
+	// https://github.com/googleapis/googleapis/blob/f7df662a24c56ecaab79cb7d808fed4d2bb4981d/google/cloud/secretmanager/v1/service.proto#L304
+	ListResponse = "ListSecretsResponse"
+
 	// CreateMethodWithSettings is a fake method for the purpose of testing
 	// the method_settings functionality in the publishing YAML.
 	//
@@ -192,6 +220,11 @@ func Service() *descriptorpb.ServiceDescriptorProto {
 				InputType:  proto.String(DescriptorInfoTypeName(GetRequest)),
 				OutputType: proto.String(DescriptorInfoTypeName(Resource)),
 			},
+			{
+				Name:       proto.String(ListMethod),
+				InputType:  proto.String(DescriptorInfoTypeName(ListRequest)),
+				OutputType: proto.String(DescriptorInfoTypeName(ListResponse)),
+			},
 		},
 	}
 }