-
Notifications
You must be signed in to change notification settings - Fork 327
Description
The current v6_prerelease branch changes the URL scheme. It goes from /twirp/<package>.<service>/<method>
to just /<package>.<service>/<method>
.
Unfortunately, this is difficult to upgrade into. Consider a service which has generated v5 code today, and which has a client which vendors that generated Go code. The service wants to upgrade to generating v6 code, and they currently register their Twirp service like this:
If they regenerate their code and don't tell the client to upgrade their vendored copy, then all client calls immediately start failing, since the client is hitting the old URL, which 404s. If the client upgrade first, same thing - all 404s. The client and server need to deploy an upgrade version simultaneously. That's unacceptably painful. We need something better.
What are the options?
- Document code changes the service owner can make
We could tell service owners that they must run their service on two URL path prefixes if they are upgrading, at least until all their clients are updated. For example, they'd need to do this:
mux := http.NewServeMux()
mux.Handle(HaberdasherPathPrefix, svc)
mux.Handle("/twirp" + HaberdasherPathPrefix, http.StripPrefix("/twirp", svc))
This is not very pleasant. It's gross legacy code, and hard to know you need to do this; I don't like solutions that require manual action from our users. It's hard to know when you can remove this code, as well. And if users already have a mux, they could get confused.
We could make this more visible by renaming the PathPrefix constant. That way, code won't compile, and hopefully the user goes and looks up what's wrong and learns that they need to make a code change. But this is a bad option; upgrading should be painless.
- Leave /twirp as a default prefix, but allow it to be customized
We could change the branch to default to using the /twirp prefix. But if the user specifies, we could remove the prefix in generated code. This would make upgrading easy - the user would only run into trouble if they enabled a custom prefix, which is kind of their own fault. Most upgrades would be painless.
But I already rejected this approach in #48, really. My argument there was that non-customizability is a feature, not a bug. It makes third-party generators harder to get right, and complicates the spec a bit. That said, it's worth looking at how we'd do this.
Protobuf files support options. We could have something like this:
// this path depends on your local machine :(
import "path/to/twitchtv/twirp/genconfig.proto"
service Haberdasher {
option (twitchtv.twirp.genconfig.prefix) = "/whatever/you/want"
rpc MakeHat(Size) returns (Hat)
}
The challenge is really that the option must be defined in some other .proto
file which gets imported by the user. Twirp would have to distribute that .proto
file somehow, and provide instructions on how to import it. This is tricky - protoc is not a friendly tool. But perhaps this is alright, since we hope that custom prefixes are the exceptional case, not the common one, so it's alright for them to be a little difficult.
- Go back to requiring /twirp prefix
Finally, we could revert this change entirely. This is appealing to me - it keeps Twirp simple and the upgrade is really clean. I like that no third-party generators become incompatible, since the protocol remains the same.
But there are a few people who are using the prerelease branch today because they hate that prefix. I'd like to hear from them on their needs, once again. We kind of touched on them in #55, but that covered other topics, and I don't think we have good written reasons that removing the /twirp prefix specifically is worth the cost. I'd like to hear those arguments.
I lean towards option 3, in absence of clear reasons the /twirp prefix needs to be removed.