[PATCH proxmox v5 18/42] notify: matcher: re-serialize expression on create/update

[Lukas Wagner <l.wagner@proxmox.com>]

This ensures that it is properly minified and does not contain any
control characters.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Michael Köppl <m.koeppl@proxmox.com>
---

Notes:
    New in v3.
    
    Changes since v3:
      - Add tests for expression normalization/minification

 proxmox-notify/src/api/matcher.rs | 80 ++++++++++++++++++++++++++++++-
 proxmox-notify/src/matcher/mod.rs | 12 +++--
 2 files changed, 88 insertions(+), 4 deletions(-)

diff --git a/proxmox-notify/src/api/matcher.rs b/proxmox-notify/src/api/matcher.rs
index 77b2e1ce..84131be3 100644
--- a/proxmox-notify/src/api/matcher.rs
+++ b/proxmox-notify/src/api/matcher.rs
@@ -68,7 +68,10 @@ pub fn get_matcher_as_expression(config: &Config, name: &str) -> Result<MatcherC
 /// Returns a `HttpError` if:
 ///   - an entity with the same name already exists (`400 Bad request`)
 ///   - the configuration could not be saved (`500 Internal server error`)
-pub fn add_matcher(config: &mut Config, matcher_config: MatcherConfig) -> Result<(), HttpError> {
+pub fn add_matcher(
+    config: &mut Config,
+    mut matcher_config: MatcherConfig,
+) -> Result<(), HttpError> {
     super::ensure_unique(config, &matcher_config.name)?;
     super::ensure_endpoints_exist(config, &matcher_config.target)?;
 
@@ -422,6 +425,81 @@ matcher: matcher2
         Ok(())
     }
 
+    fn nested_expression() -> Expression<NotificationMatcher> {
+        Expression::AllOf(vec![
+            Expression::Constant(true),
+            Expression::Not(Box::new(Expression::AnyOf(vec![Expression::Constant(
+                false,
+            )]))),
+        ])
+    }
+
+    #[test]
+    fn test_update_minifies_pretty_printed_expression() -> Result<(), HttpError> {
+        let mut config = config_with_two_matchers();
+        let digest = config.digest;
+
+        let expr = nested_expression();
+        let pretty = serde_json::to_string_pretty(&expr).unwrap();
+        assert!(pretty.contains('\n'));
+
+        update_matcher(
+            &mut config,
+            "matcher1",
+            MatcherConfigUpdater {
+                expression: Some(pretty),
+                ..Default::default()
+            },
+            None,
+            Some(&digest),
+        )?;
+
+        let matcher = get_matcher(&config, "matcher1")?;
+        let stored = matcher.expression.expect("expression should be set");
+
+        assert_eq!(stored, serde_json::to_string(&expr).unwrap());
+        assert!(!stored.contains(char::is_whitespace));
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_update_normalizes_expression_with_extra_whitespace() -> Result<(), HttpError> {
+        let mut config = config_with_two_matchers();
+        let digest = config.digest;
+
+        // Same expression as `nested_expression`, but handwritten, with all sorts of
+        // odd whitespace characters.
+        let raw = "{\r\n\t\"all-of\" :\t[\r\n  {  \"constant\":true },\r\n\t{\"not\": \
+                   {\"any-of\": [ {\"constant\" : false} ] } }\r\n]\r\n}\r\n";
+
+        update_matcher(
+            &mut config,
+            "matcher1",
+            MatcherConfigUpdater {
+                expression: Some(raw.into()),
+                ..Default::default()
+            },
+            None,
+            Some(&digest),
+        )?;
+
+        let matcher = get_matcher(&config, "matcher1")?;
+        let stored = matcher.expression.expect("expression should be set");
+
+        assert_eq!(stored, serde_json::to_string(&nested_expression()).unwrap());
+
+        // The normalized expression should stay the same after
+        // a read/write cycle of the config
+        let (raw_config, raw_private_config) = config.write().unwrap();
+        let config = Config::new(&raw_config, &raw_private_config).unwrap();
+        let matcher = get_matcher(&config, "matcher1")?;
+
+        assert_eq!(matcher.expression, Some(stored));
+
+        Ok(())
+    }
+
     #[test]
     #[cfg(feature = "legacy-matchers")]
     fn test_add_mutually_exclusive_with_expression() -> Result<(), HttpError> {
diff --git a/proxmox-notify/src/matcher/mod.rs b/proxmox-notify/src/matcher/mod.rs
index f4c11c37..a3b97952 100644
--- a/proxmox-notify/src/matcher/mod.rs
+++ b/proxmox-notify/src/matcher/mod.rs
@@ -263,7 +263,7 @@ impl MatcherConfig {
     }
 
     /// Ensure the validity of this matcher.
-    pub(crate) fn ensure_valid(&self) -> Result<(), Error> {
+    pub(crate) fn ensure_valid(&mut self) -> Result<(), Error> {
         #[cfg(feature = "legacy-matchers")]
         self.ensure_no_ignored_properties()?;
         self.ensure_valid_expression()?;
@@ -287,11 +287,17 @@ impl MatcherConfig {
         Ok(())
     }
 
-    fn ensure_valid_expression(&self) -> Result<(), Error> {
+    fn ensure_valid_expression(&mut self) -> Result<(), Error> {
         if let Some(expression) = &self.expression {
             match serde_json::from_str::<Expression<NotificationMatcher>>(expression) {
                 Ok(expr) => {
                     Self::verify_match_expression_constraints(&expr)?;
+
+                    // Storing the *serialized* expression makes sure that
+                    // we have a minified, single-line JSON string
+                    self.expression = Some(serde_json::to_string(&expr).map_err(|err| {
+                        Error::Generic(format!("could not serialize expression: {err:#}"))
+                    })?);
                 }
                 Err(err) => {
                     return Err(Error::Generic(format!(
@@ -519,7 +525,7 @@ mod tests {
             empty_field_matcher,
             empty_severity_matcher,
         ] {
-            let config = MatcherConfig {
+            let mut config = MatcherConfig {
                 name: "matcher".to_string(),
                 expression: Some(expr.into()),
                 ..Default::default()
-- 
2.47.3