From aa54f4b078fc58c1b9d1ef4d393b5e95d7896ecf Mon Sep 17 00:00:00 2001 From: Pepo Viola Date: Wed, 7 Oct 2020 12:32:10 -0300 Subject: [PATCH 01/12] initial work to review --- src/content/content_location.rs | 107 ++++++++++++++++++++++++++++++++ src/content/mod.rs | 2 + 2 files changed, 109 insertions(+) create mode 100644 src/content/content_location.rs diff --git a/src/content/content_location.rs b/src/content/content_location.rs new file mode 100644 index 00000000..919f302b --- /dev/null +++ b/src/content/content_location.rs @@ -0,0 +1,107 @@ +use crate::headers::{HeaderName, HeaderValue, Headers, CONTENT_LOCATION}; +use crate::{Status, Url}; + +/// +/// # Specifications +/// +/// - [RFC 7230, section 3.3.2: Content-Length](https://tools.ietf.org/html/rfc7230#section-3.3.2) +/// +/// # Examples +/// +/// ``` +/// # fn main() -> http_types::Result<()> { +/// # +/// use http_types::Response; +/// use http_types::content::{ContentLocation}; +/// +/// let content_location = ContentLocation::new("https://example.net/".to_string()); +/// +/// let mut res = Response::new(200); +/// content_location.apply(&mut res); +/// +/// let content_location = ContentLocation::from_headers(res)?.unwrap(); +/// assert_eq!(content_location.location(), "https://example.net/"); +/// # +/// # Ok(()) } +/// ``` +#[derive(Debug)] +pub struct ContentLocation { + url: String, +} + +#[allow(clippy::len_without_is_empty)] +impl ContentLocation { + /// Create a new instance of `Content-Location` header. + pub fn new(url: String) -> Self { + Self { url } + } + + /// Create a new instance from headers. + pub fn from_headers(headers: impl AsRef) -> crate::Result> { + let headers = match headers.as_ref().get(CONTENT_LOCATION) { + Some(headers) => headers, + None => return Ok(None), + }; + + // If we successfully parsed the header then there's always at least one + // entry. We want the last entry. + let value = headers.iter().last().unwrap(); + let url = Url::parse(value.as_str().trim()).status(400)?; + Ok(Some(Self { url : url.into_string() })) + } + + /// Sets the header. + pub fn apply(&self, mut headers: impl AsMut) { + headers.as_mut().insert(self.name(), self.value()); + } + + /// Get the `HeaderName`. + pub fn name(&self) -> HeaderName { + CONTENT_LOCATION + } + + /// Get the `HeaderValue`. + pub fn value(&self) -> HeaderValue { + let output = format!("{}", self.url); + + // SAFETY: the internal string is validated to be ASCII. + unsafe { HeaderValue::from_bytes_unchecked(output.into()) } + } + + /// Get the url. + pub fn location(&self) -> String { + String::from(&self.url) + } + + /// Set the url. + pub fn set_location(&mut self, location: &str) { + self.url = location.to_string(); + } +} + +#[cfg(test)] +mod test { + use super::*; + use crate::headers::Headers; + + #[test] + fn smoke() -> crate::Result<()> { + let content_location = ContentLocation::new("https://example.net/test".to_string()); + + let mut headers = Headers::new(); + content_location.apply(&mut headers); + + let content_location = ContentLocation::from_headers(headers)?.unwrap(); + assert_eq!(content_location.location(), "https://example.net/test"); + Ok(()) + } + + #[test] + fn bad_request_on_parse_error() -> crate::Result<()> { + let mut headers = Headers::new(); + headers.insert(CONTENT_LOCATION, ""); + let err = ContentLocation::from_headers(headers).unwrap_err(); + assert_eq!(err.status(), 400); + Ok(()) + } +} diff --git a/src/content/mod.rs b/src/content/mod.rs index 1b20ded0..6b7a48d0 100644 --- a/src/content/mod.rs +++ b/src/content/mod.rs @@ -10,6 +10,7 @@ pub mod content_encoding; mod encoding; mod encoding_proposal; +mod content_location; #[doc(inline)] pub use accept_encoding::AcceptEncoding; @@ -17,3 +18,4 @@ pub use accept_encoding::AcceptEncoding; pub use content_encoding::ContentEncoding; pub use encoding::Encoding; pub use encoding_proposal::EncodingProposal; +pub use content_location::ContentLocation; From 96052dd8a03dc1fab75c647455ba78de983d9227 Mon Sep 17 00:00:00 2001 From: Pepo Viola Date: Wed, 7 Oct 2020 19:17:08 -0300 Subject: [PATCH 02/12] use to_string and change ref to ietf --- src/content/content_location.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/content/content_location.rs b/src/content/content_location.rs index 919f302b..86bce071 100644 --- a/src/content/content_location.rs +++ b/src/content/content_location.rs @@ -4,7 +4,7 @@ use crate::{Status, Url}; /// /// # Specifications /// -/// - [RFC 7230, section 3.3.2: Content-Length](https://tools.ietf.org/html/rfc7230#section-3.3.2) +/// - [RFC 7230, section 3.3.2: Content-Length](https://tools.ietf.org/html/rfc7231#section-3.1.4.2) /// /// # Examples /// @@ -70,7 +70,7 @@ impl ContentLocation { /// Get the url. pub fn location(&self) -> String { - String::from(&self.url) + self.url.to_string() } /// Set the url. From b504712b63ef2546d84df55d08c6d73c6eff6d6c Mon Sep 17 00:00:00 2001 From: Pepo Viola Date: Fri, 9 Oct 2020 10:31:51 -0300 Subject: [PATCH 03/12] changes from feedback, use Url type for url field --- src/content/content_location.rs | 35 ++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/src/content/content_location.rs b/src/content/content_location.rs index 86bce071..14f7202b 100644 --- a/src/content/content_location.rs +++ b/src/content/content_location.rs @@ -1,10 +1,13 @@ use crate::headers::{HeaderName, HeaderValue, Headers, CONTENT_LOCATION}; use crate::{Status, Url}; +use std::convert::TryInto; + +/// Indicates an alternate location for the returned data. /// /// # Specifications /// -/// - [RFC 7230, section 3.3.2: Content-Length](https://tools.ietf.org/html/rfc7231#section-3.1.4.2) +/// - [RFC 7231, section 3.1.4.2: Content-Length](https://tools.ietf.org/html/rfc7231#section-3.1.4.2) /// /// # Examples /// @@ -26,18 +29,21 @@ use crate::{Status, Url}; /// ``` #[derive(Debug)] pub struct ContentLocation { - url: String, + url: Url, } -#[allow(clippy::len_without_is_empty)] impl ContentLocation { /// Create a new instance of `Content-Location` header. pub fn new(url: String) -> Self { - Self { url } + Self { url : Url::parse(&url).unwrap() } } /// Create a new instance from headers. - pub fn from_headers(headers: impl AsRef) -> crate::Result> { + pub fn from_headers(base_url: U, headers: impl AsRef) -> crate::Result> + where + U: TryInto, + U::Error: std::fmt::Debug, + { let headers = match headers.as_ref().get(CONTENT_LOCATION) { Some(headers) => headers, None => return Ok(None), @@ -46,8 +52,9 @@ impl ContentLocation { // If we successfully parsed the header then there's always at least one // entry. We want the last entry. let value = headers.iter().last().unwrap(); - let url = Url::parse(value.as_str().trim()).status(400)?; - Ok(Some(Self { url : url.into_string() })) + let base = base_url.try_into().expect("Could not convert into a valid url"); + let url = base.join(value.as_str().trim()).status(400)?; + Ok(Some(Self { url })) } /// Sets the header. @@ -62,7 +69,7 @@ impl ContentLocation { /// Get the `HeaderValue`. pub fn value(&self) -> HeaderValue { - let output = format!("{}", self.url); + let output = format!("{}", self.url.as_str()); // SAFETY: the internal string is validated to be ASCII. unsafe { HeaderValue::from_bytes_unchecked(output.into()) } @@ -75,7 +82,7 @@ impl ContentLocation { /// Set the url. pub fn set_location(&mut self, location: &str) { - self.url = location.to_string(); + self.url = Url::parse(location).unwrap(); } } @@ -86,21 +93,21 @@ mod test { #[test] fn smoke() -> crate::Result<()> { - let content_location = ContentLocation::new("https://example.net/test".to_string()); + let content_location = ContentLocation::new("https://example.net/test.json".to_string()); let mut headers = Headers::new(); content_location.apply(&mut headers); - let content_location = ContentLocation::from_headers(headers)?.unwrap(); - assert_eq!(content_location.location(), "https://example.net/test"); + let content_location = ContentLocation::from_headers( Url::parse("https://example.net/").unwrap(), headers )?.unwrap(); + assert_eq!(content_location.location(), "https://example.net/test.json"); Ok(()) } #[test] fn bad_request_on_parse_error() -> crate::Result<()> { let mut headers = Headers::new(); - headers.insert(CONTENT_LOCATION, ""); - let err = ContentLocation::from_headers(headers).unwrap_err(); + headers.insert(CONTENT_LOCATION, "htt://"); + let err = ContentLocation::from_headers(Url::parse("https://example.net").unwrap(), headers).unwrap_err(); assert_eq!(err.status(), 400); Ok(()) } From 05c5cf3214510b1401b49b0a55ec13f0592c6ea6 Mon Sep 17 00:00:00 2001 From: Pepo Viola Date: Fri, 9 Oct 2020 13:12:13 -0300 Subject: [PATCH 04/12] fix test --- src/content/content_location.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/content/content_location.rs b/src/content/content_location.rs index 14f7202b..a4459583 100644 --- a/src/content/content_location.rs +++ b/src/content/content_location.rs @@ -14,7 +14,7 @@ use std::convert::TryInto; /// ``` /// # fn main() -> http_types::Result<()> { /// # -/// use http_types::Response; +/// use http_types::{Response,Url}; /// use http_types::content::{ContentLocation}; /// /// let content_location = ContentLocation::new("https://example.net/".to_string()); @@ -22,7 +22,7 @@ use std::convert::TryInto; /// let mut res = Response::new(200); /// content_location.apply(&mut res); /// -/// let content_location = ContentLocation::from_headers(res)?.unwrap(); +/// let content_location = ContentLocation::from_headers(Url::parse("https://example.net/").unwrap(),res)?.unwrap(); /// assert_eq!(content_location.location(), "https://example.net/"); /// # /// # Ok(()) } From b537b583cd2baefa542462e2aa3ca5de6ec11bf2 Mon Sep 17 00:00:00 2001 From: Pepo Viola Date: Fri, 9 Oct 2020 14:25:51 -0300 Subject: [PATCH 05/12] changes from clippy suggestions --- src/content/content_location.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/content/content_location.rs b/src/content/content_location.rs index a4459583..88fb2298 100644 --- a/src/content/content_location.rs +++ b/src/content/content_location.rs @@ -69,7 +69,7 @@ impl ContentLocation { /// Get the `HeaderValue`. pub fn value(&self) -> HeaderValue { - let output = format!("{}", self.url.as_str()); + let output = self.url.to_string(); // SAFETY: the internal string is validated to be ASCII. unsafe { HeaderValue::from_bytes_unchecked(output.into()) } From 09fb048b9cf21bd69f02695fd02f142dc37107d9 Mon Sep 17 00:00:00 2001 From: Javier Viola Date: Fri, 9 Oct 2020 20:31:09 -0300 Subject: [PATCH 06/12] Update src/content/content_location.rs Co-authored-by: Jeremiah Senkpiel --- src/content/content_location.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/content/content_location.rs b/src/content/content_location.rs index 88fb2298..d46526a9 100644 --- a/src/content/content_location.rs +++ b/src/content/content_location.rs @@ -52,7 +52,7 @@ impl ContentLocation { // If we successfully parsed the header then there's always at least one // entry. We want the last entry. let value = headers.iter().last().unwrap(); - let base = base_url.try_into().expect("Could not convert into a valid url"); + let base = base_url.try_into()?; let url = base.join(value.as_str().trim()).status(400)?; Ok(Some(Self { url })) } From fdcb1a6cd60a0d817efc50b7982db3b363066803 Mon Sep 17 00:00:00 2001 From: Pepo Viola Date: Sat, 10 Oct 2020 17:02:58 -0300 Subject: [PATCH 07/12] use Url as type for struct and fmt --- src/content/content_location.rs | 26 +++++++++++++++----------- src/content/mod.rs | 4 ++-- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/content/content_location.rs b/src/content/content_location.rs index d46526a9..cb1aa173 100644 --- a/src/content/content_location.rs +++ b/src/content/content_location.rs @@ -7,7 +7,7 @@ use std::convert::TryInto; /// /// # Specifications /// -/// - [RFC 7231, section 3.1.4.2: Content-Length](https://tools.ietf.org/html/rfc7231#section-3.1.4.2) +/// - [RFC 7231, section 3.1.4.2: Content-Location](https://tools.ietf.org/html/rfc7231#section-3.1.4.2) /// /// # Examples /// @@ -17,7 +17,7 @@ use std::convert::TryInto; /// use http_types::{Response,Url}; /// use http_types::content::{ContentLocation}; /// -/// let content_location = ContentLocation::new("https://example.net/".to_string()); +/// let content_location = ContentLocation::new(Url::parse("https://example.net/")?); /// /// let mut res = Response::new(200); /// content_location.apply(&mut res); @@ -34,8 +34,8 @@ pub struct ContentLocation { impl ContentLocation { /// Create a new instance of `Content-Location` header. - pub fn new(url: String) -> Self { - Self { url : Url::parse(&url).unwrap() } + pub fn new(url: Url) -> Self { + Self { url } } /// Create a new instance from headers. @@ -43,7 +43,7 @@ impl ContentLocation { where U: TryInto, U::Error: std::fmt::Debug, - { + { let headers = match headers.as_ref().get(CONTENT_LOCATION) { Some(headers) => headers, None => return Ok(None), @@ -52,7 +52,7 @@ impl ContentLocation { // If we successfully parsed the header then there's always at least one // entry. We want the last entry. let value = headers.iter().last().unwrap(); - let base = base_url.try_into()?; + let base = base_url.try_into().unwrap(); let url = base.join(value.as_str().trim()).status(400)?; Ok(Some(Self { url })) } @@ -81,8 +81,8 @@ impl ContentLocation { } /// Set the url. - pub fn set_location(&mut self, location: &str) { - self.url = Url::parse(location).unwrap(); + pub fn set_location(&mut self, location: Url) { + self.url = location } } @@ -93,12 +93,14 @@ mod test { #[test] fn smoke() -> crate::Result<()> { - let content_location = ContentLocation::new("https://example.net/test.json".to_string()); + let content_location = ContentLocation::new(Url::parse("https://example.net/test.json")?); let mut headers = Headers::new(); content_location.apply(&mut headers); - let content_location = ContentLocation::from_headers( Url::parse("https://example.net/").unwrap(), headers )?.unwrap(); + let content_location = + ContentLocation::from_headers(Url::parse("https://example.net/").unwrap(), headers)? + .unwrap(); assert_eq!(content_location.location(), "https://example.net/test.json"); Ok(()) } @@ -107,7 +109,9 @@ mod test { fn bad_request_on_parse_error() -> crate::Result<()> { let mut headers = Headers::new(); headers.insert(CONTENT_LOCATION, "htt://"); - let err = ContentLocation::from_headers(Url::parse("https://example.net").unwrap(), headers).unwrap_err(); + let err = + ContentLocation::from_headers(Url::parse("https://example.net").unwrap(), headers) + .unwrap_err(); assert_eq!(err.status(), 400); Ok(()) } diff --git a/src/content/mod.rs b/src/content/mod.rs index 6b7a48d0..cc0b4744 100644 --- a/src/content/mod.rs +++ b/src/content/mod.rs @@ -8,14 +8,14 @@ pub mod accept_encoding; pub mod content_encoding; +mod content_location; mod encoding; mod encoding_proposal; -mod content_location; #[doc(inline)] pub use accept_encoding::AcceptEncoding; #[doc(inline)] pub use content_encoding::ContentEncoding; +pub use content_location::ContentLocation; pub use encoding::Encoding; pub use encoding_proposal::EncodingProposal; -pub use content_location::ContentLocation; From ccb085febf30d65ae8b3a9690c5676e48cfcdbfb Mon Sep 17 00:00:00 2001 From: Javier Viola Date: Mon, 12 Oct 2020 15:19:32 -0300 Subject: [PATCH 08/12] Update src/content/content_location.rs Co-authored-by: Jeremiah Senkpiel --- src/content/content_location.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/content/content_location.rs b/src/content/content_location.rs index cb1aa173..fe3a4635 100644 --- a/src/content/content_location.rs +++ b/src/content/content_location.rs @@ -52,7 +52,7 @@ impl ContentLocation { // If we successfully parsed the header then there's always at least one // entry. We want the last entry. let value = headers.iter().last().unwrap(); - let base = base_url.try_into().unwrap(); + let base = base_url.try_into()?; let url = base.join(value.as_str().trim()).status(400)?; Ok(Some(Self { url })) } From f83e425adaaf147dcac5122c0cabfdc007b6d641 Mon Sep 17 00:00:00 2001 From: Pepo Viola Date: Mon, 12 Oct 2020 21:11:10 -0300 Subject: [PATCH 09/12] use match to check error --- src/content/content_location.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/content/content_location.rs b/src/content/content_location.rs index fe3a4635..12ed2523 100644 --- a/src/content/content_location.rs +++ b/src/content/content_location.rs @@ -52,9 +52,14 @@ impl ContentLocation { // If we successfully parsed the header then there's always at least one // entry. We want the last entry. let value = headers.iter().last().unwrap(); - let base = base_url.try_into()?; - let url = base.join(value.as_str().trim()).status(400)?; - Ok(Some(Self { url })) + let base = base_url.try_into().ok(); + match base { + Some(base) => { + let url = base.join(value.as_str().trim()).status(400)?; + Ok(Some(Self { url })) + } + None => Ok(None), + } } /// Sets the header. From f4abf11dd19b3b3cc71ab2eca8779058efea4b2d Mon Sep 17 00:00:00 2001 From: Javier Viola Date: Sat, 17 Oct 2020 13:06:59 -0300 Subject: [PATCH 10/12] Update src/content/content_location.rs Co-authored-by: Yoshua Wuyts --- src/content/content_location.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/content/content_location.rs b/src/content/content_location.rs index 12ed2523..b2fdfc98 100644 --- a/src/content/content_location.rs +++ b/src/content/content_location.rs @@ -22,7 +22,8 @@ use std::convert::TryInto; /// let mut res = Response::new(200); /// content_location.apply(&mut res); /// -/// let content_location = ContentLocation::from_headers(Url::parse("https://example.net/").unwrap(),res)?.unwrap(); +/// let url = Url::parse("https://example.net/")?; +/// let content_location = ContentLocation::from_headers(url, res)?.unwrap(); /// assert_eq!(content_location.location(), "https://example.net/"); /// # /// # Ok(()) } From cc1c8e91d9cdabef0801b16ab7d00b4e916ff324 Mon Sep 17 00:00:00 2001 From: Pepo Viola Date: Sat, 17 Oct 2020 21:00:12 -0300 Subject: [PATCH 11/12] use create new error approach --- src/content/content_location.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/content/content_location.rs b/src/content/content_location.rs index b2fdfc98..00d8af49 100644 --- a/src/content/content_location.rs +++ b/src/content/content_location.rs @@ -1,4 +1,5 @@ use crate::headers::{HeaderName, HeaderValue, Headers, CONTENT_LOCATION}; +use crate::{Error, StatusCode}; use crate::{Status, Url}; use std::convert::TryInto; @@ -53,14 +54,14 @@ impl ContentLocation { // If we successfully parsed the header then there's always at least one // entry. We want the last entry. let value = headers.iter().last().unwrap(); - let base = base_url.try_into().ok(); - match base { - Some(base) => { - let url = base.join(value.as_str().trim()).status(400)?; - Ok(Some(Self { url })) - } - None => Ok(None), - } + let base = base_url.try_into().map_err(|_| { + let mut err = Error::new_adhoc("Invalid base url provided"); + err.set_status(StatusCode::BadRequest); + err + })?; + + let url = base.join(value.as_str().trim()).status(400)?; + Ok(Some(Self { url })) } /// Sets the header. From 85a5089c29fd1e43db6248f3abb2b141da4c359d Mon Sep 17 00:00:00 2001 From: Pepo Viola Date: Mon, 2 Nov 2020 23:10:02 -0300 Subject: [PATCH 12/12] changes from feedback, use bail_status --- src/content/content_location.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/content/content_location.rs b/src/content/content_location.rs index 00d8af49..e807cfd6 100644 --- a/src/content/content_location.rs +++ b/src/content/content_location.rs @@ -1,6 +1,5 @@ use crate::headers::{HeaderName, HeaderValue, Headers, CONTENT_LOCATION}; -use crate::{Error, StatusCode}; -use crate::{Status, Url}; +use crate::{bail_status as bail, Status, Url}; use std::convert::TryInto; @@ -54,11 +53,10 @@ impl ContentLocation { // If we successfully parsed the header then there's always at least one // entry. We want the last entry. let value = headers.iter().last().unwrap(); - let base = base_url.try_into().map_err(|_| { - let mut err = Error::new_adhoc("Invalid base url provided"); - err.set_status(StatusCode::BadRequest); - err - })?; + let base = match base_url.try_into() { + Ok(b) => b, + Err(_) => bail!(400, "Invalid base url provided"), + }; let url = base.join(value.as_str().trim()).status(400)?; Ok(Some(Self { url }))