search

twitchtv / twirp

A simple RPC framework with protobuf service definitions
https://twitchtv.github.io/twirp/docs/intro.html
Apache License 2.0
7.2k stars 326 forks source link

Generated clients should set Accept header to right Content-Type #81

Closed wolfeidau closed 6 years ago

wolfeidau commented 6 years ago

I am road testing twirp with amazon api gateway and lambda, this service requires the Accept header as this triggers binary encoding of the response for application/protobuf.

I am happy to do a PR for this.

mocax commented 6 years ago

I agree.

spenczar commented 6 years ago

I can get behind this, sure. I think there are two design questions though:

  1. Errors are always application/json responses, even if the client sent a application/protobuf request. Should protobuf clients set Accept: application/protobuf; application/json to say they are prepared to handle JSON error responses?

I'm not totally sure here. I think I need to know more about how intermediaries like Amazon API Gateway would handle this Accept header, and more about how they'd handle passing through an application/json error message if that wasn't in the client's Accept set.

  1. Should this be in the spec?

I think no, it shouldn't. We can add it to the Go implementation because it makes things easier for people, and the Twirp spec doesn't forbid additional headers, but it's not actually required by Twirp for any processing, so we should keep things minimal.

@wolfeidau, in your testing, are you able to get JSON-encoded errors back when you set Accept to just application/protobuf?

wolfeidau commented 6 years ago

Great writeup, I decided to go for a walk to consider this.

  1. I think it should be clear to the server what the client accepts, so either application/protobuf or application/json. This is what I have observed with REST services, even if they specify XML/JSON they may receive any junk in the HTTP errors.

  2. Yeah it is good to be explicit, this helps services like AWS Gateway and various proxy ect understand what your sending through. If I was to add it to a spec it would be recommended not must.

In my testing with API Gateway the return data can be either protobuf or JSON / HTML errors when the accept is supplied.

API gateways like the AWS one is they want the exact Accept header declared for binary responses. To most it is binary, even if the response could be text in the case of any error, this is certainly the case with AWS API gateway as errors are also JSON.

This is how most of the image/file endpoints i have encountered also work, be clear in what you are asking for say image/gif and flexible in what you will process as a response. This is hopefully in line with https://en.wikipedia.org/wiki/Robustness_principle

spenczar commented 6 years ago

I'm convinced. I've merged your fix into a v5.3.0_prerelease branch; I'd like to include #76 in the same release. Once that lands, your fix will make it into master.

wolfeidau commented 6 years ago

As mentioned would it be worth adding this to the v6 specification as well?

rynop commented 6 years ago

@wolfeidau I'm using APIG Lambda proxy integration and Twirp v6 and APIG keeps returning a bas64 encoded string for an application/protobuf response.

How'd you get APIG to base64 decode the string? I swear I had this working a few months ago when I 1st tried this.

rynop commented 6 years ago

I got it figured out. Too much to type here. Q&A at this StackOverflow