Remove /twirp prefix from routes #91

spenczar · 2018-03-15T01:10:42Z

Issue #, if available:

#86

Description of changes:

This change implements the plan discussed in #86: It removes the /twirp prefix from routes.

This would be merged into a 'v6_prerelease' branch, not master.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

marioizquierdo · 2018-03-15T18:16:03Z

clientcompat/internal/clientcompat/clientcompat.twirp.go

 		s.serveMethod(ctx, resp, req)
 		return
-	case "/twirp/twirp.clientcompat.CompatService/NoopMethod":
+	case "/twirp.clientcompat.CompatService/NoopMethod":


Just removing the prefix is not enough. The base-URL now can have path components. For example, to upgrade this service while keeping compatibility, before was mounted on "http://localhost:8000" but now it should be mounted on "http://localhost:8000/twirp". The routing should check for req.URL.Path suffix, not exact match.

Of course, I am not sure how I forgot that in this first pass. Will fix.

Oh, I do remember how: we don't want a suffix match; we expect users to use net/http.StripPrefix when setting up their server if they want to use a prefix.

Ah ok, but then we will have to be careful documenting this in the docs. Please make sure that the Haberdasher example shows how to properly mount the service in the "/twirp" prefix, with a NOTE for older versions.

marioizquierdo · 2018-03-15T18:17:05Z

clientcompat/pycompat/clientcompat_pb2_twirp.py

@@ -49,7 +49,7 @@ def __init__(self, server_address):

    def __make_request(self, body, full_method):
        req = Request(
-            url=self.__target + "/twirp" + full_method,
+            url=self.__target + full_method,


Clients should be fine with this. The only thing required to keep backwards compatibility would be to change the base-URL (__target) to include the "/twirp" prefix 👍

marioizquierdo · 2018-03-15T18:18:52Z

example/service.twirp.go

@@ -137,7 +137,7 @@ func (s *haberdasherServer) writeError(ctx context.Context, resp http.ResponseWr
 // HaberdasherPathPrefix is used for all URL paths on a twirp Haberdasher server.
 // Requests are always: POST HaberdasherPathPrefix/method
 // It can be used in an HTTP mux to route twirp requests along with non-twirp requests on other routes.
-const HaberdasherPathPrefix = "/twirp/twitch.twirp.example.Haberdasher/"
+const HaberdasherPathPrefix = "/twitch.twirp.example.Haberdasher/"


I wonder if it would be better to add a new constant name to make sure that the compiler tells the developer that there's something that needs to change after the upgrade. Since the "PathPrefix" now is just the service full name with /, maybe it would make sense to change the constant to this?

const HaberdasherFullName = "twitch.twirp.example.Haberdasher"

marioizquierdo · 2018-03-15T18:19:07Z

protoc-gen-twirp/generator.go

@@ -881,7 +881,7 @@ func (t *twirp) generateServer(file *descriptor.FileDescriptorProto, service *de
 // service. It includes a trailing slash. (for example
 // "/twirp/twitch.example.Haberdasher/").
 func pathPrefix(file *descriptor.FileDescriptorProto, service *descriptor.ServiceDescriptorProto) string {
-	return fmt.Sprintf("/twirp/%s/", fullServiceName(file, service))
+	return fmt.Sprintf("/%s/", fullServiceName(file, service))


need to update comment

rhysh · 2018-03-16T15:50:11Z

How much risk is there in keeping the symbol names the same while changing their meaning? NewHaberdasherServer now expects to get requests without a leading /twirp and will respond with a "bad_route" error if it gets requests from v5 clients. Users who upgrade from v5 to v6 without reading and understanding the patch notes will break.

We could make NewHaberdasherServer accept requests both with and without the /twirp prefix, and ship an HTTPClient ("Doer") implementation that removes the /twirp prefix (since the client behavior wouldn't be able to immediately change). It would stick around forever.

We could change the symbol names to NewHaberdasherServerV6 and NewHaberdasherProtobufClientV6 so it's clear which ones don't use the /twirp prefix. We could keep the old constructors, at least until we release v7.0.0. Or we could add the ...V6 constructors in v5.X.0 and remove the old ones in v6.0.0.

On issue tracking, it looks like this code implements the proposal at #55, minus the . vs / question described in #86.

spenczar · 2018-03-16T19:14:06Z

I am very much OK with keeping the same symbol names for the PathPrefix const and for the constructor because this change increments the major version number. This is what semantic versioning is for. Cluttering the API is not worth it.

Serving requests both with and without the /twirp prefix, on the other hand, might be a worthwhile thing that helps people upgrade; I'm more inclined to do that.

spenczar · 2018-04-09T17:55:22Z

@rhysh @marioizquierdo, please take another look.

marioizquierdo · 2018-04-13T20:03:00Z

clientcompat/pycompat/clientcompat_pb2_twirp.py

@@ -49,7 +49,7 @@ def __init__(self, server_address):



for readability it could make sense to rename server_address to base_url, with an example like http://localhost:8080/twirp

marioizquierdo · 2018-04-13T20:07:00Z

clientcompat/internal/clientcompat/clientcompat.twirp.go

 		s.serveMethod(ctx, resp, req)
 		return
-	case "/twirp/twirp.clientcompat.CompatService/NoopMethod":
+	case "/twirp.clientcompat.CompatService/NoopMethod":


Ah ok, but then we will have to be careful documenting this in the docs. Please make sure that the Haberdasher example shows how to properly mount the service in the "/twirp" prefix, with a NOTE for older versions.

Remove /twirp prefix from routes

23f4332

spenczar requested review from rhysh and marioizquierdo March 15, 2018 01:10

marioizquierdo suggested changes Mar 15, 2018

View reviewed changes

Update comment on pathPrefix now that it changed

dedbce5

marioizquierdo reviewed Apr 13, 2018

View reviewed changes

marioizquierdo approved these changes Apr 13, 2018

View reviewed changes

spenczar merged commit 825afdd into v6_prerelease Apr 19, 2018

spenczar deleted the v6_routing branch April 19, 2018 19:49

marioizquierdo mentioned this pull request Apr 12, 2019

Client URL compatibility with go client? arthurnn/twirp-ruby#33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove /twirp prefix from routes #91

Remove /twirp prefix from routes #91

spenczar commented Mar 15, 2018

marioizquierdo Mar 15, 2018

spenczar Mar 16, 2018

spenczar Mar 16, 2018

marioizquierdo Apr 13, 2018

marioizquierdo Mar 15, 2018

marioizquierdo Mar 15, 2018

marioizquierdo Mar 15, 2018

rhysh commented Mar 16, 2018

spenczar commented Mar 16, 2018 •

edited

Loading

spenczar commented Apr 9, 2018

marioizquierdo Apr 13, 2018

marioizquierdo Apr 13, 2018

Remove /twirp prefix from routes #91

Remove /twirp prefix from routes #91

Conversation

spenczar commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhysh commented Mar 16, 2018

spenczar commented Mar 16, 2018 • edited Loading

spenczar commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spenczar commented Mar 16, 2018 •

edited

Loading