Expose WriteMessage for middleware to return proto message #9
Conversation
… it in WriteMessage
prottp.go
Outdated
| var err error | ||
| isJSON := isRequestJSON(r) | ||
| if isJSON && w.Header().Get("Content-Type") == "" { |
There was a problem hiding this comment.
Since you're changing this, how about updating the headers:
Request
Content-Type: X: "I am sending you X"
Accept: Y: "Please respond with Y"
Response:
Content-Type: X: "I am sending you X"
So when reading the request body, look at Content-Type to decide if the body is PB or JSON. When writing the response body, look at Accept to see whether the client wants PB or JSON, and set Content-Type to the format that the client asked for.
Also, if it's simple, it would be good to include the protobuf name in the content type. For JSON:
Content-Type: application/json;<package name.message name>
and for PB:
Content-Type: application/x.prottp;<package name.message name>
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept
to fully implement accept specification seems quite complicated.
There was a problem hiding this comment.
to fully implement accept specification
Ah, sorry, I wasn't really suggesting implementing full support, e.g. q-factor weighting. Just a simple version that can handle either application/json or application/x.prottp.
There was a problem hiding this comment.
to fully implement accept specification seems quite complicated.
It's odd that it's not in the standard library. Accept-Language uses the same format...
|
@bodhi PTAL |
example/example_test.go
Outdated
| }, | ||
| ExpectedHeadersDump: `HTTP/1.1 200 OK | ||
| Content-Length: 31 | ||
| Content-Type: application/x.prottp;<*example.SearchResponse> |
There was a problem hiding this comment.
Sorry, another unclear description on my part.
With
Content-Type: application/json;<package name.message name>
<> were intended as placeholders, not as part of the content.
Also, I was wrong about the "parameter" (bit after ;), it needs to be <key>=<value> (and key cannot start with q...).
So instead of this:
Content-Type: application/x.prottp;<*example.SearchResponse>
I was asking for it to be this:
Content-Type: application/x.prottp;name=example.SearchResponse
I'm flexible with name. Could be messageName, ms, type, message, etc...
And it looks like you can get the message name via proto.MessageName so that would be better than using the Go type name. Especially since messages are pointers!
| var err error | ||
| var isJSON = isMimeTypeJSON(w.Header().Get("Content-Type")) | ||
| if w.Header().Get("Content-Type") == "" { | ||
| isJSON = shouldReturnJSON(r) |
There was a problem hiding this comment.
The way you use these conditionals seems a bit weird. I don't really follow why there are several checks about the content type header and isJson.
There was a problem hiding this comment.
var isJSON = isMimeTypeJSON(w.Header().Get("Content-Type"))
if w.Header().Get("Content-Type") == "" {is for other middleware can set the Content-Type, in that case, we use their settings
There was a problem hiding this comment.
situations:
- Middleware set the ContentType by using
w.Header().Set()to JSON, prottp should return JSON - HTTP Header Accept is JSON, prottp should return JSON
- HTTP Request Content-Type is JSON, prottp should return JSON
prottp.go
Outdated
| if isJSON { | ||
| w.Header().Set("Content-Type", fmt.Sprintf("%s;type=%s", jsonContentType, proto.MessageName(msg))) | ||
| } else { | ||
| w.Header().Set("Content-Type", fmt.Sprintf("%s;type=%s", xprottpContentType, proto.MessageName(msg))) |
There was a problem hiding this comment.
Minimise the code inside the conditionals from "set the content type header to the content type and the name of the message" to the actual difference: "figure out the content type", then below that, do the common work:
-if isJSON {
- w.Header().Set("Content-Type", fmt.Sprintf("%s;type=%s", jsonContentType, proto.MessageName(msg)))
-} else {
- w.Header().Set("Content-Type", fmt.Sprintf("%s;type=%s", xprottpContentType, proto.MessageName(msg)))
-}
+contentType := xprottpContentType
+if isJSON {
+ contentType = jsonContentType
+}
+w.Header().Set("Content-Type", fmt.Sprintf("%s;type=%s", contentType, proto.MessageName(msg)))|
|
||
| if isJson { | ||
| // start write body | ||
| if isJSON { |
There was a problem hiding this comment.
Is there any reason why this check of isJSON can't be unified with the check above?
There was a problem hiding this comment.
Can's start writing body, with out first w.WriteHeader write headers.
No description provided.