Set headers when writing responses to properly return status code. #99
Conversation
src/api_server.cc
Outdated
| LOG(WARNING) << "No matching resource for " << id; | ||
| } | ||
| std::map<std::string, std::string> headers = { | ||
| {"Content-Type", "text/plain"}, |
There was a problem hiding this comment.
I've chosen to use text/plain here as we're not actually returning JSON. If we decide we'd like to return an empty object, we can update this accordingly.
There was a problem hiding this comment.
Sounds good. Should we also write "Not found\n" into the response?
There was a problem hiding this comment.
Personally, I believe that rest APIs should always return JSON, so I would vote for something like {"error:"Not found"}, but I'm not sure that would play well with our current design. WDYT?
There was a problem hiding this comment.
Yeah. I've found some examples that also add "code":404. I'm happy with either one -- we are, in effect, designing this API. Let's do it!
src/api_server.cc
Outdated
| }; | ||
|
|
||
| conn->set_status(HttpServer::connection::ok); | ||
| conn->set_headers(headers); |
There was a problem hiding this comment.
Would the following work:
conn->set_headers({
{"Content-Type", "application/json"},
});? Also below.
There was a problem hiding this comment.
I've inlined it, using the syntax the way you had it resulted in the error below. Is this a happy medium?
api_server.cc:53:8: error: no matching function for call to 'boost::network::http::async_connection<boost::network::http::tags::http_server, google::MetadataApiServer::Handler>::set_headers(<brace-enclosed initializer list>)'
There was a problem hiding this comment.
Yeah, wrapping it into an std::map temporary is exactly the right fix for that. Sorry I didn't check this before suggesting.
src/api_server.cc
Outdated
| LOG(WARNING) << "No matching resource for " << id; | ||
| } | ||
| std::map<std::string, std::string> headers = { | ||
| {"Content-Type", "text/plain"}, |
There was a problem hiding this comment.
Sounds good. Should we also write "Not found\n" into the response?
supriyagarg
left a comment
supriyagarg
left a comment
There was a problem hiding this comment.
Agree with Igor's comments. Otherwise LGTM
src/api_server.cc
Outdated
| }; | ||
|
|
||
| conn->set_status(HttpServer::connection::ok); | ||
| conn->set_headers(headers); |
There was a problem hiding this comment.
Yeah, wrapping it into an std::map temporary is exactly the right fix for that. Sorry I didn't check this before suggesting.
src/api_server.cc
Outdated
| LOG(WARNING) << "No matching resource for " << id; | ||
| } | ||
| std::map<std::string, std::string> headers = { | ||
| {"Content-Type", "text/plain"}, |
There was a problem hiding this comment.
Yeah. I've found some examples that also add "code":404. I'm happy with either one -- we are, in effect, designing this API. Let's do it!
src/api_server.cc
Outdated
| {"Content-Type", "text/plain"}, | ||
| })); | ||
| json::value json_response = json::object({ | ||
| {"status_code", json::int(404)}, |
There was a problem hiding this comment.
I don't think there's a json::int. Did you mean json::number?
src/api_server.cc
Outdated
| } | ||
| conn->set_status(HttpServer::connection::not_found); | ||
| conn->set_headers(std::map<std::string, std::string>({ | ||
| {"Content-Type", "text/plain"}, |
There was a problem hiding this comment.
This is no longer true, right?
3 participants
This was introduced in #78.