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

Optional /twirp prefix #263

Closed
marioizquierdo opened this issue Aug 18, 2020 · 2 comments
Closed

Optional /twirp prefix #263

marioizquierdo opened this issue Aug 18, 2020 · 2 comments

Comments

@marioizquierdo
Copy link
Contributor

marioizquierdo commented Aug 18, 2020

Proposal to update the Twirp route protocol from the current pattern:

<baseURL>/twirp/<package>.<Service>/<Method>

to a new pattern where the /twirp prefix is optional and can be changed for any other prefix:

<baseURL>[prefix]/<package>.<Service>/<Method>

Examples of valid Twirp URLs with different prefixes:

https://example.com/twirp/package.Service/Method     // default /twirp prefix
https://example.com/a/z/prfx/package.Service/Method  // custom prefix
https://example.com/package.Service/Method           // no prefix

Backwards compatibility

This is a backwards-compatible change.

Existing clients and services will use the /twirp prefix by default, but the constructors will offer a new option to specify a different prefix, or use an empty value for no prefix.

Implementation

PR: #264

For other languages, some implementations are already prepared for "v6 routes" (removing the /twirp prefix was part of the v6 protocol draft, but it ended up being canceled). For example, the Twirp-Ruby already requires services to be mounted on routes like "http://localhost:3000/twirp" where the "twirp" prefix is already part of the baseURL. This Twirp-Typescript implementation have an option to generate code with the "v6" option to avoid adding the "/twirp" prefix.

Other languages can follow a similar process to the Go PR to allow optional prefixes.

Related issues

Similar attempts to change the /twirp prefix and routing were made before. Adding them here for reference, and also how they compare to this proposal:

  • Proposed new routing format: /package/service/method #55 Proposed new routing format: /package/service/method: This change is different. The route suffix <package>.<Service>/<Method> will not be changed, which makes backwards compatibility a lot easier. However, that issue contains valuable comments about changing the route scheme. Please check for more details.
  • Add support for custom route prefixes #48 Add support for custom route prefixes: This issue would be fixed with the proposed baseURL that allows to mount Twirp services on any sub-route.
  • gorilla mux not working with twirp handler #54 gorilla mux not working with twirp handler: This issue would be fixed as well.
  • v6 removal of /twirp prefix makes upgrading hard #179 v6 removal of /twirp prefix makes upgrading hard: This issue covers in detail why changing the URL format of Twirp requests is really hard to do without breaking backwards compatibility. In one of the options, it mentions leaving the "/twirp" path prefix as a default prefix, but allow it to be customized. The idea was to allow customization through an option on the proto file, but that requires extra steps when generating code and that is not ideal. The solution here is a mix of the proposed options 1. and 2.; leaving the baseURL outside of the proto definition, allowing to "mount" the twirp service on any prefix, so there are no extra steps when generating code, and no mandatory steps when upgrading to the latest Twirp version.
@marioizquierdo marioizquierdo changed the title Optional /twirp prefix in routes Optional /twirp prefix, custom prefix, no-prefix Aug 18, 2020
@marioizquierdo marioizquierdo changed the title Optional /twirp prefix, custom prefix, no-prefix Optional /twirp prefix Aug 18, 2020
@mfridman
Copy link
Contributor

This is a very welcome change. We use https://github.com/go-chi/chi as our mux and found composing the router meant we couldn't easily build "normal" handlers alongside twirp services (which are really just handlers).

// Distributor service
distHandler := rpc.NewDistributorServer(d, &twirphooks.DefaultHooks)
r.Mount(distHandler.PathPrefix(), distHandler)

^----- ideally can mount this along /distributor path

// "normal" handlers
r.Route("/distributor", func(r chi.Router) {
	// routes...
})

Furthermore, depending on the service we had to expose the /twirp prefix in the ingress, which isn't ideal.

Then we trim prefix, but leads to all sort of gotchas when following the request. So being able to drop the /twirp prefix would really help cleanup code (for some of us).

Happy to contribute. Whether reviewing, implementing or testing. :)

@marioizquierdo
Copy link
Contributor Author

Implemented on #264 and released as part of v7

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

No branches or pull requests

2 participants