From d19855e909bce5980f55660cf22dde3853e93146 Mon Sep 17 00:00:00 2001 From: Andrew Wonnacott Date: Wed, 22 Apr 2020 04:04:10 -0400 Subject: [PATCH] Rewrite link_checker to use a Result internally (#928) --- components/link_checker/src/lib.rs | 113 ++++++++++++----------------- components/site/src/lib.rs | 7 +- 2 files changed, 51 insertions(+), 69 deletions(-) diff --git a/components/link_checker/src/lib.rs b/components/link_checker/src/lib.rs index 7ecd6ca8..0aa55594 100644 --- a/components/link_checker/src/lib.rs +++ b/components/link_checker/src/lib.rs @@ -3,50 +3,33 @@ use reqwest::header::{HeaderMap, ACCEPT}; use reqwest::{blocking::Client, StatusCode}; use config::LinkChecker; -use errors::Result; use std::collections::HashMap; +use std::result; use std::sync::{Arc, RwLock}; -#[derive(Clone, Debug, PartialEq)] -pub struct LinkResult { - pub code: Option, - /// Whether the HTTP request didn't make it to getting a HTTP code - pub error: Option, +pub type Result = result::Result; + +pub fn is_valid(res: &Result) -> bool { + match res { + Ok(ref code) => code.is_success() || *code == StatusCode::NOT_MODIFIED, + Err(_) => false, + } } -impl LinkResult { - pub fn is_valid(&self) -> bool { - if self.error.is_some() { - return false; - } - - if let Some(c) = self.code { - return c.is_success() || c == StatusCode::NOT_MODIFIED; - } - - true - } - - pub fn message(&self) -> String { - if let Some(ref e) = self.error { - return e.clone(); - } - - if let Some(c) = self.code { - return format!("{}", c); - } - - "Unknown error".to_string() +pub fn message(res: &Result) -> String { + match res { + Ok(ref code) => format!("{}", code), + Err(ref error) => error.clone(), } } lazy_static! { // Keep history of link checks so a rebuild doesn't have to check again - static ref LINKS: Arc>> = Arc::new(RwLock::new(HashMap::new())); + static ref LINKS: Arc>> = Arc::new(RwLock::new(HashMap::new())); } -pub fn check_url(url: &str, config: &LinkChecker) -> LinkResult { +pub fn check_url(url: &str, config: &LinkChecker) -> Result { { let guard = LINKS.read().unwrap(); if let Some(res) = guard.get(url) { @@ -75,13 +58,13 @@ pub fn check_url(url: &str, config: &LinkChecker) -> LinkResult { }; match check_page_for_anchor(url, body) { - Ok(_) => LinkResult { code: Some(response.status()), error: None }, - Err(e) => LinkResult { code: None, error: Some(e.to_string()) }, + Ok(_) => Ok(response.status()), + Err(e) => Err(e.to_string()), } } Ok(response) => { if response.status().is_success() || response.status() == StatusCode::NOT_MODIFIED { - LinkResult { code: Some(response.status()), error: None } + Ok(response.status()) } else { let error_string = if response.status().is_informational() { format!("Informational status code ({}) received", response.status()) @@ -95,10 +78,10 @@ pub fn check_url(url: &str, config: &LinkChecker) -> LinkResult { format!("Non-success status code ({}) received", response.status()) }; - LinkResult { code: None, error: Some(error_string) } + Err(error_string) } } - Err(e) => LinkResult { code: None, error: Some(e.to_string()) }, + Err(e) => Err(e.to_string()), }; LINKS.write().unwrap().insert(url.to_string(), res.clone()); @@ -115,7 +98,7 @@ fn has_anchor(url: &str) -> bool { } } -fn check_page_for_anchor(url: &str, body: String) -> Result<()> { +fn check_page_for_anchor(url: &str, body: String) -> errors::Result<()> { let index = url.find('#').unwrap(); let anchor = url.get(index + 1..).unwrap(); let checks: [String; 8] = [ @@ -138,7 +121,9 @@ fn check_page_for_anchor(url: &str, body: String) -> Result<()> { #[cfg(test)] mod tests { - use super::{check_page_for_anchor, check_url, has_anchor, LinkChecker, LINKS}; + use super::{ + check_page_for_anchor, check_url, has_anchor, is_valid, message, LinkChecker, LINKS, + }; use mockito::mock; // NOTE: HTTP mock paths below are randomly generated to avoid name @@ -167,7 +152,8 @@ mod tests { .create(); let res = check_url(&url, &LinkChecker::default()); - assert!(res.is_valid()); + assert!(is_valid(&res)); + assert_eq!(message(&res), "200 OK"); assert!(LINKS.read().unwrap().get(&url).is_some()); } @@ -187,9 +173,9 @@ mod tests { let url = format!("{}{}", mockito::server_url(), "/c7qrtrv3zz"); let res = check_url(&url, &LinkChecker::default()); - assert!(res.is_valid()); - assert!(res.code.is_some()); - assert!(res.error.is_none()); + assert!(is_valid(&res)); + assert!(res.is_ok()); + assert_eq!(message(&res), "200 OK"); } #[test] @@ -225,9 +211,9 @@ mod tests { let url = format!("{}{}", mockito::server_url(), "/cav9vibhsc"); let res = check_url(&url, &LinkChecker::default()); - assert_eq!(res.is_valid(), false); - assert!(res.code.is_none()); - assert!(res.error.is_some()); + assert!(!is_valid(&res)); + assert!(res.is_err()); + assert_eq!(message(&res), "Client error status code (404 Not Found) received"); } #[test] @@ -240,9 +226,9 @@ mod tests { let url = format!("{}{}", mockito::server_url(), "/nlhab9c1vc"); let res = check_url(&url, &LinkChecker::default()); - assert_eq!(res.is_valid(), false); - assert!(res.code.is_none()); - assert!(res.error.is_some()); + assert!(!is_valid(&res)); + assert!(res.is_err()); + assert_eq!(message(&res), "Client error status code (404 Not Found) received"); } #[test] @@ -255,17 +241,18 @@ mod tests { let url = format!("{}{}", mockito::server_url(), "/qdbrssazes"); let res = check_url(&url, &LinkChecker::default()); - assert_eq!(res.is_valid(), false); - assert!(res.code.is_none()); - assert!(res.error.is_some()); + assert!(!is_valid(&res)); + assert!(res.is_err()); + assert_eq!(message(&res), "Server error status code (500 Internal Server Error) received"); } #[test] fn can_fail_unresolved_links() { let res = check_url("https://t6l5cn9lpm.lxizfnzckd", &LinkChecker::default()); - assert_eq!(res.is_valid(), false); - assert!(res.code.is_none()); - assert!(res.error.is_some()); + assert!(!is_valid(&res)); + assert!(res.is_err()); + assert!(message(&res) + .starts_with("error sending request for url (https://t6l5cn9lpm.lxizfnzckd/)")); } #[test] @@ -312,29 +299,25 @@ mod tests { #[test] fn can_check_url_for_anchor() { let url = "https://doc.rust-lang.org/std/index.html#the-rust-standard-library"; - let res = has_anchor(url); - assert_eq!(res, true); + assert!(has_anchor(url)); } #[test] fn will_return_false_when_no_anchor() { let url = "https://doc.rust-lang.org/std/index.html"; - let res = has_anchor(url); - assert_eq!(res, false); + assert!(!has_anchor(url)); } #[test] fn will_return_false_when_has_router_url() { let url = "https://doc.rust-lang.org/#/std"; - let res = has_anchor(url); - assert_eq!(res, false); + assert!(!has_anchor(url)); } #[test] fn will_return_false_when_has_router_url_alt() { let url = "https://doc.rust-lang.org/#!/std"; - let res = has_anchor(url); - assert_eq!(res, false); + assert!(!has_anchor(url)); } #[test] @@ -360,7 +343,7 @@ mod tests { // anchor check is ignored because the url matches the prefix let ignore = format!("{}{}", mockito::server_url(), "/ignore/i30hobj1cy#nonexistent"); - assert!(check_url(&ignore, &config).is_valid()); + assert!(is_valid(&check_url(&ignore, &config))); let _m2 = mock("GET", "/guvqcqwmth") .with_header("Content-Type", "text/html") @@ -380,9 +363,9 @@ mod tests { // other anchors are checked let existent = format!("{}{}", mockito::server_url(), "/guvqcqwmth#existent"); - assert!(check_url(&existent, &config).is_valid()); + assert!(is_valid(&check_url(&existent, &config))); let nonexistent = format!("{}{}", mockito::server_url(), "/guvqcqwmth#nonexistent"); - assert_eq!(check_url(&nonexistent, &config).is_valid(), false); + assert!(!is_valid(&check_url(&nonexistent, &config))); } } diff --git a/components/site/src/lib.rs b/components/site/src/lib.rs index bf39a970..bce4fadb 100644 --- a/components/site/src/lib.rs +++ b/components/site/src/lib.rs @@ -18,7 +18,6 @@ use library::{ find_taxonomies, sort_actual_pages_by_date, Library, Page, Paginator, Section, Taxonomy, TaxonomyItem, }; -use link_checker::check_url; use templates::{global_fns, render_redirect_template, ZOLA_TERA}; use utils::fs::{copy_directory, create_directory, create_file, ensure_directory_exists}; use utils::net::get_available_port; @@ -415,8 +414,8 @@ impl Site { { return None; } - let res = check_url(&link, &self.config.link_checker); - if res.is_valid() { + let res = link_checker::check_url(&link, &self.config.link_checker); + if link_checker::is_valid(&res) { None } else { Some((page_path, link, res)) @@ -442,7 +441,7 @@ impl Site { "Dead link in {} to {}: {}", page_path.to_string_lossy(), link, - check_res.message() + link_checker::message(&check_res) ) }) .collect::>()