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

Allowed pass mux interface to handler #304

Closed
tsingson opened this issue Jun 20, 2022 · 3 comments
Closed

Allowed pass mux interface to handler #304

tsingson opened this issue Jun 20, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@tsingson
Copy link

tsingson commented Jun 20, 2022

Is your feature request related to a problem? Please describe.

make it easy to replace http.ServeMux to another mux / router that 100% compatible with net/http
like this one: https://github.com/go-chi/chi

mux interface like this:

type ServeMux interface {
	ServeHTTP(w http.ResponseWriter, r *http.Request)
	Handle(pattern string, handler http.Handler)
}

and other golang router is easy to wrap the interface via POST handler ( connect-go only use POST )

like this issue https://github.com/bufbuild/connect-go/issues/258

Describe the solution you'd like

from

func NewTestHandler(svc TestHandler, opts ...connect_go.HandlerOption) (string, http.Handler) {
	mux := http.NewServeMux()
	mux.Handle("/media.recordi.v1.Test/IndexRecord", connect_go.NewUnaryHandler(
		"/media.recordi.v1.Test/IndexRecord",
		svc.IndexRecord,
		opts...,
	))
	return "/media.recordi.v1.Test/", mux
}

to

func NewTestHandler(svc TestHandler, mux ServeMux, opts ...connect_go.HandlerOption) (string, http.Handler) {
	//  mux := http.NewServeMux()
	mux.Handle("/media.recordi.v1.Test/IndexRecord", connect_go.NewUnaryHandler(
		"/media.recordi.v1.Test/IndexRecord",
		svc.IndexRecord,
		opts...,
	))
	return "/media.recordi.v1.Test/", mux
}

impl

if this suggest ok, it's easy to change the function below:

1. add new file in connect-go repo

// mux.go
package connect

import (
	"net/http"
)

type ServeMux interface {
	ServeHTTP(w http.ResponseWriter, r *http.Request)
	Handle(pattern string, handler http.Handler)
}

change this func

func generateServerConstructor(g *protogen.GeneratedFile, service *protogen.Service, names names) {
	wrapComments(g, names.ServerConstructor, " builds an HTTP handler from the service implementation.",
		" It returns the path on which to mount the handler and the handler itself.")
	g.P("//")
	wrapComments(g, "By default, handlers support the Connect, gRPC, and gRPC-Web protocols with ",
		"the binary Protobuf and JSON codecs. They also support gzip compression.")
	if isDeprecatedService(service) {
		g.P("//")
		deprecated(g)
	}
	handlerOption := connectPackage.Ident("HandlerOption")
	g.P("func ", names.ServerConstructor, "(svc ", names.Server, ", opts ...", handlerOption,
		") (string, ", httpPackage.Ident("Handler"), ") {")
	g.P("mux := ", httpPackage.Ident("NewServeMux"), "()")
	for _, method := range service.Methods {
		isStreamingServer := method.Desc.IsStreamingServer()
		isStreamingClient := method.Desc.IsStreamingClient()
		switch {
		case isStreamingClient && !isStreamingServer:
			g.P(`mux.Handle("`, procedureName(method), `", `, connectPackage.Ident("NewClientStreamHandler"), "(")
		case !isStreamingClient && isStreamingServer:
			g.P(`mux.Handle("`, procedureName(method), `", `, connectPackage.Ident("NewServerStreamHandler"), "(")
		case isStreamingClient && isStreamingServer:
			g.P(`mux.Handle("`, procedureName(method), `", `, connectPackage.Ident("NewBidiStreamHandler"), "(")
		default:
			g.P(`mux.Handle("`, procedureName(method), `", `, connectPackage.Ident("NewUnaryHandler"), "(")
		}
		g.P(`"`, procedureName(method), `",`)
		g.P("svc.", method.GoName, ",")
		g.P("opts...,")
		g.P("))")
	}
	g.P(`return "/`, reflectionName(service), `/", mux`)
	g.P("}")
	g.P()
}

to

func generateServerConstructor(g *protogen.GeneratedFile, service *protogen.Service, names names) {
	wrapComments(g, names.ServerConstructor, " builds an HTTP handler from the service implementation.",
		" It returns the path on which to mount the handler and the handler itself.")
	g.P("//")
	wrapComments(g, "By default, handlers support the Connect, gRPC, and gRPC-Web protocols with ",
		"the binary Protobuf and JSON codecs. They also support gzip compression.")
	if isDeprecatedService(service) {
		g.P("//")
		deprecated(g)
	}
	handlerOption := connectPackage.Ident("HandlerOption")
	g.P("func ", names.ServerConstructor, "(svc ", names.Server, ",mux connect_go.ServeMux,  opts ...", handlerOption,
		") (string, ", httpPackage.Ident("Handler"), ") {")
	// g.P("mux := ", httpPackage.Ident("NewServeMux"), "()")
	for _, method := range service.Methods {
		isStreamingServer := method.Desc.IsStreamingServer()
		isStreamingClient := method.Desc.IsStreamingClient()
		switch {
		case isStreamingClient && !isStreamingServer:
			g.P(`mux.Handle("`, procedureName(method), `", `, connectPackage.Ident("NewClientStreamHandler"), "(")
		case !isStreamingClient && isStreamingServer:
			g.P(`mux.Handle("`, procedureName(method), `", `, connectPackage.Ident("NewServerStreamHandler"), "(")
		case isStreamingClient && isStreamingServer:
			g.P(`mux.Handle("`, procedureName(method), `", `, connectPackage.Ident("NewBidiStreamHandler"), "(")
		default:
			g.P(`mux.Handle("`, procedureName(method), `", `, connectPackage.Ident("NewUnaryHandler"), "(")
		}
		g.P(`"`, procedureName(method), `",`)
		g.P("svc.", method.GoName, ",")
		g.P("opts...,")
		g.P("))")
	}
	g.P(`return "/`, reflectionName(service), `/", mux`)
	g.P("}")
	g.P()
}

