Re: Review Request 37403: Using AcceptMediaType Request method to validate Accept header

2015-08-13 Thread Ben Mahler 

---
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

Re: Review Request 37403: Using AcceptMediaType Request method to validate Accept header

2015-08-12 Thread Isabel Jimenez 

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

(Updated Aug. 12, 2015, 8:29 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 (updated)
-

  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

Re: Review Request 37403: Using AcceptMediaType Request method to validate Accept header

2015-08-12 Thread Isabel Jimenez 

---
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 (updated)
-

  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

Re: Review Request 37403: Using AcceptMediaType Request method to validate Accept header

2015-08-12 Thread Isabel Jimenez 


 On Aug. 12, 2015, 6:31 p.m., Anand Mazumdar wrote:
  src/master/http.cpp, line 395
  https://reviews.apache.org/r/37403/diff/1/?file=1038481#file1038481line395
 
  This would crash if there was no accept header specified ?

No this can only enter the if with an accept header


- Isabel


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


On Aug. 12, 2015, 5:50 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, 5:50 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

Re: Review Request 37403: Using AcceptMediaType Request method to validate Accept header

2015-08-12 Thread Anand Mazumdar 

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


Mainly comments around us having Accept header validations for all types of 
calls and not just Subscribe.


src/master/http.cpp (line 383)
https://reviews.apache.org/r/37403/#comment149957

Remove this TODO now.



src/master/http.cpp (line 384)
https://reviews.apache.org/r/37403/#comment149959

How about ?

// Default to sending back JSON.
ContentType responseContentType = ContentType::JSON;

if(request.acceptsMediaType(protobuf) {
  responseContent = ContentType::PROTOBUF;
} else if(request.acceptsMediaType(json)) {
  responseContent = ContentType::JSON;
} else {
  // return error
}



src/master/http.cpp (line 391)
https://reviews.apache.org/r/37403/#comment149960

Check my earlier comment on how we can simplify this. Also , we need to do 
Accept header validations for ALL call types and not just subscribe.



src/master/http.cpp (line 394)
https://reviews.apache.org/r/37403/#comment149970

How about just:

Unsupported 'blah_blah' media type in 'Accept' request-header. The 
supported media types are 'application/json' and 'application/x-protobuf' ?



src/master/http.cpp (line 395)
https://reviews.apache.org/r/37403/#comment149969

This would crash if there was no accept header specified ?



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

Add another test-case when there is no accept header set. We should not 
receive a error and response should be OK

Also add another test-case for some other call other then subscribe.

When accept header is */*



src/tests/http_api_tests.cpp (line 567)
https://reviews.apache.org/r/37403/#comment149962

nit : new line



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

Remove this comment, it's not needed only for subscribe type



src/tests/http_api_tests.cpp (line 581)
https://reviews.apache.org/r/37403/#comment149964

kill the new line, have this block with the earlier line and then put a new 
line before string body;



src/tests/http_api_tests.cpp (line 583)
https://reviews.apache.org/r/37403/#comment149965

use the serialize function already defined earlier in the text fixture.


- Anand Mazumdar


On Aug. 12, 2015, 5:50 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, 5:50 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

Re: Review Request 37403: Using AcceptMediaType Request method to validate Accept header

2015-08-12 Thread Isabel Jimenez 


 On Aug. 12, 2015, 6:31 p.m., Anand Mazumdar wrote:
  src/master/http.cpp, line 391
  https://reviews.apache.org/r/37403/diff/1/?file=1038481#file1038481line391
 
  Check my earlier comment on how we can simplify this. Also , we need to 
  do Accept header validations for ALL call types and not just subscribe.

I'm adding a TODO to move this once we start encoding error messages. In the 
meantime we shouldn't enforce an 'accept' header to clients if we're sending 
back for any other type of call 202's or error code with text error messages.


 On Aug. 12, 2015, 6:31 p.m., Anand Mazumdar wrote:
  src/master/http.cpp, line 394
  https://reviews.apache.org/r/37403/diff/1/?file=1038481#file1038481line394
 
  How about just:
  
  Unsupported 'blah_blah' media type in 'Accept' request-header. The 
  supported media types are 'application/json' and 'application/x-protobuf' ?

This format was decided in a previous review: see 
https://reviews.apache.org/r/36037/ for details


- Isabel


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


On Aug. 12, 2015, 5:50 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, 5:50 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