chore: Display stylesheet location in network-related errors

Stranger6667 · Stranger6667 · commit 33240f7986f7 · 2023-12-28T01:41:06.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## [Unreleased]
 
+### Changed
+
+- Display stylesheet location in network-related errors.
+
 ### Performance
 
 - Optimize serialization of attributes and text nodes.
diff --git a/bindings/c/CHANGELOG.md b/bindings/c/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## [Unreleased]
 
+### Changed
+
+- Display stylesheet location in network-related errors.
+
 ## [0.11.3] - 2023-12-14
 
 - Initial public release
diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs
@@ -90,7 +90,7 @@ pub unsafe extern "C" fn css_inline_to(
     if let Err(e) = options.inline_to(html, &mut buffer) {
         return match e {
             InlineError::IO(_) => CssResult::IoError,
-            InlineError::Network(_) => CssResult::RemoteStylesheetNotAvailable,
+            InlineError::Network { .. } => CssResult::RemoteStylesheetNotAvailable,
             InlineError::ParseError(_) => CssResult::InternalSelectorParseError,
             InlineError::MissingStyleSheet { .. } => CssResult::MissingStylesheet,
         };
diff --git a/bindings/javascript/__test__/index.spec.ts b/bindings/javascript/__test__/index.spec.ts
@@ -32,15 +32,32 @@ test("valid baseURL", (t) => {
 });
 
 test("invalid baseURL", (t) => {
-  const error = t.throws(
-    () => {
-      inline(
-        "<html><head><style>h1 { color:red; }</style></head><body><h1>Test</h1></body></html>",
-        { baseUrl: "invalid" },
-      );
-    },
-    undefined,
-    "relative URL without a base",
-  );
+  const error = t.throws(() => {
+    inline(
+      "<html><head><style>h1 { color:red; }</style></head><body><h1>Test</h1></body></html>",
+      { baseUrl: "foo" },
+    );
+  });
   t.is(error.code, "InvalidArg");
+  t.is(error.message, "relative URL without a base: foo");
+});
+
+test("invalid href", (t) => {
+  const error = t.throws(() => {
+    inline(
+      "<html><head><link href='http:' rel='stylesheet' type='text/css'></head><body></body></html>",
+    );
+  });
+  t.is(error.code, "GenericFailure");
+  t.is(error.message, "Invalid base URL: http:");
+});
+
+test("invalid style", (t) => {
+  const error = t.throws(() => {
+    inline(
+      "<html><head><style>h1, h2 { color:red; }</style></head><body><h1 style='@wrong { color: --- }'>Hello world!</h1></body></html>",
+    );
+  });
+  t.is(error.code, "GenericFailure");
+  t.is(error.message, "Invalid @ rule: wrong");
 });
diff --git a/bindings/javascript/__test__/wasm.spec.ts b/bindings/javascript/__test__/wasm.spec.ts
@@ -43,10 +43,34 @@ test("invalid baseURL", (t) => {
     () => {
       inline(
         "<html><head><style>h1 { color:red; }</style></head><body><h1>Test</h1></body></html>",
-        { baseUrl: "invalid" },
+        { baseUrl: "foo" },
       );
     },
     { any: true },
   );
-  t.is(error, "relative URL without a base");
+  t.is(error, "relative URL without a base: foo");
+});
+
+test("invalid href", (t) => {
+  const error = t.throws(
+    () => {
+      inline(
+        "<html><head><link href='http:' rel='stylesheet' type='text/css'></head><body></body></html>",
+      );
+    },
+    { any: true },
+  );
+  t.is(error, "Invalid base URL: http:");
+});
+
+test("invalid style", (t) => {
+  const error = t.throws(
+    () => {
+      inline(
+        "<html><head><style>h1, h2 { color:red; }</style></head><body><h1 style='@wrong { color: --- }'>Hello world!</h1></body></html>",
+      );
+    },
+    { any: true },
+  );
+  t.is(error, "Invalid @ rule: wrong");
 });
diff --git a/bindings/javascript/src/errors.rs b/bindings/javascript/src/errors.rs
@@ -3,19 +3,25 @@ use napi::bindgen_prelude::Status;
 #[cfg(target_arch = "wasm32")]
 use wasm_bindgen::prelude::JsValue;
 
-pub(crate) struct UrlError(pub(crate) css_inline::ParseError);
+pub(crate) struct UrlError {
+    pub(crate) error: css_inline::ParseError,
+    pub(crate) url: String,
+}
 
 #[cfg(not(target_arch = "wasm32"))]
 impl From<UrlError> for napi::Error {
-    fn from(err: UrlError) -> Self {
-        napi::Error::new(Status::InvalidArg, err.0.to_string())
+    fn from(error: UrlError) -> Self {
+        napi::Error::new(
+            Status::InvalidArg,
+            format!("{}: {}", error.error, error.url),
+        )
     }
 }
 
 #[cfg(target_arch = "wasm32")]
 impl From<UrlError> for JsValue {
     fn from(error: UrlError) -> Self {
-        JsValue::from_str(error.0.to_string().as_str())
+        JsValue::from_str(format!("{}: {}", error.error, error.url).as_str())
     }
 }
 
diff --git a/bindings/javascript/src/options.rs b/bindings/javascript/src/options.rs
@@ -7,7 +7,7 @@ use wasm_bindgen::prelude::JsValue;
 
 fn parse_url(url: Option<String>) -> std::result::Result<Option<css_inline::Url>, JsError> {
     Ok(if let Some(url) = url {
-        Some(css_inline::Url::parse(url.as_str()).map_err(UrlError)?)
+        Some(css_inline::Url::parse(url.as_str()).map_err(|error| UrlError { error, url })?)
     } else {
         None
     })
diff --git a/bindings/javascript/wasm/index_bg.wasm b/bindings/javascript/wasm/index_bg.wasm
diff --git a/bindings/python/CHANGELOG.md b/bindings/python/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## [Unreleased]
 
+### Changed
+
+- Display stylesheet location in network-related errors.
+
 ### Performance
 
 - Optimize serialization of attributes and text nodes.
diff --git a/bindings/python/src/lib.rs b/bindings/python/src/lib.rs
@@ -40,7 +40,7 @@ impl From<InlineErrorWrapper> for PyErr {
     fn from(error: InlineErrorWrapper) -> Self {
         match error.0 {
             rust_inline::InlineError::IO(error) => InlineError::new_err(error.to_string()),
-            rust_inline::InlineError::Network(error) => InlineError::new_err(error.to_string()),
+            rust_inline::InlineError::Network { .. } => InlineError::new_err(error.0.to_string()),
             rust_inline::InlineError::ParseError(message) => {
                 InlineError::new_err(message.to_string())
             }
@@ -51,17 +51,20 @@ impl From<InlineErrorWrapper> for PyErr {
     }
 }
 
-struct UrlError(url::ParseError);
+struct UrlError {
+    error: rust_inline::ParseError,
+    url: String,
+}
 
 impl From<UrlError> for PyErr {
     fn from(error: UrlError) -> Self {
-        exceptions::PyValueError::new_err(error.0.to_string())
+        exceptions::PyValueError::new_err(format!("{}: {}", error.error, error.url))
     }
 }
 
-fn parse_url(url: Option<String>) -> PyResult<Option<url::Url>> {
+fn parse_url(url: Option<String>) -> PyResult<Option<rust_inline::Url>> {
     Ok(if let Some(url) = url {
-        Some(url::Url::parse(url.as_str()).map_err(UrlError)?)
+        Some(rust_inline::Url::parse(url.as_str()).map_err(|error| UrlError { error, url })?)
     } else {
         None
     })
diff --git a/bindings/python/tests-py/test_inlining.py b/bindings/python/tests-py/test_inlining.py
@@ -90,10 +90,37 @@ def test_file_scheme():
 
 
 def test_invalid_base_url():
-    with pytest.raises(ValueError):
+    with pytest.raises(ValueError, match="relative URL without a base: foo"):
         css_inline.CSSInliner(base_url="foo")
 
 
+def test_invalid_href():
+    with pytest.raises(ValueError, match="Invalid base URL: http:"):
+        css_inline.inline(
+            """<html>
+    <head>
+    <link href="http:" rel="stylesheet" type="text/css">
+    </head>
+    <body>
+    </body>
+    </html>"""
+        )
+
+
+def test_invalid_style():
+    with pytest.raises(ValueError, match="Invalid @ rule: wrong"):
+        css_inline.inline(
+            """<html>
+    <head>
+    </head>
+    <style>h1, h2 { color:red; }</style>
+    <body>
+    <h1 style="@wrong { color: --- }">Hello world!</h1>
+    </body>
+    </html>"""
+        )
+
+
 @given(
     document=st.text(),
     keep_style_tags=st.booleans() | st.none(),
diff --git a/bindings/ruby/CHANGELOG.md b/bindings/ruby/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## [Unreleased]
 
+### Changed
+
+- Display stylesheet location in network-related errors.
+
 ### Performance
 
 - Optimize serialization of attributes and text nodes.
diff --git a/bindings/ruby/ext/css_inline/src/lib.rs b/bindings/ruby/ext/css_inline/src/lib.rs
@@ -92,9 +92,10 @@ impl From<InlineErrorWrapper> for magnus::Error {
             rust_inline::InlineError::IO(error) => {
                 magnus::Error::new(magnus::exception::arg_error(), error.to_string())
             }
-            rust_inline::InlineError::Network(error) => {
-                magnus::Error::new(magnus::exception::arg_error(), error.to_string())
-            }
+            rust_inline::InlineError::Network { error, location } => magnus::Error::new(
+                magnus::exception::arg_error(),
+                format!("{error}: {location}"),
+            ),
             rust_inline::InlineError::ParseError(message) => {
                 magnus::Error::new(magnus::exception::arg_error(), message.to_string())
             }
@@ -105,17 +106,23 @@ impl From<InlineErrorWrapper> for magnus::Error {
     }
 }
 
-struct UrlError(rust_inline::ParseError);
+struct UrlError {
+    error: rust_inline::ParseError,
+    url: String,
+}
 
 impl From<UrlError> for magnus::Error {
     fn from(error: UrlError) -> magnus::Error {
-        magnus::Error::new(magnus::exception::arg_error(), error.0.to_string())
+        magnus::Error::new(
+            magnus::exception::arg_error(),
+            format!("{}: {}", error.error, error.url),
+        )
     }
 }
 
 fn parse_url(url: Option<String>) -> RubyResult<Option<rust_inline::Url>> {
     Ok(if let Some(url) = url {
-        Some(rust_inline::Url::parse(url.as_str()).map_err(UrlError)?)
+        Some(rust_inline::Url::parse(url.as_str()).map_err(|error| UrlError { error, url })?)
     } else {
         None
     })
diff --git a/bindings/ruby/spec/css_inline_spec.rb b/bindings/ruby/spec/css_inline_spec.rb
@@ -64,4 +64,37 @@ def make_html(style, body)
     expect(inlined[1]).to eq(expected)
   end
 
+  it 'Shows the stylesheet location in base url errors' do
+    expect { CSSInline::CSSInliner.new(base_url: 'foo') }.to raise_error('relative URL without a base: foo')
+  end
+
+  it 'Shows the stylesheet location in network errors' do
+    expect {
+        CSSInline::CSSInliner.new.inline('''
+        <html>
+        <head>
+        <link href="http:" rel="stylesheet" type="text/css">
+        </head>
+        <body>
+        </body>
+        </html>
+        ''')
+    }.to raise_error('Invalid base URL: http:')
+  end
+
+  it 'Shows the CSS parsing errors' do
+    expect {
+        CSSInline::CSSInliner.new.inline('''
+        <html>
+    <head>
+    </head>
+    <style>h1, h2 { color:red; }</style>
+    <body>
+    <h1 style="@wrong { color: --- }">Hello world!</h1>
+    </body>
+    </html>
+        ''')
+    }.to raise_error('Invalid @ rule: wrong')
+  end
+
 end
diff --git a/css-inline/src/error.rs b/css-inline/src/error.rs
@@ -20,7 +20,12 @@ pub enum InlineError {
     IO(io::Error),
     /// Network-related problem. E.g. resource is not available.
     #[cfg(feature = "http")]
-    Network(attohttpc::Error),
+    Network {
+        /// Original network error.
+        error: attohttpc::Error,
+        /// The stylesheet location caused the error.
+        location: String,
+    },
     /// Syntax errors or unsupported selectors.
     ParseError(Cow<'static, str>),
 }
@@ -31,21 +36,14 @@ impl From<io::Error> for InlineError {
     }
 }
 
-#[cfg(feature = "http")]
-impl From<attohttpc::Error> for InlineError {
-    fn from(error: attohttpc::Error) -> Self {
-        Self::Network(error)
-    }
-}
-
 impl Error for InlineError {}
 
 impl Display for InlineError {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
         match self {
             Self::IO(error) => error.fmt(f),
             #[cfg(feature = "http")]
-            Self::Network(error) => error.fmt(f),
+            Self::Network { error, location } => f.write_fmt(format_args!("{error}: {location}")),
             Self::ParseError(error) => f.write_str(error),
             Self::MissingStyleSheet { path } => {
                 f.write_fmt(format_args!("Missing stylesheet file: {path}"))
diff --git a/css-inline/src/lib.rs b/css-inline/src/lib.rs
@@ -363,9 +363,14 @@ fn load_external(location: &str) -> Result<String> {
     if location.starts_with("https") | location.starts_with("http") {
         #[cfg(feature = "http")]
         {
-            let request = attohttpc::RequestBuilder::try_new(attohttpc::Method::GET, location)?;
-            let response = request.send()?;
-            Ok(response.text()?)
+            let into_error = |error| InlineError::Network {
+                error,
+                location: location.to_string(),
+            };
+            let request = attohttpc::RequestBuilder::try_new(attohttpc::Method::GET, location)
+                .map_err(into_error)?;
+            let response = request.send().map_err(into_error)?;
+            Ok(response.text().map_err(into_error)?)
         }
 
         #[cfg(not(feature = "http"))]
diff --git a/css-inline/tests/test_inlining.rs b/css-inline/tests/test_inlining.rs
@@ -661,7 +661,12 @@ fn remote_network_stylesheet_invalid_url() {
 <body>
 </body>
 </html>"#;
-    assert!(inline(html).is_err());
+    let error = inline(html).expect_err("Should fail");
+    #[cfg(feature = "http")]
+    let expected = "Invalid base URL: http:";
+    #[cfg(not(feature = "http"))]
+    let expected = "Loading external URLs requires the `http` feature";
+    assert_eq!(error.to_string(), expected);
 }
 
 #[test]
