[PATCH proxmox v2 13/34] notify: api: support new expression parameter

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

Add support for the new 'expression' parameter to the add_matcher and
updater_matcher API functions. Also add some validation that won't allow
to create a configuration with both old properties and new the
'expression'.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-notify/src/api/matcher.rs | 119 ++++++++++++++++++++++++++++++
 proxmox-notify/src/matcher/mod.rs |  85 ++++++++++++++++++++-
 2 files changed, 202 insertions(+), 2 deletions(-)

diff --git a/proxmox-notify/src/api/matcher.rs b/proxmox-notify/src/api/matcher.rs
index b2bf15ae..3713dfef 100644
--- a/proxmox-notify/src/api/matcher.rs
+++ b/proxmox-notify/src/api/matcher.rs
@@ -40,6 +40,10 @@ pub fn add_matcher(config: &mut Config, matcher_config: MatcherConfig) -> Result
     super::ensure_unique(config, &matcher_config.name)?;
     super::ensure_endpoints_exist(config, &matcher_config.target)?;
 
+    matcher_config
+        .ensure_valid()
+        .map_err(|err| http_err!(BAD_REQUEST, "invalid matcher config: {err}"))?;
+
     config
         .config
         .set_data(&matcher_config.name, MATCHER_TYPENAME, &matcher_config)
@@ -83,6 +87,7 @@ pub fn update_matcher(
                 DeleteableMatcherProperty::InvertMatch => matcher.invert_match = None,
                 DeleteableMatcherProperty::Comment => matcher.comment = None,
                 DeleteableMatcherProperty::Disable => matcher.disable = None,
+                DeleteableMatcherProperty::Expression => matcher.expression = None,
             }
         }
     }
@@ -115,11 +120,19 @@ pub fn update_matcher(
         matcher.disable = Some(disable);
     }
 
+    if let Some(expression) = matcher_updater.expression {
+        matcher.expression = Some(expression);
+    }
+
     if let Some(target) = matcher_updater.target {
         super::ensure_endpoints_exist(config, target.as_slice())?;
         matcher.target = target;
     }
 
+    matcher
+        .ensure_valid()
+        .map_err(|err| http_err!(BAD_REQUEST, "invalid matcher config: {err}"))?;
+
     config
         .config
         .set_data(name, MATCHER_TYPENAME, &matcher)
@@ -150,8 +163,12 @@ pub fn delete_matcher(config: &mut Config, name: &str) -> Result<(), HttpError>
 
 #[cfg(all(test, feature = "sendmail"))]
 mod tests {
+    use proxmox_match_expression::Expression;
+
     use super::*;
+
     use crate::matcher::MatchModeOperator;
+    use crate::matcher::expression::NotificationMatcher;
 
     fn empty_config() -> Config {
         Config::new("", "").unwrap()
@@ -172,6 +189,11 @@ matcher: matcher2
         .unwrap()
     }
 
+    fn valid_expression_string() -> String {
+        let expr = Expression::<NotificationMatcher>::Constant(true);
+        serde_json::to_string(&expr).unwrap()
+    }
+
     #[test]
     fn test_update_not_existing_returns_error() -> Result<(), HttpError> {
         let mut config = empty_config();
@@ -261,4 +283,101 @@ matcher: matcher2
 
         Ok(())
     }
+
+    #[test]
+    fn test_update_matcher_mutually_exclusive_with_expression() -> Result<(), HttpError> {
+        let mut config = config_with_two_matchers();
+        let digest = config.digest;
+
+        let proto = MatcherConfigUpdater {
+            expression: Some(valid_expression_string()),
+            ..Default::default()
+        };
+
+        let mut updater = proto.clone();
+        updater.match_field = Some(vec!["exact:foo=bar".parse().unwrap()]);
+
+        assert!(update_matcher(&mut config, "matcher1", updater, None, Some(&digest),).is_err());
+
+        let mut updater = proto.clone();
+        updater.match_calendar = Some(vec!["mon..sun 12-13".parse().unwrap()]);
+
+        assert!(update_matcher(&mut config, "matcher1", updater, None, Some(&digest),).is_err());
+
+        let mut updater = proto.clone();
+        updater.match_severity = Some(vec!["info,warning".parse().unwrap()]);
+
+        assert!(update_matcher(&mut config, "matcher1", updater, None, Some(&digest),).is_err());
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_update_invalid_expression() -> Result<(), HttpError> {
+        let mut config = config_with_two_matchers();
+        let digest = config.digest;
+
+        assert!(
+            update_matcher(
+                &mut config,
+                "matcher1",
+                MatcherConfigUpdater {
+                    expression: Some("invalid".into()),
+                    ..Default::default()
+                },
+                None,
+                Some(&digest),
+            )
+            .is_err()
+        );
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_add_mutually_exclusive_with_expression() -> Result<(), HttpError> {
+        let mut config = empty_config();
+
+        let proto = MatcherConfig {
+            name: "matcher3".into(),
+            expression: Some(valid_expression_string()),
+            ..Default::default()
+        };
+
+        let mut entity = proto.clone();
+        entity.match_field = vec!["exact:foo=bar".parse().unwrap()];
+
+        assert!(add_matcher(&mut config, entity).is_err());
+
+        let mut entity = proto.clone();
+        entity.match_severity = vec!["info,warning".parse().unwrap()];
+
+        assert!(add_matcher(&mut config, entity).is_err());
+
+        let mut entity = proto.clone();
+        entity.match_calendar = vec!["mon..sun 12-13".parse().unwrap()];
+
+        assert!(add_matcher(&mut config, entity).is_err());
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_add_invalid_expression() -> Result<(), HttpError> {
+        let mut config = empty_config();
+
+        assert!(
+            add_matcher(
+                &mut config,
+                MatcherConfig {
+                    name: "matcher2".into(),
+                    expression: Some("invalid".into()),
+                    ..Default::default()
+                }
+            )
+            .is_err()
+        );
+
+        Ok(())
+    }
 }
diff --git a/proxmox-notify/src/matcher/mod.rs b/proxmox-notify/src/matcher/mod.rs
index 4a1beecf..12110e95 100644
--- a/proxmox-notify/src/matcher/mod.rs
+++ b/proxmox-notify/src/matcher/mod.rs
@@ -76,7 +76,7 @@ pub enum MatchModeOperator {
             optional: true,
         },
     })]
