From 1949d44d0c44a550f16fb93f3ee3ce0e6b7b70f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 20 Feb 2017 16:30:14 +0100 Subject: [PATCH] Hash-fetch errors in case upstream returns non-200 (#4599) --- hash-fetch/src/client.rs | 139 ++++++++++++++++++++++++++++++++++++++ hash-fetch/src/urlhint.rs | 10 +-- util/fetch/src/client.rs | 33 +++++++++ util/fetch/src/lib.rs | 2 + 4 files changed, 179 insertions(+), 5 deletions(-) diff --git a/hash-fetch/src/client.rs b/hash-fetch/src/client.rs index 453f8925a..cffc10e63 100644 --- a/hash-fetch/src/client.rs +++ b/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 for Error { fn from(error: FetchError) -> Self { Error::Fetch(error) @@ -115,6 +134,10 @@ impl HashFetch for Client { let future = self.fetch.fetch(&url).then(move |result| { fn validate_hash(path: PathBuf, hash: H256, result: Result) -> Result { 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; + + fn new() -> Result 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); + } +} diff --git a/hash-fetch/src/urlhint.rs b/hash-fetch/src/urlhint.rs index 227f24dc3..1588b5482 100644 --- a/hash-fetch/src/urlhint.rs +++ b/hash-fetch/src/urlhint.rs @@ -264,7 +264,7 @@ fn as_string(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>>, pub responses: Mutex>>, } - const REGISTRAR: &'static str = "8e4e9b13d4b45cb0befc93c3061b1408f67316b2"; - const URLHINT: &'static str = "deadbeefcafe0000000000000000000000000000"; + pub const REGISTRAR: &'static str = "8e4e9b13d4b45cb0befc93c3061b1408f67316b2"; + 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( diff --git a/util/fetch/src/client.rs b/util/fetch/src/client.rs index 09fe4741b..18c8d87d9 100644 --- a/util/fetch/src/client.rs +++ b/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); 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> for Abort { } } +/// Fetch pub trait Fetch: Clone + Send + Sync + 'static { + /// Result type type Result: Future + Send + 'static; + /// Creates new Fetch object. fn new() -> Result 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)>, 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 for Error { enum ResponseInner { Response(reqwest::Response), Reader(Box), + 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(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 { 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), }; diff --git a/util/fetch/src/lib.rs b/util/fetch/src/lib.rs index 34091f4bc..21905c532 100644 --- a/util/fetch/src/lib.rs +++ b/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;