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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Fri, Jul 24, 2026 at 08:46:19AM +0200, Arthur Bied-Charreton wrote:
> 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(),
> > +                ));
> > +            }

Would be nicer to collect all the above into a string and then have a
single error "'expression' is mutually exclusive with: {list}".

> > +
> > +            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 :)

Depends on what "valid" means - that they don't crash, or that they do
what the user wants them to do, in which case I'd ask what the empty
field is supposed to do anyway ;-)

Code wise I doubt fixing this is worth it.
Do we expect users to run into this?

Note that for the inline version, they have to match a regex which uses
`SAFE_ID_REGEX_STR` for the field name, which does not match an empty
string, so that fails to parse.

Given that, we *could* consider this the job of `FieldMatcher`'s
deserialize implementation. Could redirect the field deserialization
via `#[serde(deserialize_with = "foo")]`.

For the severity matchers, neither `FromStr` nor the new version do any
checking.

> > +        Ok(())
> > +    }
> >  }
> >  
> >  #[api]
> > @@ -237,6 +276,8 @@ pub enum DeleteableMatcherProperty {
> >      MatchSeverity,
> >      /// Delete `mode`
> >      Mode,
> > +    /// Delete `expression`
> > +    Expression,
> >      /// Delete `target`
> >      Target,
> >  }
> > -- 
> > 2.47.3