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.

Sign up for GitHub

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

Add default HTTP route for all methods #124

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Add default HTTP route for all methods #124

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
53b81e5
Add default HTTP route for all methods
emcfarlane Apr 16, 2024
244db9f
Regenerate for tests
emcfarlane Apr 16, 2024
6cb215f
Add test for default route
emcfarlane Apr 16, 2024
71205ee
Create implicit overwritable default route target
emcfarlane Apr 17, 2024
6ca7a80
Fix lint
emcfarlane Apr 17, 2024
d8738f3
Change wording to allow overriding
emcfarlane Apr 17, 2024
8fd3335
Generate test for stream types
emcfarlane Apr 18, 2024
07867f3
Add check for REST stream types
emcfarlane Apr 18, 2024
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
4 changes: 2 additions & 2 deletions internal/examples/pets/internal/proto/buf.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@ deps:
- remote: buf.build
owner: googleapis
repository: googleapis
commit: 28151c0d0a1641bf938a7672c500e01d
digest: shake256:49215edf8ef57f7863004539deff8834cfb2195113f0b890dd1f67815d9353e28e668019165b9d872395871eeafcbab3ccfdb2b5f11734d3cca95be9e8d139de
commit: 4ed3bc159a8b4ac68fe253218760d035
digest: shake256:7149cf5e9955c692d381e557830555d4e93f205a0f1b8e2dfdae46d029369aa3fc1980e35df0d310f7cc3b622f93e19ad276769a283a967dd3065ddfd3a40e13
223 changes: 147 additions & 76 deletions internal/gen/vanguard/test/v1/content.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

39 changes: 39 additions & 0 deletions internal/gen/vanguard/test/v1/content_grpc.pb.go
View file
Open in desktop

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

31 changes: 31 additions & 0 deletions internal/gen/vanguard/test/v1/testv1connect/content.connect.go
View file
Open in desktop

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

4 changes: 2 additions & 2 deletions internal/proto/buf.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@ deps:
- remote: buf.build
owner: googleapis
repository: googleapis
commit: 711e289f6a384c4caeebaff7c6931ade
digest: shake256:e08fb55dad7469f69df00304eed31427d2d1576e9aab31e6bf86642688e04caaf0372f15fe6974cf79432779a635b3ea401ca69c943976dc42749524e4c25d94
commit: 4ed3bc159a8b4ac68fe253218760d035
digest: shake256:7149cf5e9955c692d381e557830555d4e93f205a0f1b8e2dfdae46d029369aa3fc1980e35df0d310f7cc3b622f93e19ad276769a283a967dd3065ddfd3a40e13
10 changes: 10 additions & 0 deletions internal/proto/vanguard/test/v1/content.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ service ContentService {
}
// Subscribe to updates for changes to content.
rpc Subscribe(stream SubscribeRequest) returns (stream SubscribeResponse);
// Delete a file at the given path.
rpc Delete(DeleteRequest) returns (google.protobuf.Empty) {
// This method does not have an HTTP mapping, but is still callable via its
// default POST HTTP mapping.
}
}

