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

Implement Twirp protocol v7 #181

Closed
marioizquierdo opened this issue Oct 30, 2020 · 2 comments · Fixed by #182
Closed

Implement Twirp protocol v7 #181

marioizquierdo opened this issue Oct 30, 2020 · 2 comments · Fixed by #182

Comments

@marioizquierdo
Copy link

Changes required to implement the new protocol:

  • Error code resource_exhausted should map to 429 (instead of 403).
  • Allow any arbitrary URL prefixes instead of forcing the "/twirp".

See Protocol v7 differences and the Go implementation v7.0.0 release notes for details.

@ccmtaylor
Copy link
Member

thanks for getting in touch, @marioizquierdo! We were planning to implement v7 already, but it's good to have a tracking issue. Since you authored the v7 release notes, I'd appreciate your input on some of the Go implementation changes that aren't mentioned in the protocol differences. In particular:

twitchtv/twirp#271 Server JSON responses using EmitDefaults: responses include all fields, even if they have zero-values. This can be reverted to the previous behavior (skip zero-values) with the server option: twirp.WithServerJSONSkipDefaults(true).

It doesn't make a functional difference, but would you suggest that other twirp implementations adopt these changes, too?

twitchtv/twirp#257 Go services handle literal routes. Fixing part of the issue twitchtv/twirp#244, affecting cross-language communications when the proto definitions are not using the recommended Protobuf Style Guide.

Currently, twinagle clients and servers use what witchtv/twirp#257 calls "literal routes". E.g.

package pkg;
service foo {
  rpc bar(Request) returns (Response);
}

will result in an HTTP POST to /twirp/pkg.foo/bar, and the server side only accepts these routes, too. Would you recommend any changes here?

@marioizquierdo
Copy link
Author

Sure.

While the other changes included in the Go v7.0.0 release are not necessarily changing the protocol, it is nice to keep an eye on them because there may be similar implementation details here that could improve as well.

JSON with emit_defaults: twitchtv/twirp#271 addresses the common confusion when using the API from cURL or another way to manually debug/inspect the API. Including zero values makes those responses less confusing. It doesn't affect the protocol because Twirp clients in any language use protobuf-json de-serialization, which works either way with or without default values included in the response. It is a good thing to implement. I also maintain the twirp-ruby implementation and added this in the PR arthurnn/twirp-ruby#57 because it is easy to do (with most proto implementations) and is backwards compatible. In the Ruby implementation I didn't add an option to revert to old behavior (if needed, I can add that later).

Literal routes: twitchtv/twirp#257 is fixing an issue on the Go client. If the twinagle implementation here is already handling literal routes, then it is already implementing the protocol properly 👍 . You would have a problem only if someone calls your twinagle server with a Go client generated with an older version, and only then it would be a problem if their proto names didn't follow naming best practices, in that case the older Go client would send CamelCased routes, that would probably cause a 404 bad_route error. If this happens now, probably the best path forward is to tell them to upgrade the Go generator and use the client option WithClientLiteralURLs(true) to make sure they also implement the protocol properly 👍

In a nutshell, to incorporate the other v7 changes in the Go implementation, I would change JSON responses to use emit_defaults: true (or whatever the Scala equivalent), and nothing else.

ccmtaylor added a commit that referenced this issue Nov 11, 2020
Implement Twirp protocol v7

* update HTTP status code mapping for resource_exhausted error
* make `/twirp` HTTP path prefix configurable
* json: include default values in server responses

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

Successfully merging a pull request may close this issue.

2 participants