Add draft of v6 specification #80

spenczar · 2018-02-14T22:02:58Z

This is a prerelease draft to explain planned changes coming in v6. The only change
from v5 (so far) is a change of the URL scheme Twirp uses.

Issue #, if available:

Relates to #55, but doesn't implement it yet.

Description of changes:

Here's diff PROTOCOL.md PROTOCOL_V6_DRAFT.md:

< # Twirp Wire Protocol
---
> # EXPERIMENTAL Twirp v6 Wire Protocol
3,4c3,31
< This document defines the Twirp wire protocol over HTTP. The
< current protocol version is v5.
---
> This document is a draft of a new version ("v6") of the Twirp wire
> protocol over HTTP. This draft is EXPERIMENTAL and subject to change.
> v6 of Twirp is not released or considered stable; instead, this
> document lists planned changes which may be available in prerelease
> distributions.
>
> ## Changes from v5
>
> ### URL scheme
>
> In [v5](./PROTOCOL.md), URLs followed this format:
>
> **URL ::= Base-URL "/twirp/" [ Package "." ] Service "/" Method**
>
> In v6, the "/twirp/" prefix is removed:
>
> **URL ::= Base-URL "/" [ Package "." ] Service "/" Method**
>
> The "/twirp/" prefix is removed for three reasons:
>
>  - Trademark concerns: some very large organizations don't want to
>    take any legal risks and are concerned that "twirp" could become
>    trademarked.
>  - Feels like advertising: To some users, putting "twirp" in all your
>    routes feels like it's just supposed to pump Twirp's brand, and
>    provides no value back to users.
>  - Homophonous with "twerp": In some Very Serious settings (like
>    government websites), it's not okay that "twirp" sounds like
>    "twerp", which means something like "insignificant pest."
28c55
< **URL ::= Base-URL "/twirp/" [ Package "." ] Service "/" Method**
---
> **URL ::= Base-URL "/" [ Package "." ] Service "/" Method**
109c136
< POST /twirp/example.echoer.Echo/Hello HTTP/1.1
---
> POST /example.echoer.Echo/Hello HTTP/1.1
120c147
< POST /twirp/example.echoer.Echo/Hello HTTP/1.1
---
> POST /example.echoer.Echo/Hello HTTP/1.1

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

This is a prerelease draft to explain planned changes. The only change from v5 (so far) is a change of the URL scheme Twirp uses.

spenczar · 2018-02-14T22:03:38Z

cc @mocax, @rhysh, @marioizquierdo, @Xe.

marioizquierdo · 2018-02-15T02:42:41Z

docs/spec_v6.md

+   provides no value back to users.
+ - Homophonous with "twerp": In some Very Serious settings (like
+   government websites), it's not okay that "twirp" sounds like
+   "twerp", which means something like "insignificant pest."


This is a concern only for whoever is updating from v5 to v6, I think it is better to keep the v6 (latest) spec clear without this. It may be enough with adding this info in the Release Notes for v6, or if you want to keep this info in the spec is better to add it in the v5 spec (as a notice that it changed after v6).

Yeah, I'll move this sort of thing into a changelog doc that will go unpublished until we release v6.

marioizquierdo · 2018-02-15T02:44:19Z

docs/spec_v6.md

+* **Base-URL** is the virtual location of a Twirp API server, which is
+  typically published via API documentation or service discovery.
+  Currently, it should only contain URL `scheme` and `authority`. For
+  example, "https://example.com".


in v6, the Base-URL should allow a path prefix as well, for example "http://mydomain.com/twirp", specially important to ease backwards compatibility with v5 services.

We should remove this restriction. The Base-URL can be any valid URL. There is no need to add any restriction. For example, why do we need to rule out port?

A URL "authority" includes a port, but you're right that we should allow path prefixes.

I don't think we should allow any valid URL. We should not permit query parameters or fragments. I think this should be come "it should only contain a URL scheme, authority, and path, as defined in RFC 3986 Section 3."

