You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Isabel Jimenez <co...@isabeljimenez.com> on 2015/08/12 19:50:03 UTC
Review Request 37403: Using AcceptMediaType Request method to validate
Accept header
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37403/
-----------------------------------------------------------
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
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
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
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
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
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> 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
>
>
Re: Review Request 37403: Using AcceptMediaType Request method to
validate Accept header
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> 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
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
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
>
>