Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve comments & add procedure consts to generated code #480

Merged
merged 5 commits into from
Mar 25, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 57 additions & 13 deletions cmd/protoc-gen-connect-go/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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
akshayjshah marked this conversation as resolved.
Show resolved Hide resolved
protoPackageFieldNum = 2
)

func main() {
Expand Down Expand Up @@ -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() {
Expand All @@ -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 ",
Expand All @@ -144,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 ",
bufdev marked this conversation as resolved.
Show resolved Hide resolved
"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.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of documenting this in the generated code, is it worth adding a utility function to convert a procedure to a protoreflect.FullName? That seems like a useful method and we could simplify the generated code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping to minimize the number of places that the connect-go runtime couples closely to the protobuf runtime. I suspect that the most common reason people would make this translation is to get a method descriptor, so I'm planning to just expose that directly in a subsequent PR.

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()
Expand Down Expand Up @@ -212,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("),")
}
Expand Down Expand Up @@ -329,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("))")
Expand Down Expand Up @@ -410,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.GoName, m.GoName)
}

func reflectionName(service *protogen.Service) string {
Expand Down
3 changes: 3 additions & 0 deletions connect.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
8 changes: 4 additions & 4 deletions connect_ext_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
Expand All @@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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)
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions internal/gen/connect/ping/v1/ping.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

55 changes: 40 additions & 15 deletions internal/gen/connect/ping/v1/pingv1connect/ping.connect.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions internal/proto/connect/ping/v1/ping.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down