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/07/10 22:55:52 UTC
Review Request 36402: Adding 'Accept' header in request
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding a method for Accept header in request + refactor of Accept-Encoding
Diffs
-----
3rdparty/libprocess/include/process/http.hpp 72b6d27
3rdparty/libprocess/src/encoder.hpp c5ff761
3rdparty/libprocess/src/http.cpp d168579
3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
Diff: https://reviews.apache.org/r/36402/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review91859
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36402]
All tests passed.
- Mesos ReviewBot
On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 15, 2015, 11:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On Aug. 3, 2015, 9:38 p.m., Ben Mahler wrote:
> > Is this ready to review? Mind updating the 'issues' accordingly?
Just did, thanks for comments :)
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review93970
-----------------------------------------------------------
On Aug. 4, 2015, 5:42 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 5:42 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review93970
-----------------------------------------------------------
Is this ready to review? Mind updating the 'issues' accordingly?
- Ben Mahler
On Aug. 3, 2015, 8:19 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2015, 8:19 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On Aug. 10, 2015, 7:03 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 219-220
> > <https://reviews.apache.org/r/36402/diff/10/?file=1032819#file1032819line219>
> >
> > I'm not sure I understand why you do this.
> > Can you please comment or explain?
Here we introduce two other strings we want to make sure we use to validate what the client is sending in its header.
If the client sends '*/*' and we want to make sure he accepts 'foo/bar', but we only compare 'foo/bar' with '*/*' we will reject a valid header.
So we introduce what we are expecting the client to send as a valid header so we can compare both and send back true when the client is sending '*/*' and we compare it first with 'foo/bar' then with 'foo/*' then finally with '*/*'. This means the client accepts anything ('foo/bar' included) so we validate the header.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94781
-----------------------------------------------------------
On Aug. 5, 2015, 7:11 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2015, 7:11 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94781
-----------------------------------------------------------
Ship it!
Maybe the code around checking for acceptable headers would benefit from using a RegEx instead of "parsing by hand" - but apart from that, LGTM.
3rdparty/libprocess/src/http.cpp (lines 213 - 215)
<https://reviews.apache.org/r/36402/#comment149377>
you can just use `strings.trim()` here
3rdparty/libprocess/src/http.cpp (lines 219 - 220)
<https://reviews.apache.org/r/36402/#comment149381>
I'm not sure I understand why you do this.
Can you please comment or explain?
- Marco Massenzio
On Aug. 5, 2015, 7:11 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2015, 7:11 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94981
-----------------------------------------------------------
Ship it!
Mostly some minor comments, I'll make the adjustments and land this for you!
3rdparty/libprocess/src/http.cpp (lines 220 - 221)
<https://reviews.apache.org/r/36402/#comment149725>
Should we do this the same way in both acceptsEncoding and acceptsMediaType? Note that trim only strips from the front and back, so we should be stripping after we've tokenized on ';'. Also, strings::pairs will result in keys that contain whitespace as well with this approach.
I'd suggest we keep the whitespace removal the same as acceptsEncoding for now.
3rdparty/libprocess/src/http.cpp (line 226)
<https://reviews.apache.org/r/36402/#comment149727>
Might be nice to avoid the implicit relyance on tokens being non-empty here as well.
3rdparty/libprocess/src/tests/http_tests.cpp (lines 661 - 669)
<https://reviews.apache.org/r/36402/#comment149718>
These should all use ; to delimit the q value rather than ,
3rdparty/libprocess/src/tests/http_tests.cpp (line 672)
<https://reviews.apache.org/r/36402/#comment149716>
Let's put this in the loop where it's used?
3rdparty/libprocess/src/tests/http_tests.cpp (lines 675 - 677)
<https://reviews.apache.org/r/36402/#comment149717>
How about:
```
EXPECT_FALSE(request.acceptsMediaType("test/*"))
<< "Not expecting '" << accept << "' to match 'test/*'";
```
3rdparty/libprocess/src/tests/http_tests.cpp (lines 682 - 693)
<https://reviews.apache.org/r/36402/#comment149720>
Ditto here, q values are delimited by semi colons, it looks like this test treats q values as possible accept types.
3rdparty/libprocess/src/tests/http_tests.cpp (lines 698 - 700)
<https://reviews.apache.org/r/36402/#comment149723>
How about:
```
EXPECT_TRUE(request.acceptsMediaType("text/html"))
<< "Expecting '" << accept << "' to match 'text/html'";
```
- Ben Mahler
On Aug. 10, 2015, 9:52 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 9:52 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/
-----------------------------------------------------------
(Updated Aug. 10, 2015, 9:52 p.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding a method for Accept header in request + refactor of Accept-Encoding
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp b8d9300
3rdparty/libprocess/src/http.cpp 4dcbd74
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
Diff: https://reviews.apache.org/r/36402/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94321
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37097, 36402]
All tests passed.
- Mesos ReviewBot
On Aug. 5, 2015, 7:11 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2015, 7:11 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/
-----------------------------------------------------------
(Updated Aug. 5, 2015, 7:11 p.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding a method for Accept header in request + refactor of Accept-Encoding
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp b8d9300
3rdparty/libprocess/src/http.cpp 4dcbd74
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
Diff: https://reviews.apache.org/r/36402/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94203
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37097, 36402]
All tests passed.
- Mesos ReviewBot
On Aug. 5, 2015, 12:32 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2015, 12:32 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/
-----------------------------------------------------------
(Updated Aug. 5, 2015, 12:32 a.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding a method for Accept header in request + refactor of Accept-Encoding
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp b8d9300
3rdparty/libprocess/src/http.cpp 4dcbd74
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
Diff: https://reviews.apache.org/r/36402/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/
-----------------------------------------------------------
(Updated Aug. 5, 2015, 12:24 a.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding a method for Accept header in request + refactor of Accept-Encoding
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp b8d9300
3rdparty/libprocess/src/http.cpp 4dcbd74
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
Diff: https://reviews.apache.org/r/36402/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/
-----------------------------------------------------------
(Updated Aug. 4, 2015, 11:05 p.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding a method for Accept header in request + refactor of Accept-Encoding
Diffs
-----
3rdparty/libprocess/include/process/http.hpp b8d9300
3rdparty/libprocess/src/http.cpp 4dcbd74
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
Diff: https://reviews.apache.org/r/36402/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/
-----------------------------------------------------------
(Updated Aug. 4, 2015, 11:05 p.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding a method for Accept header in request + refactor of Accept-Encoding
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp b8d9300
3rdparty/libprocess/src/http.cpp 4dcbd74
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
Diff: https://reviews.apache.org/r/36402/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94112
-----------------------------------------------------------
3rdparty/libprocess/src/http.cpp (line 155)
<https://reviews.apache.org/r/36402/#comment148623>
This will not crash, tokenize will always return a vector of at least one if one of the parameter strings is not empty and in this case the second one is always ";".
(https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp#L208)
- Isabel Jimenez
On Aug. 4, 2015, 5:42 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 5:42 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > Looking better! Can you please pull out the fixes to 'acceptsEncoding' into a separate review? Also, would like to see tests for the cases that weren't handled correctly (e.g. "gzipp;q=1.0" should not match "gzip"). Pulling the changes apart helps avoid extra round-trips on reviews (see here: http://mesos.apache.org/documentation/latest/effective-code-reviewing/) :)
Just added the dependency patch in the description of this RR.
> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 158-160
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line158>
> >
> > We don't need to use pairs anymore, right? Looks like we should just call tokenize again on 'tokens[1]' (if present).
I don't think there's a real difference. Is this change really necessary?
> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 220-222
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line220>
> >
> > Ditto here, can we tokenize 'tokens[1]' instead of using strings::pairs?
See comment above.
> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, line 236
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line236>
> >
> > newline here?
Added.
> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, line 693
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028799#file1028799line693>
> >
> > Mind moving this above with the other http tests?
Moved.
> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 696-703
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028799#file1028799line696>
> >
> > We don't generally wrap things this far, can you just do the following?
> >
> > ```
> > vector<string> wrongHeaders = {
> > "test,q=0.0",
> > ...
> > };
> > ```
> >
> > Ditto below.
Changed style.
> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 709-710
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028799#file1028799line709>
> >
> > Ditto here, can you just print 'accept'?
Printing the exact case where it failed may help later to debug in case of failure.
> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, line 706
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028799#file1028799line706>
> >
> > 'auto' here isn't buying us much over just 'const string&', we try to use 'auto' where it helps readability
> >
> > Also please use foreach for now
I don't see why it will not help with readability.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94105
-----------------------------------------------------------
On Aug. 4, 2015, 11:05 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 11:05 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, line 219
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line219>
> >
> > This will crash the program if tokens is empty!
>
> Isabel Jimenez wrote:
> This will not crash, tokenize will always return a vector of at least one if one of the parameter strings is not empty and in this case the second one is always ";".
> (https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp#L208)
>
> Ben Mahler wrote:
> That is a test of strings::split, here is a tokenize test which returns an empty vector:
>
> https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp#L163
>
> Isabel Jimenez wrote:
> Right, my bad, I'll check that accept.get() does not return an empty string then.
Actually, I'm sorry I got confused by my mistake on the example test. It still won't crash since it will never enter the foreach with an empty content.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94105
-----------------------------------------------------------
On Aug. 4, 2015, 5:42 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 5:42 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, line 156
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line156>
> >
> > This will crash the program if tokens is empty!
Please see comment below.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94105
-----------------------------------------------------------
On Aug. 4, 2015, 5:42 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 5:42 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Ben Mahler <be...@gmail.com>.
> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, line 219
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line219>
> >
> > This will crash the program if tokens is empty!
>
> Isabel Jimenez wrote:
> This will not crash, tokenize will always return a vector of at least one if one of the parameter strings is not empty and in this case the second one is always ";".
> (https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp#L208)
That is a test of strings::split, here is a tokenize test which returns an empty vector:
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp#L163
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94105
-----------------------------------------------------------
On Aug. 4, 2015, 5:42 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 5:42 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, line 219
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line219>
> >
> > This will crash the program if tokens is empty!
>
> Isabel Jimenez wrote:
> This will not crash, tokenize will always return a vector of at least one if one of the parameter strings is not empty and in this case the second one is always ";".
> (https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp#L208)
>
> Ben Mahler wrote:
> That is a test of strings::split, here is a tokenize test which returns an empty vector:
>
> https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp#L163
Right, my bad, I'll check that accept.get() does not return an empty string then.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94105
-----------------------------------------------------------
On Aug. 4, 2015, 5:42 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 5:42 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, line 219
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line219>
> >
> > This will crash the program if tokens is empty!
This will not crash, tokenize will always return a vector of at least one if one of the parameter strings is not empty and in this case the second one is always ";".
(https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp#L208)
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94105
-----------------------------------------------------------
On Aug. 4, 2015, 5:42 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 5:42 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94105
-----------------------------------------------------------
Looking better! Can you please pull out the fixes to 'acceptsEncoding' into a separate review? Also, would like to see tests for the cases that weren't handled correctly (e.g. "gzipp;q=1.0" should not match "gzip"). Pulling the changes apart helps avoid extra round-trips on reviews (see here: http://mesos.apache.org/documentation/latest/effective-code-reviewing/) :)
3rdparty/libprocess/include/process/http.hpp (lines 119 - 126)
<https://reviews.apache.org/r/36402/#comment148611>
Let's keep the RFC reference for the implementation, mind moving it back?
Also, please remove my TODO for now, we can see how Request evolves over time.
3rdparty/libprocess/src/http.cpp (line 146)
<https://reviews.apache.org/r/36402/#comment148609>
Why the change here? Should still say 'encoding' right?
3rdparty/libprocess/src/http.cpp (line 155)
<https://reviews.apache.org/r/36402/#comment148607>
This will crash the program if tokens is empty!
3rdparty/libprocess/src/http.cpp (line 156)
<https://reviews.apache.org/r/36402/#comment148620>
The style checker doesn't like this
3rdparty/libprocess/src/http.cpp (lines 157 - 159)
<https://reviews.apache.org/r/36402/#comment148610>
We don't need to use pairs anymore, right? Looks like we should just call tokenize again on 'tokens[1]' (if present).
3rdparty/libprocess/src/http.cpp (line 218)
<https://reviews.apache.org/r/36402/#comment148608>
This will crash the program if tokens is empty!
3rdparty/libprocess/src/http.cpp (lines 219 - 221)
<https://reviews.apache.org/r/36402/#comment148612>
Ditto here, can we tokenize 'tokens[1]' instead of using strings::pairs?
3rdparty/libprocess/src/http.cpp (line 235)
<https://reviews.apache.org/r/36402/#comment148619>
newline here?
3rdparty/libprocess/src/tests/http_tests.cpp (line 693)
<https://reviews.apache.org/r/36402/#comment148613>
Mind moving this above with the other http tests?
3rdparty/libprocess/src/tests/http_tests.cpp (lines 696 - 703)
<https://reviews.apache.org/r/36402/#comment148614>
We don't generally wrap things this far, can you just do the following?
```
vector<string> wrongHeaders = {
"test,q=0.0",
...
};
```
Ditto below.
3rdparty/libprocess/src/tests/http_tests.cpp (line 706)
<https://reviews.apache.org/r/36402/#comment148615>
'auto' here isn't buying us much over just 'const string&', we try to use 'auto' where it helps readability
Also please use foreach for now
3rdparty/libprocess/src/tests/http_tests.cpp (lines 709 - 710)
<https://reviews.apache.org/r/36402/#comment148618>
Ditto here, can you just print 'accept'?
3rdparty/libprocess/src/tests/http_tests.cpp (line 727)
<https://reviews.apache.org/r/36402/#comment148616>
Ditto here, 'auto' doesn't seem very helpful to readability here.
Also, please use foreach for now.
3rdparty/libprocess/src/tests/http_tests.cpp (lines 729 - 731)
<https://reviews.apache.org/r/36402/#comment148617>
Can you just print out 'accept' rather than grabbing the header and calling .get()?
- Ben Mahler
On Aug. 4, 2015, 5:42 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 5:42 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/
-----------------------------------------------------------
(Updated Aug. 4, 2015, 5:42 p.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding a method for Accept header in request + refactor of Accept-Encoding
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp b8d9300
3rdparty/libprocess/src/http.cpp 4dcbd74
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
Diff: https://reviews.apache.org/r/36402/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94019
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36402]
All tests passed.
- Mesos ReviewBot
On Aug. 3, 2015, 8:19 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2015, 8:19 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/
-----------------------------------------------------------
(Updated Aug. 3, 2015, 8:19 p.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding a method for Accept header in request + refactor of Accept-Encoding
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp b8d9300
3rdparty/libprocess/src/http.cpp 4dcbd74
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5
Diff: https://reviews.apache.org/r/36402/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On July 16, 2015, 12:10 a.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, line 126
> > <https://reviews.apache.org/r/36402/diff/3/?file=1012922#file1012922line126>
> >
> > Whoops ! Let's return false as earlier. Also add a test-case for empty check. ( Accept-Encoding )
>
> Isabel Jimenez wrote:
> Added a TODO since it's not standard to return false here.
>
> Anand Mazumdar wrote:
> Looks like I am missing something here. This is a method for Accept-Encoding. If the header is not present and you pass "gzip" as argument to the method , it should return false as was the case earlier since the client can't accept "gzip" ( gzip is a bad example here owing to the exception in the rfc around it/compress). Did you get confused with it being the "Accept" header ?
>
> Re-opening the issue for now. I think we can get rid of the TODO here.
`Accept` and `Accept-encoding` have the same behavior in this case. From RFC 14.3 Accept-Encoding : If no Accept-Encoding field is present in a request, the server MAY assume that the client will accept any content coding.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review91843
-----------------------------------------------------------
On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 15, 2015, 11:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Anand Mazumdar <ma...@gmail.com>.
> On July 16, 2015, 12:10 a.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, line 126
> > <https://reviews.apache.org/r/36402/diff/3/?file=1012922#file1012922line126>
> >
> > Whoops ! Let's return false as earlier. Also add a test-case for empty check. ( Accept-Encoding )
>
> Isabel Jimenez wrote:
> Added a TODO since it's not standard to return false here.
Looks like I am missing something here. This is a method for Accept-Encoding. If the header is not present and you pass "gzip" as argument to the method , it should return false as was the case earlier since the client can't accept "gzip" ( gzip is a bad example here owing to the exception in the rfc around it/compress). Did you get confused with it being the "Accept" header ?
Re-opening the issue for now. I think we can get rid of the TODO here.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review91843
-----------------------------------------------------------
On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 15, 2015, 11:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Anand Mazumdar <ma...@gmail.com>.
> On July 16, 2015, 12:10 a.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, line 126
> > <https://reviews.apache.org/r/36402/diff/3/?file=1012922#file1012922line126>
> >
> > Whoops ! Let's return false as earlier. Also add a test-case for empty check. ( Accept-Encoding )
>
> Isabel Jimenez wrote:
> Added a TODO since it's not standard to return false here.
>
> Anand Mazumdar wrote:
> Looks like I am missing something here. This is a method for Accept-Encoding. If the header is not present and you pass "gzip" as argument to the method , it should return false as was the case earlier since the client can't accept "gzip" ( gzip is a bad example here owing to the exception in the rfc around it/compress). Did you get confused with it being the "Accept" header ?
>
> Re-opening the issue for now. I think we can get rid of the TODO here.
>
> Isabel Jimenez wrote:
> `Accept` and `Accept-encoding` have the same behavior in this case. From RFC 14.3 Accept-Encoding : If no Accept-Encoding field is present in a request, the server MAY assume that the client will accept any content coding.
// TODO(ijimenez): Return true here, from RFC:
// If no Accept header field is present, then it is assumed
// that the client accepts all media types.
Looks like there was some confusion here around the TODO . It did not make sense here since this method was for Accept-Encoding. You can modify it to:
// TODO(ijimenez): Return true here, from RFC:
// If no Accept-Encoding header field is present, then it may be assumed
// that the client accepts all content-codings.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review91843
-----------------------------------------------------------
On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 15, 2015, 11:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On July 16, 2015, 12:10 a.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 135-143
> > <https://reviews.apache.org/r/36402/diff/3/?file=1012922#file1012922line135>
> >
> > Can we get these comments back ?
They're on the header file now, do we want them duplciate here?
> On July 16, 2015, 12:10 a.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 145-146
> > <https://reviews.apache.org/r/36402/diff/3/?file=1012922#file1012922line145>
> >
> > Can we put these comments back ?
Because rules aren't here anymore but in the header file. I thought it was confusing to let this here.
> On July 16, 2015, 12:10 a.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 169-177
> > <https://reviews.apache.org/r/36402/diff/3/?file=1012922#file1012922line169>
> >
> > Same here.
same here, moved to header.
> On July 16, 2015, 12:10 a.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, line 126
> > <https://reviews.apache.org/r/36402/diff/3/?file=1012922#file1012922line126>
> >
> > Whoops ! Let's return false as earlier. Also add a test-case for empty check. ( Accept-Encoding )
Added a TODO since it's not standard to return false here.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review91843
-----------------------------------------------------------
On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 15, 2015, 11:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review91843
-----------------------------------------------------------
Have not gone through the whole review yet. It would be worth discussing this with @bmahler more on what he thinks about my comment on refactoring acceptMediaType and then go over this again based on his feedback.
3rdparty/libprocess/src/http.cpp (line 125)
<https://reviews.apache.org/r/36402/#comment145496>
Whoops ! Let's return false as earlier. Also add a test-case for empty check. ( Accept-Encoding )
3rdparty/libprocess/src/http.cpp
<https://reviews.apache.org/r/36402/#comment145495>
Can we get these comments back ?
3rdparty/libprocess/src/http.cpp (lines 135 - 136)
<https://reviews.apache.org/r/36402/#comment145510>
Can we put these comments back ?
3rdparty/libprocess/src/http.cpp
<https://reviews.apache.org/r/36402/#comment145511>
Same here.
3rdparty/libprocess/src/http.cpp (line 163)
<https://reviews.apache.org/r/36402/#comment145509>
How do we intend to use this method ?
Let's say we have 2 media types that we support "application/json" and "application/x-protobuf".
I would need to call acceptMediaType twice ( in a loop ):
- acceptMediaType("application/json") , if it returns false , call again with
- acceptMediaType("application/x-protobuf").
Return back a 415 if both of these return false.
Why not we change the method signature to:
Try<std::string> Request::findFirstAcceptedType(const std::vector<std::string>& acceptedMediaTypes)
that just returns a string with the first match and an Error if none of the options in acceptedMediaTypes match.
We did not face the same issue with Accept-Encoding may be because gzip is the only encoding we support ( and not deflate ? )
3rdparty/libprocess/src/http.cpp (line 196)
<https://reviews.apache.org/r/36402/#comment145512>
Nit-pick : Modify the comment string here, gzip;q=0.0 does not hold good here.
- Anand Mazumdar
On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 15, 2015, 11:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review92343
-----------------------------------------------------------
Thanks Isabel!
Could we remove the changes to 'acceptEncoding'? The references to the RFC are more appropriate in the implementation, rather than the API documentation. In the API documentation, we can refer people to the RFC, and we may want to take note of places where we're not following the RFC (e.g. missing 'Accept-Encoding' header and curl). But I originally wrote these comments to guide someone who is trying to understand the implementation. Also, it's independent :)
How about using candidates (similarly to 'acceptsEncoding')? That would make both of these pretty similar and easier to understand. Also note that we should watch out for the existing bug in 'acceptsEncoding':
```
bool Request::acceptsMediaType(const string& mediaType_) const
{
vector<string> mediaType = strings::tokenize(mediaType_, "/");
if (mediaType.size() != 2) {
return false;
}
Option<string> accept = headers.get("Accept");
// If no Accept header field is present, then it is assumed
// that the client accepts all media types.
if (accept.isNone()) {
return true;
}
// Remove spaces and tabs for easier parsing.
accept = strings::remove(accept.get(), " ");
accept = strings::remove(accept.get(), "\t");
accept = strings::remove(accept.get(), "\n");
// First look for the exact media type, followed by
// a type/* match, followed by */*.
vector<string> candidates;
candidates.push_back(mediaType[0] + "/" + mediaType[1]);
candidates.push_back(mediaType[0] + "/*");
candidates.push_back("*/*");
foreach (const string& candidate, candidates) {
// Similar code to 'acceptsEncoding'.
// NOTE that it has a bug because startswith will match
// only by prefix!
}
...
}
```
3rdparty/libprocess/src/http.cpp (lines 122 - 127)
<https://reviews.apache.org/r/36402/#comment146454>
Actually, thinking over this again, I recall that curl doesn't set Accept-Encoding by default and does not decompress by default, which is pretty annoying. We should probably leave a NOTE here about this for now, as to why this returns false.
Also, s/Accept/Accept-Encoding/
3rdparty/libprocess/src/http.cpp (line 142)
<https://reviews.apache.org/r/36402/#comment146471>
Only doing startswith was actually a bug on my part! e.g. gzipper;q=1.0 would match gzip..
3rdparty/libprocess/src/http.cpp (line 168)
<https://reviews.apache.org/r/36402/#comment146456>
What do you mean here by "fully support", can you spell out what part is not supported? It looks like we don't need to:
"Note that [HTML20] included an optional "level" parameter; in practice, this parameter was never used and has been removed from this specification. [HTML30] also suggested a "version" parameter; in practice, this parameter also was never used and has been removed from this specification."
3rdparty/libprocess/src/http.cpp (line 170)
<https://reviews.apache.org/r/36402/#comment146462>
s/accepted/accept/
3rdparty/libprocess/src/http.cpp (line 183)
<https://reviews.apache.org/r/36402/#comment146464>
s/content/accepted/ or s/content/acceptable/
3rdparty/libprocess/src/http.cpp (lines 185 - 187)
<https://reviews.apache.org/r/36402/#comment146461>
This seems like an malformed 'Accept' header, any reason to prefer returning false instead of just skipping? Ideally, we've validated already against malformed requests?
3rdparty/libprocess/src/http.cpp (lines 189 - 190)
<https://reviews.apache.org/r/36402/#comment146466>
Whoops, don't you need to check the size of this? It looks like valid input needs 2 tokens? Perhaps we should do this near the beginning?
3rdparty/libprocess/src/http.cpp (lines 190 - 199)
<https://reviews.apache.org/r/36402/#comment146465>
Let's get some newlines in here to make it more readable :)
3rdparty/libprocess/src/http.cpp (lines 192 - 193)
<https://reviews.apache.org/r/36402/#comment146467>
Is startswith sufficient? Seems like you need a full match?
Hm.. it doesn't look like `"*/foo"` should be considered valid, but this code considers `"*/foo"` to match `"bar/foo"`.
3rdparty/libprocess/src/http.cpp (lines 197 - 199)
<https://reviews.apache.org/r/36402/#comment146468>
Hm.. we should split on ';' before, it's not really part of the content token.
- Ben Mahler
On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 15, 2015, 11:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/
-----------------------------------------------------------
(Updated July 15, 2015, 11:54 p.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding a method for Accept header in request + refactor of Accept-Encoding
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp 72b6d27
3rdparty/libprocess/src/http.cpp d168579
3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
Diff: https://reviews.apache.org/r/36402/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/
-----------------------------------------------------------
(Updated July 15, 2015, 11:22 p.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding a method for Accept header in request + refactor of Accept-Encoding
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp 72b6d27
3rdparty/libprocess/src/http.cpp d168579
3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
Diff: https://reviews.apache.org/r/36402/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/
-----------------------------------------------------------
(Updated July 15, 2015, 10:51 p.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding a method for Accept header in request + refactor of Accept-Encoding
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp 72b6d27
3rdparty/libprocess/src/encoder.hpp c5ff761
3rdparty/libprocess/src/http.cpp d168579
3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
Diff: https://reviews.apache.org/r/36402/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review91361
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36402]
All tests passed.
- Mesos ReviewBot
On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 10, 2015, 8:55 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/encoder.hpp c5ff761
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On July 10, 2015, 10:13 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, line 216
> > <https://reviews.apache.org/r/36402/diff/1/?file=1008565#file1008565line216>
> >
> > I did not review the entire patch but I stopped at this. Seems like I am missing something here.
> >
> > In my understanding, you can't use the same method for parsing both the "Accept", "Accept-Encoding" as the rules for both of them are entirely different ! :)
> >
> > Let's take an example from the RFC : http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html
> >
> > So the accept header also understands media-range i.e. you can specify "*/*" or "text/*" et al meaning all media types can be accepted or in the second case all media types of text/something can only be accepted and so on.
> >
> > There are a couple of other things like accept-param and accept-extension that also need to be handled.
> >
> > I assume that your motive for this change was to add a validation operation for "Accept" similar to "Accept-Encoding" header that validates if the header values are well-formed and should be accepted ? If that is the case, you would need to write a separate method/logic and not just use the existing acceptEncoding method.
> >
> > Long story short, we need two methods:
> > 1. Validate if the "Accept" header is well formed. ( the one you already built minus the mentioned caveats above )
> >
> > Also , it would be good to add a second one:
> > 2. Given all the accept types mentioned in the "Accept" header , and the accept types we support ,is it possible for us to send a response back ? If not send a 415 back.
> >
> > What do you think ?
>
> Isabel Jimenez wrote:
> The AcceptHeader method groups validation that's common to both 'Accept' and 'Accept-Encoding'.
> The logic was already there I just moved it so we can use it for both, and if needed and more things to each one separately.
>
> I plan to add 'accept-param' and 'accept-extension' support in a different patch.
>
> I also added a TODO to consider handling all this by returning Response, what do you think, should we make that change now?
>
> Anand Mazumdar wrote:
> Unfortunately, This does not make much sense to me. Let's take an example from your test-case.
>
> requests[2].headers["Accept"] = "foo, test;q=0.0";
>
> This is not a "VALID" accept header at all if you see its grammer carefully. The only thing that "Accept" and "Accept-Encoding" have in common is the q-value syntax and how you specify them on one line i.e. delimited by "," and ";". :)
>
> An "Accept" header must have a "type"/"subtype" or a "type/*".
>
> Makes sense ? ( I am re-opening the issue )
I was assuming that for 'accept' headers users will always provide 'type/subtype', but you're right. Fix this.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review91356
-----------------------------------------------------------
On July 15, 2015, 10:51 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 15, 2015, 10:51 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/encoder.hpp c5ff761
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On July 10, 2015, 10:13 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, line 216
> > <https://reviews.apache.org/r/36402/diff/1/?file=1008565#file1008565line216>
> >
> > I did not review the entire patch but I stopped at this. Seems like I am missing something here.
> >
> > In my understanding, you can't use the same method for parsing both the "Accept", "Accept-Encoding" as the rules for both of them are entirely different ! :)
> >
> > Let's take an example from the RFC : http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html
> >
> > So the accept header also understands media-range i.e. you can specify "*/*" or "text/*" et al meaning all media types can be accepted or in the second case all media types of text/something can only be accepted and so on.
> >
> > There are a couple of other things like accept-param and accept-extension that also need to be handled.
> >
> > I assume that your motive for this change was to add a validation operation for "Accept" similar to "Accept-Encoding" header that validates if the header values are well-formed and should be accepted ? If that is the case, you would need to write a separate method/logic and not just use the existing acceptEncoding method.
> >
> > Long story short, we need two methods:
> > 1. Validate if the "Accept" header is well formed. ( the one you already built minus the mentioned caveats above )
> >
> > Also , it would be good to add a second one:
> > 2. Given all the accept types mentioned in the "Accept" header , and the accept types we support ,is it possible for us to send a response back ? If not send a 415 back.
> >
> > What do you think ?
The AcceptHeader method groups validation that's common to both 'Accept' and 'Accept-Encoding'.
The logic was already there I just moved it so we can use it for both, and if needed and more things to each one separately.
I plan to add 'accept-param' and 'accept-extension' support in a different patch.
I also added a TODO to consider handling all this by returning Response, what do you think, should we make that change now?
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review91356
-----------------------------------------------------------
On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 10, 2015, 8:55 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/encoder.hpp c5ff761
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Anand Mazumdar <ma...@gmail.com>.
> On July 10, 2015, 10:13 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, line 216
> > <https://reviews.apache.org/r/36402/diff/1/?file=1008565#file1008565line216>
> >
> > I did not review the entire patch but I stopped at this. Seems like I am missing something here.
> >
> > In my understanding, you can't use the same method for parsing both the "Accept", "Accept-Encoding" as the rules for both of them are entirely different ! :)
> >
> > Let's take an example from the RFC : http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html
> >
> > So the accept header also understands media-range i.e. you can specify "*/*" or "text/*" et al meaning all media types can be accepted or in the second case all media types of text/something can only be accepted and so on.
> >
> > There are a couple of other things like accept-param and accept-extension that also need to be handled.
> >
> > I assume that your motive for this change was to add a validation operation for "Accept" similar to "Accept-Encoding" header that validates if the header values are well-formed and should be accepted ? If that is the case, you would need to write a separate method/logic and not just use the existing acceptEncoding method.
> >
> > Long story short, we need two methods:
> > 1. Validate if the "Accept" header is well formed. ( the one you already built minus the mentioned caveats above )
> >
> > Also , it would be good to add a second one:
> > 2. Given all the accept types mentioned in the "Accept" header , and the accept types we support ,is it possible for us to send a response back ? If not send a 415 back.
> >
> > What do you think ?
>
> Isabel Jimenez wrote:
> The AcceptHeader method groups validation that's common to both 'Accept' and 'Accept-Encoding'.
> The logic was already there I just moved it so we can use it for both, and if needed and more things to each one separately.
>
> I plan to add 'accept-param' and 'accept-extension' support in a different patch.
>
> I also added a TODO to consider handling all this by returning Response, what do you think, should we make that change now?
Unfortunately, This does not make much sense to me. Let's take an example from your test-case.
requests[2].headers["Accept"] = "foo, test;q=0.0";
This is not a "VALID" accept header at all if you see its grammer carefully. The only thing that "Accept" and "Accept-Encoding" have in common is the q-value syntax and how you specify them on one line i.e. delimited by "," and ";". :)
An "Accept" header must have a "type"/"subtype" or a "type/*".
Makes sense ? ( I am re-opening the issue )
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review91356
-----------------------------------------------------------
On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 10, 2015, 8:55 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/encoder.hpp c5ff761
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review91356
-----------------------------------------------------------
3rdparty/libprocess/src/http.cpp (line 206)
<https://reviews.apache.org/r/36402/#comment144723>
I did not review the entire patch but I stopped at this. Seems like I am missing something here.
In my understanding, you can't use the same method for parsing both the "Accept", "Accept-Encoding" as the rules for both of them are entirely different ! :)
Let's take an example from the RFC : http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html
So the accept header also understands media-range i.e. you can specify "*/*" or "text/*" et al meaning all media types can be accepted or in the second case all media types of text/something can only be accepted and so on.
There are a couple of other things like accept-param and accept-extension that also need to be handled.
I assume that your motive for this change was to add a validation operation for "Accept" similar to "Accept-Encoding" header that validates if the header values are well-formed and should be accepted ? If that is the case, you would need to write a separate method/logic and not just use the existing acceptEncoding method.
Long story short, we need two methods:
1. Validate if the "Accept" header is well formed. ( the one you already built minus the mentioned caveats above )
Also , it would be good to add a second one:
2. Given all the accept types mentioned in the "Accept" header , and the accept types we support ,is it possible for us to send a response back ? If not send a 415 back.
What do you think ?
- Anand Mazumdar
On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 10, 2015, 8:55 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/encoder.hpp c5ff761
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36402: Adding 'Accept' header in request
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review91362
-----------------------------------------------------------
3rdparty/libprocess/src/http.cpp (lines 164 - 183)
<https://reviews.apache.org/r/36402/#comment144729>
These appear to me to be valuable information for a caller too - please move those "up" to be the method's Doxy (javadoc) in addition to writing up the method's description.
Please make sure to make them consistent with our style for comments.
3rdparty/libprocess/src/http.cpp (line 190)
<https://reviews.apache.org/r/36402/#comment144730>
ditto
3rdparty/libprocess/src/tests/http_tests.cpp (lines 676 - 689)
<https://reviews.apache.org/r/36402/#comment144731>
```
vector<string> bogusHeaders = { "test;q=0.0",
"foo",
"foo, test;q=0.0",
"*, test;q=0.0",
"*;q=0.0, foo",
"\n foo",
"foo,\ttest;q=0.0"};
http::Request request;
for (auto accept : bogusHeaders) {
request.headers["Accept"] = accept;
EXPECT_FALSE(request.acceptsMediaType("test").get())
}
```
Let's please make the most out of our newly-acquired C++11 abilities :)
3rdparty/libprocess/src/tests/http_tests.cpp (line 695)
<https://reviews.apache.org/r/36402/#comment144732>
same here
- Marco Massenzio
On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 10, 2015, 8:55 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/encoder.hpp c5ff761
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>