Re: Review Request 68420: Added /files API test for reserved query characters.

[Andrew Schwartzmeyer <an...@schwartzmeyer.com>]

> On Aug. 23, 2018, 1:02 p.m., Benjamin Mahler wrote:
> > src/tests/files_tests.cpp
> > Lines 331-332 (patched)
> > <https://reviews.apache.org/r/68420/diff/1/?file=2074999#file2074999line331>
> >
> >     This is a little odd to read without the context, this tests reserved characters by using `%`, but we could use anything other reserved character to acheive the same test, e.g. `+` seems the simplest:
> >     
> >     ```
> >       // We use a reserved character for query parameters `+` to
> >       // ensure it is encoded and decoded correctly.
> >       const string filename = "foo+bar";
> >     ```
> >     
> >     If you'd like to stick with using `%`, probably we should just say:
> >     
> >     ```
> >       // We use a reserved character for query parameters `%` to
> >       // ensure it is encoded and decoded correctly.
> >       const string filename = "foo%3Abar";
> >     ```
> >     
> >     And not do line 334 as it's irrelevant to this test
> 
> Andrew Schwartzmeyer wrote:
>     You couldn't use `+` because it wouldn't have the same problem: the second decode would not turn `+` into something else (this is why we haven't run into this problem before with other characters, even a literal `:`). The problem is that `%3A` _can_ be decoded, and so decoding more times than encoding causes it to erroneously become `:`. The number of encodes and decodes must be balanced.
>     
>     Also, line 334 is relevant to this test: it's demonstrating that the query can be erroneously decoded too many times.
> 
> Andrew Schwartzmeyer wrote:
>     I explained why thits is explicitly an entire percent-encoding, and am considering this fixed. Thanks!

Okay I am going to assume this was "fix it, then ship it" as you marked the other two as ship it, and I'd like to get at least the tests and libprocess fix in.


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68420/#review207830
-----------------------------------------------------------


On Aug. 28, 2018, 10:40 a.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68420/
> -----------------------------------------------------------
> 
> (Updated Aug. 28, 2018, 10:40 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9168
>     https://issues.apache.org/jira/browse/MESOS-9168
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Due to a bug in the HTTP client in libprocess (MESOS-9168), the
> use of reserved query characters in paths did not work when using
> the HTTP client in libprocess. This adds a test to ensure that
> the /files API correctly handles reserved query characters in
> file paths.
> 
> 
> Diffs
> -----
> 
>   src/tests/files_tests.cpp d09279077ec704167991933349ee943ab44d0aa9 
> 
> 
> Diff: https://reviews.apache.org/r/68420/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>