From 7f5112caadf1b632bcec7c65a3fd31a0af0352ea Mon Sep 17 00:00:00 2001 From: Olivier Lemasle Date: Wed, 14 Oct 2020 22:38:32 +0200 Subject: [PATCH] Fix -module option for protoc-gen-grpc-gateway 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 #1753 --- .../internal/gengateway/BUILD.bazel | 2 + .../internal/gengateway/generator.go | 32 +--- .../internal/gengateway/generator_test.go | 151 ++++++++++++------ protoc-gen-grpc-gateway/main.go | 4 +- 4 files changed, 109 insertions(+), 80 deletions(-) diff --git a/protoc-gen-grpc-gateway/internal/gengateway/BUILD.bazel b/protoc-gen-grpc-gateway/internal/gengateway/BUILD.bazel index 4c9445ab4ce..09ef6b7be90 100644 --- a/protoc-gen-grpc-gateway/internal/gengateway/BUILD.bazel +++ b/protoc-gen-grpc-gateway/internal/gengateway/BUILD.bazel @@ -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", ], ) diff --git a/protoc-gen-grpc-gateway/internal/gengateway/generator.go b/protoc-gen-grpc-gateway/internal/gengateway/generator.go index c6064b0e637..707c8e16942 100644 --- a/protoc-gen-grpc-gateway/internal/gengateway/generator.go +++ b/protoc-gen-grpc-gateway/internal/gengateway/generator.go @@ -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{ @@ -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, } @@ -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) @@ -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) { diff --git a/protoc-gen-grpc-gateway/internal/gengateway/generator_test.go b/protoc-gen-grpc-gateway/internal/gengateway/generator_test.go index 0ea59b8a7eb..8255f9fbd88 100644 --- a/protoc-gen-grpc-gateway/internal/gengateway/generator_test.go +++ b/protoc-gen-grpc-gateway/internal/gengateway/generator_test.go @@ -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 { @@ -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( @@ -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( @@ -123,7 +130,8 @@ func TestGenerateOutputPath(t *testing.T) { Name: "example_pb", }, ), - expected: "example", + expectedPath: "example", + expectedFinalPath: "example", }, { file: newExampleFileDescriptorWithGoPkg( @@ -133,7 +141,9 @@ func TestGenerateOutputPath(t *testing.T) { }, ), pathType: pathTypeSourceRelative, - expected: ".", + + expectedPath: ".", + expectedFinalPath: ".", }, { file: newExampleFileDescriptorWithGoPkg( @@ -143,7 +153,9 @@ func TestGenerateOutputPath(t *testing.T) { }, ), pathType: pathTypeSourceRelative, - expected: ".", + + expectedPath: ".", + expectedFinalPath: ".", }, { file: newExampleFileDescriptorWithGoPkg( @@ -153,7 +165,9 @@ func TestGenerateOutputPath(t *testing.T) { }, ), modulePath: "example.com/path/root", - expected: ".", + + expectedPath: "example.com/path/root", + expectedFinalPath: ".", }, { file: newExampleFileDescriptorWithGoPkg( @@ -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( @@ -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 @@ -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( @@ -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 - } + }) } } diff --git a/protoc-gen-grpc-gateway/main.go b/protoc-gen-grpc-gateway/main.go index 8fba8b38df2..e7651784730 100644 --- a/protoc-gen-grpc-gateway/main.go +++ b/protoc-gen-grpc-gateway/main.go @@ -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).") @@ -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)