Skip to content

Commit

Permalink
Fix -module option for protoc-gen-grpc-gateway
Browse files Browse the repository at this point in the history 
When option module=... is used, protoc-gen-grpc-gateway removes the module
prefix a first time, then uses protogen, which tries to remove the prefix
a second time.

Fixes grpc-ecosystem#1753
  • Loading branch information
@olivierlemasle
olivierlemasle committed Oct 14, 2020
1 parent 1aab03c commit 7f5112c
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 80 deletions.
2 changes: 2 additions & 0 deletions protoc-gen-grpc-gateway/internal/gengateway/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ go_test(
deps = [
"//internal/descriptor:go_default_library",
"//internal/httprule:go_default_library",
"@org_golang_google_protobuf//compiler/protogen:go_default_library",
"@org_golang_google_protobuf//proto:go_default_library",
"@org_golang_google_protobuf//types/descriptorpb:go_default_library",
"@org_golang_google_protobuf//types/pluginpb:go_default_library",
],
)
32 changes: 7 additions & 25 deletions protoc-gen-grpc-gateway/internal/gengateway/generator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,12 @@ type generator struct {
useRequestContext bool
registerFuncSuffix string
pathType pathType
modulePath string
allowPatchFeature bool
standalone bool
}

// New returns a new generator which generates grpc gateway files.
func New(reg *descriptor.Registry, useRequestContext bool, registerFuncSuffix, pathTypeString, modulePathString string,
func New(reg *descriptor.Registry, useRequestContext bool, registerFuncSuffix, pathTypeString string,
allowPatchFeature, standalone bool) gen.Generator {
var imports []descriptor.GoPackage
for _, pkgpath := range []string{
Expand Down Expand Up @@ -87,7 +86,6 @@ func New(reg *descriptor.Registry, useRequestContext bool, registerFuncSuffix, p
useRequestContext: useRequestContext,
registerFuncSuffix: registerFuncSuffix,
pathType: pathType,
modulePath: modulePathString,
allowPatchFeature: allowPatchFeature,
standalone: standalone,
}
Expand All @@ -112,11 +110,7 @@ func (g *generator) Generate(targets []*descriptor.File) ([]*descriptor.Response
return nil, err
}

name, err := g.getFilePath(file)
if err != nil {
glog.Errorf("%v: %s", err, code)
return nil, err
}
name := g.getFilePath(file)
ext := filepath.Ext(name)
base := strings.TrimSuffix(name, ext)
filename := fmt.Sprintf("%s.pb.gw.go", base)
Expand All @@ -131,25 +125,13 @@ func (g *generator) Generate(targets []*descriptor.File) ([]*descriptor.Response
return files, nil
}

func (g *generator) getFilePath(file *descriptor.File) (string, error) {
func (g *generator) getFilePath(file *descriptor.File) string {
name := file.GetName()
switch {
case g.modulePath != "" && g.pathType != pathTypeImport:
return "", errors.New("cannot use module= with paths=")

case g.modulePath != "":
trimPath, pkgPath := g.modulePath+"/", file.GoPkg.Path+"/"
if !strings.HasPrefix(pkgPath, trimPath) {
return "", fmt.Errorf("%v: file go path does not match module prefix: %v", file.GoPkg.Path, trimPath)
}
return filepath.Join(strings.TrimPrefix(pkgPath, trimPath), filepath.Base(name)), nil

case g.pathType == pathTypeImport && file.GoPkg.Path != "":
return fmt.Sprintf("%s/%s", file.GoPkg.Path, filepath.Base(name)), nil

default:
return name, nil
if g.pathType == pathTypeImport && file.GoPkg.Path != "" {
return fmt.Sprintf("%s/%s", file.GoPkg.Path, filepath.Base(name))
}

return name
}

func (g *generator) generate(file *descriptor.File) (string, error) {
Expand Down
151 changes: 98 additions & 53 deletions protoc-gen-grpc-gateway/internal/gengateway/generator_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
package gengateway

import (
"errors"
"fmt"
"path/filepath"
"strings"
"testing"

"github.com/grpc-ecosystem/grpc-gateway/v2/internal/descriptor"
"google.golang.org/protobuf/compiler/protogen"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/types/descriptorpb"
"google.golang.org/protobuf/types/pluginpb"
)

func newExampleFileDescriptor() *descriptor.File {
Expand Down Expand Up @@ -101,11 +103,15 @@ func TestGenerateServiceWithoutBindings(t *testing.T) {

func TestGenerateOutputPath(t *testing.T) {
cases := []struct {
file *descriptor.File
pathType pathType
modulePath string
expected string
expectedError error
file *descriptor.File
pathType pathType
modulePath string

// the path that function Generate should output
expectedPath string
// the path after protogen has remove the module prefix
expectedFinalPath string
expectedError bool
}{
{
file: newExampleFileDescriptorWithGoPkg(
Expand All @@ -114,7 +120,8 @@ func TestGenerateOutputPath(t *testing.T) {
Name: "example_pb",
},
),
expected: "example.com/path/to/example",
expectedPath: "example.com/path/to/example",
expectedFinalPath: "example.com/path/to/example",
},
{
file: newExampleFileDescriptorWithGoPkg(
Expand All @@ -123,7 +130,8 @@ func TestGenerateOutputPath(t *testing.T) {
Name: "example_pb",
},
),
expected: "example",
expectedPath: "example",
expectedFinalPath: "example",
},
{
file: newExampleFileDescriptorWithGoPkg(
Expand All @@ -133,7 +141,9 @@ func TestGenerateOutputPath(t *testing.T) {
},
),
pathType: pathTypeSourceRelative,
expected: ".",

expectedPath: ".",
expectedFinalPath: ".",
},
{
file: newExampleFileDescriptorWithGoPkg(
Expand All @@ -143,7 +153,9 @@ func TestGenerateOutputPath(t *testing.T) {
},
),
pathType: pathTypeSourceRelative,
expected: ".",

expectedPath: ".",
expectedFinalPath: ".",
},
{
file: newExampleFileDescriptorWithGoPkg(
Expand All @@ -153,7 +165,9 @@ func TestGenerateOutputPath(t *testing.T) {
},
),
modulePath: "example.com/path/root",
expected: ".",

expectedPath: "example.com/path/root",
expectedFinalPath: ".",
},
{
file: newExampleFileDescriptorWithGoPkg(
Expand All @@ -163,7 +177,9 @@ func TestGenerateOutputPath(t *testing.T) {
},
),
modulePath: "example.com/path/to",
expected: "example",

expectedPath: "example.com/path/to/example",
expectedFinalPath: "example",
},
{
file: newExampleFileDescriptorWithGoPkg(
Expand All @@ -173,7 +189,9 @@ func TestGenerateOutputPath(t *testing.T) {
},
),
modulePath: "example.com/path/to",
expected: "example/with/many/nested/paths",

expectedPath: "example.com/path/to/example/with/many/nested/paths",
expectedFinalPath: "example/with/many/nested/paths",
},

// Error cases
Expand All @@ -186,7 +204,7 @@ func TestGenerateOutputPath(t *testing.T) {
),
modulePath: "example.com/path/root",
pathType: pathTypeSourceRelative, // Not allowed
expectedError: errors.New("cannot use module= with paths="),
expectedError: true,
},
{
file: newExampleFileDescriptorWithGoPkg(
Expand All @@ -195,50 +213,77 @@ func TestGenerateOutputPath(t *testing.T) {
Name: "example_pb",
},
),
modulePath: "example.com/path/root",
expectedError: errors.New("example.com/path/rootextra: file go path does not match module prefix: example.com/path/root/"),
modulePath: "example.com/path/root", // Not a prefix of path
expectedError: true,
},
}

for _, c := range cases {
g := &generator{
pathType: c.pathType,
modulePath: c.modulePath,
}
for i, c := range cases {
t.Run(fmt.Sprintf("case %d", i), func(t *testing.T) {
g := &generator{
pathType: c.pathType,
}

file := c.file
gots, err := g.Generate([]*descriptor.File{crossLinkFixture(file)})

// We don't expect an error during generation
if err != nil {
t.Errorf("Generate(%#v) failed with %v; wants success", file, err)
return
}

if len(gots) != 1 {
t.Errorf("Generate(%#v) failed; expects one result, got %d", file, len(gots))
return
}

file := c.file
gots, err := g.Generate([]*descriptor.File{crossLinkFixture(file)})
got := gots[0]
if got.Name == nil {
t.Errorf("Generate(%#v) failed; expects non-nil Name(%v)", file, got.Name)
return
}

gotPath := filepath.Dir(*got.Name)
if gotPath != c.expectedPath && !c.expectedError {
t.Errorf("Generate(%#v) failed; got path: %s expected path: %s", file, gotPath, c.expectedPath)
return
}

// We now use codegen to verify how it optionally removes the module prefix

reqParam := ""
if c.modulePath != "" {
reqParam = "module=" + c.modulePath
}
req := &pluginpb.CodeGeneratorRequest{Parameter: &reqParam}
plugin, err := protogen.Options{}.New(req)
if err != nil {
t.Errorf("Unexpected error during plugin creation: %v", err)
}

genFile := plugin.NewGeneratedFile(got.GetName(), protogen.GoImportPath(got.GoPkg.Path))
_, _ = genFile.Write([]byte(got.GetContent()))
resp := plugin.Response()

if !c.expectedError && resp.GetError() != "" {
t.Errorf("Unexpected error in protogen response: %v", resp.GetError())
return
}

if c.expectedError {
if resp.GetError() == "" {
t.Error("Expected an non-null error in protogen response")
}
return
}

// If we expect an error response, check it matches what we want
if c.expectedError != nil {
if err == nil || err.Error() != c.expectedError.Error() {
t.Errorf("Generate(%#v) failed with %v; wants error of: %v", file, err, c.expectedError)
finalName := resp.File[0].GetName()
gotPath = filepath.Dir(finalName)
if gotPath != c.expectedFinalPath {
t.Errorf("After protogen, got path: %s expected path: %s", gotPath, c.expectedFinalPath)
return
}
return
}

// Handle case where we don't expect an error
if err != nil {
t.Errorf("Generate(%#v) failed with %v; wants success", file, err)
return
}

if len(gots) != 1 {
t.Errorf("Generate(%#v) failed; expects on result got %d", file, len(gots))
return
}

got := gots[0]
if got.Name == nil {
t.Errorf("Generate(%#v) failed; expects non-nil Name(%v)", file, got.Name)
return
}

gotPath := filepath.Dir(*got.Name)
expectedPath := c.expected
if gotPath != expectedPath {
t.Errorf("Generate(%#v) failed; got path: %s expected path: %s", file, gotPath, expectedPath)
return
}
})
}
}
4 changes: 2 additions & 2 deletions protoc-gen-grpc-gateway/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ var (
allowDeleteBody = flag.Bool("allow_delete_body", false, "unless set, HTTP DELETE methods may not have a body")
grpcAPIConfiguration = flag.String("grpc_api_configuration", "", "path to gRPC API Configuration in YAML format")
pathType = flag.String("paths", "", "specifies how the paths of generated files are structured")
modulePath = flag.String("module", "", "specifies a module prefix that will be stripped from the go package to determine the output directory")
_ = flag.String("module", "", "specifies a module prefix that will be stripped from the go package to determine the output directory")
allowRepeatedFieldsInBody = flag.Bool("allow_repeated_fields_in_body", false, "allows to use repeated field in `body` and `response_body` field of `google.api.http` annotation option")
repeatedPathParamSeparator = flag.String("repeated_path_param_separator", "csv", "configures how repeated fields should be split. Allowed values are `csv`, `pipes`, `ssv` and `tsv`.")
allowPatchFeature = flag.Bool("allow_patch_feature", true, "determines whether to use PATCH feature involving update masks (using google.protobuf.FieldMask).")
Expand Down Expand Up @@ -90,7 +90,7 @@ func main() {
targets = append(targets, f)
}

g := gengateway.New(reg, *useRequestContext, *registerFuncSuffix, *pathType, *modulePath, *allowPatchFeature, *standalone)
g := gengateway.New(reg, *useRequestContext, *registerFuncSuffix, *pathType, *allowPatchFeature, *standalone)
files, err := g.Generate(targets)
for _, f := range files {
glog.V(1).Infof("NewGeneratedFile %q in %s", f.GetName(), f.GoPkg)
Expand Down

0 comments on commit 7f5112c

Please sign in to comment.