then rebuild the plugins

go install github.com/bufbuild/connect-go/cmd/protoc-gen-connect-go

then , re-generate

buf generate

it's work like


func Example_handler() {
	// protoc-gen-connect-go generates constructors that return plain net/http
	// Handlers, so they're compatible with most Go HTTP routers and middleware
	// (for example, net/http's StripPrefix). Each handler automatically supports
	// the Connect, gRPC, and gRPC-Web protocols.
	mux := http.NewServeMux()
	mux.Handle(
		pingv1connect.NewPingServiceHandler(
			&ExamplePingServer{}, mux, // our business logic
		),
	)
	// You can serve gRPC's health and server reflection APIs using
	// github.com/bufbuild/connect-grpchealth-go and
	// github.com/bufbuild/connect-grpcreflect-go.
	_ = http.ListenAndServeTLS(
		"localhost:8080",
		"internal/testdata/server.crt",
		"internal/testdata/server.key",
		mux,
	)
	// To serve HTTP/2 requests without TLS (as many gRPC clients expect), import
	// golang.org/x/net/http2/h2c and golang.org/x/net/http2 and change to:
	// _ = http.ListenAndServe(
	// 	"localhost:8080",
	// 	h2c.NewHandler(mux, &http2.Server{}),
	// )
}

test pass

/Users/qinshen/go/src/github.com/tsingson/connect-go   go test ./...
ok  	github.com/bufbuild/connect-go	1.305s
?   	github.com/bufbuild/connect-go/cmd/protoc-gen-connect-go	[no test files]
ok  	github.com/bufbuild/connect-go/internal/assert	0.012s
?   	github.com/bufbuild/connect-go/internal/gen/connect/error/v1	[no test files]
?   	github.com/bufbuild/connect-go/internal/gen/connect/ping/v1	[no test files]
?   	github.com/bufbuild/connect-go/internal/gen/connect/ping/v1/pingv1connect	[no test files]
?   	github.com/bufbuild/connect-go/internal/gen/connectext/grpc/status/v1	[no test files]
/Users/qinshen/go/src/github.com/tsingson/connect-go   go test -bench .
goos: darwin
goarch: amd64
pkg: github.com/bufbuild/connect-go
cpu: Intel(R) Core(TM) i7-4980HQ CPU @ 2.80GHz
BenchmarkConnect/unary-8         	     172	   5874945 ns/op
BenchmarkREST/unary-8            	      74	  16407789 ns/op
PASS
ok  	github.com/bufbuild/connect-go	3.739s
/Users/qinshen/go/src/github.com/tsingson/connect-go 
@tsingson tsingson added the enhancement New feature or request label Jun 20, 2022
@akshayjshah
Copy link
Member

akshayjshah commented Jun 20, 2022

Thanks for the detailed examples, @tsingson! We definitely want connect-go to work with a variety of mux implementations. Unfortunately, I don't fully understand the problem you're having - the generated code uses the standard library's http.ServeMux, but that's just an implementation detail.

You can use connect-go with chi today. I'm sure that this works, because we use connect and chi together in the Buf Schema Registry 😃

mux := chi.NewMux()
route, handler := pingv1connect.NewPingServiceHandler(&ExamplePingServer{})
mux.Post(route+"*", http.HandlerFunc(handler.ServeHTTP))

Or, if you prefer:

mux := chi.NewMux()
route, handler := pingv1connect.NewPingServiceHandler(&ExamplePingServer{})
mux.Method(http.MethodPost, route+"*", handler)

@tsingson
Copy link
Author

tsingson commented Jun 21, 2022

Thanks for the detailed examples, @tsingson! We definitely want connect-go to work with a variety of mux implementations. Unfortunately, I don't fully understand the problem you're having - the generated code uses the standard library's http.ServeMux, but that's just an implementation detail.

You can use connect-go with chi today. I'm sure that this works, because we use connect and chi together in the Buf Schema Registry 😃

mux := chi.NewMux()
route, handler := pingv1connect.NewPingServiceHandler(&ExamplePingServer{})
mux.Post(route+"*", http.HandlerFunc(handler.ServeHTTP))

Or, if you prefer:

mux := chi.NewMux()
route, handler := pingv1connect.NewPingServiceHandler(&ExamplePingServer{})
mux.Method(http.MethodPost, route+"*", handler)

Thanks for the quick reply and code example.
connect-go just uses the HTTP POST static Mux, which is good via http.ServeMux inside generated code.
I m re-thinking about this, maybe my options is not a good one.

I'm integrating connect-go into my project ( to replace drpc, another good one RPC ) , and go some performance testting. So far, it's working well.

thanks Buf team, great work on GRPC!!

@rdeusser
Copy link

I just wanted to post this here in case someone else gets stuck like I did. If your connect routes aren't handled from the root / and they're handled from say /api, you need to strip the prefix. Like so:

r := chi.NewRouter()
route, handler := pingv1connect.NewPingServiceHandler(&ExamplePingServer{})
r.Handle(route+"*", http.StripPrefix("/api", handler))

I was confused on why I was getting a 404 when clearly the route existed, but I didn't realize that the generated handlers return 404 if if url path isn't an exact match, hence the need to strip the prefix. Hope this helps someone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants