> On Nov. 20, 2014, 9:11 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/include/process/http.hpp, lines 415-416 > > <https://reviews.apache.org/r/28253/diff/1/?file=770208#file770208line415> > > > > Here's what we'll have if we added join: > > > > parse: takes a __decoded__ query string, construct a key,value map of > > the parameters > > > > join: takes a key,value map of parameters, creates an __encoded__ query > > string. > > > > That's where I find this to be non-intuitive: > > > > ``` > > join(parse(query)) == encode(query) > > ``` > > > > It seems like only request encoding should deal with constructing an > > encoded query string, is that not the case? Would like to avoid exposing > > this if possible, and if not possible, would like to make this more > > intuitive. > > Cody Maloney wrote: > So that behavior of parse is going to produce incorrect results I > believe. Suppose one of my parameters in my query string is a small blob of > JSON (Contains '=' and '&' potentially). If we encode/decode on the outside > of query string parsing/joining, we can't represent this (The & will look > like the start of a new parameter). > > ``` > std::map<> query = {{"a%20","b&c=d!@#foobar"}} > > EXPECT_EQ(query, parse(join(query))); > ``` > > Based on my reading of rfc3986 2.1 (snippet inline): > ``` > A percent-encoding mechanism is used to represent a data octet in a > component when that octet's corresponding character is outside the > allowed set or is being used as a delimiter of, or within, the > component. > ``` > > This should work, and is the reason why there is percent encoding. From > what I can tell grepping throuhg the source code nothing currently does the > decoding before we parse the query string, so making the decode live inside > of the parse function should work. Currently callers just end up with percent > encoded chunks in their query string, which causes badness (And could explain > an issue which a customer was having with files/browse.json and things not > showing up) > > Ben Mahler wrote: > Hm.. so I did the grep and as expected the callers parse decoded strings. > > The original use of parse is in Decoder: > > ``` > static int on_message_complete(http_parser* p) > { > DataDecoder* decoder = (DataDecoder*) p->data; > > // Parse the query key/values. > Try<std::string> decoded = http::decode(decoder->query); > if (decoded.isError()) { > return 1; > } > decoder->request->query = http::query::parse(decoded.get()); > ... > ``` > > Note that here we parse a decoded string, as expected. The other use > appears to be within the http code in the master that parses request > __bodies__: > > ``` > Future<Response> Master::Http::shutdown(const Request& request) > { > if (request.method != "POST") { > return BadRequest("Expecting POST"); > } > > // Parse the query string in the request body (since this is a POST) > // in order to determine the framework ID to shutdown. > hashmap<string, string> values = > process::http::query::parse(request.body); > ``` > > Bodies are not expected to be percent encoded, IIUC. > > Anything I missed...? > > Cody Maloney wrote: > So we do, not sure how I missed that... > > So I can fix up the decoding to be inside the query string. Simple extra > patch, then add some round trip tests. Sound good? > > The body one is interesting in that it looks like we're doing it > incorrectly: > > http://stackoverflow.com/questions/4007969/application-x-www-form-urlencoded-or-multipart-form-data/4073451#4073451 > > We're assuming application/x-www-form-urlencoded at the moment which we > should decode. None of the requests happen to be from non-libprocess things > or contain illegal characters so it happens to work. > > *random side thoughts*: We aren't a very good HTTP/webserver / parsing > into request/response objects, processing Content-Type, doing simple clean > routing. The more I dig into this code the more I think it would be nice if > there was just an nginx, apache, proxygen / etc. which are good at doing the > HTTP parts and we just take their parsing + methods, getting us out of the > game of writing an HTTP server, then just have the bits to bridge / convert > their request objects to the appropriate libprocess dispatches.
> So I can fix up the decoding to be inside the query string. Do you mean decoding to be inside query::parse? Sounds bad because it cannot assume the string is always encoded and decoding is not idempotent IIUC. If you _really_ need this, let's just provide the map -> string functionality, and leave the encoding responsibility to the Request encoder. As much as possible, the Encoder / Decoder code should be the only code responsible for encoding and decoding, but maybe I'm missing some context on this. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28253/#review62422 ----------------------------------------------------------- On Nov. 20, 2014, 12:50 a.m., Cody Maloney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28253/ > ----------------------------------------------------------- > > (Updated Nov. 20, 2014, 12:50 a.m.) > > > Review request for mesos. > > > Bugs: MESOS-2132 > https://issues.apache.org/jira/browse/MESOS-2132 > > > Repository: mesos-git > > > Description > ------- > > This is needed to turn Request objects into http request which we send across > the wire. The existing HTTP request sending code expects the query string to > be already constructed. > > Ideally people can either just forward the query string they get to send it > back across the wire or they can manipulate it using the native C++ datatype > (A hashmap), and then have it automatically re-encoded. > > The parsing logic to go from a string to a map is already exists in this > header, this just adds the other direction. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/http.hpp > 9cf05acbb724ab9af8010d1788621d37a0e48e86 > 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358 > 3rdparty/libprocess/src/tests/http_tests.cpp > a90e65f77904da0a45e1cc0cc9889ae69354a1a5 > > Diff: https://reviews.apache.org/r/28253/diff/ > > > Testing > ------- > > make distcheck ubuntu 14.04 > > > Thanks, > > Cody Maloney > >
