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.

Sign up for GitHub

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

Jump to bottom

Mention Protobuf Style Guides on the v7 protocol #269

Merged
merged 3 commits into from
Sep 25, 2020
Merged

Mention Protobuf Style Guides on the v7 protocol #269

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
662f81b
Mention Protobuf Style Guides on the v7 protocol
marioizquierdo Sep 14, 2020
a124e14
Merge branch 'master' of github.com:twitchtv/twirp into protocol_rout…
marioizquierdo Sep 25, 2020
07d35cf
Improve docs on routing, leave spec clean
marioizquierdo Sep 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions docs/spec_v7.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,17 @@ the following components.
* **Method** is the proto `rpc` name for an API method. For example,
`CreateEvent`.

The package, service and method names are taken from the proto file
definition. The [Protobuf spec](https://developers.google.com/protocol-buffers/docs/reference/proto3-spec#identifiers)
allows to use CamelCased and underscored_names. However, Twirp
implementations usually need to map Service and Method names to
their specific naming conventions, which may cause naming collisions.
For example, Go requires exported variables to start with a capital,
so the names `MyMethod` and `myMethod` will map to the same interface method.
To avoid this problem, Twirp implementations should expect and fully support
names that follow the [Protobuf Style Guide](https://developers.google.com/protocol-buffers/docs/style#services),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this implying that Twirp implementations may not fully support names that do not follow these conventions? That is to say, if you skip these conventions you're doing so "at your own risk" of compatibility problems. If this is the case, I think a little stronger warning may be needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've been back and forth at length over this on #244

The issue is that, this is technically not a protocol problem, but the Go implementation does not respect the protocol, and looking closer it is not possible to entirely avoid naming collisions. This problem can be avoided by users if they respect the Protobuf Style Guide. And even if they don't it is usually not a problem because it is rare that someone uses potentially conflicting names like MyMethod and myMethod.

We started including the recommendation for users on the docs: https://github.com/twitchtv/twirp/blob/master/docs/routing.md#naming-style.

The message here in the protocol spec is for developers working on a new Twirp implementation, in case they are wondering how to ensure full compatibility with the spec and other language implementations. Which is what happened to @ofpiyush, that started this conversation in detail.

Does it make more sense? Maybe I could use different words to make that a little more clear?

Copy link
Contributor

@thesilentg thesilentg Sep 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If this is for developers working on a new Twirp implementation, I'd say give them the ability to treat names which do not follow the style guide in any way they desire (C-style Undefined Behavior). Something like "Twirp implementations MUST support names that follow the Protobuf Style Guide, and names which do not follow the Protobuf Style Guide MAY be modified by the implementation."

We should then update the Twirp user documentation to say that cross-language compatibility guarantees are only made if a user follows the Protobuf Style Guide for naming.

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue I (and other language implementations) ran into was with the wire protocol, not service and method names of generated code. Specifically, which URL should one serve and send requests to.

Service/ Method naming convention is only required for languages where case is meaningful. Go is the only language with this feature as far as I know.

In all other languages, one can just use the literal name for service names and methods in whatever format the protobuf file has, without any issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a few more notes in the routing documentation. The protocol can be left without details about the Go implementation, the Service and Method parts of the routes should match the definitions in the proto files exactly, and the Go implementation will soon be able to do this thanks to #257 and #277

in particular, Service and Method names that are CamelCased (with an initial capital).

### Requests

Twirp always uses HTTP POST method to send requests, because it
Expand Down