Fix: Bring back error output for links (#1553)

With the last lychee release, we simplified the status output for links. While this reduced the visual noise, it also accidentally caused the source of errors to not be printed anymore. This change brings back the additional error information as part of the final report output. Furthermore, it shows the error information in the progress output if verbose mode is activated. Fixes #1487
lycheeverse · Nov 6, 2024 · 7156434 · 7156434
1 parent 2cdec32
commit 7156434
Show file tree

Hide file tree

Showing 16 changed files with 389 additions and 90 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/fixtures/TEST_DETAILED_JSON_OUTPUT_ERROR.md b/fixtures/TEST_DETAILED_JSON_OUTPUT_ERROR.md
@@ -1,8 +1,8 @@
-# Test detailed JSON output error
+# Test Detailed JSON Output Error
 
-This file is used to test if the error details are parsed properly in the json
+This file is used to test if the error details are parsed properly in the JSON
 format.
 
-[The website](https://expired.badssl.com/) produce SSL expired certificate
-error. Such network error has no status code but it can be identified by error
-status details.
+[The website](https://expired.badssl.com/) produces an SSL expired certificate
+error. Such a network error has no status code, but it can be identified by
+error status details.
diff --git a/fixtures/TEST_INVALID_URLS.html b/fixtures/TEST_INVALID_URLS.html
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html lang="en">
+  <head>
+    <meta charset="UTF-8" />
+    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
+    <title>Invalid URLs</title>
+  </head>
+  <body>
+    <ul>
+      <li>
+        <a href="https://httpbin.org/status/404"
+          >https://httpbin.org/status/404</a
+        >
+      </li>
+      <li>
+        <a href="https://httpbin.org/status/500"
+          >https://httpbin.org/status/500</a
+        >
+      </li>
+      <li>
+        <a href="https://httpbin.org/status/502"
+          >https://httpbin.org/status/502</a
+        >
+      </li>
+    </ul>
+  </body>
+</html>
diff --git a/lychee-bin/Cargo.toml b/lychee-bin/Cargo.toml
@@ -54,6 +54,7 @@ tabled = "0.16.0"
 tokio = { version = "1.41.0", features = ["full"] }
 tokio-stream = "0.1.16"
 toml = "0.8.19"
+url = "2.5.2"
 
 [dev-dependencies]
 assert_cmd = "2.0.16"

diff --git a/lychee-bin/src/commands/check.rs b/lychee-bin/src/commands/check.rs
@@ -336,7 +336,15 @@ fn show_progress(
     formatter: &dyn ResponseFormatter,
     verbose: &Verbosity,
 ) -> Result<()> {
-    let out = formatter.format_response(response.body());
+    // In case the log level is set to info, we want to show the detailed
+    // response output. Otherwise, we only show the essential information
+    // (typically the status code and the URL, but this is dependent on the
+    // formatter).
+    let out = if verbose.log_level() >= log::Level::Info {
+        formatter.format_detailed_response(response.body())
+    } else {
+        formatter.format_response(response.body())
+    };
 
     if let Some(pb) = progress_bar {
         pb.inc(1);
@@ -424,7 +432,7 @@ mod tests {
 
         assert!(!buf.is_empty());
         let buf = String::from_utf8_lossy(&buf);
-        assert_eq!(buf, "[200] http://127.0.0.1/ | Cached: OK (cached)\n");
+        assert_eq!(buf, "[200] http://127.0.0.1/ | OK (cached)\n");
     }
 
     #[tokio::test]

diff --git a/lychee-bin/src/formatters/response/color.rs b/lychee-bin/src/formatters/response/color.rs
@@ -10,52 +10,74 @@ use super::{ResponseFormatter, MAX_RESPONSE_OUTPUT_WIDTH};
 /// has not explicitly requested raw, uncolored output.
 pub(crate) struct ColorFormatter;
 
-impl ResponseFormatter for ColorFormatter {
-    fn format_response(&self, body: &ResponseBody) -> String {
-        // Determine the color based on the status.
-        let status_color = match body.status {
+impl ColorFormatter {
+    /// Determine the color for formatted output based on the status of the
+    /// response.
+    fn status_color(status: &Status) -> &'static once_cell::sync::Lazy<console::Style> {
+        match status {
             Status::Ok(_) | Status::Cached(CacheStatus::Ok(_)) => &GREEN,
             Status::Excluded
             | Status::Unsupported(_)
             | Status::Cached(CacheStatus::Excluded | CacheStatus::Unsupported) => &DIM,
             Status::Redirected(_) => &NORMAL,
             Status::UnknownStatusCode(_) | Status::Timeout(_) => &YELLOW,
             Status::Error(_) | Status::Cached(CacheStatus::Error(_)) => &PINK,
-        };
+        }
+    }
 
-        let status_formatted = format_status(&body.status);
+    /// Format the status code or text for the color formatter.
+    ///
+    /// - Numeric status codes are right-aligned.
+    /// - Textual statuses are left-aligned.
+    /// - Padding is taken into account.
+    fn format_status(status: &Status) -> String {
+        let status_code_or_text = status.code_as_string();
+
+        // Calculate the effective padding. Ensure it's non-negative to avoid panic.
+        let padding = MAX_RESPONSE_OUTPUT_WIDTH.saturating_sub(status_code_or_text.len() + 2); // +2 for brackets
+
+        format!(
+            "{}[{:>width$}]",
+            " ".repeat(padding),
+            status_code_or_text,
+            width = status_code_or_text.len()
+        )
+    }
 
-        let colored_status = status_color.apply_to(status_formatted);
+    /// Color and format the response status.
+    fn format_response_status(status: &Status) -> String {
+        let status_color = ColorFormatter::status_color(status);
+        let formatted_status = ColorFormatter::format_status(status);
+        status_color.apply_to(formatted_status).to_string()
+    }
+}
 
-        // Construct the output.
+impl ResponseFormatter for ColorFormatter {
+    fn format_response(&self, body: &ResponseBody) -> String {
+        let colored_status = ColorFormatter::format_response_status(&body.status);
         format!("{} {}", colored_status, body.uri)
     }
-}
 
-/// Format the status code or text for the color formatter.
-///
-/// Numeric status codes are right-aligned.
-/// Textual statuses are left-aligned.
-/// Padding is taken into account.
-fn format_status(status: &Status) -> String {
-    let status_code_or_text = status.code_as_string();
-
-    // Calculate the effective padding. Ensure it's non-negative to avoid panic.
-    let padding = MAX_RESPONSE_OUTPUT_WIDTH.saturating_sub(status_code_or_text.len() + 2); // +2 for brackets
-
-    format!(
-        "{}[{:>width$}]",
-        " ".repeat(padding),
-        status_code_or_text,
-        width = status_code_or_text.len()
-    )
+    /// Provide some more detailed information about the response
+    /// This prints the entire response body, including the exact error message
+    /// (if available).
+    fn format_detailed_response(&self, body: &ResponseBody) -> String {
+        let colored_status = ColorFormatter::format_response_status(&body.status);
+        format!("{colored_status} {body}")
+    }
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
     use http::StatusCode;
     use lychee_lib::{ErrorKind, Status, Uri};
+    use pretty_assertions::assert_eq;
+
+    /// Helper function to strip ANSI color codes for tests
+    fn strip_ansi_codes(s: &str) -> String {
+        console::strip_ansi_codes(s).to_string()
+    }
 
     // Helper function to create a ResponseBody with a given status and URI
     fn mock_response_body(status: Status, uri: &str) -> ResponseBody {
@@ -65,20 +87,18 @@ mod tests {
         }
     }
 
-    #[cfg(test)]
-    /// Helper function to strip ANSI color codes for tests
-    fn strip_ansi_codes(s: &str) -> String {
-        console::strip_ansi_codes(s).to_string()
+    #[test]
+    fn test_format_status() {
+        let status = Status::Ok(StatusCode::OK);
+        assert_eq!(ColorFormatter::format_status(&status).trim_start(), "[200]");
     }
 
     #[test]
     fn test_format_response_with_ok_status() {
         let formatter = ColorFormatter;
         let body = mock_response_body(Status::Ok(StatusCode::OK), "https://example.com");
-        assert_eq!(
-            strip_ansi_codes(&formatter.format_response(&body)),
-            "     [200] https://example.com/"
-        );
+        let formatted_response = strip_ansi_codes(&formatter.format_response(&body));
+        assert_eq!(formatted_response, "     [200] https://example.com/");
     }
 
     #[test]
@@ -88,10 +108,8 @@ mod tests {
             Status::Error(ErrorKind::InvalidUrlHost),
             "https://example.com/404",
         );
-        assert_eq!(
-            strip_ansi_codes(&formatter.format_response(&body)),
-            "   [ERROR] https://example.com/404"
-        );
+        let formatted_response = strip_ansi_codes(&formatter.format_response(&body));
+        assert_eq!(formatted_response, "   [ERROR] https://example.com/404");
     }
 
     #[test]
@@ -100,7 +118,22 @@ mod tests {
         let long_uri =
             "https://example.com/some/very/long/path/to/a/resource/that/exceeds/normal/lengths";
         let body = mock_response_body(Status::Ok(StatusCode::OK), long_uri);
-        let formatted_response = formatter.format_response(&body);
+        let formatted_response = strip_ansi_codes(&formatter.format_response(&body));
         assert!(formatted_response.contains(long_uri));
     }
+
+    #[test]
+    fn test_detailed_response_output() {
+        let formatter = ColorFormatter;
+        let body = mock_response_body(
+            Status::Error(ErrorKind::InvalidUrlHost),
+            "https://example.com/404",
+        );
+
+        let response = strip_ansi_codes(&formatter.format_detailed_response(&body));
+        assert_eq!(
+            response,
+            "   [ERROR] https://example.com/404 | URL is missing a host"
+        );
+    }
 }
diff --git a/lychee-bin/src/formatters/response/emoji.rs b/lychee-bin/src/formatters/response/emoji.rs
@@ -8,19 +8,32 @@ use super::ResponseFormatter;
 /// visual output.
 pub(crate) struct EmojiFormatter;
 
-impl ResponseFormatter for EmojiFormatter {
-    fn format_response(&self, body: &ResponseBody) -> String {
-        let emoji = match body.status {
+impl EmojiFormatter {
+    /// Determine the color for formatted output based on the status of the
+    /// response.
+    const fn emoji_for_status(status: &Status) -> &'static str {
+        match status {
             Status::Ok(_) | Status::Cached(CacheStatus::Ok(_)) => "✅",
             Status::Excluded
             | Status::Unsupported(_)
             | Status::Cached(CacheStatus::Excluded | CacheStatus::Unsupported) => "🚫",
             Status::Redirected(_) => "↪️",
             Status::UnknownStatusCode(_) | Status::Timeout(_) => "⚠️",
             Status::Error(_) | Status::Cached(CacheStatus::Error(_)) => "❌",
-        };
+        }
+    }
+}
+
+impl ResponseFormatter for EmojiFormatter {
+    fn format_response(&self, body: &ResponseBody) -> String {
+        let emoji = EmojiFormatter::emoji_for_status(&body.status);
         format!("{} {}", emoji, body.uri)
     }
+
+    fn format_detailed_response(&self, body: &ResponseBody) -> String {
+        let emoji = EmojiFormatter::emoji_for_status(&body.status);
+        format!("{emoji} {body}")
+    }
 }
 
 #[cfg(test)]
@@ -92,4 +105,18 @@ mod emoji_tests {
             "⚠️ https://example.com/unknown"
         );
     }
+
+    #[test]
+    fn test_detailed_response_output() {
+        let formatter = EmojiFormatter;
+        let body = mock_response_body(
+            Status::Error(ErrorKind::InvalidUrlHost),
+            "https://example.com/404",
+        );
+
+        // Just assert the output contains the string
+        assert!(formatter
+            .format_detailed_response(&body)
+            .ends_with("| URL is missing a host"));
+    }
 }
diff --git a/lychee-bin/src/formatters/response/mod.rs b/lychee-bin/src/formatters/response/mod.rs
@@ -25,4 +25,13 @@ pub(crate) const MAX_RESPONSE_OUTPUT_WIDTH: usize = 10;
 pub(crate) trait ResponseFormatter: Send + Sync {
     /// Format the response body into a human-readable string
     fn format_response(&self, body: &ResponseBody) -> String;
+
+    /// Detailed response formatter (defaults to the normal formatter)
+    ///
+    /// This can be used for output modes which want to provide more detailed
+    /// information. It is also used if the output is set to verbose mode
+    /// (i.e. `-v`, `-vv` and above).
+    fn format_detailed_response(&self, body: &ResponseBody) -> String {
+        self.format_response(body)
+    }
 }
diff --git a/lychee-bin/src/formatters/response/plain.rs b/lychee-bin/src/formatters/response/plain.rs
@@ -14,7 +14,7 @@ pub(crate) struct PlainFormatter;
 
 impl ResponseFormatter for PlainFormatter {
     fn format_response(&self, body: &ResponseBody) -> String {
-        body.to_string()
+        format!("[{}] {}", body.status.code_as_string(), body)
     }
 }
 
@@ -51,18 +51,17 @@ mod plain_tests {
         );
         assert_eq!(
             formatter.format_response(&body),
-            "[ERROR] https://example.com/404 | Failed: URL is missing a host"
+            "[ERROR] https://example.com/404 | URL is missing a host"
         );
     }
 
     #[test]
     fn test_format_response_with_excluded_status() {
         let formatter = PlainFormatter;
         let body = mock_response_body(Status::Excluded, "https://example.com/not-checked");
-        assert_eq!(formatter.format_response(&body), body.to_string());
         assert_eq!(
             formatter.format_response(&body),
-            "[EXCLUDED] https://example.com/not-checked | Excluded"
+            "[EXCLUDED] https://example.com/not-checked"
         );
     }
 
@@ -73,7 +72,6 @@ mod plain_tests {
             Status::Redirected(StatusCode::MOVED_PERMANENTLY),
             "https://example.com/redirect",
         );
-        assert_eq!(formatter.format_response(&body), body.to_string());
         assert_eq!(
             formatter.format_response(&body),
             "[301] https://example.com/redirect | Redirect (301 Moved Permanently): Moved Permanently"
@@ -87,8 +85,6 @@ mod plain_tests {
             Status::UnknownStatusCode(StatusCode::from_u16(999).unwrap()),
             "https://example.com/unknown",
         );
-        assert_eq!(formatter.format_response(&body), body.to_string());
-        // Check the actual string representation of the status code
         assert_eq!(
             formatter.format_response(&body),
             "[999] https://example.com/unknown | Unknown status (999 <unknown status code>)"