Optional twirp prefix

[marioizquierdo]

Implements proposal on issue #263

Tasks:

• Client option to allow using a different prefix, with default value "/twirp".
• Server routing should match the route suffix instead of matching the full path. This allows using a mux and mounting the Twirp service in a sub-route, so that the baseURL can include other prefixes.
• Add tests cases using baseURLs with custom prefixes and with no prefix. Also test using a mux to force a path prefix.
• Review and update docs to clarify that the prefix is optional.
• Review code comments to clarify that the prefix is optional and may be a different one.
• Update the spec as v7. The update is backwards compatible, but allowing different prefixes besides "/twirp" means that other implementations may need to be updated to be spec-compliant.

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

[bufdev]

nit: I would name this NoPathPrefix

[marioizquierdo]

I rested on this idea for a few days (allow baseURL to have any prefix, and allow to remove the "/twirp" prefix)... but I am not happy with it. Allowing the server to handle any route with any suffix is not entirely backwards compatible (sending requests like "/twirp/foo/bar/arbitrary/prefix/pck.Svc/Mtd" will also work, which may not be desired).

I will change the implementation to keep the allowed baseURL to be scheme and domain only (e.g. "http://example.com") and the prefix is provided through a client and service option, that is "/twirp" by default, so that servers will work exactly like they do now (only allow requests on the exact path) but still allow for other prefixes, and they need to be explicitly specified.

[marioizquierdo]

Using the server and client options, we can now specify a different prefix (default is "/twirp):

// server handling routes on a custom prefix
svcImpl := PickyHatmaker(1)
server := NewHaberdasherServer(svcImpl, 
    twirp.WithServerPathPrefix("/my/custom/prefix"))
s := httptest.NewServer(server)

// client sending requests on a custom prefix
c := NewHaberdasherJSONClient(s.URL, http.DefaultClient, 
    twirp.WithClientPathPrefix("/my/custom/prefix"))
resp, err := c.MakeHat(ctx, &Size{Inches: 1})

The generated constant <Service>PathPrefix includes the default "/twirp" prefix, and we can not change that because the prefix is not known at the time of code-generation. I changed the comments to clarify:

// HaberdasherPathPrefix is a convenience constant that could used to identify URL paths.
// Should be used with caution, it only matches routes generated by Twirp Go clients,
// that add a "/twirp" prefix by default, and use CamelCase service and method names.
// More info: https://twitchtv.github.io/twirp/docs/routing.html
const HaberdasherPathPrefix = "/twirp/twirp.internal.twirptest.Haberdasher/"

The existing method server.PathPrefix() will return a path with the right prefix:

server.PathPrefix() //=> "/my/custom/prefix/twirp.internal.twirptest.Haberdasher/"

It may be a little confusing to specify twirp.WithServerPathPrefix only for the <prefix> in the route, but the service.PathPrefix() returns the full service path (minus the method). But this seems minor.

[marioizquierdo]

Added options for the server constructor, which allows to specify the option for the path prefix.

To be consistent with the client constructor, I made the ServerHooks part of the options, which changes the constructor signature a little bit:

Before, the server constructor would require a hooks parameter that could be nil:

NewHaberdasherServer(svcImpl, nil)
NewHaberdasherServer(svcImpl, hooks)

Now, the hooks can be specified as an option:

NewHaberdasherServer(svcImpl)
NewHaberdasherServer(svcImpl, twirp.WithServerHooks(hooks))
NewHaberdasherServer(svcImpl, twirp.WithServerHooks(hooks), twirp.WithServerPathPrefix("/twirp"))

The new constructor signature is New<Scv>Server(svc Svc, opts ...interface{}). Using ...interface{} instead of ...twirp.ServerOption for backwards compatibility; using nil or hooks as a second parameter still works like before.

[marioizquierdo]

Using nil still works (for backwards compatibility), but the last constructor argument is now a variadic opts ...interface{}, which could be nil, the hooks or an option WithHooks(hooks). I removed the leading nil value from examples and tests where hooks are not part of the focus.

[marioizquierdo]

I realize that, while this change is backwards-compatible in code, it is not backwards-compatible in the spec; the spec change means that other implementations that only allow a "/twirp" prefix are suddenly not implementing the spec properly. I guess the best path forward is to make a v6 spec. Since the previous v6 spec was archived (streams not happening), would it make sense to make a v7 spec? or is it better to make a 5.1 spec? or maybe it is fine to go with v6? @spenczar do you have a preference here? (I think either way is probably fine)

[marioizquierdo]

We will be releasing this as v7, both the spec and the Twirp version. This release will contain other PRs with some other bugfixes. We will skip v6 for clarity.

[bufdev]

Thank you for doing this!