> On Dec. 19, 2017, 1:08 a.m., Benjamin Mahler wrote: > > 3rdparty/libprocess/src/process.cpp > > Lines 3530-3531 (patched) > > <https://reviews.apache.org/r/64574/diff/1/?file=1915259#file1915259line3531> > > > > This approach doesn't seem quite right to me, since it prevents the > > user of libprocess from seeing what the original path was. Have you > > considered any other approaches? (also I think you only needed to trim from > > the tail?) > > Alexander Rukletsov wrote: > Why so? `name` is a local variable based on request path and used to find > appropriate handler. A user of libprocess can still access > `request->url.path`. > > I do trim from the tail only (see two lines above). > > I did consider returning 301 with a trimmed path, but decided not to > since I'm not sure we're allowed to return 301 codes in response to operator > API endpoints. > > Benjamin Mahler wrote: > Ah yes, I see that it's a local variable now, sorry about that! > > A couple of questions: > > (1) What happens on the `route()` side? Can a caller setup two routes for > "path" and "path/"? And if so, what happens when requests come in for "path" > and "path/"? > > (2) Maybe we could preserve the original trimming of prefix (since it > belongs with the removal of the "ID" comment), and instead add the suffix > stripping along with a comment about the overall idea here? As it stands, I > think the reader will have a hard time realizing that we've decided to treat > trailing slash paths the same as non-trailing slash paths?
(1) Currently, libprocess allows to route both `"/path"` and `"/path/"` and moreover to different handlers. With this patch, route to `"/path/"` is not wired and hence has no effects. We can do following things to improve the logic here: - disallow `"/path/"` in `route()` - normalize `"/path/"` to `"/path"` in `route()` (might result in handler overwrite) - allow both `"/path/"` and `"/path"` and try both if `request.path` handler is not found I have a light tendency to the first option because of its simplicity. Note that we already normalize `"/path/"` to `"/path"` on [the help page](https://github.com/apache/mesos/blob/634c8af2618c57a1405d20717fa909b399486f37/3rdparty/libprocess/src/help.cpp#L95-L98). (2) Sure. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64574/#review194106 ----------------------------------------------------------- On Dec. 13, 2017, 1:39 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64574/ > ----------------------------------------------------------- > > (Updated Dec. 13, 2017, 1:39 p.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Mahler. > > > Bugs: MESOS-5333 > https://issues.apache.org/jira/browse/MESOS-5333 > > > Repository: mesos > > > Description > ------- > > Prior to this patch, adding a trailing '/' to a valid URL path, e.g., > "/state/", yielded a 404 response. This patch ensures that two URLs > which differ only in trailing '/' produce the same result. > > > Diffs > ----- > > 3rdparty/libprocess/src/process.cpp > 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf > 3rdparty/libprocess/src/tests/http_tests.cpp > 9daac715f0242921b7f9f5c20b3eb27f1be802d4 > > > Diff: https://reviews.apache.org/r/64574/diff/1/ > > > Testing > ------- > > Ensured the modified test fails without the fix. > `make check` on Mac OS 10.11.6 and several Linux distributions. > > > Thanks, > > Alexander Rukletsov > >