Yeah this would be be great - we use path prefixes to route between our services so twirp is a non-starter for us atm.

marioizquierdo · 2018-02-15T02:55:11Z

docs/spec_v6.md

+  `CalendarService`.
+* **Method** is the proto `rpc` name for an API method. For example,
+  `CreateEvent`.
+


Would it make sense to add an example? It may be useful, specially because Base-URL contains "/", and Package contains "." on it (same symbols used as separators). The example should make it visually clear that Twirp URLs need to be parsed from right to left (extract Method first, then Service, then optional Package, and the rest is the Base-URL).

There is no need to parse the URL. The server registers a handler against a URL, and all it needs to do is URL lookup using a hash table.

There's an example further down that I think should be enough.

I agree that URLs don't need to be parsed like that necessarily. For example, if the user mounts the Twirp server under a path prefix, we could say they should trim the path prefix from request URLs before handing the request off to Twirp in the Go implementation. In any case, this is an implementation-specific concern.

Needing to parse right-to-left is worrisome. That will be hard to implement consistently and to explain concisely. It sounds like we're not quite heading in that direction .. but if we are, we should stop.

@rhysh, certainly agreed, but I don't think that's what we're headed to. In Go, users would do something like this:

mux.Handle("/userdefined" + HaberdasherPathPrefix, http.StripPrefix("/userdefined", svc))

😅 I retire the sentence "need to be parsed from right to left". Sorry that was misleading. The URLs can be parsed in multiple ways, one of them is by starting from the right, but it could also be removing the Base-URL first and then using a regexp, etc. What I mean is that one example will make it easier to understand the things that need to be consider when implementing a Twirp router, or just to understand the routes themselves.

mocax · 2018-02-15T05:03:34Z

PROTOCOL_V6_DRAFT.md

+
+**URL ::= Base-URL "/" [ Package "." ] Service "/" Method**
+
+The "/twirp/" prefix is removed for three reasons:


The "/twirp/" prefix is not really removed. It can be part of the Base-URL if the service provider wants to keep it. It is just no longer part of the spec.

Right, we should definitely allow path prefixes.

mocax · 2018-02-15T05:04:47Z

PROTOCOL_V6_DRAFT.md

+
+In v6, the "/twirp/" prefix is removed:
+
+**URL ::= Base-URL "/" [ Package "." ] Service "/" Method**


Please remove the first "/" and make it part of Base-URL. The less client logic, the better.

I think we need the leading "/". If it's in the Base-URL, we need to say that the URL must not have an empty path. Otherwise, we'd allow a Base-URL of https://example.com which would lead to this:
https://example.comtwirp.example.Haberdasher/MakeHat

which seems clearly invalid.

Wouldn't you use the ResolveReference method on *url.URL in order to support both cases?

Eg. https://play.golang.org/p/Zj6ehRCuZ_G

@peterhellberg That might be a good way to make the API nice for Go users, but this question is on what the spec should be, which is a language-independent question. We're trying to work out what the BNF for the URL scheme should look like, which is probably a bit pedantic, but important to get right.

mocax · 2018-02-15T05:07:59Z

PROTOCOL_V6_DRAFT.md

