Investigate import issue with grpc-ecosystem/gateway plugin #1200
Assignees
Comments
|
Alright, I've prepared a new patch that does 3 things:
Beforeservice1
└── v1
├── service
│ └── service1v1gateway
│ └── service.pb.gw.go
├── service.pb.go
└── service1v1grpc
└── service_grpc.pb.goAfterPatch changes--- A/0001-patch-v2.19.1-a.patch
+++ B/0001-patch-v2.19.1-b.patch
@@ -1,19 +1,82 @@
+From 0f9e702df8d8b0f4147248cc7c8b4088d60d66b5 Mon Sep 17 00:00:00 2001
-From 27a62c6f989e1401296d5c422c9ef14c8431e778 Mon Sep 17 00:00:00 2001
From: Mike Fridman <mfridman@buf.build>
+Date: Tue, 7 May 2024 16:27:33 -0400
+Subject: [PATCH] patch v2.19.1-b
-Date: Tue, 7 May 2024 16:32:35 -0400
-Subject: [PATCH] patch v2.19.1-a
---
+ internal/descriptor/buf_build.go | 55 ++++++++++++++
+ internal/descriptor/registry.go | 10 +++
+ internal/descriptor/services.go | 1 +
- internal/descriptor/registry.go | 31 +++++++-
- internal/descriptor/services.go | 31 ++++++++
internal/descriptor/types.go | 19 ++++-
+ .../internal/gengateway/generator.go | 38 ++++++++--
+ .../internal/gengateway/generator_test.go | 72 +++++++++++++++++++
+ protoc-gen-grpc-gateway/main.go | 18 +++--
+ 7 files changed, 202 insertions(+), 11 deletions(-)
+ create mode 100644 internal/descriptor/buf_build.go
- .../internal/gengateway/generator.go | 35 ++++++++-
- .../internal/gengateway/generator_test.go | 74 +++++++++++++++++++
- protoc-gen-grpc-gateway/main.go | 18 ++++-
- 6 files changed, 196 insertions(+), 12 deletions(-)
+diff --git internal/descriptor/buf_build.go internal/descriptor/buf_build.go
+new file mode 100644
+index 00000000..6348916a
+--- /dev/null
++++ internal/descriptor/buf_build.go
+@@ -0,0 +1,55 @@
++package descriptor
++
++import (
++ "path/filepath"
++ "strings"
++)
++
++// SetSeparatePackage sets separatePackage
++func (r *Registry) SetSeparatePackage(use bool) {
++ r.separatePackage = use
++}
++
++// IncludeAdditionalImports adds additionalImports to the registry on a per-package basis
++func (r *Registry) IncludeAdditionalImports(svc *Service, goPkg GoPackage) {
++ if !r.separatePackage {
++ return
++ }
++ if r.additionalImports == nil {
++ r.additionalImports = make(map[string][]string)
++ }
++ // when generating a separate package for the gateway, we need to generate an import statement
++ // for the gRPC stubs that are no longer in the same package. This is done by adding the grpc
++ // package to the additionalImports list. In order to prepare a valid import statement, we'll replace
++ // the source package name, something like: ../pet/v1/v1petgateway with ../pet/v1/v1petgrpc
++ const (
++ baseTypePackageName = "protocolbuffers"
++ baseTypePackageSubPath = baseTypePackageName + "/go"
++ grpcPackageName = "grpc"
++ grpcPackageSubPath = grpcPackageName + "/go"
++ )
++ packageName := strings.TrimSuffix(goPkg.Name, "gateway") + grpcPackageName
++ svc.GRPCFile = &File{
++ GoPkg: GoPackage{
++ // additionally, as the `go_package` option is passed through from the generator, and can only be
++ // set the one time, without making major changes, we'll use the package name sent through the
++ // options as a basis, and replace the source package name with the grpc package name.
++ Path: strings.Replace(
++ filepath.Join(goPkg.Path, packageName),
++ baseTypePackageSubPath,
++ grpcPackageSubPath,
++ 1,
++ ),
++ Name: strings.Replace(packageName, baseTypePackageSubPath, grpcPackageSubPath, 1),
++ },
++ }
++ r.additionalImports[goPkg.Path] = append(r.additionalImports[goPkg.Path], svc.GRPCFile.GoPkg.Path)
++}
++
++// GetAdditionalImports returns additionalImports
++func (r *Registry) GetAdditionalImports(goPkg GoPackage) []string {
++ if !r.separatePackage || r.additionalImports == nil {
++ return nil
++ }
++ return r.additionalImports[goPkg.Path]
++}
diff --git internal/descriptor/registry.go internal/descriptor/registry.go
+index 421c0089..26bed31a 100644
-index 421c0089..e44501d2 100644
--- internal/descriptor/registry.go
+++ internal/descriptor/registry.go
@@ -157,6 +157,13 @@ type Registry struct {
@@ -25,16 +88,15 @@
+
+ // additionalImports is a list of additional imports to be added to the generated code.
+ // N.B. additional imports is not a flag option
++ additionalImports map[string][]string
-+ additionalImports []string
+
// preserveRPCOrder, if true, will ensure the order of paths emitted in openapi swagger files mirror
// the order of RPC methods found in proto files. If false, emitted paths will be ordered alphabetically.
preserveRPCOrder bool
+@@ -252,6 +259,9 @@ func (r *Registry) loadFile(filePath string, file *protogen.File) {
-@@ -251,7 +258,9 @@ func (r *Registry) loadFile(filePath string, file *protogen.File) {
- if r.standalone {
pkg.Alias = "ext" + cases.Title(language.AmericanEnglish).String(pkg.Name)
}
+
--
+ if r.separatePackage {
+ pkg.Name += "gateway"
+ }
@@ -41,86 +103,15 @@
if err := r.ReserveGoPackageAlias(pkg.Name, pkg.Path); err != nil {
for i := 0; ; i++ {
alias := fmt.Sprintf("%s_%d", pkg.Name, i)
-@@ -866,6 +875,26 @@ func (r *Registry) GetAllowPatchFeature() bool {
- return r.allowPatchFeature
- }
-
-+// SetSeparatePackage sets separatePackage
-+func (r *Registry) SetSeparatePackage(use bool) {
-+ r.separatePackage = use
-+}
-+
-+// GetSeparatePackage returns separatePackage
-+func (r *Registry) GetSeparatePackage() bool {
-+ return r.separatePackage
-+}
-+
-+// SetAdditionalImports sets additionalImports
-+func (r *Registry) SetAdditionalImports(imports []string) {
-+ r.additionalImports = imports
-+}
-+
-+// GetAdditionalImports returns additionalImports
-+func (r *Registry) GetAdditionalImports() []string {
-+ return r.additionalImports
-+}
-+
- // SetPreserveRPCOrder sets preserveRPCOrder
- func (r *Registry) SetPreserveRPCOrder(preserve bool) {
- r.preserveRPCOrder = preserve
diff --git internal/descriptor/services.go internal/descriptor/services.go
+index ad1764ce..ae58f7e3 100644
-index ad1764ce..c73bb413 100644
--- internal/descriptor/services.go
+++ internal/descriptor/services.go
+@@ -29,6 +29,7 @@ func (r *Registry) loadServices(file *File) error {
-@@ -3,6 +3,7 @@ package descriptor
- import (
- "errors"
- "fmt"
-+ "path/filepath"
- "strings"
-
- "github.com/grpc-ecosystem/grpc-gateway/v2/internal/httprule"
-@@ -20,6 +21,7 @@ func (r *Registry) loadServices(file *File) error {
- grpclog.Infof("Loading services from %s", file.GetName())
- }
- var svcs []*Service
-+ var additionalImports []string
- for _, sd := range file.GetService() {
- if grpclog.V(2) {
- grpclog.Infof("Registering %s", sd.GetName())
-@@ -29,6 +31,35 @@ func (r *Registry) loadServices(file *File) error {
ServiceDescriptorProto: sd,
ForcePrefixedName: r.standalone,
}
++ r.IncludeAdditionalImports(svc, file.GoPkg)
-+ if r.separatePackage {
-+ // when generating a separate package for the gateway, we need to generate an import statement
-+ // for the gRPC stubs that are no longer in the same package. This is done by adding the grpc
-+ // package to the additionalImports list. In order to prepare a valid import statement, we'll replace
-+ // the source package name, something like: ../pet/v1/v1petgateway with ../pet/v1/v1petgrpc
-+ const (
-+ baseTypePackageName = "protocolbuffers"
-+ baseTypePackageSubPath = baseTypePackageName + "/go"
-+ grpcPackageName = "grpc"
-+ grpcPackageSubPath = grpcPackageName + "/go"
-+ )
-+ packageName := strings.TrimSuffix(svc.File.GoPkg.Name, "gateway") + grpcPackageName
-+ svc.GRPCFile = &File{
-+ GoPkg: GoPackage{
-+ // additionally, as the `go_package` option is passed through from the generator, and can only be
-+ // set the one time, without making major changes, we'll use the package name sent through the
-+ // options as a basis, and replace the source package name with the grpc package name.
-+ Path: strings.Replace(
-+ filepath.Join(svc.File.GoPkg.Path, packageName),
-+ baseTypePackageSubPath,
-+ grpcPackageSubPath,
-+ 1,
-+ ),
-+ Name: strings.Replace(packageName, baseTypePackageSubPath, grpcPackageSubPath, 1),
-+ },
-+ }
-+ additionalImports = append(additionalImports, svc.GRPCFile.GoPkg.Path)
-+ }
-+ r.SetAdditionalImports(additionalImports)
for _, md := range sd.GetMethod() {
if grpclog.V(2) {
grpclog.Infof("Processing %s.%s", sd.GetName(), md.GetName())
@@ -176,19 +167,18 @@
// Method wraps descriptorpb.MethodDescriptorProto for richer features.
diff --git protoc-gen-grpc-gateway/internal/gengateway/generator.go protoc-gen-grpc-gateway/internal/gengateway/generator.go
+index 62766254..dc14103f 100644
-index 62766254..033d1c09 100644
--- protoc-gen-grpc-gateway/internal/gengateway/generator.go
+++ protoc-gen-grpc-gateway/internal/gengateway/generator.go
+@@ -5,6 +5,7 @@ import (
-@@ -5,6 +5,8 @@ import (
"fmt"
"go/format"
"path"
-+ "path/filepath"
+ "strings"
"github.com/grpc-ecosystem/grpc-gateway/v2/internal/descriptor"
gen "github.com/grpc-ecosystem/grpc-gateway/v2/internal/generator"
+@@ -22,11 +23,18 @@ type generator struct {
-@@ -22,11 +24,18 @@ type generator struct {
registerFuncSuffix string
allowPatchFeature bool
standalone bool
@@ -209,7 +199,7 @@
var imports []descriptor.GoPackage
for _, pkgpath := range []string{
"context",
+@@ -65,6 +73,7 @@ func New(reg *descriptor.Registry, useRequestContext bool, registerFuncSuffix st
-@@ -65,6 +74,7 @@ func New(reg *descriptor.Registry, useRequestContext bool, registerFuncSuffix st
registerFuncSuffix: registerFuncSuffix,
allowPatchFeature: allowPatchFeature,
standalone: standalone,
@@ -217,7 +207,7 @@
}
}
+@@ -90,10 +99,23 @@ func (g *generator) Generate(targets []*descriptor.File) ([]*descriptor.Response
-@@ -90,10 +100,19 @@ func (g *generator) Generate(targets []*descriptor.File) ([]*descriptor.Response
grpclog.Errorf("%v: %s", err, code)
return nil, err
}
@@ -225,10 +215,14 @@
+ fileNamePrefix := file.GeneratedFilenamePrefix
+ if g.separatePackage {
+ goPkg = descriptor.GoPackage{
++ Path: path.Join(file.GoPkg.Path, file.GoPkg.Name),
-+ Path: filepath.Join(file.GoPkg.Path, file.GoPkg.Name),
+ Name: file.GoPkg.Name,
+ }
++ fileNamePrefix = path.Join(
++ path.Dir(file.GeneratedFilenamePrefix),
++ file.GoPkg.Name,
++ path.Base(file.GeneratedFilenamePrefix),
++ )
-+ fileNamePrefix = path.Join(file.GeneratedFilenamePrefix, file.GoPkg.Name, filepath.Base(file.GeneratedFilenamePrefix))
+ }
files = append(files, &descriptor.ResponseFile{
- GoPkg: file.GoPkg,
@@ -239,11 +233,11 @@
Content: proto.String(string(formatted)),
},
})
+@@ -109,6 +131,14 @@ func (g *generator) generate(file *descriptor.File) (string, error) {
-@@ -109,6 +128,14 @@ func (g *generator) generate(file *descriptor.File) (string, error) {
imports = append(imports, pkg)
}
++ for _, additionalImport := range g.reg.GetAdditionalImports(file.GoPkg) {
-+ for _, additionalImport := range g.reg.GetAdditionalImports() {
+ elems := strings.Split(additionalImport, "/")
+ imports = append(imports, descriptor.GoPackage{
+ Path: additionalImport,
@@ -255,7 +249,7 @@
imports = append(imports, file.GoPkg)
}
diff --git protoc-gen-grpc-gateway/internal/gengateway/generator_test.go protoc-gen-grpc-gateway/internal/gengateway/generator_test.go
+index 2c5fe023..ad01fb8d 100644
-index 2c5fe023..816fbca1 100644
--- protoc-gen-grpc-gateway/internal/gengateway/generator_test.go
+++ protoc-gen-grpc-gateway/internal/gengateway/generator_test.go
@@ -1,6 +1,8 @@
@@ -267,7 +261,7 @@
"testing"
"github.com/grpc-ecosystem/grpc-gateway/v2/internal/descriptor"
+@@ -96,3 +98,73 @@ func TestGenerator_Generate(t *testing.T) {
-@@ -96,3 +98,75 @@ func TestGenerator_Generate(t *testing.T) {
t.Fatalf("invalid name %q, expected %q", gotName, expectedName)
}
}
@@ -295,7 +289,6 @@
+ for _, enum := range f.Enums {
+ enum.ForcePrefixedName = true
+ }
-+ imports := make([]string, 0)
+ for _, svc := range f.Services {
+ packageName := strings.TrimSuffix(svc.File.GoPkg.Name, "gateway") + "grpc"
+ svc.ForcePrefixedName = true
@@ -311,9 +304,8 @@
+ Name: strings.Replace(packageName, "protocolbuffers/go", "grpc/go", 1),
+ },
+ }
++ reg.IncludeAdditionalImports(svc, f.GoPkg)
-+ imports = append(reg.GetAdditionalImports(), svc.GRPCFile.GoPkg.Path)
+ }
-+ reg.SetAdditionalImports(imports)
+ }
+ result, err := g.Generate(targets)
+ if err != nil {Patch against
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Likely related to grpc-ecosystem/grpc-gateway#3801
The Generated SDK code is generating wrong imports and grpc-ecosystem/grpc-gateway#3801 (comment) explains fairly well how to reproduce it. Based on that structure when the
separate_package=trueoption is enabled it builds an incorrect import in `service1/v1/service/service1v1gateway, specificallyI suspect it's using a filename instead of the full path, but that's just a hunch.
### Debug notes:✅ strategy: directory❌ strategy: allThe text was updated successfully, but these errors were encountered: