[librsvg: 16/32] RenderingError: separate out InvalidId and IdNotFound

[Federico Mena Quintero <federico src gnome org>]

commit c2fe4c412bc0fdc211176f4228307341a53cc9d0
Author: Federico Mena Quintero <federico gnome org>
Date:   Thu Nov 26 13:47:40 2020 -0600

    RenderingError: separate out InvalidId and IdNotFound
    
    The former now gets a String payload, so the internal
    DefsLookupErrorKind does not need to be exposed in the public API.
    The latter is a plain variant.
    
    The C API no longer emits a g_warning if the caller tries to look up
    an element ID in an external file; the relevant functions will just
    return an error.

 src/api.rs                        |  2 +-
 src/c_api/handle.rs               | 26 +++++---------------------
 src/error.rs                      | 17 +++++++++++++++--
 src/handle.rs                     |  4 ++--
 tests/api.c                       | 33 +++++++++++++--------------------
 tests/src/api.rs                  | 26 ++++++++++----------------
 tests/src/intrinsic_dimensions.rs | 39 +++++++++++++++++++--------------------
 7 files changed, 65 insertions(+), 82 deletions(-)
---
diff --git a/src/api.rs b/src/api.rs
index 5c8f95f4..a7a97acc 100644
--- a/src/api.rs
+++ b/src/api.rs
@@ -144,7 +144,7 @@
 #![warn(missing_docs)]
 
 pub use crate::{
-    error::{DefsLookupErrorKind, HrefError, LoadingError, RenderingError},
+    error::{LoadingError, RenderingError},
     length::{LengthUnit, RsvgLength as Length},
 };
 
