feat: add capability to handle http requests with repeated slashes#258
feat: add capability to handle http requests with repeated slashes#258dxfg wants to merge 2 commits intoenarx:mainfrom
Conversation
dxfg
force-pushed
the
feat/slashes
branch
3 times, most recently
from
cdde52e to
f8c3a08
Compare
Co-authored-by: Roman <roman@profian.com> Signed-off by: Sriram <tsr23@wharton.upenn.edu> Signed-off-by: Sriram <tsr23@wharton.upenn.edu>
Signed-off-by: Sriram <tsr23@wharton.upenn.edu>
rvolosatovs
left a comment
rvolosatovs
left a comment
There was a problem hiding this comment.
implementation looks overall, but I don't see how the tests are relevant for the task at hand.
You should be using the actual URLs that this function handles, e.g. https://store.profian.com//api////v0.2.0////examples//////fibonacci-rust////_tag///0.2.0//tree///PATH//IN/TREE
| .trim_start_matches('/') | ||
| .trim_end_matches('/') |
There was a problem hiding this comment.
This is redundant due to the split and the filter. You'll never get slashes here.
BTW, just FYI, trim_matches trims from both sides
| .uri("https://www.rust-lang.org/") | ||
| .header("User-Agent", "my-awesome-agent/1.0") | ||
| .body(hyper::Body::empty()); | ||
| println!("{:?}", request); |
| let res = handle(request.unwrap()).await; | ||
|
|
||
| // Temporary print to ensure test is working as intended. | ||
| // println!("{}", res.into_response().status()); |
|
|
||
| #[async_std::test] | ||
| async fn multiple_slashes_missing() { | ||
| let request = Request::builder() | ||
| .uri("https://www.rust-lang.org/") | ||
| .header("User-Agent", "my-awesome-agent/1.0") | ||
| .body(hyper::Body::empty()); | ||
| println!("{:?}", request); | ||
|
|
||
| let res = handle(request.unwrap()).await; | ||
|
|
||
| // Temporary print to ensure test is working as intended. | ||
| // println!("{}", res.into_response().status()); | ||
| assert_eq!(res.into_response().status(), 404); | ||
|
|
||
| let request = Request::builder() | ||
| .uri("https://www.rust-lang.org///") | ||
| .header("User-Agent", "my-awesome-agent///1.0") | ||
| .body(hyper::Body::empty()); | ||
| println!("{:?}", request); | ||
|
|
||
| let res = handle(request.unwrap()).await; | ||
|
|
||
| // Temporary print to ensure test is working as intended. | ||
| // println!("{}", res.into_response().status()); | ||
| assert_eq!(res.into_response().status(), 404); | ||
| return (); | ||
| } | ||
|
|
||
| /// Unit test to handle multiple slash path parsing | ||
| #[async_std::test] | ||
| async fn multiple_slashes_found() { | ||
| let request = Request::builder() | ||
| .uri("http://httpbin.org/ip") | ||
| .body(hyper::Body::empty()); | ||
| println!("{:?}", request); | ||
|
|
||
| let res = handle(request.unwrap()).await; | ||
|
|
||
| // Temporary print to ensure test is working as intended. | ||
| // println!("{}", res.into_response().status()); | ||
| assert_eq!(res.into_response().status(), 404); | ||
|
|
||
| let request = Request::builder() | ||
| .uri("http://httpbin.org///ip") | ||
| .body(hyper::Body::empty()); | ||
| println!("{:?}", request); | ||
|
|
||
| let res = handle(request.unwrap()).await; | ||
|
|
||
| // Temporary print to ensure test is working as intended. | ||
| // println!("{}", res.into_response().status()); | ||
| assert_eq!(res.into_response().status(), 404); | ||
| return (); | ||
| } |
There was a problem hiding this comment.
If we want to have unit tests in this file, they have to reside in mod tests, see https://doc.rust-lang.org/book/ch11-01-writing-tests.html
It could also be a better idea to write a test for this in https://github.com/profianinc/drawbridge/blob/f03fb5815f28f5196ff4c93c4137791dc6aab67b/tests/mod.rs instead.
We could just add a few direct URL calls there, something like https://github.com/profianinc/drawbridge/blob/1871d2a7f2519c835818e33a9ca6074e67109006/crates/app/tests/helpers/tree.rs, except with additional slashes.
Basically we need to just test at least one URL, something like: https://store.profian.com//api////v0.2.0////examples//////fibonacci-rust////_tag///0.2.0//tree//PATH/////IN/TREE
if that works, we can assume that everything works.
This could be added to the end of the integration test, where the tree already exists.
|
|
||
| let request = Request::builder() | ||
| .uri("https://www.rust-lang.org///") | ||
| .header("User-Agent", "my-awesome-agent///1.0") |
|
Status: waiting for review from Roman to be addressed. |
5 participants
WIP PR to add capability to handle http requests w/ repeated slashes.