-
Notifications
You must be signed in to change notification settings - Fork 326
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
Proposed new routing format: /package/service/method #55
Comments
Terrible idea for migration simplicity: a v5 RoundTripper in a box that tries the old path if the new path doesn't work. |
One advantage of the |
@cep21 That's true, you'd need to wrap each twirp service with your middleware wrapper, instead of being able to do it once. Most programs only serve one Twirp service, but some might serve multiple. This would mean an extra line of code for each one. That seems like a pretty mild loss. You could encapsulate it with a function that loops over your services if it's really bad, like this: type middleware func(http.Handler) http.Handler
func mountTwirpServices(mux *http.ServeMux, svcs map[string]http.Handler, m middleware) {
for prefix, svc := range svcs {
mux.Handle(prefix, m(svc))
}
} |
Internally at Twitch having the I like the proposed path schema: |
I reached out to @spenczar about this via DM. Just sharing some of my thoughts on this. As part of my dta.gov.au team, we're evaluating some API toolkits/specs/etc. that could be used in various whole of government solutions. Whilst tooling/SKDs should mask the path most of the time, the reality is that if we build APIs that are used by other gov departments/services, it's possible that they won't or can't use any of the SDKs that would mask the path. Instead we'd give them some sort of "documentation" for how to construct HTTP requests to call the API. Putting something like:
in the documentation might not be received that well even though you and I know it's just a word. It may not give confidence that your service is taking things seriously, etc. I don't have a strong opinion on what the best format is for the path, but I wonder if at the least it could just use something else instead of |
Two new concerns with
In light of this, @wora suggested still using a prefix. Two options:
We can be pretty sure that
I lean a little towards |
I agree that we should get over with the trademark risk entirely and focus
on the project itself.
|
I prefer to have a customizable prefix (does that what |
If consensus is use prefix, why not just stick with I have no opinion on the naming, just as long as |
@fajran No, @doapp-ryanp, |
IMHO we should just not enforce a prefix -- as commented here you can mount your twirp service wherever you want (including at Of course, as this is a backwards incompatible change in itself (unless you do choose to mount at EDIT: The client would need a baseurl anyway, it would now just need the mounted prefix as well. |
@wora, a significant problem with Many examples of confused people dealing with dollar signs in URLs that pop up on google:
I am sure there would be more. This is enough for me to say |
@thechriswalker Yes, the issue is making sure clients know the right prefix. It would be best if clients only need a hostport, not a prefix too. Your point that the server can be mounted at a special place, and the client can include that prefix if they must, is a really good one, though. That might mean we have coverage for the exceptional cases where we would collide, and where a collision is unavoidable because changing the package is considered unacceptable. |
Trpc feels like it had clear advantages with little downside. I think it is enough away from twirp or twitch that serious business won't mind |
@spenczar I think the difference between having the "address" of the service as I am slightly biased as I am thinking of using twirp exposed over the public internet from the same web server as the rest of a web app, so would want a customizable prefix. |
@thechriswalker, another good point re: sharing the scheme, host, and port anyway. Your use case, exposing it next to other web app routes, is one we want to support well. |
I agree with @thechriswalker. A client needs at least one piece of information to locate the server, and we can standardize the |
I agree that Twirp services should be "mountable" on any prefix. For consistency, we can call it "baseURL" in the client (instead of "addr" or "host"). This will also solve a somewhat surprising behavior with the current router switch, that only matches requests in the root url even if the twirp httpHandler was mounted with a mux on top of another base path. I think it makes sense to asume that Twirp services are mountable on any "baseURL", and then clients receive that "baseURL" as parameter. This also makes sense for other languages, not just Go. If for v6 we keep the URL format for Twirp in the form |
Yes. We have an agreement here. I am drafting the wire protocol spec with Spencer. Stay tuned. |
@marioizquierdo, good catch that the router's current switch doesn't permit remounting. If we advise people to mount on a separate prefix if they need to avoid collisions, we will also need to either:
The second seems more reasonable to me. |
I have a request and a suggestion for the migration, regardless of the format we choose. First, we release a version (v5.x.0) where servers can understand both v5 and v6 URIs but where clients still send v5 URIs. An org that is currently using v5.1.0 could safely upgrade all clients and servers to v5.x.0 in any order. After finishing that migration, they could then upgrade their clients and servers to v6.0.0, again in any order. Second, I suggest that v5 path prefixes be named "v5" rather than "legacy". Someday v6 may be legacy too. For example:
|
@rhysh, your suggestion on names of the The two-step migration sounds good and interesting too. It's kinda weird that we'd be making a |
@rhysh @spenczar Related to my previous comment, if we keep the current path format The v6 upgrade instructions would tell services to also mount twirp on the |
I think we should change the server code to accept both package.service/method and package/service/method, and encourage people to use later. I think the server change is very trivial and the change does offer long term value to users. The routing can be done by different proxy layers, there is no chance for us to update them. It is a very tiny cost for us to avoid the pain for customers. |
Right, @wora, if v6 servers accept both But as @marioizquierdo points out, v6 clients can talk to v5 servers only if we generate them to use I think we have agreement that we should drop the @Xe is the one who suggested this - could you talk a bit about the benefits of using slashes here? |
A proto package typically represents a single API. You can write a single routing rule per package, without worrying about adding new interfaces to the same package. With the same URL length, / is much better for dispatching than . For upgrade, we can add a boolean flag to the v6 client generator to produce the old . separator, with a transition window of 6 months. The usage of Twirp probably allows changing the spec now. Or we can stay as is forever. |
@mocax, I really don't want to add a boolean flag. Flags make the behavior dependent on the letters somebody typed in on the command line when they generated the code rather than dependent on the proto document + the Twirp spec. That makes the system much less predictable. If we do this, we'll just do it for clients, and servers will be backwards compatible. I agree that we should do it sooner than later, while Twirp is still young enough to handle spec changes. |
I don't like to have flag at all. If we decide to switch, we should just do it. |
Yeah, no flags, because it also makes multi-language support a lot harder, then all service generators in all other languages will also need to support the flag! In terms of backwards compatibility, it is fairy easy to make the Go server generator to support both v5 and v6 routes, but that also imposes extra burden on other languages. Should all other languages support v5 routes as well? Ideally Twirp would support scenarios like a client generated in twirp-v5 in Go talking to a service generated in twirp-v8 in Rust. In any case, it is probably early enough to consider this kind of change (actually, is now or never!!!). It is probably totally OK for the future server generator in Rust to only support v6+ clients. |
@spenczar close to any decision on this? My team would like to use Twirp in production, however I'm waiting for this change to occur (we are not fans of the v5 routing scheme, for reasons brought up in this thread). |
@doapp-ryanp I think we have consensus here, we will change the routing in the way listed. We should make a However, when it comes to actually implementing that new spec, I would like a v6 release to include streaming support. That requires two things:
I think that, for now, we do a v6-alpha release which includes just routing changes and a sketch for a possible streaming API, with very loud notices that the streaming API be broken. We'll need to maintain a separate branch until we lock in the streaming API. So, immediate action items:
You should be good to use twirp after step 3, @doapp-ryanp, if you're comfortable working with a prerelease version while the streaming API gets some use. The only unstable part of the prerelease would be streaming APIs; we would promise not to break other stuff. |
On the subject of changing the routes, what are your thoughts on lowercasing the service and method names? It seems a bit odd to have case sensitivity in a URL. |
@nullbio That doesn't bother me too much. It's unusual, for sure, but it shouldn't matter, as you shouldn't be seeing URLs too often anyway. If you want to discuss more, could you open an issue for that topic? |
Per URL spec, URL path is case sensitive. Protocol Buffers are case
sensitive as well. Let's not bother reinventing the wheel here. We should
follow the spec, not argue against the spec.
|
The protocol v6 was archived. The are no current plans to update the route path formats. For reference, there is more talk about this in #179 |
PR #263 implements a solution for the twirp prefix in a backwards compatible way. It updates the wire protocol from:
to:
The |
I think we could improve Twirp's routing scheme. It has two problems right now: the
/twirp/
prefix, and thepackage.service
scheme (instead ofpackage/service
).Some people have mentioned to me that they're hesitant to use Twirp because the routes are prefixed with /twirp, which is concerning to them for several reasons:
We currently have the
/twirp
prefix to avoid colliding with gRPC's routes, which live at/package.service/method
, and to reserve a space for future APIs (like a reflection server).Slashes-based routes (instead of dot-based) would be preferable because lots of existing technology expects slash-based routing. Load balancers and proxies use slashes for namespaces. If Twirp used a slash after the package name, users could leverage this much more easily. This is often possible with dots, just annoying, as you have to write a (possibly pretty complex) regex.
I think we can fix both of these in one go. This would be a breaking change at the protocol level, so it's something to take very seriously, but I think we can do it without much pain. It would just take a version bump to v6.
Proposed Implementation:
I propose we use a new routing scheme:
/<package>/<service>/<method>
.This does not collide with gRPC's routes, which is good. I think it gives us flexibility to put future routes in by putting them under a Twirp package. We could provide a reflection service like this one, which lists available services:
It would live at
/twirp.meta/Reflector
, which seems good to me.Regarding compatibility: generated clients would always use the new routing scheme. Generated servers would be able to use both, in case your users have old generated clients still around. We would accomplish this by exporting two path prefixes:
Users can mount both on a mux if they want to support old clients. If not, they can just use the new one.
If we do this, there are other consequences.
The pain of those other consequences is a good reason that we should try to get this out promptly, if we do it.
The text was updated successfully, but these errors were encountered: