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

Reply via email to