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