Fix -module option for protoc-gen-grpc-gateway (#1754)

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

Author: olivierlemasle

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",
     ],
 )

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) {

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
-		}
+		})
 	}
 }

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)