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