Re: Review Request 67414: Added default message bodies to libprocess HTTP error responses.

[Alexander Rukletsov <ru...@gmail.com>]

> On June 5, 2018, 10:46 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/include/process/http.hpp
> > Line 701 (original), 701-702 (patched)
> > <https://reviews.apache.org/r/67414/diff/1/?file=2034492#file2034492line701>
> >
> >     Why not using `process::http::Status::string()`  https://github.com/apache/mesos/blob/2198b961d24b788564d36490cf52f78d7ec07655/3rdparty/libprocess/src/http.cpp#L176-L180 instead?
> >     
> >     Maybe then you can even put a single line into `Response` c-tor instead of modifying individual classes.
> 
> Benno Evers wrote:
>     I didn't know about this function, but looking at it it seems to be in direct violation of our style guide at (https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables) so we should probably look into deprecating it rather than promoting further usage.
>     
>     But even w/o that issue, I'm not sure that
>     
>     ```
>       BadRequest()
>         : Response(Status::string(Status::BAD_REQUEST), Status::BAD_REQUEST) {}
>     ```
>     
>     is better than
>     
>     ```
>       BadRequest()
>         : Response("400 Bad Request.", Status::BAD_REQUEST) {}
>     ```
>     
>     
>     I dont think putting it in the default constructor of `Response` is a good idea, because we more or less endorse constructing responses like this
>     
>     ```
>       process::http::OK response;
>       response.type = response.PATH;
>       response.path = path;
>       response.headers["Content-Type"] = "application/octet-stream";
>       response.headers["Content-Disposition"] =
>         strings::format("attachment; filename=%s", path).get();
>     ```
>     
>     and we would need to audit all usages of such responses to ensure they're not confused by suddenly having an additional body.
> 
> Alexander Rukletsov wrote:
>     What I don't like about the current approach is the duplication: we already have `code -> string` mapping in http.cpp, it would be good to reuse it here so that they don't go out of sync.
> 
> Benno Evers wrote:
>     I guess I don't really see why it would be a problem if the two go out of sync - many pages have very elaborate 404 error pages including images, animations and lots of text, so I don't think any tool will have the assumption that the response body equals the stringified HTTP status code.

Discussed with Benno offline. I'm convinced that the string does not have to be auto generated, so I'm dropping the issue.


- Alexander


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67414/#review204313
-----------------------------------------------------------


On June 1, 2018, 3:14 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67414/
> -----------------------------------------------------------
> 
> (Updated June 1, 2018, 3:14 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-8999
>     https://issues.apache.org/jira/browse/MESOS-8999
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> By default on error libprocess would only return a response
> with the correct status code and no response body.
> 
> However, most browsers do not visually indicate the response
> status code, making it hard for the user to figure out what
> exactly the problem was.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/http.hpp 055447e13117c0a3ba79d0fc326ece657e8f064f 
> 
> 
> Diff: https://reviews.apache.org/r/67414/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>