mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anand Mazumdar" <mazumdar.an...@gmail.com>
Subject Re: Review Request 36402: Adding 'Accept' header in request
Date Fri, 10 Jul 2015 23:07:40 GMT


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


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