message IndexRequest {
Expand All @@ -66,6 +71,11 @@ message DownloadResponse {
google.api.HttpBody file = 1;
}

message DeleteRequest {
// The path to the file to delete.
string filename = 1;
}

message SubscribeRequest {
repeated string filename_patterns = 1;
}
Expand Down
8 changes: 8 additions & 0 deletions transcoder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,14 @@ func (t *Transcoder) registerMethod(handler http.Handler, methodDesc protoreflec
methodConf.streamType = connect.StreamTypeUnary
}

// Add the default HTTP POST route for the method.
if err := t.addRule(&annotations.HttpRule{
Pattern: &annotations.HttpRule_Post{Post: methodPath},
Body: "*",
}, methodConf); err != nil {
return err
}
// Add the declared HTTP rules for the method.
Copy link
Member

Choose a reason for hiding this comment

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

This seems wrong. We always add the default first and we always fail on error. So if the source annotations or separate provided annotations also include the default mapping (but configured explicitly), this will fail. That seems bad.

It also seems bad that a default mapping could otherwise prevent registration of an explicit one. I could see a case (albeit not a good practice) where a user wants to rename/split an RPC in two, and in effect needs to use a route that looks like the default mapping of method A, but have it route to method B. This prevents that.

So I think we'll probably want special knowledge for these rules in the route trie that they are defaut rules. If a conflict is encountered, always discard the default rule and use the explicitly configured one instead. Only report conflicts between two explicitly configured rules, not between an explicitly configured one and a default. Does that make sense?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added an implicit rule which may be overridden, PTAL.

if httpRule, ok := getHTTPRuleExtension(methodDesc); ok {
if err := t.addRule(httpRule, methodConf); err != nil {
return err
Expand Down
41 changes: 12 additions & 29 deletions vanguard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,19 +71,26 @@ const (
// The returned handler does the routing and dispatch to the RPC handlers
// associated with each provided service. Routing supports more than just the
// service path provided to NewService since HTTP transcoding annotations are
// used to also support REST-ful URI paths for each method.
// used to also support RESTful URI paths for each method.
//
// The returned handler also acts like a middleware, transparently "upgrading"
// RESTful routing can be established either through google.api.http annotations in the
// service's schema or by configuration using [WithRules]. These annotations define mappings
// between HTTP methods and paths to RPC methods. Refer to the [annotations.HttpRule] message
// for detailed information. By default, transcoding is provided for a POST request to the
// service's fully-qualified name and method name. This is effectively the mapping of a
// request of POST /GRPC_SERVICE_FULL_NAME/METHOD_NAME. No additional mappings conflicting
// with this default mapping may be added.
//
// Additionally, the returned handler also acts as a middleware, transparently "upgrading"
// the RPC handlers to support incoming request protocols they wouldn't otherwise
// support. This can be used to upgrade Connect handlers to support REST requests
// (based on HTTP transcoding configuration) or gRPC handlers to support Connect,
// gRPC-Web, or REST. This can even be used with a reverse proxy handler, to
// translate all incoming requests to a single protocol that another backend server
// supports.
//
// If any options given implement ServiceOption, they are treated as default service
// options and apply to all configured services, unless overridden by a particular
// service.
// Any options implementing ServiceOption are considered default service options and are applied
// to all configured services, unless overridden by a specific service.
func NewTranscoder(services []*Service, opts ...TranscoderOption) (*Transcoder, error) {
for _, svc := range services {
if svc.err != nil {
Expand Down Expand Up @@ -127,7 +134,6 @@ func NewTranscoder(services []*Service, opts ...TranscoderOption) (*Transcoder,
methods: map[string]*methodConfig{},
}

var restOnlyServices []protoreflect.ServiceDescriptor
for _, svc := range services {
resolvedOpts := defaultServiceOptions
for _, opt := range svc.opts {
Expand All @@ -136,33 +142,10 @@ func NewTranscoder(services []*Service, opts ...TranscoderOption) (*Transcoder,
if err := transcoder.registerService(svc, resolvedOpts); err != nil {
return nil, err
}
if len(resolvedOpts.protocols) == 1 {
_, ok := resolvedOpts.protocols[ProtocolREST]
if ok {
restOnlyServices = append(restOnlyServices, svc.schema)
}
}
}
if err := transcoder.registerRules(transcoderOpts.rules); err != nil {
return nil, err
}

// Finally, check that any services with only REST as target protocol
// actually have at least one method with REST mappings.
for _, svcDesc := range restOnlyServices {
methods := svcDesc.Methods()
var numSupportedMethods int
for i, length := 0, methods.Len(); i < length; i++ {
methodDesc := methods.Get(i)
if transcoder.methods[methodPath(methodDesc)].httpRule != nil {
numSupportedMethods++
}
}
if numSupportedMethods == 0 {
return nil, fmt.Errorf("service %s only supports REST target protocol but has no methods with HTTP rules", svcDesc.FullName())
}
}
Comment on lines -150 to -164
Copy link
Member

Choose a reason for hiding this comment

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

We still need a check here -- if the service only has client and/or bidi stream methods that do not use google.api.HttpBody, then we will not have any REST routes for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We were missing checks to ensure a non http body couldn't be a target. I've moved to checking at runtime that the stream can be handled by the protocol. Any method can bind, but non http body streams will fail with an unsupported error. We may be able to relax this restriction in future versions. This feels more consistent than special casing REST rules.

Copy link
Member

Choose a reason for hiding this comment

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

This feels more consistent than special casing REST rules.

REST is a special case because it can't handle all kinds of methods whereas the other protocols can.

Failing at runtime is a terrible developer experience. If we know the config cannot work, we should fail fast.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The config is not always controllable. For example these stream proto files are valid rules but we don't support the behaviour. I think we should still be able to register this service as this config may be valid behaviour in the future. I also don't see the terrible developer experience. For example if you register a service with one non-streaming and the other streaming endpoint the current behaviour will register the one and silently discard the other method. So now if you test the endpoints one will work and the other will return a 404. For this case I think it's much clearer to the user to fail with an explicit error that this streaming behaviour is not currently supported. The routes are valid but return an error.

Copy link
Member

Choose a reason for hiding this comment

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

For example if you register a service with one non-streaming and the other streaming endpoint the current behaviour will register the one and silently discard the other method.

Sure, that situation would not be an error. What I said was "if the service only has client and/or bidi stream methods that do not use google.api.HttpBody, then we will not have any REST routes for it." What I am saying is that it should be an error if there are zero valid/supported routes. That is also what this existing check was looking for.

I also don't see the terrible developer experience.

If I configure a transcoder to always translate to REST, and yet the transcoder will always 404 because there are zero supported routes, that would be frustrating as a user to have that deploy and startup successfully and then not work at all. It is obvious that a transcoder with zero routes is not their intent. Failing fast means they can trivially check that the server is correctly configured in CI and also prevent bad roll-outs.


return transcoder, nil
}

Expand Down
31 changes: 31 additions & 0 deletions vanguard_restxrpc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,37 @@ func TestMux_RESTxRPC(t *testing.T) {
code: http.StatusOK,
body: &testv1.Book{Name: "shelves/1/books/1"},
},
}, {
name: "GetBook-DefaultMethod",
input: input{
method: http.MethodPost,
path: "/vanguard.test.v1.LibraryService/GetBook",
body: &testv1.GetBookRequest{Name: "shelves/1/books/1"},
meta: http.Header{
"Message": []string{"hello"},
},
},
stream: testStream{
method: testv1connect.LibraryServiceGetBookProcedure,
reqHeader: http.Header{
"Message": []string{"hello"},
},
rspHeader: http.Header{
"Message": []string{"world"},
},
msgs: []testMsg{
{in: &testMsgIn{
msg: &testv1.GetBookRequest{Name: "shelves/1/books/1"},
}},
{out: &testMsgOut{
msg: &testv1.Book{Name: "shelves/1/books/1"},
}},
},
},
output: output{
code: http.StatusOK,
body: &testv1.Book{Name: "shelves/1/books/1"},
},
}, {
name: "GetBook-NotAllowed",
input: input{
Expand Down
22 changes: 22 additions & 0 deletions vanguard_rpcxrest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,28 @@ func TestMux_RPCxREST(t *testing.T) {
},
}},
},
}, {
name: "Delete",
input: func(clients testClients, hdr http.Header) (http.Header, []proto.Message, http.Header, error) {
msgs := []proto.Message{
&testv1.DeleteRequest{Filename: "message.txt"},
}
return outputFromUnary(ctx, clients.contentClient.Delete, hdr, msgs)
},
stream: testStream{
method: "/vanguard.test.v1.ContentService/Delete",
msgs: []testMsg{
{in: &testMsgIn{
msg: &testv1.DeleteRequest{Filename: "message.txt"},
}},
{out: &testMsgOut{
msg: &emptypb.Empty{},
}},
},
},
output: output{
messages: []proto.Message{&emptypb.Empty{}},
},
}}

for _, opts := range testOpts {
Expand Down
11 changes: 0 additions & 11 deletions vanguard_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,17 +124,6 @@ func TestServiceWithSchema(t *testing.T) {
_, ok := timestampType.New().Interface().(*timestamppb.Timestamp)
assert.True(t, ok)
})
t.Run("fails_for_rest_only_because_no_http_rules", func(t *testing.T) {
t.Parallel()
svc := NewServiceWithSchema(
svcDesc,
http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}),
WithTargetProtocols(ProtocolREST),
)

_, err := NewTranscoder([]*Service{svc})
require.ErrorContains(t, err, "service foo.bar.baz.v1.BlahService only supports REST target protocol but has no methods with HTTP rules")
})
}

func TestRuleSelector(t *testing.T) {
Expand Down
Loading