Hash-fetch errors in case upstream returns non-200 (#4599)

hash-fetch/src/client.rs

@@ -50,12 +50,31 @@ pub enum Error {
 		/// Computed hash
 		got: H256,
 	},
+	/// Server didn't respond with OK status.
+	InvalidStatus,
 	/// IO Error while validating hash.
 	IO(io::Error),
 	/// Error during fetch.
 	Fetch(FetchError),
 }
 
+#[cfg(test)]
+impl PartialEq for Error {
+	fn eq(&self, other: &Self) -> bool {
+		use Error::*;
+		match (self, other)  {
+			(&HashMismatch { expected, got }, &HashMismatch { expected: e, got: g }) => {
+				expected == e && got == g
+			},
+			(&NoResolution, &NoResolution) => true,
+			(&InvalidStatus, &InvalidStatus) => true,
+			(&IO(_), &IO(_)) => true,
+			(&Fetch(_), &Fetch(_)) => true,
+			_ => false,
+		}
+	}
+}
+
 impl From<FetchError> for Error {
 	fn from(error: FetchError) -> Self {
 		Error::Fetch(error)
@@ -115,6 +134,10 @@ impl<F: Fetch + 'static> HashFetch for Client<F> {
 				let future = self.fetch.fetch(&url).then(move |result| {
 					fn validate_hash(path: PathBuf, hash: H256, result: Result<Response, FetchError>) -> Result<PathBuf, Error> {
 						let response = result?;
+						if !response.is_success() {
+							return Err(Error::InvalidStatus);
+						}
+
 						// Read the response
 						let mut reader = io::BufReader::new(response);
 						let mut writer = io::BufWriter::new(fs::File::create(&path)?);
@@ -160,3 +183,119 @@ fn random_temp_path() -> PathBuf {
 	path.push(file);
 	path
 }
+
+#[cfg(test)]
+mod tests {
+	use std::sync::{Arc, mpsc};
+	use util::{Mutex, FromHex};
+	use futures::future;
+	use fetch::{self, Fetch};
+	use parity_reactor::Remote;
+	use urlhint::tests::{FakeRegistrar, URLHINT};
+	use super::{Error, Client, HashFetch};
+
+
+	#[derive(Clone)]
+	struct FakeFetch {
+		return_success: bool
+	}
+
+	impl Fetch for FakeFetch {
+		type Result = future::Ok<fetch::Response, fetch::Error>;
+
+		fn new() -> Result<Self, fetch::Error> where Self: Sized {
+			Ok(FakeFetch { return_success: true })
+		}
+
+		fn fetch_with_abort(&self, url: &str, _abort: fetch::Abort) -> Self::Result {
+			assert_eq!(url, "https://ethcore.io/assets/images/ethcore-black-horizontal.png");
+			future::ok(if self.return_success {
+				let cursor = ::std::io::Cursor::new(b"result");
+				fetch::Response::from_reader(cursor)
+			} else {
+				fetch::Response::not_found()
+			})
+		}
+	}
+
+	fn registrar() -> FakeRegistrar {
+		let mut registrar = FakeRegistrar::new();
+		registrar.responses = Mutex::new(vec![
+			Ok(format!("000000000000000000000000{}", URLHINT).from_hex().unwrap()),
+			Ok("00000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000000000000000000000000000000deadcafebeefbeefcafedeaddeedfeedffffffff000000000000000000000000000000000000000000000000000000000000003d68747470733a2f2f657468636f72652e696f2f6173736574732f696d616765732f657468636f72652d626c61636b2d686f72697a6f6e74616c2e706e67000000".from_hex().unwrap()),
+		]);
+		registrar
+	}
+
+	#[test]
+	fn should_return_error_if_hash_not_found() {
+		// given
+		let contract = Arc::new(FakeRegistrar::new());
+		let fetch = FakeFetch { return_success: false };
+		let client = Client::with_fetch(contract.clone(), fetch, Remote::new_sync());
+
+		// when
+		let (tx, rx) = mpsc::channel();
+		client.fetch(2.into(), Box::new(move |result| {
+			tx.send(result).unwrap();
+		}));
+
+		// then
+		let result = rx.recv().unwrap();
+		assert_eq!(result.unwrap_err(), Error::NoResolution);
+	}
+
+	#[test]
+	fn should_return_error_if_response_is_not_successful() {
+		// given
+		let registrar = Arc::new(registrar());
+		let fetch = FakeFetch { return_success: false };
+		let client = Client::with_fetch(registrar.clone(), fetch, Remote::new_sync());
+
+		// when
+		let (tx, rx) = mpsc::channel();
+		client.fetch(2.into(), Box::new(move |result| {
+			tx.send(result).unwrap();
+		}));
+
+		// then
+		let result = rx.recv().unwrap();
+		assert_eq!(result.unwrap_err(), Error::InvalidStatus);
+	}
+	#[test]
+	fn should_return_hash_mismatch() {
+		// given
+		let registrar = Arc::new(registrar());
+		let fetch = FakeFetch { return_success: true };
+		let client = Client::with_fetch(registrar.clone(), fetch, Remote::new_sync());
+
+		// when
+		let (tx, rx) = mpsc::channel();
+		client.fetch(2.into(), Box::new(move |result| {
+			tx.send(result).unwrap();
+		}));
+
+		// then
+		let result = rx.recv().unwrap();
+		let hash = "0x06b0a4f426f6713234b2d4b2468640bc4e0bb72657a920ad24c5087153c593c8".into();
+		assert_eq!(result.unwrap_err(), Error::HashMismatch { expected: 2.into(), got: hash });
+	}
+
+	#[test]
+	fn should_return_path_if_hash_matches() {
+		// given
+		let registrar = Arc::new(registrar());
+		let fetch = FakeFetch { return_success: true };
+		let client = Client::with_fetch(registrar.clone(), fetch, Remote::new_sync());
+
+		// when
+		let (tx, rx) = mpsc::channel();
+		client.fetch("0x06b0a4f426f6713234b2d4b2468640bc4e0bb72657a920ad24c5087153c593c8".into(), Box::new(move |result| {
+			tx.send(result).unwrap();
+		}));
+
+		// then
+		let result = rx.recv().unwrap();
+		assert!(result.is_ok(), "Should return path, got: {:?}", result);
+	}
+}

hash-fetch/src/urlhint.rs

@@ -264,7 +264,7 @@ fn as_string<T: fmt::Debug>(e: T) -> String {
 }
 
 #[cfg(test)]
-mod tests {
+pub mod tests {
 	use std::sync::Arc;
 	use std::str::FromStr;
 	use rustc_serialize::hex::FromHex;
@@ -273,16 +273,16 @@ mod tests {
 	use super::guess_mime_type;
 	use util::{Bytes, Address, Mutex, ToPretty};
 
-	struct FakeRegistrar {
+	pub struct FakeRegistrar {
 		pub calls: Arc<Mutex<Vec<(String, String)>>>,
 		pub responses: Mutex<Vec<Result<Bytes, String>>>,
 	}
 
-	const REGISTRAR: &'static str = "8e4e9b13d4b45cb0befc93c3061b1408f67316b2";
+	pub const REGISTRAR: &'static str = "8e4e9b13d4b45cb0befc93c3061b1408f67316b2";
-	const URLHINT: &'static str = "deadbeefcafe0000000000000000000000000000";
+	pub const URLHINT: &'static str = "deadbeefcafe0000000000000000000000000000";
 
 	impl FakeRegistrar {
-		fn new() -> Self {
+		pub fn new() -> Self {
 			FakeRegistrar {
 				calls: Arc::new(Mutex::new(Vec::new())),
 				responses: Mutex::new(

util/fetch/src/client.rs

@@ -26,10 +26,12 @@ use mime::{self, Mime};
 use parking_lot::RwLock;
 use reqwest;
 
+/// Fetch abort control
 #[derive(Default, Debug, Clone)]
 pub struct Abort(Arc<AtomicBool>);
 
 impl Abort {
+	/// Returns `true` if request is aborted.
 	pub fn is_aborted(&self) -> bool {
 		self.0.load(atomic::Ordering::SeqCst)
 	}
@@ -41,9 +43,12 @@ impl From<Arc<AtomicBool>> for Abort {
 	}
 }
 
+/// Fetch
 pub trait Fetch: Clone + Send + Sync + 'static {
+	/// Result type
 	type Result: Future<Item=Response, Error=Error> + Send + 'static;
 
+	/// Creates new Fetch object.
 	fn new() -> Result<Self, Error> where Self: Sized;
 
 	/// Spawn the future in context of this `Fetch` thread pool.
@@ -76,6 +81,7 @@ pub trait Fetch: Clone + Send + Sync + 'static {
 
 const CLIENT_TIMEOUT_SECONDS: u64 = 5;
 
+/// Fetch client
 pub struct Client {
 	client: RwLock<(time::Instant, Arc<reqwest::Client>)>,
 	pool: CpuPool,
@@ -189,9 +195,12 @@ impl Future for FetchTask {
 	}
 }
 
+/// Fetch Error
 #[derive(Debug)]
 pub enum Error {
+	/// Internal fetch error
 	Fetch(reqwest::Error),
+	/// Request aborted
 	Aborted,
 }
 
@@ -204,17 +213,20 @@ impl From<reqwest::Error> for Error {
 enum ResponseInner {
 	Response(reqwest::Response),
 	Reader(Box<io::Read + Send>),
+	NotFound,
 }
 
 impl fmt::Debug for ResponseInner {
 	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
 		match *self {
 			ResponseInner::Response(ref response) => response.fmt(f),
+			ResponseInner::NotFound => write!(f, "Not found"),
 			ResponseInner::Reader(_) => write!(f, "io Reader"),
 		}
 	}
 }
 
+/// A fetch response type.
 #[derive(Debug)]
 pub struct Response {
 	inner: ResponseInner,
@@ -224,6 +236,7 @@ pub struct Response {
 }
 
 impl Response {
+	/// Creates new successfuly response reading from a file.
 	pub fn from_reader<R: io::Read + Send + 'static>(reader: R) -> Self {
 		Response {
 			inner: ResponseInner::Reader(Box::new(reader)),
@@ -233,13 +246,31 @@ impl Response {
 		}
 	}
 
+	/// Creates 404 response (useful for tests)
+	pub fn not_found() -> Self {
+		Response {
+			inner: ResponseInner::NotFound,
+			abort: Abort::default(),
+			limit: None,
+			read: 0,
+		}
+	}
+
+	/// Returns status code of this response.
 	pub fn status(&self) -> reqwest::StatusCode {
 		match self.inner {
 			ResponseInner::Response(ref r) => *r.status(),
+			ResponseInner::NotFound => reqwest::StatusCode::NotFound,
 			_ => reqwest::StatusCode::Ok,
 		}
 	}
 
+	/// Returns `true` if response status code is successful.
+	pub fn is_success(&self) -> bool {
+		self.status() == reqwest::StatusCode::Ok
+	}
+
+	/// Returns `true` if content type of this response is `text/html`
 	pub fn is_html(&self) -> bool {
 		match self.content_type() {
 			Some(Mime(mime::TopLevel::Text, mime::SubLevel::Html, _)) => true,
@@ -247,6 +278,7 @@ impl Response {
 		}
 	}
 
+	/// Returns content type of this response (if present)
 	pub fn content_type(&self) -> Option<Mime> {
 		match self.inner {
 			ResponseInner::Response(ref r) => {
@@ -266,6 +298,7 @@ impl io::Read for Response {
 
 		let res = match self.inner {
 			ResponseInner::Response(ref mut response) => response.read(buf),
+			ResponseInner::NotFound => return Ok(0),
 			ResponseInner::Reader(ref mut reader) => reader.read(buf),
 		};
 

util/fetch/src/lib.rs

@@ -16,6 +16,8 @@
 
 //! A service to fetch any HTTP / HTTPS content.
 
+#![warn(missing_docs)]
+
 #[macro_use]
 extern crate log;