+In [ABNF syntax](https://tools.ietf.org/html/rfc5234), Twirp's URLs
+have the following format:
+
+**URL ::= Base-URL "/" [ Package "." ] Service "/" Method**


This URL schema is identical to gRPC, which seems confusing. At least it would conflict with existing logging, monitoring, routing, load balancing systems. While I am not saying gRPC should own this URL schema, it is better not to be identical between Twirp and gRPC.

I thought the schema was supposed to be URL ::= Base-URL "/" [ Package "/" ] Service "/" Method

@mocax, In case of conflicts, users could mount Twirp at a different path prefix, so I'm not very worried about that.

@Xe, Right! Don't know how I forgot that part.

mocax · 2018-02-15T05:10:04Z

docs/spec_v6.md

+* **Base-URL** is the virtual location of a Twirp API server, which is
+  typically published via API documentation or service discovery.
+  Currently, it should only contain URL `scheme` and `authority`. For
+  example, "https://example.com".


We should remove this restriction. The Base-URL can be any valid URL. There is no need to add any restriction. For example, why do we need to rule out port?

mocax · 2018-02-15T05:11:48Z

docs/spec_v6.md

+  `CalendarService`.
+* **Method** is the proto `rpc` name for an API method. For example,
+  `CreateEvent`.
+


There is no need to parse the URL. The server registers a handler against a URL, and all it needs to do is URL lookup using a hash table.

spenczar · 2018-02-19T20:35:03Z

All concerns addressed, I believe - please take another look, y'all.

rhysh

I'm concerned about the complexity of using '/' as a delimiter when the package may be unset.

rhysh · 2018-02-20T17:33:09Z

docs/spec_changelog.md

+```
+
+Version 6 changes this format to remove the mandatory `"/twirp"` prefix, and
+changes `Package` and `Service` to be delimited with a `/` instead of a `.`


Packages will still contain . characters; we're not changing the internal delimiter. How about this phrasing:

"and changes the separator between Package and Service to be a / instead of a . character"

rhysh · 2018-02-20T17:35:14Z

docs/spec_changelog.md

+to set any prefix you like.
+
+If you loved the old `/twirp` prefix, you can still use it by using a base URL
+that ends with `/twirp`. You're no longer forced into use it, however.


Users can keep that prefix, but 1) they'll need to remove it before passing the request to the Twirp generated code, and 2) the rest of the URI path won't be the same as v5 because of the .->/ change (unless the package is unset).

Right, I'll clarify this.

rhysh · 2018-02-20T17:36:16Z

docs/spec_v6.md

+
+This document is a draft of a new version ("v6") of the Twirp wire
+protocol over HTTP. This draft is EXPERIMENTAL and subject to change.
+v6 of Twirp is not released or considered stable; instead, this


"not yet released ...", unless you want something like "even-numbered releases are unstable"

Sure, not yet released - I'm just trying to explain that it's a draft.

rhysh · 2018-02-20T17:41:49Z

docs/spec_v6.md

+  `CalendarService`.
+* **Method** is the proto `rpc` name for an API method. For example,
+  `CreateEvent`.
+


Needing to parse right-to-left is worrisome. That will be hard to implement consistently and to explain concisely. It sounds like we're not quite heading in that direction .. but if we are, we should stop.

rhysh · 2018-02-20T17:53:12Z

docs/spec_v6.md

+**Proto Request**
+
+```
+POST /userprefix/example.echoer/Echo/Hello HTTP/1.1


Using / as a delimiter forces us to deal with cases of overlap. Consider the following two:

One has no package declaration, and describes a service called "Foo" with a method "Hello". It gets requests like POST /userprefix/Foo/Hello.

The other has a package declaration of "package Foo;" .. not matching the normal lower-case pattern, but a package declaration nonetheless. It describes some service, maybe "Bar" with a method "Goodbye". It gets requests like POST /userprefix/Foo/Bar/Goodbye.

How does a user route requests to those two services? Do they need to count the slashes? Peeking into the later parts of the URI path and only partially understanding them is an invitation for maintenance and security problems down the road.

Oof, this is an extremely good point, and might torpedo the / idea here. I'm going to remove the . -> / change from this commit.

Let's discuss in #86.

marioizquierdo · 2018-02-20T19:53:11Z

docs/spec_changelog.md

+
+```bnf
+**URL ::= Base-URL "/" [ Package "/" ] Service "/" Method**
+```


I'm not sure if there was already a discussion about using "." vs "/" for the package separator, but I am definitely not convinced that doing the . -> / change is a good thing. Let me list the advantages that I can see with each one:

Advantages of "/" separator:

Looks nicer when described like this: Base/Package/Service/Method.

It is different from gRPC routes. This was mentioned before as a pro, but I don't clearly see the advantage of this; only the very trained eye will be able to tell the difference at a glance, and I don't see the advantage of keeping a Twirp and a gRPC service both in the same Base-URL, only differentiated by the "." vs "/" syntax.

If there is no package or the package has no dots, and the Base-URL also has no dots, then the resulting URL will not have dots. Maybe this is easier to recognize by some middleware?

Advantages of "." separator:

Somewhat backwards-compatible. With the "/", service implementations will need to explicitly handle both new and old paths. With the "." this is not entirely true, because even if the service didn't support old routes, the user could still add the "/twirp" prefix on the Base-URL to deal with compatibility.

Since packages are optional, the "." makes it easier to tell if a URL has a package (blahblah.com/api/myserviceapi.CoolAPI/CreateStuff) or not (blahblah.com/api/CoolAPI/CreateStuff).

Avoids service path conflicts. For example, given the services Foo1 with package package and name Foo, and Foo2 with no package and the same name Foo. With the "." a URL like http://fooservice.com/package/Foo/Method is clearly mapping to Foo2 with the Base-URL http://fooservice.com/package, but with the "/" the same URL is ambiguous. This is probably an edge case because in most cases the user controls the Base-URL, but it is still possible.

I may be missing some other point (please comment!), but the advantages of the "/" separator look a little weak to me.

Excellent points, and I agree that we under-designed this. I think PR feedback is a tough place to do design discussion, though. Could you chime in on #86? You can just paste in what you have here :)

Ok I'll add this points to #86. Now I realize they are very similar to what @rhysh was talking about, but presented in a different order. It may help to add clarity to design decisions.

marioizquierdo · 2018-02-20T20:03:33Z

docs/spec_v6.md

+  `CalendarService`.
+* **Method** is the proto `rpc` name for an API method. For example,
+  `CreateEvent`.
+


😅 I retire the sentence "need to be parsed from right to left". Sorry that was misleading. The URLs can be parsed in multiple ways, one of them is by starting from the right, but it could also be removing the Base-URL first and then using a regexp, etc. What I mean is that one example will make it easier to understand the things that need to be consider when implementing a Twirp router, or just to understand the routes themselves.

mocax · 2018-02-20T21:32:01Z

For protos without package, we don't really need to care. It is extremely rare and should be highly discouraged. I don't think it would cause any real problem.

spenczar · 2018-02-21T00:06:31Z

@mocax, I don't think I agree. It may be extremely rare within Google, but it is not so rare in the wild. We need to make a spec sturdy enough that this sort of thing doesn't cause problems for our users.

We could require package be set (making it an even breakier change!) or we could back away and keep . as the delimiter. Let's discuss that in #86. For now, I'm going to change this PR to only remove the /twirp prefix.

We might end up using / as delimiter, but it needs to be more thoroughly designed. Design will take place in #88.

mocax · 2018-02-21T15:37:52Z

Given all the troubles for change . to / as the separator, I agree we should drop the idea. It doesn't seem to have enough real value for such a breaking change. I think we can move forward without it.

spenczar · 2018-02-27T20:54:06Z

@marioizquierdo and @rhysh, please take another look. I think I've addressed all concerns here.

Xe · 2018-02-27T22:17:42Z

docs/spec_v6.md

+}
+```
+
+### Error Codes


I very much like this section and how it's formatted

Xe · 2018-02-27T22:18:06Z

docs/spec_v6.md

+
+| Twirp Error Code    | HTTP Status | Description
+| ------------------- | ----------- | -----------
+| canceled            | 408 | The operation was cancelled.


Are these error codes sent as strings over the wire?

Yep, they are.

marioizquierdo

LGTM!

rhysh

LGTM after two protobuf syntax fixes. The other two nits are up to you.

rhysh · 2018-03-02T21:36:29Z

docs/spec_v6.md

+}
+
+message HelloRequest {
+  string message;


string message = 1;

rhysh · 2018-03-02T21:36:35Z

docs/spec_v6.md

+}
+
+message HelloResponse {
+  string message;


string message = 1;

rhysh · 2018-03-02T21:39:53Z

docs/spec_v6.md

+* **msg**: A human-readable message describing the error
+  as a string.
+* **meta**: (optional) An object with string values holding
+  arbitrary additional metadata describing the error.


Use of "arbitrary" here makes it sound like they don't need to be strings .. even though "string values" makes it clear that they need to be.

Consider s/metadata/string-formatted metadata/

rhysh · 2018-03-02T21:41:37Z

docs/spec_v6.md

+
+| Twirp Error Code    | HTTP Status | Description
+| ------------------- | ----------- | -----------
+| canceled            | 408 | The operation was cancelled.


The error code is canceled, which is the spelling the Go project uses. Consider s/cancelled/canceled/ in the description for consistency :)

Add draft of v6 specification

8d0d0a6

This is a prerelease draft to explain planned changes. The only change from v5 (so far) is a change of the URL scheme Twirp uses.

spenczar added this to the v6 milestone Feb 14, 2018

marioizquierdo suggested changes Feb 15, 2018

View reviewed changes

mocax reviewed Feb 15, 2018

View reviewed changes

spenczar mentioned this pull request Feb 15, 2018

Proposed new routing format: /package/service/method #55

Closed

spenczar added 7 commits February 19, 2018 09:38

Move spec changes to a changelog doc

be09b6e

Remove PROTOCOL_V6_DRAFT.md in favor of docs/spec_v6.md

f304123

Remove changelog from spec doc

460b0c4

De-duplicate overview in v6 spec draft

088a080

Permit any prefix in URLs

021ea2e

Set correct ID for spec changelog doc

a923021

Add spec v6 draft to sidebar of doc website

8f6cf8c

rhysh suggested changes Feb 20, 2018

View reviewed changes

marioizquierdo reviewed Feb 20, 2018

View reviewed changes

Leave . as delimiter between package and service

d71aa7d

We might end up using / as delimiter, but it needs to be more thoroughly designed. Design will take place in #88.

Xe reviewed Feb 27, 2018

View reviewed changes

docs/spec_v6.md

}

```

