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

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

On Fri, Jul 24, 2026 at 12:01:45PM +0200, Wolfgang Bumiller wrote:
> 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 ;-)
yes i also would not know what an empty field is supposed to do, which
is why i think erroring on it would make sense. 
> 
> 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.

that regex uses '.*' for the field value, it only fails on empty field 
names, so 'exact:hostname=' does 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.
InlineSeverityMatcher's FromStr implementation actually does fail on
empty severities ("".split(",") returns [""] so it fails to construct a
Severity from the empty string). the new one just deserializes directly
into a vec, which succeeds if the severities list is empty. 

i agree that recursing through the whole matcher might be overkill, but
adding deserialization-time checks would make this more robust at
basically no cost imo
> 
> > > +        Ok(())
> > > +    }
> > >  }
> > >  
> > >  #[api]
> > > @@ -237,6 +276,8 @@ pub enum DeleteableMatcherProperty {
> > >      MatchSeverity,
> > >      /// Delete `mode`
> > >      Mode,
> > > +    /// Delete `expression`
> > > +    Expression,
> > >      /// Delete `target`
> > >      Target,
> > >  }
> > > -- 
> > > 2.47.3