diff --git a/src/c_api/handle.rs b/src/c_api/handle.rs
index 5573f3cb..140f84fe 100644
--- a/src/c_api/handle.rs
+++ b/src/c_api/handle.rs
@@ -54,8 +54,7 @@ use glib::types::instance_of;
 use gobject_sys::{GEnumValue, GFlagsValue};
 
 use crate::api::{
-    CairoRenderer, DefsLookupErrorKind, IntrinsicDimensions, Loader, LoadingError, RenderingError,
-    SvgHandle,
+    CairoRenderer, IntrinsicDimensions, Loader, LoadingError, RenderingError, SvgHandle,
 };
 
 use crate::{
@@ -860,7 +859,7 @@ impl CHandle {
 
     fn has_sub(&self, id: &str) -> Result<bool, RenderingError> {
         let handle = self.get_handle_ref()?;
-        handle.has_element_with_id(id).map_err(warn_on_invalid_id)
+        handle.has_element_with_id(id)
     }
 
     fn get_dimensions_or_empty(&self) -> RsvgDimensionData {
@@ -908,7 +907,7 @@ impl CHandle {
 
         inner.size_callback.end_loop();
 
-        res.map_err(warn_on_invalid_id)
+        res
     }
 
     fn get_position_sub(&self, id: Option<&str>) -> Result<RsvgPositionData, RenderingError> {
@@ -933,7 +932,6 @@ impl CHandle {
                     y: checked_i32(ink_r.y)?,
                 })
             })
-            .map_err(warn_on_invalid_id)
     }
 
     fn make_renderer<'a>(&self, handle_ref: &'a Ref<'_, SvgHandle>) -> CairoRenderer<'a> {
@@ -1049,7 +1047,6 @@ impl CHandle {
         renderer
             .geometry_for_layer(id, viewport)
             .map(|(i, l)| (RsvgRectangle::from(i), RsvgRectangle::from(l)))
-            .map_err(warn_on_invalid_id)
     }
 
     fn render_layer(
@@ -1064,9 +1061,7 @@ impl CHandle {
 
         let renderer = self.make_renderer(&handle);
 
-        renderer
-            .render_layer(cr, id, viewport)
-            .map_err(warn_on_invalid_id)
+        renderer.render_layer(cr, id, viewport)
     }
 
     fn get_geometry_for_element(
@@ -1080,7 +1075,6 @@ impl CHandle {
         renderer
             .geometry_for_element(id)
             .map(|(i, l)| (RsvgRectangle::from(i), RsvgRectangle::from(l)))
-            .map_err(warn_on_invalid_id)
     }
 
     fn render_element(
@@ -1095,9 +1089,7 @@ impl CHandle {
 
         let renderer = self.make_renderer(&handle);
 
-        renderer
-            .render_element(cr, id, element_viewport)
-            .map_err(warn_on_invalid_id)
+        renderer.render_element(cr, id, element_viewport)
     }
 
     fn get_intrinsic_dimensions(&self) -> Result<IntrinsicDimensions, RenderingError> {
@@ -2169,14 +2161,6 @@ fn check_cairo_context(cr: &cairo::Context) -> Result<(), RenderingError> {
     }
 }
 
-fn warn_on_invalid_id(e: RenderingError) -> RenderingError {
-    if e == RenderingError::InvalidId(DefsLookupErrorKind::CannotLookupExternalReferences) {
-        rsvg_g_warning("the public API is not allowed to look up external references");
-    }
-
-    e
-}
-
 pub(crate) fn set_gerror(err: *mut *mut glib_sys::GError, code: u32, msg: &str) {
     unsafe {
         // this is RSVG_ERROR_FAILED, the only error code available in RsvgError
diff --git a/src/error.rs b/src/error.rs
index a1957b29..7708c357 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -138,8 +138,11 @@ pub enum RenderingError {
     /// A particular implementation-defined limit was exceeded.
     LimitExceeded(String),
 
+    /// Tried to reference an SVG element that does not exist.
+    IdNotFound,
+
     /// Tried to reference an SVG element from a fragment identifier that is incorrect.
-    InvalidId(DefsLookupErrorKind),
+    InvalidId(String),
 
     /// Not enough memory was available for rendering.
     // FIXME: right now this is only returned from pixbuf_utils.rs
@@ -149,6 +152,15 @@ pub enum RenderingError {
     HandleIsNotLoaded,
 }
 
+impl From<DefsLookupErrorKind> for RenderingError {
+    fn from(e: DefsLookupErrorKind) -> RenderingError {
+        match e {
+            DefsLookupErrorKind::NotFound => RenderingError::IdNotFound,
+            _ => RenderingError::InvalidId(format!("{}", e)),
+        }
+    }
+}
+
 impl error::Error for RenderingError {}
 
 impl fmt::Display for RenderingError {
@@ -158,7 +170,8 @@ impl fmt::Display for RenderingError {
             RenderingError::OutOfMemory => write!(f, "out of memory"),
             RenderingError::HandleIsNotLoaded => write!(f, "SVG data is not loaded into handle"),
             RenderingError::Cairo(ref status) => write!(f, "cairo error: {:?}", status),
-            RenderingError::InvalidId(ref id) => write!(f, "invalid id: {:?}", id),
+            RenderingError::IdNotFound => write!(f, "element id not found"),
+            RenderingError::InvalidId(ref s) => write!(f, "invalid id: {:?}", s),
         }
     }
 }
diff --git a/src/handle.rs b/src/handle.rs
index 3463eb88..440a4117 100644
--- a/src/handle.rs
+++ b/src/handle.rs
@@ -102,7 +102,7 @@ impl Handle {
 
             Err(DefsLookupErrorKind::NotFound) => Ok(false),
 
-            Err(e) => Err(RenderingError::InvalidId(e)),
+            Err(e) => Err(e.into()),
         }
     }
 
@@ -139,7 +139,7 @@ impl Handle {
 
     fn get_node_or_root(&self, id: Option<&str>) -> Result<Node, RenderingError> {
         if let Some(id) = id {
-            self.lookup_node(id).map_err(RenderingError::InvalidId)
+            Ok(self.lookup_node(id)?)
         } else {
             Ok(self.document.root())
         }
diff --git a/tests/api.c b/tests/api.c
index d84e1d05..1bb17e79 100644
--- a/tests/api.c
+++ b/tests/api.c
@@ -1255,32 +1255,25 @@ empty_write_close (void)
 static void
 cannot_request_external_elements (void)
 {
-    if (g_test_subprocess ()) {
-        /* We want to test that using one of the _sub() functions will fail
-         * if the element's id is within an external file.  First, ensure
-         * that the main file and the external file actually exist.
-         */
-
-        char *filename = get_test_filename ("example.svg");
+    /* We want to test that using one of the _sub() functions will fail
+     * if the element's id is within an external file.
+     */
 
-        RsvgHandle *handle;
-        GError *error = NULL;
-        RsvgPositionData pos;
+    char *filename = get_test_filename ("example.svg");
 
-        handle = rsvg_handle_new_from_file (filename, &error);
-        g_free (filename);
+    RsvgHandle *handle;
+    GError *error = NULL;
+    RsvgPositionData pos;
 
-        g_assert_nonnull (handle);
-        g_assert_no_error (error);
+    handle = rsvg_handle_new_from_file (filename, &error);
+    g_free (filename);
 
-        g_assert_false (rsvg_handle_get_position_sub (handle, &pos, "dpi.svg#one"));
+    g_assert_nonnull (handle);
+    g_assert_no_error (error);
 
-        g_object_unref (handle);
-    }
+    g_assert_false (rsvg_handle_get_position_sub (handle, &pos, "dpi.svg#one"));
 
-    g_test_trap_subprocess (NULL, 0, 0);
-    g_test_trap_assert_failed ();
-    g_test_trap_assert_stderr ("*WARNING*the public API is not allowed to look up external references*");
+    g_object_unref (handle);
 }
 
 static void
diff --git a/tests/src/api.rs b/tests/src/api.rs
index 065632da..764492b8 100644
--- a/tests/src/api.rs
+++ b/tests/src/api.rs
@@ -1,5 +1,5 @@
 use cairo;
-use librsvg::{CairoRenderer, DefsLookupErrorKind, HrefError, RenderingError};
+use librsvg::{CairoRenderer, RenderingError};
 
 use librsvg::{
     surface_utils::shared_surface::{SharedImageSurface, SurfaceType},
@@ -22,26 +22,20 @@ fn has_element_with_id_works() {
     assert!(svg.has_element_with_id("#foo").unwrap());
     assert!(!svg.has_element_with_id("#bar").unwrap());
 
-    assert_eq!(
+    assert!(matches!(
         svg.has_element_with_id(""),
-        Err(RenderingError::InvalidId(DefsLookupErrorKind::HrefError(
-            HrefError::ParseError
-        )))
-    );
+        Err(RenderingError::InvalidId(_))
+    ));
 
-    assert_eq!(
+    assert!(matches!(
         svg.has_element_with_id("not a fragment"),
-        Err(RenderingError::InvalidId(
-            DefsLookupErrorKind::CannotLookupExternalReferences
-        ))
-    );
+        Err(RenderingError::InvalidId(_))
+    ));
 
-    assert_eq!(
+    assert!(matches!(
         svg.has_element_with_id("notfragment#fragment"),
-        Err(RenderingError::InvalidId(
-            DefsLookupErrorKind::CannotLookupExternalReferences
-        ))
-    );
+        Err(RenderingError::InvalidId(_))
+    ));
 }
 
 #[test]
diff --git a/tests/src/intrinsic_dimensions.rs b/tests/src/intrinsic_dimensions.rs
index feee7935..68bbf0c6 100644
--- a/tests/src/intrinsic_dimensions.rs
+++ b/tests/src/intrinsic_dimensions.rs
@@ -3,8 +3,7 @@ use cairo;
 use librsvg::{
     surface_utils::shared_surface::{SharedImageSurface, SurfaceType},
     test_utils::compare_to_surface,
-    CairoRenderer, DefsLookupErrorKind, HrefError, IntrinsicDimensions, Length, LengthUnit,
-    RenderingError,
+    CairoRenderer, IntrinsicDimensions, Length, LengthUnit, RenderingError,
 };
 
 use crate::utils::{load_svg, render_document, SurfaceSize};
@@ -387,10 +386,10 @@ fn layer_geometry_for_nonexistent_element() {
 
     let renderer = CairoRenderer::new(&svg);
 
-    match renderer.geometry_for_layer(Some("#foo"), &viewport) {
-        Err(RenderingError::InvalidId(DefsLookupErrorKind::NotFound)) => (),
-        _ => panic!(),
-    }
+    assert!(matches!(
+        renderer.geometry_for_layer(Some("#foo"), &viewport),
+        Err(RenderingError::IdNotFound)
+    ));
 }
 
 #[test]
@@ -410,20 +409,20 @@ fn layer_geometry_for_invalid_id() {
     };
 
     let renderer = CairoRenderer::new(&svg);
-    match renderer.geometry_for_layer(Some("foo"), &viewport) {
-        Err(RenderingError::InvalidId(DefsLookupErrorKind::CannotLookupExternalReferences)) => (),
-        _ => panic!(),
-    }
-
-    match renderer.geometry_for_layer(Some("foo.svg#foo"), &viewport) {
-        Err(RenderingError::InvalidId(DefsLookupErrorKind::CannotLookupExternalReferences)) => (),
-        _ => panic!(),
-    }
-
-    match renderer.geometry_for_layer(Some(""), &viewport) {
-        Err(RenderingError::InvalidId(DefsLookupErrorKind::HrefError(HrefError::ParseError))) => (),
-        _ => panic!(),
-    }
+    assert!(matches!(
+        renderer.geometry_for_layer(Some("foo"), &viewport),
+        Err(RenderingError::InvalidId(_))
+    ));
+
+    assert!(matches!(
+        renderer.geometry_for_layer(Some("foo.svg#foo"), &viewport),
+        Err(RenderingError::InvalidId(_))
+    ));
+
+    assert!(matches!(
+        renderer.geometry_for_layer(Some(""), &viewport),
+        Err(RenderingError::InvalidId(_))
+    ));
 }
 
 #[test]