Re: [PATCH proxmox 10/29] notify: matcher: add InlineCalendarMatcher

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

On Fri, Jul 24, 2026 at 08:47:00AM +0200, Arthur Bied-Charreton wrote:
> On Thu, Jul 09, 2026 at 01:56:57PM +0200, Lukas Wagner wrote:
> > This move the inline serialization format into a new-type wrapper around
> > CalendarMatcher.
> > 
> > This allows us to keep the existing single-line serialization format for
> > the old configuration keys (match-calendar), while deriving a regular
> > Serializer for CalendarMatcher that will be used in the new expression
> > based matcher.
> > 
> one comment inline
> > Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> > ---
> >  proxmox-notify/src/matcher/calendar.rs | 103 +++++++++++++++++++++----
> >  proxmox-notify/src/matcher/mod.rs      |   4 +-
> >  2 files changed, 88 insertions(+), 19 deletions(-)
> > 
> > diff --git a/proxmox-notify/src/matcher/calendar.rs b/proxmox-notify/src/matcher/calendar.rs
> > index 970801c5..c6263eb5 100644
> > --- a/proxmox-notify/src/matcher/calendar.rs
> > +++ b/proxmox-notify/src/matcher/calendar.rs
> > @@ -1,49 +1,104 @@
> >  use std::fmt;
> >  use std::str::FromStr;
> >  
> > +use serde::{Deserialize, Serialize};
> > +
> >  use proxmox_time::DailyDuration;
> >  
> >  use crate::{Error, Notification};
> >  
> >  use super::MatchDirective;
> >  
> > -/// Match timestamp of the notification.
> > +/// Convenience wrapper around [`DailyDuration`] that implements [`Serialize`] and
> > +/// [`Deserialize`].
> >  #[derive(Clone, Debug)]
> > +struct DailyDurationWrapper {
> > +    schedule: String,
> > +    daily_duration: DailyDuration,
> > +}
> > +
> > +impl fmt::Display for DailyDurationWrapper {
> > +    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> > +        f.write_str(self.as_str())
> > +    }
> > +}
> > +
> > +impl FromStr for DailyDurationWrapper {
> > +    type Err = Error;
> > +
> > +    fn from_str(s: &str) -> Result<Self, Error> {
> > +        let daily_duration = proxmox_time::parse_daily_duration(s)
> we might wanna trim s here? this fails if s has a leading whitespace,
> which is a code path reachable via the UI (see [0]), and even if/when 
> that is fixed, i feel like it would be nicer to not fail to parse 
> because of surrounding whitespace (could also be done at another level
> of the call stack), since that would still be reachable via pvesh etc.

I would avoid allowing white space in the `FromStr` implementation and
prefer to keep the responsibility here with the caller, to stay
consistent with other `FromStr` implementations. (Eg. the std lib fails
to parse " 33" or "33 " as an `usize`.)

> 
> [0] https://lore.proxmox.com/pve-devel/20260709115716.299836-1-l.wagner@proxmox.com/T/#mad9286504ecf2264333be5abe8b9e1f4966aeaeb
> > +            .map_err(|e| Error::Generic(format!("could not parse schedule: {e}")))?;
> > +
> > +        Ok(Self {
> > +            daily_duration,
> > +            schedule: s.to_string(),
> > +        })
> > +    }
> > +}
> > +
> > +impl DailyDurationWrapper {
> > +    fn as_str(&self) -> &str {
> > +        &self.schedule
> > +    }
> > +}
> > +
> > +proxmox_serde::forward_deserialize_to_from_str!(DailyDurationWrapper);
> > +proxmox_serde::forward_serialize_to_display!(DailyDurationWrapper);
> > +
> > +/// Match timestamp of the notification.
> > +#[derive(Clone, Debug, Serialize, Deserialize)]
> > +#[serde(rename_all = "kebab-case")]
> >  pub struct CalendarMatcher {
> > -    schedule: DailyDuration,
> > -    original: String,
> > +    schedule: DailyDurationWrapper,
> >  }
> >  
> >  impl MatchDirective for CalendarMatcher {
> >      fn matches(&self, notification: &Notification) -> Result<bool, Error> {
> >          self.schedule
> > +            .daily_duration
> >              .time_match(notification.metadata.timestamp, false)
> >              .map_err(|err| Error::Generic(format!("could not match timestamp: {err}")))
> >      }
> >  }
> >  
> > -impl fmt::Display for CalendarMatcher {
> > -    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> > -        f.write_str(&self.original)
> > +/// Match the timestamp of a notification.
> > +///
> > +/// This is a wrapper that serializes into an inline format that can be used
> > +/// in a section config key-value pair.
> > +#[derive(Clone, Debug)]
> > +pub struct InlineCalendarMatcher(CalendarMatcher);
> > +
> > +impl MatchDirective for InlineCalendarMatcher {
> > +    fn matches(&self, notification: &Notification) -> Result<bool, Error> {
> > +        self.0.matches(notification)
> >      }
> >  }
> >  
> > -impl FromStr for CalendarMatcher {
> > +impl fmt::Display for InlineCalendarMatcher {
> > +    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> > +        f.write_str(self.0.schedule.as_str())
> > +    }
> > +}
> > +
> > +impl FromStr for InlineCalendarMatcher {
> >      type Err = Error;
> >  
> >      fn from_str(s: &str) -> Result<Self, Error> {
> > -        let schedule = proxmox_time::parse_daily_duration(s)
> > -            .map_err(|e| Error::Generic(format!("could not parse schedule: {e}")))?;
> > -
> > -        Ok(Self {
> > -            schedule,
> > -            original: s.to_string(),
> > -        })
> > +        Ok(Self(CalendarMatcher {
> > +            schedule: s.parse()?,
> > +        }))
> >      }
> >  }
> >  
> > -proxmox_serde::forward_deserialize_to_from_str!(CalendarMatcher);
> > -proxmox_serde::forward_serialize_to_display!(CalendarMatcher);
> > +impl InlineCalendarMatcher {
> > +    pub fn into_inner(self) -> CalendarMatcher {
> > +        self.0
> > +    }
> > +}
> > +
> > +proxmox_serde::forward_deserialize_to_from_str!(InlineCalendarMatcher);
> > +proxmox_serde::forward_serialize_to_display!(InlineCalendarMatcher);
> >  
> >  #[cfg(test)]
> >  mod test {
> > @@ -63,7 +118,21 @@ mod test {
> >  
> >          // Match on a wide rage to avoid issues when running this test case
> >          // in a different time zone.
> > -        let matcher: CalendarMatcher = "thu..sat 0-23".parse().unwrap();
> > +        let matcher: InlineCalendarMatcher = "thu..sat 0-23".parse().unwrap();
> >          assert!(matcher.matches(&notification).unwrap());
> >      }
> > +
> > +    #[test]
> > +    fn test_calendar_matcher_de_ser_roundtrip() {
> > +        let calendar_matcher = "{ \"schedule\": \"thu..sat 0-23\" }";
> > +
> > +        let calendar_matcher: CalendarMatcher = serde_json::from_str(calendar_matcher).unwrap();
> > +
> > +        let s = serde_json::to_string(&calendar_matcher).unwrap();
> > +        let m: CalendarMatcher = serde_json::from_str(&s).unwrap();
> > +
> > +        let a = InlineCalendarMatcher(m);
> > +
> > +        assert_eq!(a.to_string(), "thu..sat 0-23");
> > +    }
> >  }
> > diff --git a/proxmox-notify/src/matcher/mod.rs b/proxmox-notify/src/matcher/mod.rs
> > index 0267a2b9..a5d53d8a 100644
> > --- a/proxmox-notify/src/matcher/mod.rs
> > +++ b/proxmox-notify/src/matcher/mod.rs
> > @@ -14,7 +14,7 @@ pub mod calendar;
> >  pub mod field;
> >  pub mod severity;
> >  
> > -use calendar::CalendarMatcher;
> > +use calendar::InlineCalendarMatcher;
> >  use field::InlineFieldMatcher;
> >  use severity::InlineSeverityMatcher;
> >  
> > @@ -112,7 +112,7 @@ pub struct MatcherConfig {
> >      /// List of matched severity levels.
> >      #[serde(default, skip_serializing_if = "Vec::is_empty")]
> >      #[updater(serde(skip_serializing_if = "Option::is_none"))]
> > -    pub match_calendar: Vec<CalendarMatcher>,
> > +    pub match_calendar: Vec<InlineCalendarMatcher>,
> >      /// Decide if 'all' or 'any' match statements must match.
> >      #[serde(skip_serializing_if = "Option::is_none")]
> >      pub mode: Option<MatchModeOperator>,
> > -- 
> > 2.47.3