Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: generator options are parsed into a standalone struct #479

Merged
merged 5 commits into from
Nov 19, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 129 additions & 49 deletions internal/gengapic/gengapic.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ import (
"unicode"
"unicode/utf8"

yaml "gopkg.in/yaml.v2"

"github.com/golang/protobuf/proto"
"github.com/golang/protobuf/protoc-gen-go/descriptor"
plugin "github.com/golang/protobuf/protoc-gen-go/plugin"
Expand All @@ -37,6 +35,7 @@ import (
"github.com/googleapis/gapic-generator-go/internal/pbinfo"
"github.com/googleapis/gapic-generator-go/internal/printer"
"google.golang.org/genproto/googleapis/api/annotations"
"gopkg.in/yaml.v2"
)

const (
Expand All @@ -52,82 +51,159 @@ const (

var headerParamRegexp = regexp.MustCompile(`{([_.a-z]+)`)

func Gen(genReq *plugin.CodeGeneratorRequest) (*plugin.CodeGeneratorResponse, error) {
var pkgPath, pkgName, outDir string
var g generator
type Transport int

const (
grpc Transport = iota
rest
)

type options struct {
pkgPath string
pkgName string
outDir string
relLvl string
modulePrefix string
grpcConfPath string
serviceConfigPath string
sampleOnly bool
transports []Transport
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Java assumes that either gRPC or REST will be used at a particular time. Does this array imply that callers can pass in both transport types?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the design doc, the generators will support specifying which transports to generate code for via the transport option, which takes a +-separated list. The eventual default will be grpc+rest. The end developer can then specify which of these generated transports to use when communicating with the server.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is the implication. Python allows multiple transports to be generated for the surface with runtime choice via dependency injection. This is supported by the spec: see this heading.

}

func TransportSliceEqual(a, b []Transport) bool {
software-dov marked this conversation as resolved.
Show resolved Hide resolved
software-dov marked this conversation as resolved.
Show resolved Hide resolved
if len(a) != len(b) {
return false
}
for i, v := range a {
if v != b[i] {
return false
}
}
return true
}

func OptsEqual(a, b *options) bool {
software-dov marked this conversation as resolved.
Show resolved Hide resolved
if a == nil {
return b == nil
}

return a.pkgPath == b.pkgPath &&
a.pkgName == b.pkgName &&
a.outDir == b.outDir &&
a.relLvl == b.relLvl &&
a.modulePrefix == b.modulePrefix &&
a.grpcConfPath == b.grpcConfPath &&
a.serviceConfigPath == b.serviceConfigPath &&
a.sampleOnly == b.sampleOnly &&
TransportSliceEqual(a.transports, b.transports)
}

func ParseOptions(parameter *string) (*options, error) {
opts := options{sampleOnly: false}

if genReq.Parameter == nil {
return &g.resp, errors.E(nil, paramError)
if parameter == nil {
return nil, errors.E(nil, paramError)
}

// parse plugin params, ignoring unknown values
for _, s := range strings.Split(*genReq.Parameter, ",") {
for _, s := range strings.Split(*parameter, ",") {
// check for the boolean flag, sample-only, that disables client generation
if s == "sample-only" {
return &g.resp, nil
return &options{sampleOnly: true}, nil
}

e := strings.IndexByte(s, '=')
if e < 0 {
return &g.resp, errors.E(nil, "invalid plugin option format, must be key=value: %s", s)
return nil, errors.E(nil, "invalid plugin option format, must be key=value: %s", s)
}

key, val := s[:e], s[e+1:]
if val == "" {
return &g.resp, errors.E(nil, "invalid plugin option value, missing value in key=value: %s", s)
return nil, errors.E(nil, "invalid plugin option value, missing value in key=value: %s", s)
}

switch key {
case "go-gapic-package":
p := strings.IndexByte(s, ';')

if p < 0 {
return &g.resp, errors.E(nil, paramError)
return nil, errors.E(nil, paramError)
}

pkgPath = s[e+1 : p]
pkgName = s[p+1:]
outDir = filepath.FromSlash(pkgPath)
opts.pkgPath = s[e+1 : p]
opts.pkgName = s[p+1:]
opts.outDir = filepath.FromSlash(opts.pkgPath)
case "gapic-service-config":
f, err := os.Open(val)
if err != nil {
return &g.resp, errors.E(nil, "error opening service config: %v", err)
}

err = yaml.NewDecoder(f).Decode(&g.serviceConfig)
if err != nil {
return &g.resp, errors.E(nil, "error decoding service config: %v", err)
}
opts.serviceConfigPath = val
case "grpc-service-config":
f, err := os.Open(val)
if err != nil {
return &g.resp, errors.E(nil, "error opening gRPC service config: %v", err)
}

g.grpcConf, err = conf.New(f)
if err != nil {
return &g.resp, errors.E(nil, "error parsing gPRC service config: %v", err)
}
opts.grpcConfPath = val
case "module":
g.modulePrefix = val
opts.modulePrefix = val
case "release-level":
g.relLvl = strings.ToLower(val)
opts.relLvl = strings.ToLower(val)
case "transport":
for _, t := range strings.Split(val, "+") {
switch t {
case "grpc":
opts.transports = append(opts.transports, grpc)
case "rest":
opts.transports = append(opts.transports, rest)
default:
return nil, errors.E(nil, "invalid transport option: %s", t)
}
}
}
}

if pkgPath == "" || pkgName == "" || outDir == "" {
return &g.resp, errors.E(nil, paramError)
if opts.pkgPath == "" || opts.pkgName == "" || opts.outDir == "" {
return nil, errors.E(nil, paramError)
}

if g.modulePrefix != "" {
if !strings.HasPrefix(outDir, g.modulePrefix) {
return &g.resp, errors.E(nil, "go-gapic-package %q does not match prefix %q", outDir, g.modulePrefix)
if opts.modulePrefix != "" {
if !strings.HasPrefix(opts.outDir, opts.modulePrefix) {
return nil, errors.E(nil, "go-gapic-package %q does not match prefix %q", opts.outDir, opts.modulePrefix)
}
outDir = strings.TrimPrefix(outDir, g.modulePrefix+"/")
opts.outDir = strings.TrimPrefix(opts.outDir, opts.modulePrefix+"/")
}

return &opts, nil
}

func Gen(genReq *plugin.CodeGeneratorRequest) (*plugin.CodeGeneratorResponse, error) {
var g generator
g.init(genReq.ProtoFile)

opts, err := ParseOptions(genReq.Parameter)
if err != nil {
return &g.resp, err
}

if opts.serviceConfigPath != "" {
f, err := os.Open(opts.serviceConfigPath)
if err != nil {
return &g.resp, errors.E(nil, "error opening service config: %v", err)
}
defer f.Close()

err = yaml.NewDecoder(f).Decode(&g.serviceConfig)
if err != nil {
return &g.resp, errors.E(nil, "error decoding service config: %v", err)
}
}
if opts.grpcConfPath != "" {
f, err := os.Open(opts.grpcConfPath)
if err != nil {
return &g.resp, errors.E(nil, "error opening gRPC service config: %v", err)
}
defer f.Close()

g.grpcConf, err = conf.New(f)
if err != nil {
return &g.resp, errors.E(nil, "error parsing gPRC service config: %v", err)
}
}
g.opts = opts

var genServs []*descriptor.ServiceDescriptorProto
for _, f := range genReq.ProtoFile {
if !strContains(genReq.FileToGenerate, f.GetName()) {
Expand All @@ -149,30 +225,30 @@ func Gen(genReq *plugin.CodeGeneratorRequest) (*plugin.CodeGeneratorResponse, er
// Keep the current behavior for now, but we could revisit this later.
outFile := pbinfo.ReduceServName(s.GetName(), "")
outFile = camelToSnake(outFile)
outFile = filepath.Join(outDir, outFile)
outFile = filepath.Join(g.opts.outDir, outFile)

g.reset()
if err := g.gen(s, pkgName); err != nil {
if err := g.gen(s, g.opts.pkgName); err != nil {
software-dov marked this conversation as resolved.
Show resolved Hide resolved
return &g.resp, err
}
g.commit(outFile+"_client.go", pkgName)
g.commit(outFile+"_client.go", g.opts.pkgName)

g.reset()
if err := g.genExampleFile(s, pkgName); err != nil {
if err := g.genExampleFile(s, g.opts.pkgName); err != nil {
return &g.resp, errors.E(err, "example: %s", s.GetName())
}
g.imports[pbinfo.ImportSpec{Name: pkgName, Path: pkgPath}] = true
g.commit(outFile+"_client_example_test.go", pkgName+"_test")
g.imports[pbinfo.ImportSpec{Name: g.opts.pkgName, Path: g.opts.pkgPath}] = true
g.commit(outFile+"_client_example_test.go", g.opts.pkgName+"_test")
}

g.reset()
scopes, err := collectScopes(genServs, g.serviceConfig)
if err != nil {
return &g.resp, err
}
g.genDocFile(pkgPath, pkgName, time.Now().Year(), scopes)
g.genDocFile(g.opts.pkgPath, g.opts.pkgName, time.Now().Year(), scopes)
software-dov marked this conversation as resolved.
Show resolved Hide resolved
g.resp.File = append(g.resp.File, &plugin.CodeGeneratorResponse_File{
Name: proto.String(filepath.Join(outDir, "doc.go")),
Name: proto.String(filepath.Join(g.opts.outDir, "doc.go")),
Content: proto.String(g.pt.String()),
})

Expand Down Expand Up @@ -218,6 +294,10 @@ type generator struct {
// The Go module prefix to strip from the go-gapic-package
// used as the generated file name.
modulePrefix string

// Options for the generator determining module names, transports,
// config file paths, etc.
opts *options
}

func (g *generator) init(files []*descriptor.FileDescriptorProto) {
Expand Down
43 changes: 43 additions & 0 deletions internal/gengapic/gengapic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -499,3 +499,46 @@ func Test_isLRO(t *testing.T) {
}
}
}

func Test_transportParse(t *testing.T) {
software-dov marked this conversation as resolved.
Show resolved Hide resolved
for _, tst := range []struct {
param string
expectedOpts options
expectErr bool
}{
{
software-dov marked this conversation as resolved.
Show resolved Hide resolved
param: "transport=grpc,go-gapic-package=path;pkg",
expectedOpts: options{
transports: []Transport{grpc},
pkgPath: "path",
pkgName: "pkg",
outDir: "path",
},
expectErr: false,
},
{
param: "transport=rest+grpc,go-gapic-package=path;pkg",
expectedOpts: options{
transports: []Transport{rest, grpc},
pkgPath: "path",
pkgName: "pkg",
outDir: "path",
},
expectErr: false,
},
{
param: "transport=tcp,go-gapic-package=path;pkg",
expectedOpts: options{},
expectErr: true,
},
} {
opts, err := ParseOptions(&tst.param)
if tst.expectErr {
software-dov marked this conversation as resolved.
Show resolved Hide resolved
if err == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually very confusing: for some reason, if I lift the err == nil check into the top level expression, it causes spurious test failures for the last test case. Does anyone have any idea as to why this might be?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if tst.expectErr && (err == nil) doesn't work? Weird....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion for debugging this: have the test print ("%v", err). I worry this may hide a simple bug, so we should try to resolve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I figured it out: the test struct is storing the expected options by value, but ParseOptions is returning by pointer, so the comparison fails because sometimes the returned options is nil, and the expected options cannot be nil. Changing the expected options field to a pointer fixed this.

t.Errorf("ParseOptions(%s) expected error", tst.param)
}
} else if !OptsEqual(opts, &tst.expectedOpts) {
t.Errorf("ParseOptions(%s) = %v, want %v", tst.param, opts, tst.expectedOpts)
}
}
}