From 3fcd5f6935404be202cd8d62cc62bd01b633f9f9 Mon Sep 17 00:00:00 2001 From: Nikolay Kim Date: Thu, 30 Nov 2017 19:01:25 -0800 Subject: [PATCH] use http::Uri for uri parsing --- Cargo.toml | 2 +- src/error.rs | 5 ++-- src/h1.rs | 32 ++++--------------------- src/h2.rs | 5 +--- src/httprequest.rs | 49 ++++++++++++++++++++++++-------------- src/middlewares/logger.rs | 13 ++++++---- src/recognizer.rs | 2 +- src/ws.rs | 37 ++++++++++++++-------------- tests/test_httprequest.rs | 48 +++++++++++++++++++++---------------- tests/test_httpresponse.rs | 5 ++-- 10 files changed, 98 insertions(+), 100 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9aab5a1f6..7c55a6ffc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -37,7 +37,7 @@ log = "0.3" failure = "0.1" failure_derive = "0.1" time = "0.1" -http = "0.1" +http = "^0.1.2" httparse = "0.1" http-range = "0.1" mime = "0.3" diff --git a/src/error.rs b/src/error.rs index f1cefba50..c6c4a7eb9 100644 --- a/src/error.rs +++ b/src/error.rs @@ -12,6 +12,7 @@ use httparse; use failure::Fail; use http2::Error as Http2Error; use http::{header, StatusCode, Error as HttpError}; +use http::uri::InvalidUriBytes; use http_range::HttpRangeParseError; use serde_json::error::Error as JsonError; @@ -110,8 +111,8 @@ pub enum ParseError { #[fail(display="Invalid Method specified")] Method, /// An invalid `Uri`, such as `exam ple.domain`. - #[fail(display="Uri error")] - Uri, + #[fail(display="Uri error: {}", _0)] + Uri(InvalidUriBytes), /// An invalid `HttpVersion`, such as `HTP/1.1` #[fail(display="Invalid HTTP version specified")] Version, diff --git a/src/h1.rs b/src/h1.rs index 96293aec6..d4c86a8ac 100644 --- a/src/h1.rs +++ b/src/h1.rs @@ -6,13 +6,12 @@ use std::collections::VecDeque; use actix::Arbiter; use httparse; -use http::{Method, Version, HttpTryFrom, HeaderMap}; +use http::{Uri, Method, Version, HttpTryFrom, HeaderMap}; use http::header::{self, HeaderName, HeaderValue}; use bytes::{Bytes, BytesMut, BufMut}; use futures::{Future, Poll, Async}; use tokio_io::{AsyncRead, AsyncWrite}; use tokio_core::reactor::Timeout; -use percent_encoding; use pipeline::Pipeline; use encoding::PayloadType; @@ -515,31 +514,8 @@ impl Reader { let slice = buf.split_to(len).freeze(); let path = slice.slice(path.0, path.1); - - // manually split path, path was found to be utf8 by httparse - let uri = { - if let Ok(path) = percent_encoding::percent_decode(&path).decode_utf8() { - let parts: Vec<&str> = path.splitn(2, '?').collect(); - if parts.len() == 2 { - Some((parts[0].to_owned(), parts[1].to_owned())) - } else { - Some((parts[0].to_owned(), String::new())) - } - } else { - None - } - }; - let (path, query) = if let Some(uri) = uri { - uri - } else { - let parts: Vec<&str> = unsafe{ - std::str::from_utf8_unchecked(&path)}.splitn(2, '?').collect(); - if parts.len() == 2 { - (parts[0].to_owned(), parts[1][1..].to_owned()) - } else { - (parts[0].to_owned(), String::new()) - } - }; + // path was found to be utf8 by httparse + let uri = Uri::from_shared(path).map_err(ParseError::Uri)?; // convert headers let mut headers = HeaderMap::with_capacity(headers_len); @@ -558,7 +534,7 @@ impl Reader { } let (mut psender, payload) = Payload::new(false); - let msg = HttpRequest::new(method, path, version, headers, query, payload); + let msg = HttpRequest::new(method, uri, version, headers, payload); let decoder = if msg.upgrade() { Decoder::eof() diff --git a/src/h2.rs b/src/h2.rs index 28deda672..929fb9924 100644 --- a/src/h2.rs +++ b/src/h2.rs @@ -216,14 +216,11 @@ impl Entry { router: &Rc>) -> Entry where H: HttpHandler + 'static { - let path = parts.uri.path().to_owned(); - let query = parts.uri.query().unwrap_or("").to_owned(); - // Payload and Content-Encoding let (psender, payload) = Payload::new(false); let mut req = HttpRequest::new( - parts.method, path, parts.version, parts.headers, query, payload); + parts.method, parts.uri, parts.version, parts.headers, payload); // set remote addr req.set_remove_addr(addr); diff --git a/src/httprequest.rs b/src/httprequest.rs index 49f1da246..393330153 100644 --- a/src/httprequest.rs +++ b/src/httprequest.rs @@ -6,7 +6,7 @@ use std::collections::HashMap; use bytes::BytesMut; use futures::{Async, Future, Stream, Poll}; use url::form_urlencoded; -use http::{header, Method, Version, HeaderMap, Extensions}; +use http::{header, Uri, Method, Version, HeaderMap, Extensions}; use {Cookie, HttpRange}; use recognizer::Params; @@ -18,9 +18,8 @@ use error::{ParseError, PayloadError, struct HttpMessage { version: Version, method: Method, - path: String, + uri: Uri, prefix: usize, - query: String, headers: HeaderMap, extensions: Extensions, params: Params, @@ -35,9 +34,8 @@ impl Default for HttpMessage { fn default() -> HttpMessage { HttpMessage { method: Method::GET, - path: String::new(), + uri: Uri::default(), prefix: 0, - query: String::new(), version: Version::HTTP_11, headers: HeaderMap::new(), params: Params::empty(), @@ -56,15 +54,14 @@ pub struct HttpRequest(Rc, Rc); impl HttpRequest<()> { /// Construct a new Request. #[inline] - pub fn new(method: Method, path: String, version: Version, - headers: HeaderMap, query: String, payload: Payload) -> HttpRequest + pub fn new(method: Method, uri: Uri, + version: Version, headers: HeaderMap, payload: Payload) -> HttpRequest { HttpRequest( Rc::new(HttpMessage { method: method, - path: path, + uri: uri, prefix: 0, - query: query, version: version, headers: headers, params: Params::empty(), @@ -104,6 +101,10 @@ impl HttpRequest { &mut self.as_mut().extensions } + /// Read the Request Uri. + #[inline] + pub fn uri(&self) -> &Uri { &self.0.uri } + /// Read the Request method. #[inline] pub fn method(&self) -> &Method { &self.0.method } @@ -123,7 +124,7 @@ impl HttpRequest { /// The target path of this Request. #[inline] pub fn path(&self) -> &str { - &self.0.path + self.0.uri.path() } pub(crate) fn set_prefix(&mut self, idx: usize) { @@ -155,8 +156,10 @@ impl HttpRequest { #[inline] pub fn query(&self) -> HashMap { let mut q: HashMap = HashMap::new(); - for (key, val) in form_urlencoded::parse(self.0.query.as_ref()) { - q.insert(key.to_string(), val.to_string()); + if let Some(query) = self.0.uri.query().as_ref() { + for (key, val) in form_urlencoded::parse(query.as_ref()) { + q.insert(key.to_string(), val.to_string()); + } } q } @@ -166,7 +169,11 @@ impl HttpRequest { /// E.g., id=10 #[inline] pub fn query_string(&self) -> &str { - &self.0.query + if let Some(query) = self.0.uri.query().as_ref() { + query + } else { + "" + } } /// Return request cookies. @@ -364,7 +371,7 @@ impl Clone for HttpRequest { impl fmt::Debug for HttpRequest { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let res = write!(f, "\nHttpRequest {:?} {}:{}\n", - self.0.version, self.0.method, self.0.path); + self.0.version, self.0.method, self.0.uri); if !self.query_string().is_empty() { let _ = write!(f, " query: ?{:?}\n", self.query_string()); } @@ -418,7 +425,9 @@ impl Future for UrlEncoded { #[cfg(test)] mod tests { use super::*; + use std::str::FromStr; use payload::Payload; + use http::Uri; #[test] fn test_urlencoded_error() { @@ -426,7 +435,8 @@ mod tests { headers.insert(header::TRANSFER_ENCODING, header::HeaderValue::from_static("chunked")); let mut req = HttpRequest::new( - Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new(), Payload::empty()); + Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); assert_eq!(req.urlencoded().err().unwrap(), UrlencodedError::Chunked); @@ -436,7 +446,8 @@ mod tests { headers.insert(header::CONTENT_LENGTH, header::HeaderValue::from_static("xxxx")); let mut req = HttpRequest::new( - Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new(), Payload::empty()); + Method::GET, Uri::from_str("/").unwrap(), Version::HTTP_11, + headers, Payload::empty()); assert_eq!(req.urlencoded().err().unwrap(), UrlencodedError::UnknownLength); @@ -446,7 +457,8 @@ mod tests { headers.insert(header::CONTENT_LENGTH, header::HeaderValue::from_static("1000000")); let mut req = HttpRequest::new( - Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new(), Payload::empty()); + Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); assert_eq!(req.urlencoded().err().unwrap(), UrlencodedError::Overflow); @@ -456,7 +468,8 @@ mod tests { headers.insert(header::CONTENT_LENGTH, header::HeaderValue::from_static("10")); let mut req = HttpRequest::new( - Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new(), Payload::empty()); + Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); assert_eq!(req.urlencoded().err().unwrap(), UrlencodedError::ContentType); } diff --git a/src/middlewares/logger.rs b/src/middlewares/logger.rs index 72ca5103f..92117ff00 100644 --- a/src/middlewares/logger.rs +++ b/src/middlewares/logger.rs @@ -288,8 +288,9 @@ impl<'a> fmt::Display for FormatDisplay<'a> { mod tests { use Body; use super::*; + use std::str::FromStr; use time; - use http::{Method, Version, StatusCode}; + use http::{Method, Version, StatusCode, Uri}; use http::header::{self, HeaderMap}; use payload::Payload; @@ -300,7 +301,8 @@ mod tests { let mut headers = HeaderMap::new(); headers.insert(header::USER_AGENT, header::HeaderValue::from_static("ACTIX-WEB")); let mut req = HttpRequest::new( - Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new(), Payload::empty()); + Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); let resp = HttpResponse::build(StatusCode::OK) .header("X-Test", "ttt") .force_close().body(Body::Empty).unwrap(); @@ -331,7 +333,8 @@ mod tests { let mut headers = HeaderMap::new(); headers.insert(header::USER_AGENT, header::HeaderValue::from_static("ACTIX-WEB")); let req = HttpRequest::new( - Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new(), Payload::empty()); + Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); let resp = HttpResponse::build(StatusCode::OK) .force_close().body(Body::Empty).unwrap(); let entry_time = time::now(); @@ -348,8 +351,8 @@ mod tests { assert!(s.contains("ACTIX-WEB")); let req = HttpRequest::new( - Method::GET, "/".to_owned(), Version::HTTP_11, HeaderMap::new(), - "test".to_owned(), Payload::empty()); + Method::GET, Uri::from_str("/?test").unwrap(), + Version::HTTP_11, HeaderMap::new(), Payload::empty()); let resp = HttpResponse::build(StatusCode::OK) .force_close().body(Body::Empty).unwrap(); let entry_time = time::now(); diff --git a/src/recognizer.rs b/src/recognizer.rs index 1a1b9a52e..efa472621 100644 --- a/src/recognizer.rs +++ b/src/recognizer.rs @@ -131,7 +131,7 @@ fn parse(pattern: &str) -> String { } if hard_stop { - panic!("{id:*} section has to be last lection of pattern"); + panic!("Tail '*' section has to be last lection of pattern"); } if in_param { diff --git a/src/ws.rs b/src/ws.rs index 3bda41d6a..cd9cf0059 100644 --- a/src/ws.rs +++ b/src/ws.rs @@ -335,33 +335,32 @@ impl WsWriter { #[cfg(test)] mod tests { use super::*; + use std::str::FromStr; use payload::Payload; - use http::{Method, HeaderMap, Version, header}; + use http::{Method, HeaderMap, Version, Uri, header}; #[test] fn test_handshake() { - let req = HttpRequest::new(Method::POST, "/".to_owned(), - Version::HTTP_11, HeaderMap::new(), - String::new(), Payload::empty()); + let req = HttpRequest::new(Method::POST, Uri::from_str("/").unwrap(), + Version::HTTP_11, HeaderMap::new(), Payload::empty()); assert_eq!(WsHandshakeError::GetMethodRequired, handshake(&req).err().unwrap()); - let req = HttpRequest::new(Method::GET, "/".to_owned(), - Version::HTTP_11, HeaderMap::new(), - String::new(), Payload::empty()); + let req = HttpRequest::new(Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, HeaderMap::new(), Payload::empty()); assert_eq!(WsHandshakeError::NoWebsocketUpgrade, handshake(&req).err().unwrap()); let mut headers = HeaderMap::new(); headers.insert(header::UPGRADE, header::HeaderValue::from_static("test")); - let req = HttpRequest::new(Method::GET, "/".to_owned(), - Version::HTTP_11, headers, String::new(), Payload::empty()); + let req = HttpRequest::new(Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); assert_eq!(WsHandshakeError::NoWebsocketUpgrade, handshake(&req).err().unwrap()); let mut headers = HeaderMap::new(); headers.insert(header::UPGRADE, header::HeaderValue::from_static("websocket")); - let req = HttpRequest::new(Method::GET, "/".to_owned(), - Version::HTTP_11, headers, String::new(), Payload::empty()); + let req = HttpRequest::new(Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); assert_eq!(WsHandshakeError::NoConnectionUpgrade, handshake(&req).err().unwrap()); let mut headers = HeaderMap::new(); @@ -369,8 +368,8 @@ mod tests { header::HeaderValue::from_static("websocket")); headers.insert(header::CONNECTION, header::HeaderValue::from_static("upgrade")); - let req = HttpRequest::new(Method::GET, "/".to_owned(), - Version::HTTP_11, headers, String::new(), Payload::empty()); + let req = HttpRequest::new(Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); assert_eq!(WsHandshakeError::NoVersionHeader, handshake(&req).err().unwrap()); let mut headers = HeaderMap::new(); @@ -380,8 +379,8 @@ mod tests { header::HeaderValue::from_static("upgrade")); headers.insert(SEC_WEBSOCKET_VERSION, header::HeaderValue::from_static("5")); - let req = HttpRequest::new(Method::GET, "/".to_owned(), - Version::HTTP_11, headers, String::new(), Payload::empty()); + let req = HttpRequest::new(Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); assert_eq!(WsHandshakeError::UnsupportedVersion, handshake(&req).err().unwrap()); let mut headers = HeaderMap::new(); @@ -391,8 +390,8 @@ mod tests { header::HeaderValue::from_static("upgrade")); headers.insert(SEC_WEBSOCKET_VERSION, header::HeaderValue::from_static("13")); - let req = HttpRequest::new(Method::GET, "/".to_owned(), - Version::HTTP_11, headers, String::new(), Payload::empty()); + let req = HttpRequest::new(Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); assert_eq!(WsHandshakeError::BadWebsocketKey, handshake(&req).err().unwrap()); let mut headers = HeaderMap::new(); @@ -404,8 +403,8 @@ mod tests { header::HeaderValue::from_static("13")); headers.insert(SEC_WEBSOCKET_KEY, header::HeaderValue::from_static("13")); - let req = HttpRequest::new(Method::GET, "/".to_owned(), - Version::HTTP_11, headers, String::new(), Payload::empty()); + let req = HttpRequest::new(Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); assert_eq!(StatusCode::SWITCHING_PROTOCOLS, handshake(&req).unwrap().status()); } } diff --git a/tests/test_httprequest.rs b/tests/test_httprequest.rs index 96318368a..e69ee9249 100644 --- a/tests/test_httprequest.rs +++ b/tests/test_httprequest.rs @@ -3,16 +3,25 @@ extern crate http; extern crate time; use std::str; +use std::str::FromStr; use actix_web::*; use actix_web::dev::*; -use http::{header, Method, Version, HeaderMap}; +use http::{header, Method, Version, HeaderMap, Uri}; +#[test] +fn test_debug() { + let req = HttpRequest::new( + Method::GET, Uri::from_str("/").unwrap(), Version::HTTP_11, + HeaderMap::new(), Payload::empty()); + let _ = format!("{:?}", req); +} + #[test] fn test_no_request_cookies() { let mut req = HttpRequest::new( - Method::GET, "/".to_owned(), Version::HTTP_11, HeaderMap::new(), - String::new(), Payload::empty()); + Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, HeaderMap::new(), Payload::empty()); assert!(req.cookies().is_empty()); let _ = req.load_cookies(); assert!(req.cookies().is_empty()); @@ -25,7 +34,8 @@ fn test_request_cookies() { header::HeaderValue::from_static("cookie1=value1; cookie2=value2")); let mut req = HttpRequest::new( - Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new(), Payload::empty()); + Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); assert!(req.cookies().is_empty()); { let cookies = req.load_cookies().unwrap(); @@ -48,9 +58,8 @@ fn test_request_cookies() { #[test] fn test_no_request_range_header() { - let req = HttpRequest::new(Method::GET, "/".to_owned(), - Version::HTTP_11, HeaderMap::new(), - String::new(), Payload::empty()); + let req = HttpRequest::new(Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, HeaderMap::new(), Payload::empty()); let ranges = req.range(100).unwrap(); assert!(ranges.is_empty()); } @@ -61,8 +70,8 @@ fn test_request_range_header() { headers.insert(header::RANGE, header::HeaderValue::from_static("bytes=0-4")); - let req = HttpRequest::new(Method::GET, "/".to_owned(), - Version::HTTP_11, headers, String::new(), Payload::empty()); + let req = HttpRequest::new(Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); let ranges = req.range(100).unwrap(); assert_eq!(ranges.len(), 1); assert_eq!(ranges[0].start, 0); @@ -71,10 +80,8 @@ fn test_request_range_header() { #[test] fn test_request_query() { - let req = HttpRequest::new(Method::GET, "/".to_owned(), - Version::HTTP_11, HeaderMap::new(), - "id=test".to_owned(), Payload::empty()); - + let req = HttpRequest::new(Method::GET, Uri::from_str("/?id=test").unwrap(), + Version::HTTP_11, HeaderMap::new(), Payload::empty()); assert_eq!(req.query_string(), "id=test"); let query = req.query(); assert_eq!(&query["id"], "test"); @@ -82,9 +89,8 @@ fn test_request_query() { #[test] fn test_request_match_info() { - let mut req = HttpRequest::new(Method::GET, "/value/".to_owned(), - Version::HTTP_11, HeaderMap::new(), - "?id=test".to_owned(), Payload::empty()); + let mut req = HttpRequest::new(Method::GET, Uri::from_str("/value/?id=test").unwrap(), + Version::HTTP_11, HeaderMap::new(), Payload::empty()); let rec = RouteRecognizer::new("/".to_owned(), vec![("/{key}/".to_owned(), 1)]); let (params, _) = rec.recognize(req.path()).unwrap(); @@ -97,15 +103,16 @@ fn test_request_match_info() { #[test] fn test_chunked() { let req = HttpRequest::new( - Method::GET, "/".to_owned(), Version::HTTP_11, HeaderMap::new(), - String::new(), Payload::empty()); + Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, HeaderMap::new(), Payload::empty()); assert!(!req.chunked().unwrap()); let mut headers = HeaderMap::new(); headers.insert(header::TRANSFER_ENCODING, header::HeaderValue::from_static("chunked")); let req = HttpRequest::new( - Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new(), Payload::empty()); + Method::GET, Uri::from_str("/").unwrap(), Version::HTTP_11, + headers, Payload::empty()); assert!(req.chunked().unwrap()); let mut headers = HeaderMap::new(); @@ -114,6 +121,7 @@ fn test_chunked() { headers.insert(header::TRANSFER_ENCODING, header::HeaderValue::from_str(s).unwrap()); let req = HttpRequest::new( - Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new(), Payload::empty()); + Method::GET, Uri::from_str("/").unwrap(), + Version::HTTP_11, headers, Payload::empty()); assert!(req.chunked().is_err()); } diff --git a/tests/test_httpresponse.rs b/tests/test_httpresponse.rs index 983ee0af8..8a239ae70 100644 --- a/tests/test_httpresponse.rs +++ b/tests/test_httpresponse.rs @@ -3,8 +3,9 @@ extern crate http; extern crate time; use actix_web::*; +use std::str::FromStr; use time::Duration; -use http::{header, Method, Version, HeaderMap}; +use http::{header, Method, Version, HeaderMap, Uri}; #[test] @@ -14,7 +15,7 @@ fn test_response_cookies() { header::HeaderValue::from_static("cookie1=value1; cookie2=value2")); let mut req = HttpRequest::new( - Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new(), Payload::empty()); + Method::GET, Uri::from_str("/").unwrap(), Version::HTTP_11, headers, Payload::empty()); let cookies = req.load_cookies().unwrap(); let resp = httpcodes::HTTPOk