### Error Codes

Copy link

Xe Feb 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I very much like this section and how it's formatted

spenczar reacted with hooray emoji

Xe reviewed Feb 27, 2018

View reviewed changes

marioizquierdo approved these changes Mar 2, 2018

View reviewed changes

rhysh approved these changes Mar 2, 2018

View reviewed changes

Fix minor syntax issues in spec

51f8463

spenczar merged commit 500dc2d into master Mar 3, 2018

spenczar deleted the v6_spec_draft branch March 3, 2018 00:20

spenczar mentioned this pull request Jun 6, 2018

WIP: Version 6 #112

Closed

marioizquierdo mentioned this pull request Apr 12, 2019

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

Closed


		URL ::= Base-URL "/" [ Package "." ] Service "/" Method

		The "/twirp/" prefix is removed for three reasons:


		In v6, the "/twirp/" prefix is removed:

		URL ::= Base-URL "/" [ Package "." ] Service "/" Method

Add draft of v6 specification #80

Add draft of v6 specification #80

Conversation

spenczar commented Feb 14, 2018

spenczar commented Feb 14, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spenczar commented Feb 19, 2018

rhysh left a 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

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

mocax commented Feb 20, 2018 via email

spenczar commented Feb 21, 2018

mocax commented Feb 21, 2018 via email

spenczar commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marioizquierdo left a comment

Choose a reason for hiding this comment

rhysh left a 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