Add default HTTP route for all methods #124
Conversation
emcfarlane
changed the title
|
This results in routes that are extremely close to the Connect protocol, and differ in only subtle/confusing ways (the error format being slightly different). What is the actual use case for this? Do we suspect someone will have a client that expects or needs these automatic routes? |
There was a problem hiding this comment.
In that thread, the one use case that comes up is for use of google.api.HttpBody, so I suppose for that, this PR is in fact valuable. But I have some concerns (below).
transcoder.go
Outdated
| // 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Added an implicit rule which may be overridden, PTAL.
| // 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()) | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
| methodConf.httpRule = firstTarget |
There was a problem hiding this comment.
Removed the assignment here to make it visible to the caller that we modify methodConf. Previously this was always taking any WithRules to be the last write and therefore the "first" rule, but I don't believe that's intentional. So now we append to the list of targets to also make it easier to debug how many routes are registered for a single method.
This PR adds default HTTP bindings to all methods. A method will effectively have an implicit annotation of
POSTwith the path matching the services full name and method name like/SERIVCE_FULL_NAME/MEHTOD_NAMEand a body of*. This matches the default configuration of popular transcoding services such as google cloud.The default rule may be overridden if an explicit binding is provided, although this isn't recommended.
Closes #123