From d935ca0ef8ea9478900b26897701f820b9542bf2 Mon Sep 17 00:00:00 2001 From: Sam Rijs Date: Mon, 5 Feb 2018 20:23:40 +1100 Subject: [PATCH] move to the failure crate for error reporting --- Cargo.toml | 1 + src/engine/mod.rs | 41 ++++++++++------------------ src/interactors/crates.rs | 26 +++++++----------- src/interactors/github.rs | 52 +++++++++++------------------------- src/main.rs | 1 + src/models/crates.rs | 10 +++---- src/models/repo.rs | 35 ++++++++++-------------- src/parsers/manifest.rs | 30 ++++++++------------- src/server/mod.rs | 6 ++--- src/utils/throttle.rs | 56 +++++++++++++++++++-------------------- 10 files changed, 100 insertions(+), 158 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 95dd912..25f9445 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,6 +5,7 @@ authors = ["Sam Rijs "] [dependencies] base64 = "0.9.0" +failure = "0.1.1" futures = "0.1.18" hyper = "0.11.15" hyper-tls = "0.1.2" diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 96af9bc..a973e3d 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -1,6 +1,7 @@ use std::sync::Arc; use std::time::Duration; +use failure::Error; use futures::{Future, Stream, stream}; use hyper::Client; use hyper::client::HttpConnector; @@ -10,15 +11,15 @@ use tokio_service::Service; mod analyzer; -use ::utils::throttle::{Throttle, ThrottledError}; +use ::utils::throttle::Throttle; use ::models::repo::{Repository, RepoPath}; use ::models::crates::{CrateName, CrateRelease, CrateManifest, AnalyzedDependencies}; -use ::parsers::manifest::{ManifestParseError, parse_manifest_toml}; +use ::parsers::manifest::parse_manifest_toml; -use ::interactors::crates::{QueryCrateError, query_crate}; -use ::interactors::github::{RetrieveFileAtPathError, retrieve_file_at_path}; +use ::interactors::crates::query_crate; +use ::interactors::github::retrieve_file_at_path; use ::interactors::github::GetPopularRepos; use self::analyzer::DependencyAnalyzer; @@ -41,16 +42,6 @@ impl Engine { } } -#[derive(Debug)] -pub struct GetPopularReposError(ThrottledError<::interactors::github::GetPopularReposError>); - -#[derive(Debug)] -pub enum AnalyzeDependenciesError { - QueryCrate(QueryCrateError), - RetrieveFileAtPath(RetrieveFileAtPathError), - ParseManifest(ManifestParseError) -} - const FETCH_RELEASES_CONCURRENCY: usize = 10; pub struct AnalyzeDependenciesOutcome { @@ -60,15 +51,14 @@ pub struct AnalyzeDependenciesOutcome { impl Engine { pub fn get_popular_repos(&self) -> - impl Future, Error=GetPopularReposError> + impl Future, Error=Error> { self.get_popular_repos.call(()) - .map_err(GetPopularReposError) - .map(|repos| repos.clone()) + .from_err().map(|repos| repos.clone()) } pub fn analyze_dependencies(&self, repo_path: RepoPath) -> - impl Future + impl Future { let manifest_future = self.retrieve_manifest(&repo_path); @@ -83,9 +73,9 @@ impl Engine { let release_futures = engine.fetch_releases(main_deps.chain(dev_deps).chain(build_deps)); - let analyzed_deps_future = stream::iter_ok(release_futures) + let analyzed_deps_future = stream::iter_ok::<_, Error>(release_futures) .buffer_unordered(FETCH_RELEASES_CONCURRENCY) - .fold(analyzer, |mut analyzer, releases| { analyzer.process(releases); Ok(analyzer) }) + .fold(analyzer, |mut analyzer, releases| { analyzer.process(releases); Ok(analyzer) as Result<_, Error> }) .map(|analyzer| analyzer.finalize()); analyzed_deps_future.map(move |analyzed_deps| { @@ -98,24 +88,21 @@ impl Engine { } fn fetch_releases>(&self, names: I) -> - impl Iterator, Error=AnalyzeDependenciesError>> + impl Iterator, Error=Error>> { let client = self.client.clone(); names.into_iter().map(move |name| { query_crate(client.clone(), name) - .map_err(AnalyzeDependenciesError::QueryCrate) .map(|resp| resp.releases) }) } fn retrieve_manifest(&self, repo_path: &RepoPath) -> - impl Future + impl Future { - retrieve_file_at_path(self.client.clone(), &repo_path, "Cargo.toml") - .map_err(AnalyzeDependenciesError::RetrieveFileAtPath) + retrieve_file_at_path(self.client.clone(), &repo_path, "Cargo.toml").from_err() .and_then(|manifest_source| { - parse_manifest_toml(&manifest_source) - .map_err(AnalyzeDependenciesError::ParseManifest) + parse_manifest_toml(&manifest_source).map_err(|err| err.into()) }) } } diff --git a/src/interactors/crates.rs b/src/interactors/crates.rs index 5347b65..fcadd83 100644 --- a/src/interactors/crates.rs +++ b/src/interactors/crates.rs @@ -1,6 +1,6 @@ +use failure::Error; use futures::{Future, Stream, IntoFuture, future}; -use hyper::{Error as HyperError, Method, Request, Response, StatusCode}; -use hyper::error::UriError; +use hyper::{Error as HyperError, Method, Request, Response}; use tokio_service::Service; use semver::Version; use serde_json; @@ -20,7 +20,7 @@ struct QueryCratesVersionsBody { versions: Vec } -fn convert_body(name: &CrateName, body: QueryCratesVersionsBody) -> Result { +fn convert_body(name: &CrateName, body: QueryCratesVersionsBody) -> Result { let releases = body.versions.into_iter().map(|version| { CrateRelease { name: name.clone(), @@ -38,33 +38,25 @@ pub struct QueryCrateResponse { pub releases: Vec } -#[derive(Debug)] -pub enum QueryCrateError { - Uri(UriError), - Status(StatusCode), - Transport(HyperError), - Decode(serde_json::Error) -} - pub fn query_crate(service: S, crate_name: CrateName) -> - impl Future + impl Future where S: Service { let uri_future = format!("{}/crates/{}/versions", CRATES_API_BASE_URI, crate_name.as_ref()) - .parse().into_future().map_err(QueryCrateError::Uri); + .parse().into_future().from_err(); uri_future.and_then(move |uri| { let request = Request::new(Method::Get, uri); - service.call(request).map_err(QueryCrateError::Transport).and_then(move |response| { + service.call(request).from_err().and_then(move |response| { let status = response.status(); if !status.is_success() { - future::Either::A(future::err(QueryCrateError::Status(status))) + future::Either::A(future::err(format_err!("Status code: {}", status))) } else { - let body_future = response.body().concat2().map_err(QueryCrateError::Transport); + let body_future = response.body().concat2().from_err(); let decode_future = body_future.and_then(|body| { serde_json::from_slice::(&body) - .map_err(QueryCrateError::Decode) + .map_err(|err| err.into()) }); let convert_future = decode_future.and_then(move |body| convert_body(&crate_name, body)); future::Either::B(convert_future) diff --git a/src/interactors/github.rs b/src/interactors/github.rs index f22169f..43be257 100644 --- a/src/interactors/github.rs +++ b/src/interactors/github.rs @@ -1,27 +1,17 @@ -use std::string::FromUtf8Error; - +use failure::Error; use futures::{Future, IntoFuture, Stream, future}; -use hyper::{Error as HyperError, Method, Request, Response, StatusCode}; -use hyper::error::UriError; +use hyper::{Error as HyperError, Method, Request, Response}; use hyper::header::UserAgent; use tokio_service::Service; use serde_json; -use ::models::repo::{Repository, RepoPath, RepoValidationError}; +use ::models::repo::{Repository, RepoPath}; const GITHUB_API_BASE_URI: &'static str = "https://api.github.com"; const GITHUB_USER_CONTENT_BASE_URI: &'static str = "https://raw.githubusercontent.com"; -#[derive(Debug)] -pub enum RetrieveFileAtPathError { - Uri(UriError), - Transport(HyperError), - Status(StatusCode), - Decode(FromUtf8Error) -} - pub fn retrieve_file_at_path(service: S, repo_path: &RepoPath, file_path: &str) -> - impl Future + impl Future where S: Service { let uri_future = format!("{}/{}/{}/master/{}", @@ -29,34 +19,25 @@ pub fn retrieve_file_at_path(service: S, repo_path: &RepoPath, file_path: &st repo_path.qual.as_ref(), repo_path.name.as_ref(), file_path - ).parse().into_future().map_err(RetrieveFileAtPathError::Uri); + ).parse().into_future().from_err(); uri_future.and_then(move |uri| { let request = Request::new(Method::Get, uri); - service.call(request).map_err(RetrieveFileAtPathError::Transport).and_then(|response| { + service.call(request).from_err().and_then(|response| { let status = response.status(); if !status.is_success() { - future::Either::A(future::err(RetrieveFileAtPathError::Status(status))) + future::Either::A(future::err(format_err!("Status code: {}", status))) } else { - let body_future = response.body().concat2().map_err(RetrieveFileAtPathError::Transport); + let body_future = response.body().concat2().from_err(); let decode_future = body_future - .and_then(|body| String::from_utf8(body.to_vec()).map_err(RetrieveFileAtPathError::Decode)); + .and_then(|body| String::from_utf8(body.to_vec()).map_err(|err| err.into())); future::Either::B(decode_future) } }) }) } -#[derive(Debug)] -pub enum GetPopularReposError { - Uri(UriError), - Transport(HyperError), - Status(StatusCode), - Decode(serde_json::Error), - Validate(RepoValidationError) -} - #[derive(Deserialize)] struct GithubSearchResponse { items: Vec @@ -83,7 +64,7 @@ impl Service for GetPopularRepos { type Request = (); type Response = Vec; - type Error = GetPopularReposError; + type Error = Error; type Future = Box>; fn call(&self, _req: ()) -> Self::Future { @@ -91,24 +72,23 @@ impl Service for GetPopularRepos let service = self.0.clone(); let uri_future = format!("{}/search/repositories?q=language:rust&sort=stars", GITHUB_API_BASE_URI) - .parse().into_future().map_err(GetPopularReposError::Uri); + .parse().into_future().from_err(); Box::new(uri_future.and_then(move |uri| { let mut request = Request::new(Method::Get, uri); request.headers_mut().set(UserAgent::new("deps.rs")); - service.call(request).map_err(GetPopularReposError::Transport).and_then(|response| { + service.call(request).from_err().and_then(|response| { let status = response.status(); if !status.is_success() { - future::Either::A(future::err(GetPopularReposError::Status(status))) + future::Either::A(future::err(format_err!("Status code: {}", status))) } else { - let body_future = response.body().concat2().map_err(GetPopularReposError::Transport); + let body_future = response.body().concat2().from_err(); let decode_future = body_future - .and_then(|body| serde_json::from_slice(body.as_ref()).map_err(GetPopularReposError::Decode)); + .and_then(|body| serde_json::from_slice(body.as_ref()).map_err(|err| err.into())); future::Either::B(decode_future.and_then(|search_response: GithubSearchResponse| { search_response.items.into_iter().map(|item| { - let path = RepoPath::from_parts("github", &item.owner.login, &item.name) - .map_err(GetPopularReposError::Validate)?; + let path = RepoPath::from_parts("github", &item.owner.login, &item.name)?; Ok(Repository { path, description: item.description }) }).collect::, _>>() })) diff --git a/src/main.rs b/src/main.rs index 02d8377..a7ec00a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -4,6 +4,7 @@ #![feature(proc_macro)] extern crate base64; +#[macro_use] extern crate failure; extern crate futures; extern crate hyper; extern crate hyper_tls; diff --git a/src/models/crates.rs b/src/models/crates.rs index a93538f..494af33 100644 --- a/src/models/crates.rs +++ b/src/models/crates.rs @@ -2,6 +2,7 @@ use std::borrow::Borrow; use std::collections::BTreeMap; use std::str::FromStr; +use failure::Error; use semver::{Version, VersionReq}; #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] @@ -25,19 +26,16 @@ impl AsRef for CrateName { } } -#[derive(Debug)] -pub struct CrateNameValidationError; - impl FromStr for CrateName { - type Err = CrateNameValidationError; + type Err = Error; - fn from_str(input: &str) -> Result { + fn from_str(input: &str) -> Result { let is_valid = input.chars().all(|c| { c.is_ascii_alphanumeric() || c == '_' || c == '-' }); if !is_valid { - Err(CrateNameValidationError) + Err(format_err!("failed to validate crate name: {}", input)) } else { Ok(CrateName(input.to_string())) } diff --git a/src/models/repo.rs b/src/models/repo.rs index 6f39bc5..41df4f7 100644 --- a/src/models/repo.rs +++ b/src/models/repo.rs @@ -1,5 +1,7 @@ use std::str::FromStr; +use failure::Error; + #[derive(Clone, Debug)] pub struct Repository { pub path: RepoPath, @@ -14,7 +16,7 @@ pub struct RepoPath { } impl RepoPath { - pub fn from_parts(site: &str, qual: &str, name: &str) -> Result { + pub fn from_parts(site: &str, qual: &str, name: &str) -> Result { Ok(RepoPath { site: site.parse()?, qual: qual.parse()?, @@ -23,9 +25,6 @@ impl RepoPath { } } -#[derive(Debug)] -pub struct RepoValidationError; - #[derive(Clone, Copy, Debug)] pub enum RepoSite { Github @@ -40,12 +39,12 @@ impl RepoSite { } impl FromStr for RepoSite { - type Err = RepoValidationError; + type Err = Error; - fn from_str(input: &str) -> Result { + fn from_str(input: &str) -> Result { match input { "github" => Ok(RepoSite::Github), - _ => Err(RepoValidationError) + _ => Err(format_err!("unknown repo site identifier")) } } } @@ -62,18 +61,15 @@ impl AsRef for RepoSite { pub struct RepoQualifier(String); impl FromStr for RepoQualifier { - type Err = RepoValidationError; + type Err = Error; - fn from_str(input: &str) -> Result { + fn from_str(input: &str) -> Result { let is_valid = input.chars().all(|c| { c.is_ascii_alphanumeric() || c == '.' || c == '-' || c == '_' }); - if !is_valid { - Err(RepoValidationError) - } else { - Ok(RepoQualifier(input.to_string())) - } + ensure!(is_valid, "invalid repo qualifier"); + Ok(RepoQualifier(input.to_string())) } } @@ -87,18 +83,15 @@ impl AsRef for RepoQualifier { pub struct RepoName(String); impl FromStr for RepoName { - type Err = RepoValidationError; + type Err = Error; - fn from_str(input: &str) -> Result { + fn from_str(input: &str) -> Result { let is_valid = input.chars().all(|c| { c.is_ascii_alphanumeric() || c == '.' || c == '-' || c == '_' }); - if !is_valid { - Err(RepoValidationError) - } else { - Ok(RepoName(input.to_string())) - } + ensure!(is_valid, "invalid repo name"); + Ok(RepoName(input.to_string())) } } diff --git a/src/parsers/manifest.rs b/src/parsers/manifest.rs index cd1b813..545c7ec 100644 --- a/src/parsers/manifest.rs +++ b/src/parsers/manifest.rs @@ -1,16 +1,10 @@ use std::collections::BTreeMap; -use semver::{ReqParseError, VersionReq}; +use failure::Error; +use semver::VersionReq; use toml; -use ::models::crates::{CrateName, CrateDeps, CrateManifest, CrateNameValidationError}; - -#[derive(Debug)] -pub enum ManifestParseError { - Serde(toml::de::Error), - Name(CrateNameValidationError), - Version(ReqParseError) -} +use ::models::crates::{CrateName, CrateDeps, CrateManifest}; #[derive(Serialize, Deserialize, Debug)] struct CargoTomlComplexDependency { @@ -44,11 +38,11 @@ struct CargoToml { build_dependencies: BTreeMap } -fn convert_dependency(cargo_dep: (String, CargoTomlDependency)) -> Option> { +fn convert_dependency(cargo_dep: (String, CargoTomlDependency)) -> Option> { match cargo_dep { (name, CargoTomlDependency::Simple(string)) => { - Some(name.parse().map_err(ManifestParseError::Name).and_then(|parsed_name| { - string.parse().map_err(ManifestParseError::Version) + Some(name.parse::().map_err(|err| err.into()).and_then(|parsed_name| { + string.parse::().map_err(|err| err.into()) .map(|version| (parsed_name, version)) })) } @@ -57,8 +51,8 @@ fn convert_dependency(cargo_dep: (String, CargoTomlDependency)) -> Option().map_err(|err| err.into()).and_then(|parsed_name| { + string.parse::().map_err(|err| err.into()) .map(|version| (parsed_name, version)) }) }) @@ -67,12 +61,10 @@ fn convert_dependency(cargo_dep: (String, CargoTomlDependency)) -> Option Result { - let cargo_toml = toml::de::from_str::(input) - .map_err(ManifestParseError::Serde)?; +pub fn parse_manifest_toml(input: &str) -> Result { + let cargo_toml = toml::de::from_str::(input)?; - let crate_name = cargo_toml.package.name.parse() - .map_err(ManifestParseError::Name)?; + let crate_name = cargo_toml.package.name.parse::()?; let dependencies = cargo_toml.dependencies .into_iter().filter_map(convert_dependency).collect::, _>>()?; diff --git a/src/server/mod.rs b/src/server/mod.rs index 181f340..759d2e3 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -98,7 +98,7 @@ impl Server { Err(err) => { let mut response = Response::new(); response.set_status(StatusCode::BadRequest); - response.set_body(format!("{:?}", err)); + response.set_body(format!("Error: {}", err)); future::ok(response) }, Ok(popular) => @@ -121,7 +121,7 @@ impl Server { Err(err) => { let mut response = Response::new(); response.set_status(StatusCode::BadRequest); - response.set_body(format!("{:?}", err)); + response.set_body(format!("Error: {}", err)); future::Either::A(future::ok(response)) }, Ok(repo_path) => { @@ -131,7 +131,7 @@ impl Server { if format != StatusFormat::Svg { let mut response = Response::new(); response.set_status(StatusCode::BadRequest); - response.set_body(format!("{:?}", err)); + response.set_body(format!("Error: {}", err)); future::Either::A(future::ok(response)) } else { future::Either::A(future::ok(views::status_svg(None))) diff --git a/src/utils/throttle.rs b/src/utils/throttle.rs index 5f3b620..21d89a5 100644 --- a/src/utils/throttle.rs +++ b/src/utils/throttle.rs @@ -1,21 +1,23 @@ -use std::error::Error; use std::fmt::{Debug, Display, Formatter, Result as FmtResult}; use std::time::{Duration, Instant}; use std::ops::Deref; use std::sync::Mutex; +use failure::{Error, Fail}; use futures::{Future, Poll}; use futures::future::{Shared, SharedError, SharedItem}; use tokio_service::Service; -pub struct Throttle> { +pub struct Throttle + where S: Service +{ inner: S, duration: Duration, current: Mutex)>> } impl Debug for Throttle - where S: Service + Debug, + where S: Service + Debug { fn fmt(&self, fmt: &mut Formatter) -> FmtResult { fmt.debug_struct("Throttle") @@ -25,7 +27,9 @@ impl Debug for Throttle } } -impl> Throttle { +impl Throttle + where S: Service +{ pub fn new(service: S, duration: Duration) -> Throttle { Throttle { inner: service, @@ -35,10 +39,12 @@ impl> Throttle { } } -impl> Service for Throttle { +impl Service for Throttle + where S: Service +{ type Request = (); type Response = ThrottledItem; - type Error = ThrottledError; + type Error = ThrottledError; type Future = Throttled; fn call(&self, _: ()) -> Self::Future { @@ -69,9 +75,9 @@ impl Debug for Throttled } } -impl Future for Throttled { +impl> Future for Throttled { type Item = ThrottledItem; - type Error = ThrottledError; + type Error = ThrottledError; fn poll(&mut self) -> Poll { self.0.poll() @@ -92,32 +98,24 @@ impl Deref for ThrottledItem { } #[derive(Debug)] -pub struct ThrottledError(SharedError); +pub struct ThrottledError(SharedError); -impl Deref for ThrottledError { - type Target = E; +impl Fail for ThrottledError { + fn cause(&self) -> Option<&Fail> { + Some(self.0.cause()) + } - fn deref(&self) -> &E { - &self.0.deref() + fn backtrace(&self) -> Option<&::failure::Backtrace> { + Some(self.0.backtrace()) + } + + fn causes(&self) -> ::failure::Causes { + self.0.causes() } } -impl Display for ThrottledError - where E: Display, -{ +impl Display for ThrottledError { fn fmt(&self, f: &mut Formatter) -> FmtResult { - self.0.fmt(f) - } -} - -impl Error for ThrottledError - where E: Error, -{ - fn description(&self) -> &str { - self.0.description() - } - - fn cause(&self) -> Option<&Error> { - self.0.cause() + Display::fmt(&self.0, f) } }