From 3f0fa0002450f9d25261944d6a48a6dc8c063846 Mon Sep 17 00:00:00 2001 From: Akshay Shah Date: Fri, 17 Mar 2023 22:05:19 -0700 Subject: [PATCH 1/4] Propagate top-level comments from proto schemas If the source schema has leading or leading detached comments on the protobuf syntax or package declarations, propagate them to the generated code. This matches the behavior of protoc-gen-go and protoc-gen-go-grpc, with one small change: we keep protobuf package comments adjacent to the Go package declaration, so that they appear as the package-level documentation in GoDoc. Fixes #470. --- cmd/protoc-gen-connect-go/main.go | 24 +++++++++++++++++++ internal/gen/connect/ping/v1/ping.pb.go | 6 +++++ .../ping/v1/pingv1connect/ping.connect.go | 5 ++++ internal/proto/connect/ping/v1/ping.proto | 4 ++++ 4 files changed, 39 insertions(+) diff --git a/cmd/protoc-gen-connect-go/main.go b/cmd/protoc-gen-connect-go/main.go index 0f2004e0..07314802 100644 --- a/cmd/protoc-gen-connect-go/main.go +++ b/cmd/protoc-gen-connect-go/main.go @@ -51,6 +51,7 @@ import ( "github.com/bufbuild/connect-go" "google.golang.org/protobuf/compiler/protogen" + "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/types/descriptorpb" "google.golang.org/protobuf/types/pluginpb" ) @@ -68,6 +69,11 @@ const ( usage = "See https://connect.build/docs/go/getting-started to learn how to use this plugin.\n\nFlags:\n -h, --help\tPrint this help and exit.\n --version\tPrint the version and exit." commentWidth = 97 // leave room for "// " + + // To propagate top-level comments, we need the field number of the syntax + // declaration and the package name in the file descriptor. + protoSyntaxFieldNum = 12 + protoPackageFieldNum = 2 ) func main() { @@ -124,6 +130,15 @@ func generate(plugin *protogen.Plugin, file *protogen.File) { } func generatePreamble(g *protogen.GeneratedFile, file *protogen.File) { + syntaxPath := protoreflect.SourcePath{protoSyntaxFieldNum} + syntaxLocation := file.Desc.SourceLocations().ByPath(syntaxPath) + for _, comment := range syntaxLocation.LeadingDetachedComments { + leadingComments(g, protogen.Comments(comment), false /* deprecated */) + } + g.P() + leadingComments(g, protogen.Comments(syntaxLocation.LeadingComments), false /* deprecated */) + g.P() + g.P("// Code generated by ", filepath.Base(os.Args[0]), ". DO NOT EDIT.") g.P("//") if file.Proto.GetOptions().GetDeprecated() { @@ -132,6 +147,15 @@ func generatePreamble(g *protogen.GeneratedFile, file *protogen.File) { g.P("// Source: ", file.Desc.Path()) } g.P() + + pkgPath := protoreflect.SourcePath{protoPackageFieldNum} + pkgLocation := file.Desc.SourceLocations().ByPath(pkgPath) + for _, comment := range pkgLocation.LeadingDetachedComments { + leadingComments(g, protogen.Comments(comment), false /* deprecated */) + } + g.P() + leadingComments(g, protogen.Comments(pkgLocation.LeadingComments), false /* deprecated */) + g.P("package ", file.GoPackageName) g.P() wrapComments(g, "This is a compile-time assertion to ensure that this generated file ", diff --git a/internal/gen/connect/ping/v1/ping.pb.go b/internal/gen/connect/ping/v1/ping.pb.go index c8b7edd8..77b60416 100644 --- a/internal/gen/connect/ping/v1/ping.pb.go +++ b/internal/gen/connect/ping/v1/ping.pb.go @@ -12,12 +12,18 @@ // See the License for the specific language governing permissions and // limitations under the License. +// The canonical location for this file is +// https://github.com/bufbuild/connect-go/blob/main/internal/proto/connect/ping/v1/ping.proto. + // Code generated by protoc-gen-go. DO NOT EDIT. // versions: // protoc-gen-go v1.28.1 // protoc (unknown) // source: connect/ping/v1/ping.proto +// The connect.ping.v1 package contains an echo service designed to test the +// connect-go implementation. + package pingv1 import ( diff --git a/internal/gen/connect/ping/v1/pingv1connect/ping.connect.go b/internal/gen/connect/ping/v1/pingv1connect/ping.connect.go index 67dea969..473f9434 100644 --- a/internal/gen/connect/ping/v1/pingv1connect/ping.connect.go +++ b/internal/gen/connect/ping/v1/pingv1connect/ping.connect.go @@ -12,10 +12,15 @@ // See the License for the specific language governing permissions and // limitations under the License. +// The canonical location for this file is +// https://github.com/bufbuild/connect-go/blob/main/internal/proto/connect/ping/v1/ping.proto. + // Code generated by protoc-gen-connect-go. DO NOT EDIT. // // Source: connect/ping/v1/ping.proto +// The connect.ping.v1 package contains an echo service designed to test the +// connect-go implementation. package pingv1connect import ( diff --git a/internal/proto/connect/ping/v1/ping.proto b/internal/proto/connect/ping/v1/ping.proto index d7316265..b113fff7 100644 --- a/internal/proto/connect/ping/v1/ping.proto +++ b/internal/proto/connect/ping/v1/ping.proto @@ -12,8 +12,12 @@ // See the License for the specific language governing permissions and // limitations under the License. +// The canonical location for this file is +// https://github.com/bufbuild/connect-go/blob/main/internal/proto/connect/ping/v1/ping.proto. syntax = "proto3"; +// The connect.ping.v1 package contains an echo service designed to test the +// connect-go implementation. package connect.ping.v1; message PingRequest { From 5c87842b507cfdc7a739c4d41085e912f438b34a Mon Sep 17 00:00:00 2001 From: Akshay Shah Date: Fri, 17 Mar 2023 22:28:12 -0700 Subject: [PATCH 2/4] Generate constants for procedure names Generate constants for each RPC's fully-qualified name. This is particularly useful in Connect interceptors and net/http middleware, where users often want their code to behave differently depending on the RPC being called. (For example, they may have a subset of procedures exempted from authentication.) Without this PR, the best approach we can offer is either hard-coding a string literal (fragile if the package or RPC is moved, renamed, or deleted) or reflection using the descriptor embedded in protoc-gen-go's output. protoc-gen-go-grpc recently began doing something very similar to this, so it seems reasonable for us to do the same. Of the open issues, this fixes #326. --- cmd/protoc-gen-connect-go/main.go | 46 ++++++++++++----- connect.go | 3 ++ connect_ext_test.go | 8 +-- .../v1/collidev1connect/collide.connect.go | 18 +++++-- .../ping/v1/pingv1connect/ping.connect.go | 50 +++++++++++++------ 5 files changed, 90 insertions(+), 35 deletions(-) diff --git a/cmd/protoc-gen-connect-go/main.go b/cmd/protoc-gen-connect-go/main.go index 07314802..79b198f6 100644 --- a/cmd/protoc-gen-connect-go/main.go +++ b/cmd/protoc-gen-connect-go/main.go @@ -168,12 +168,37 @@ func generatePreamble(g *protogen.GeneratedFile, file *protogen.File) { } func generateServiceNameConstants(g *protogen.GeneratedFile, services []*protogen.Service) { + var numMethods int g.P("const (") for _, service := range services { constName := fmt.Sprintf("%sName", service.Desc.Name()) wrapComments(g, constName, " is the fully-qualified name of the ", service.Desc.Name(), " service.") g.P(constName, ` = "`, service.Desc.FullName(), `"`) + numMethods += len(service.Methods) + } + g.P(")") + g.P() + + if numMethods == 0 { + return + } + wrapComments(g, "These constants are the fully-qualified names of the RPCs defined in this package. ", + "They're exposed at runtime as Spec.Procedure and as the final two segments of the HTTP route.") + g.P("//") + wrapComments(g, "Note that these are different from the fully-qualified method names used by ", + "google.golang.org/protobuf/reflect/protoreflect. To convert from these constants to ", + "reflection-formatted method names, remove the leading slash and convert the ", + "remaining slash to a period.") + g.P("const (") + for _, service := range services { + for _, method := range service.Methods { + // The runtime exposes this value as Spec.Procedure, so we should use the + // same term here. + wrapComments(g, procedureConstName(method), " is the fully-qualified name of the ", + service.Desc.Name(), "'s ", method.Desc.Name(), " RPC.") + g.P(procedureConstName(method), ` = "`, fmt.Sprintf("/%s/%s", service.Desc.FullName(), method.Desc.Name()), `"`) + } } g.P(")") g.P() @@ -236,7 +261,7 @@ func generateClientImplementation(g *protogen.GeneratedFile, service *protogen.S "(", ) g.P("httpClient,") - g.P(`baseURL + "`, procedureName(method), `",`) + g.P(`baseURL + `, procedureConstName(method), `,`) g.P("opts...,") g.P("),") } @@ -353,15 +378,15 @@ func generateServerConstructor(g *protogen.GeneratedFile, service *protogen.Serv isStreamingClient := method.Desc.IsStreamingClient() switch { case isStreamingClient && !isStreamingServer: - g.P(`mux.Handle("`, procedureName(method), `", `, connectPackage.Ident("NewClientStreamHandler"), "(") + g.P(`mux.Handle(`, procedureConstName(method), `, `, connectPackage.Ident("NewClientStreamHandler"), "(") case !isStreamingClient && isStreamingServer: - g.P(`mux.Handle("`, procedureName(method), `", `, connectPackage.Ident("NewServerStreamHandler"), "(") + g.P(`mux.Handle(`, procedureConstName(method), `, `, connectPackage.Ident("NewServerStreamHandler"), "(") case isStreamingClient && isStreamingServer: - g.P(`mux.Handle("`, procedureName(method), `", `, connectPackage.Ident("NewBidiStreamHandler"), "(") + g.P(`mux.Handle(`, procedureConstName(method), `, `, connectPackage.Ident("NewBidiStreamHandler"), "(") default: - g.P(`mux.Handle("`, procedureName(method), `", `, connectPackage.Ident("NewUnaryHandler"), "(") + g.P(`mux.Handle(`, procedureConstName(method), `, `, connectPackage.Ident("NewUnaryHandler"), "(") } - g.P(`"`, procedureName(method), `",`) + g.P(procedureConstName(method), `,`) g.P("svc.", method.GoName, ",") g.P("opts...,") g.P("))") @@ -434,13 +459,8 @@ func serverSignatureParams(g *protogen.GeneratedFile, method *protogen.Method, n g.QualifiedGoIdent(method.Output.GoIdent) + "], error)" } -func procedureName(method *protogen.Method) string { - return fmt.Sprintf( - "/%s.%s/%s", - method.Parent.Desc.ParentFile().Package(), - method.Parent.Desc.Name(), - method.Desc.Name(), - ) +func procedureConstName(m *protogen.Method) string { + return fmt.Sprintf("%s%sProcedure", m.Parent.Desc.Name(), m.Desc.Name()) } func reflectionName(service *protogen.Service) string { diff --git a/connect.go b/connect.go index ff6d49d5..d9459e1d 100644 --- a/connect.go +++ b/connect.go @@ -271,6 +271,9 @@ type HTTPClient interface { } // Spec is a description of a client call or a handler invocation. +// +// If you're using Protobuf, protoc-gen-connect-go generates a constant for the +// fully-qualified Procedure corresponding to each RPC in your schema. type Spec struct { StreamType StreamType Procedure string // for example, "/acme.foo.v1.FooService/Bar" diff --git a/connect_ext_test.go b/connect_ext_test.go index 95095933..99e29b9d 100644 --- a/connect_ext_test.go +++ b/connect_ext_test.go @@ -1520,7 +1520,7 @@ func TestStreamForServer(t *testing.T) { client, server := newPingServer(&pluggablePingServer{ cumSum: func(ctx context.Context, stream *connect.BidiStream[pingv1.CumSumRequest, pingv1.CumSumResponse]) error { assert.Equal(t, stream.Spec().StreamType, connect.StreamTypeBidi) - assert.Equal(t, stream.Spec().Procedure, "/connect.ping.v1.PingService/CumSum") + assert.Equal(t, stream.Spec().Procedure, pingv1connect.PingServicePingProcedure) assert.False(t, stream.Spec().IsClient) return nil }, @@ -1535,7 +1535,7 @@ func TestStreamForServer(t *testing.T) { client, server := newPingServer(&pluggablePingServer{ countUp: func(ctx context.Context, req *connect.Request[pingv1.CountUpRequest], stream *connect.ServerStream[pingv1.CountUpResponse]) error { assert.Equal(t, stream.Conn().Spec().StreamType, connect.StreamTypeServer) - assert.Equal(t, stream.Conn().Spec().Procedure, "/connect.ping.v1.PingService/CountUp") + assert.Equal(t, stream.Conn().Spec().Procedure, pingv1connect.PingServiceCountUpProcedure) assert.False(t, stream.Conn().Spec().IsClient) assert.Nil(t, stream.Send(&pingv1.CountUpResponse{Number: 1})) return nil @@ -1591,7 +1591,7 @@ func TestStreamForServer(t *testing.T) { client, server := newPingServer(&pluggablePingServer{ sum: func(ctx context.Context, stream *connect.ClientStream[pingv1.SumRequest]) (*connect.Response[pingv1.SumResponse], error) { assert.Equal(t, stream.Spec().StreamType, connect.StreamTypeClient) - assert.Equal(t, stream.Spec().Procedure, "/connect.ping.v1.PingService/Sum") + assert.Equal(t, stream.Spec().Procedure, pingv1connect.PingServiceSumProcedure) assert.False(t, stream.Spec().IsClient) assert.True(t, stream.Receive()) msg := stream.Msg() @@ -1847,7 +1847,7 @@ func TestGRPCErrorMetadataIsTrailersOnly(t *testing.T) { req, err := http.NewRequestWithContext( context.Background(), http.MethodPost, - server.URL+"/connect.ping.v1.PingService/Fail", + server.URL+pingv1connect.PingServiceFailProcedure, bytes.NewReader(body), ) assert.Nil(t, err) diff --git a/internal/gen/connect/collide/v1/collidev1connect/collide.connect.go b/internal/gen/connect/collide/v1/collidev1connect/collide.connect.go index 698beaa9..0fd964e5 100644 --- a/internal/gen/connect/collide/v1/collidev1connect/collide.connect.go +++ b/internal/gen/connect/collide/v1/collidev1connect/collide.connect.go @@ -39,6 +39,18 @@ const ( CollideServiceName = "connect.collide.v1.CollideService" ) +// These constants are the fully-qualified names of the RPCs defined in this package. They're +// exposed at runtime as Spec.Procedure and as the final two segments of the HTTP route. +// +// Note that these are different from the fully-qualified method names used by +// google.golang.org/protobuf/reflect/protoreflect. To convert from these constants to +// reflection-formatted method names, remove the leading slash and convert the remaining slash to a +// period. +const ( + // CollideServiceImportProcedure is the fully-qualified name of the CollideService's Import RPC. + CollideServiceImportProcedure = "/connect.collide.v1.CollideService/Import" +) + // CollideServiceClient is a client for the connect.collide.v1.CollideService service. type CollideServiceClient interface { Import(context.Context, *connect_go.Request[v1.ImportRequest]) (*connect_go.Response[v1.ImportResponse], error) @@ -56,7 +68,7 @@ func NewCollideServiceClient(httpClient connect_go.HTTPClient, baseURL string, o return &collideServiceClient{ _import: connect_go.NewClient[v1.ImportRequest, v1.ImportResponse]( httpClient, - baseURL+"/connect.collide.v1.CollideService/Import", + baseURL+CollideServiceImportProcedure, opts..., ), } @@ -84,8 +96,8 @@ type CollideServiceHandler interface { // and JSON codecs. They also support gzip compression. func NewCollideServiceHandler(svc CollideServiceHandler, opts ...connect_go.HandlerOption) (string, http.Handler) { mux := http.NewServeMux() - mux.Handle("/connect.collide.v1.CollideService/Import", connect_go.NewUnaryHandler( - "/connect.collide.v1.CollideService/Import", + mux.Handle(CollideServiceImportProcedure, connect_go.NewUnaryHandler( + CollideServiceImportProcedure, svc.Import, opts..., )) diff --git a/internal/gen/connect/ping/v1/pingv1connect/ping.connect.go b/internal/gen/connect/ping/v1/pingv1connect/ping.connect.go index 473f9434..617bbbc8 100644 --- a/internal/gen/connect/ping/v1/pingv1connect/ping.connect.go +++ b/internal/gen/connect/ping/v1/pingv1connect/ping.connect.go @@ -44,6 +44,26 @@ const ( PingServiceName = "connect.ping.v1.PingService" ) +// These constants are the fully-qualified names of the RPCs defined in this package. They're +// exposed at runtime as Spec.Procedure and as the final two segments of the HTTP route. +// +// Note that these are different from the fully-qualified method names used by +// google.golang.org/protobuf/reflect/protoreflect. To convert from these constants to +// reflection-formatted method names, remove the leading slash and convert the remaining slash to a +// period. +const ( + // PingServicePingProcedure is the fully-qualified name of the PingService's Ping RPC. + PingServicePingProcedure = "/connect.ping.v1.PingService/Ping" + // PingServiceFailProcedure is the fully-qualified name of the PingService's Fail RPC. + PingServiceFailProcedure = "/connect.ping.v1.PingService/Fail" + // PingServiceSumProcedure is the fully-qualified name of the PingService's Sum RPC. + PingServiceSumProcedure = "/connect.ping.v1.PingService/Sum" + // PingServiceCountUpProcedure is the fully-qualified name of the PingService's CountUp RPC. + PingServiceCountUpProcedure = "/connect.ping.v1.PingService/CountUp" + // PingServiceCumSumProcedure is the fully-qualified name of the PingService's CumSum RPC. + PingServiceCumSumProcedure = "/connect.ping.v1.PingService/CumSum" +) + // PingServiceClient is a client for the connect.ping.v1.PingService service. type PingServiceClient interface { // Ping sends a ping to the server to determine if it's reachable. @@ -70,27 +90,27 @@ func NewPingServiceClient(httpClient connect_go.HTTPClient, baseURL string, opts return &pingServiceClient{ ping: connect_go.NewClient[v1.PingRequest, v1.PingResponse]( httpClient, - baseURL+"/connect.ping.v1.PingService/Ping", + baseURL+PingServicePingProcedure, opts..., ), fail: connect_go.NewClient[v1.FailRequest, v1.FailResponse]( httpClient, - baseURL+"/connect.ping.v1.PingService/Fail", + baseURL+PingServiceFailProcedure, opts..., ), sum: connect_go.NewClient[v1.SumRequest, v1.SumResponse]( httpClient, - baseURL+"/connect.ping.v1.PingService/Sum", + baseURL+PingServiceSumProcedure, opts..., ), countUp: connect_go.NewClient[v1.CountUpRequest, v1.CountUpResponse]( httpClient, - baseURL+"/connect.ping.v1.PingService/CountUp", + baseURL+PingServiceCountUpProcedure, opts..., ), cumSum: connect_go.NewClient[v1.CumSumRequest, v1.CumSumResponse]( httpClient, - baseURL+"/connect.ping.v1.PingService/CumSum", + baseURL+PingServiceCumSumProcedure, opts..., ), } @@ -151,28 +171,28 @@ type PingServiceHandler interface { // and JSON codecs. They also support gzip compression. func NewPingServiceHandler(svc PingServiceHandler, opts ...connect_go.HandlerOption) (string, http.Handler) { mux := http.NewServeMux() - mux.Handle("/connect.ping.v1.PingService/Ping", connect_go.NewUnaryHandler( - "/connect.ping.v1.PingService/Ping", + mux.Handle(PingServicePingProcedure, connect_go.NewUnaryHandler( + PingServicePingProcedure, svc.Ping, opts..., )) - mux.Handle("/connect.ping.v1.PingService/Fail", connect_go.NewUnaryHandler( - "/connect.ping.v1.PingService/Fail", + mux.Handle(PingServiceFailProcedure, connect_go.NewUnaryHandler( + PingServiceFailProcedure, svc.Fail, opts..., )) - mux.Handle("/connect.ping.v1.PingService/Sum", connect_go.NewClientStreamHandler( - "/connect.ping.v1.PingService/Sum", + mux.Handle(PingServiceSumProcedure, connect_go.NewClientStreamHandler( + PingServiceSumProcedure, svc.Sum, opts..., )) - mux.Handle("/connect.ping.v1.PingService/CountUp", connect_go.NewServerStreamHandler( - "/connect.ping.v1.PingService/CountUp", + mux.Handle(PingServiceCountUpProcedure, connect_go.NewServerStreamHandler( + PingServiceCountUpProcedure, svc.CountUp, opts..., )) - mux.Handle("/connect.ping.v1.PingService/CumSum", connect_go.NewBidiStreamHandler( - "/connect.ping.v1.PingService/CumSum", + mux.Handle(PingServiceCumSumProcedure, connect_go.NewBidiStreamHandler( + PingServiceCumSumProcedure, svc.CumSum, opts..., )) From b8c3164f44bacd212d0b5a074a5d503d40217b3b Mon Sep 17 00:00:00 2001 From: Akshay Shah Date: Fri, 24 Mar 2023 16:06:50 -0700 Subject: [PATCH 3/4] Handle lower-case protobuf identifiers --- cmd/protoc-gen-connect-go/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/protoc-gen-connect-go/main.go b/cmd/protoc-gen-connect-go/main.go index 79b198f6..3f5198a4 100644 --- a/cmd/protoc-gen-connect-go/main.go +++ b/cmd/protoc-gen-connect-go/main.go @@ -460,7 +460,7 @@ func serverSignatureParams(g *protogen.GeneratedFile, method *protogen.Method, n } func procedureConstName(m *protogen.Method) string { - return fmt.Sprintf("%s%sProcedure", m.Parent.Desc.Name(), m.Desc.Name()) + return fmt.Sprintf("%s%sProcedure", m.Parent.GoName, m.GoName) } func reflectionName(service *protogen.Service) string { From 55b645c964e4cf1698b86255e1a51fcc83eec7bd Mon Sep 17 00:00:00 2001 From: Akshay Shah Date: Fri, 24 Mar 2023 16:13:16 -0700 Subject: [PATCH 4/4] Fix typo updating test code to constants --- connect_ext_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/connect_ext_test.go b/connect_ext_test.go index 99e29b9d..e9dbfdc9 100644 --- a/connect_ext_test.go +++ b/connect_ext_test.go @@ -1520,7 +1520,7 @@ func TestStreamForServer(t *testing.T) { client, server := newPingServer(&pluggablePingServer{ cumSum: func(ctx context.Context, stream *connect.BidiStream[pingv1.CumSumRequest, pingv1.CumSumResponse]) error { assert.Equal(t, stream.Spec().StreamType, connect.StreamTypeBidi) - assert.Equal(t, stream.Spec().Procedure, pingv1connect.PingServicePingProcedure) + assert.Equal(t, stream.Spec().Procedure, pingv1connect.PingServiceCumSumProcedure) assert.False(t, stream.Spec().IsClient) return nil },