-#[derive(Debug, Serialize, Deserialize, Updater, Default)]
+#[derive(Clone, Debug, Serialize, Deserialize, Updater, Default)]
 #[serde(rename_all = "kebab-case")]
 /// Config for notification matchers.
 pub struct MatcherConfig {
@@ -148,7 +148,6 @@ impl MatcherConfig {
         self.warn_about_invalid_configuration();
 
         let expression = if let Some(expression_str) = &self.expression {
-
             serde_json::from_str(expression_str).map_err(|err| {
                 Error::FilterFailed(format!("could not deserialize filter expression: {err:#}"))
             })?
@@ -204,6 +203,86 @@ impl MatcherConfig {
         }
     }
 
+    /// Ensure the validity of this matcher.
+    pub(crate) fn ensure_valid(&self) -> Result<(), Error> {
+        self.ensure_no_ignored_properties()?;
+        self.ensure_valid_expression()?;
+
+        Ok(())
+    }
+
+    fn ensure_no_ignored_properties(&self) -> Result<(), Error> {
+        if self.expression.is_some() {
+            let conflicting_properties = self.properties_conflicting_with_expression();
+
+            if !conflicting_properties.is_empty() {
+                let joined = conflicting_properties.join(", ");
+                return Err(Error::Generic(format!(
+                    "'expression' property is mutually exclusive with: {joined}"
+                )));
+            }
+        }
+
+        Ok(())
+    }
+
+    fn ensure_valid_expression(&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)?;
+                }
+                Err(err) => {
+                    return Err(Error::Generic(format!(
+                        "'expression' is not valid: {err:#}"
+                    )));
+                }
+            }
+        }
+
+        Ok(())
+    }
+
+    /// Recursively walk expression nodes and verify any additional constraints on them:
+    ///
+    ///   - all-of/any-of/one-of *must* have child nodes:
+    ///     The generic proxmox-match-expression crates handles empty combinators just fine,
+    ///     however, from a UX perspective, it makes sense to require them to have child nodes.
+    fn verify_match_expression_constraints(
+        expr: &Expression<NotificationMatcher>,
+    ) -> Result<(), Error> {
+        match expr {
+            Expression::AllOf(expressions) => {
+                if expressions.is_empty() {
+                    return Err(Error::Generic(
+                        "'expression' may not contain empty 'all-of' rule".to_string(),
+                    ));
+                }
+            }
+            Expression::AnyOf(expressions) => {
+                if expressions.is_empty() {
+                    return Err(Error::Generic(
+                        "'expression' may not contain empty 'any-of' rule".to_string(),
+                    ));
+                }
+            }
+            Expression::OneOf(expressions) => {
+                if expressions.is_empty() {
+                    return Err(Error::Generic(
+                        "'expression' may not contain empty 'one-of' rule".to_string(),
+                    ));
+                }
+            }
+            _ => {}
+        }
+
+        for child_expression in expr.children() {
+            Self::verify_match_expression_constraints(child_expression)?;
+        }
+
+        Ok(())
+    }
+
     fn warn_about_invalid_configuration(&self) {
         if self.expression.is_some() {
             let conflicting_properties = self.properties_conflicting_with_expression();
@@ -260,6 +339,8 @@ pub enum DeleteableMatcherProperty {
     MatchSeverity,
     /// Delete `mode`
     Mode,
+    /// Delete `expression`
+    Expression,
     /// Delete `target`
     Target,
 }
-- 
2.47.3