[pve-devel] [PATCH proxmox] notify: make template rendering helpers more robust

[pve-devel] [PATCH proxmox] notify: make template rendering helpers more robust

[Lukas Wagner]

This commit has the aim of making template rendering a bit more
robust. It does so by a.) Accepting also strings for helpers that
expect a number, parsing the number if needed, and b.) Ignoring errors
if a template helper fails to render a value and showing an error in
the logs, instead of failing to render the whole template (leading
to no notification being sent).

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---

 proxmox-notify/examples/render.rs        |  2 +-
 proxmox-notify/src/renderer/html.rs      |  2 +-
 proxmox-notify/src/renderer/mod.rs       | 58 ++++++++++++++++++------
 proxmox-notify/src/renderer/plaintext.rs |  4 +-
 4 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/proxmox-notify/examples/render.rs b/proxmox-notify/examples/render.rs
index 5c1f2b2..c0a6f27 100644
--- a/proxmox-notify/examples/render.rs
+++ b/proxmox-notify/examples/render.rs
@@ -43,7 +43,7 @@ fn main() -> Result<(), Error> {
             "data" : [
                 {
                     "vmid": 1001,
-                    "size": 1024 * 1024,
+                    "size": "1048576"
                 },
                 {
                     "vmid": 1002,
diff --git a/proxmox-notify/src/renderer/html.rs b/proxmox-notify/src/renderer/html.rs
index e9ff2b4..f794902 100644
--- a/proxmox-notify/src/renderer/html.rs
+++ b/proxmox-notify/src/renderer/html.rs
@@ -42,7 +42,7 @@ fn render_html_table(
             let entry = row.get(&column.id).unwrap_or(&Value::Null);
 
             let text = if let Some(renderer) = &column.renderer {
-                renderer.render(entry)?
+                renderer.render(entry)
             } else {
                 value_to_string(entry)
             };
diff --git a/proxmox-notify/src/renderer/mod.rs b/proxmox-notify/src/renderer/mod.rs
index f37e672..24f14aa 100644
--- a/proxmox-notify/src/renderer/mod.rs
+++ b/proxmox-notify/src/renderer/mod.rs
@@ -29,20 +29,31 @@ fn value_to_string(value: &Value) -> String {
     }
 }
 
-/// Render a serde_json::Value as a byte size with proper units (IEC, base 2)
+/// Render a `serde_json::Value` as a byte size with proper units (IEC, base 2).
+/// Accepts `serde_json::Value::{Number,String}`.
 ///
-/// Will return `None` if `val` does not contain a number.
+/// Will return `None` if `val` does not contain a number/parseable string.
 fn value_to_byte_size(val: &Value) -> Option<String> {
-    let size = val.as_f64()?;
+    let size = match val {
+        Value::Number(n) => n.as_f64(),
+        Value::String(s) => s.parse().ok(),
+        _ => None,
+    }?;
+
     Some(format!("{}", HumanByte::new_binary(size)))
 }
 
 /// Render a serde_json::Value as a duration.
 /// The value is expected to contain the duration in seconds.
+/// Accepts `serde_json::Value::{Number,String}`.
 ///
-/// Will return `None` if `val` does not contain a number.
+/// Will return `None` if `val` does not contain a number/parseable string.
 fn value_to_duration(val: &Value) -> Option<String> {
-    let duration = val.as_u64()?;
+    let duration = match val {
+        Value::Number(n) => n.as_u64(),
+        Value::String(s) => s.parse().ok(),
+        _ => None,
+    }?;
     let time_span = TimeSpan::from(Duration::from_secs(duration));
 
     Some(format!("{time_span}"))
@@ -50,10 +61,15 @@ fn value_to_duration(val: &Value) -> Option<String> {
 
 /// Render as serde_json::Value as a timestamp.
 /// The value is expected to contain the timestamp as a unix epoch.
+/// Accepts `serde_json::Value::{Number,String}`.
 ///
-/// Will return `None` if `val` does not contain a number.
+/// Will return `None` if `val` does not contain a number/parseable string.
 fn value_to_timestamp(val: &Value) -> Option<String> {
-    let timestamp = val.as_i64()?;
+    let timestamp = match val {
+        Value::Number(n) => n.as_i64(),
+        Value::String(s) => s.parse().ok(),
+        _ => None,
+    }?;
     proxmox_time::strftime_local("%F %H:%M:%S", timestamp).ok()
 }
 
@@ -95,16 +111,15 @@ pub enum ValueRenderFunction {
 }
 
 impl ValueRenderFunction {
-    fn render(&self, value: &Value) -> Result<String, HandlebarsRenderError> {
+    fn render(&self, value: &Value) -> String {
         match self {
             ValueRenderFunction::HumanBytes => value_to_byte_size(value),
             ValueRenderFunction::Duration => value_to_duration(value),
             ValueRenderFunction::Timestamp => value_to_timestamp(value),
         }
-        .ok_or_else(|| {
-            HandlebarsRenderError::new(format!(
-                "could not render value {value} with renderer {self:?}"
-            ))
+        .unwrap_or_else(|| {
+            log::error!("could not render value {value} with renderer {self:?}");
+            String::from("ERROR")
         })
     }
 
@@ -141,7 +156,7 @@ impl ValueRenderFunction {
                         .ok_or(HandlebarsRenderError::new("parameter not found"))?;
 
                     let value = param.value();
-                    out.write(&self.render(value)?)?;
+                    out.write(&self.render(value))?;
 
                     Ok(())
                 },
@@ -364,4 +379,21 @@ val3        val4
 
         Ok(())
     }
+
+    #[test]
+    fn test_helpers() {
+        assert_eq!(value_to_byte_size(&json!(1024)), Some("1 KiB".to_string()));
+        assert_eq!(
+            value_to_byte_size(&json!("1024")),
+            Some("1 KiB".to_string())
+        );
+
+        assert_eq!(value_to_duration(&json!(60)), Some("1min ".to_string()));
+        assert_eq!(value_to_duration(&json!("60")), Some("1min ".to_string()));
+
+        // The rendered value is in localtime, so we only check if the result is `Some`...
+        // ... otherwise the test will break in another timezone :S
+        assert!(value_to_timestamp(&json!(60)).is_some());
+        assert!(value_to_timestamp(&json!("60")).is_some());
+    }
 }
diff --git a/proxmox-notify/src/renderer/plaintext.rs b/proxmox-notify/src/renderer/plaintext.rs
index c8079d7..437e412 100644
--- a/proxmox-notify/src/renderer/plaintext.rs
+++ b/proxmox-notify/src/renderer/plaintext.rs
@@ -20,7 +20,7 @@ fn optimal_column_widths(table: &Table) -> HashMap<&str, usize> {
             let entry = row.get(&column.id).unwrap_or(&Value::Null);
 
             let text = if let Some(renderer) = &column.renderer {
-                renderer.render(entry).unwrap_or_default()
+                renderer.render(entry)
             } else {
                 value_to_string(entry)
             };
@@ -63,7 +63,7 @@ fn render_plaintext_table(
             let width = widths.get(column.label.as_str()).unwrap_or(&0);
 
             let text = if let Some(renderer) = &column.renderer {
-                renderer.render(entry)?
+                renderer.render(entry)
             } else {
                 value_to_string(entry)
             };
-- 
2.39.2

[pve-devel] applied: [PATCH proxmox] notify: make template rendering helpers more robust

[Thomas Lamprecht]

Am 25/08/2023 um 13:35 schrieb Lukas Wagner:
> This commit has the aim of making template rendering a bit more
> robust. It does so by a.) Accepting also strings for helpers that
> expect a number, parsing the number if needed, and b.) Ignoring errors
> if a template helper fails to render a value and showing an error in
> the logs, instead of failing to render the whole template (leading
> to no notification being sent).
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> 
>  proxmox-notify/examples/render.rs        |  2 +-
>  proxmox-notify/src/renderer/html.rs      |  2 +-
>  proxmox-notify/src/renderer/mod.rs       | 58 ++++++++++++++++++------
>  proxmox-notify/src/renderer/plaintext.rs |  4 +-
>  4 files changed, 49 insertions(+), 17 deletions(-)
> 
>

applied, thanks!