mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 37403: Using AcceptMediaType Request method to validate Accept header
Date Thu, 13 Aug 2015 18:57:39 GMT

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

Ship it!


Some minor comments and noticed that we were not setting the response content type! I'll get
these cleaned up and commit


src/tests/http_api_tests.cpp (line 41)
<https://reviews.apache.org/r/37403/#comment150168>

    Why the move?



src/tests/http_api_tests.cpp (line 563)
<https://reviews.apache.org/r/37403/#comment150172>

    How about 'NotAcceptable'?



src/tests/http_api_tests.cpp (line 572)
<https://reviews.apache.org/r/37403/#comment150169>

    No need for the std:: prefix here?



src/tests/http_api_tests.cpp (line 575)
<https://reviews.apache.org/r/37403/#comment150186>

    It's pretty implicit but http::streaming::post is already being passed the content type
and will overwrite it, so this doesn't do anything.



src/tests/http_api_tests.cpp (line 576)
<https://reviews.apache.org/r/37403/#comment150171>

    How about we write a valid format? e.g. text/html
    
    Note that these are supposed to be formatted as type/subtype



src/tests/http_api_tests.cpp (line 606)
<https://reviews.apache.org/r/37403/#comment150187>

    This get overwritten to the parameterized content type when we pass 'contentType' into
http::streaming::post. Otherwise this test would fail because we encode as either PROTOBUF
or JSON depending on the test parameter.



src/tests/http_api_tests.cpp (line 660)
<https://reviews.apache.org/r/37403/#comment150188>

    In all of these tests, how about we also validate the content type coming back in the
response? Seems an important thing to test for 'Accept' working correctly.
    
    In particular, these don't test the behavior that JSON is the default?
    
    By adding this, I found a bug :) We aren't setting the content type of the response!


- Ben Mahler


On Aug. 12, 2015, 11:12 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37403/
> -----------------------------------------------------------
> 
> (Updated Aug. 12, 2015, 11:12 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2497
>     https://issues.apache.org/jira/browse/MESOS-2497
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Using AcceptMediaType Request method to validate request 'Accept' HTTP header in Scheduler
HTTP API
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 579c009 
>   src/tests/http_api_tests.cpp aef3c4b 
> 
> Diff: https://reviews.apache.org/r/37403/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message