Re: [PATCH proxmox 12/29] notify: api: support new expression parameter

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

On Thu, Jul 09, 2026 at 01:56:59PM +0200, Lukas Wagner wrote:
> 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>
one comment inline
> ---
>  proxmox-notify/src/api/matcher.rs | 119 ++++++++++++++++++++++++++++++
>  proxmox-notify/src/matcher/mod.rs |  43 ++++++++++-
>  2 files changed, 161 insertions(+), 1 deletion(-)
> 
[...]
> diff --git a/proxmox-notify/src/matcher/mod.rs b/proxmox-notify/src/matcher/mod.rs
> index 429692a4..60725e00 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 {
> @@ -216,6 +216,45 @@ impl MatcherConfig {
>              );
>          }
>      }
> +
> +    /// Ensure the validity of this matcher.
> +    pub(crate) fn ensure_valid(&self) -> Result<(), Error> {
> +        if let Some(expression) = &self.expression {
> +            if self.invert_match.is_some() {
> +                return Err(Error::Generic(
> +                    "'expression' and 'invert-match' are mutually exclusive properties".into(),
> +                ));
> +            }
> +            if self.mode.is_some() {
> +                return Err(Error::Generic(
> +                    "'expression' and 'mode' are mutually exclusive properties".into(),
> +                ));
> +            }
> +            if !self.match_field.is_empty() {
> +                return Err(Error::Generic(
> +                    "'expression' and 'match-field' are mutually exclusive properties".into(),
> +                ));
> +            }
> +            if !self.match_severity.is_empty() {
> +                return Err(Error::Generic(
> +                    "'expression' and 'match-severity' are mutually exclusive properties".into(),
> +                ));
> +            }
> +            if !self.match_calendar.is_empty() {
> +                return Err(Error::Generic(
> +                    "'expression' and 'match-calendar' are mutually exclusive properties".into(),
> +                ));
> +            }
> +
> +            if let Err(err) = serde_json::from_str::<Expression<NotificationMatcher>>(expression) {
> +                return Err(Error::Generic(format!(
> +                    "'expression' is not valid: {err:#}"
> +                )));
> +            }
> +        }
> +
i think this is missing a check for the length of FieldMatcher::Exact 
fields and SeverityMatcher severities. afaict them being empty is pretty 
much always an error? if a user wants a matcher that never matches there
is the inverted "Always match" option, which is made for that. 

this is preexisting, the old backend also did not check it, but i think
it would be a good time to add that :)
> +        Ok(())
> +    }
>  }
>  
>  #[api]
> @@ -237,6 +276,8 @@ pub enum DeleteableMatcherProperty {
>      MatchSeverity,
>      /// Delete `mode`
>      Mode,
> +    /// Delete `expression`
> +    Expression,
>      /// Delete `target`
>      Target,
>  }
> -- 
> 2.47.3
> 
> 
> 
> 
>