From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id D703A1FF0A5 for ; Fri, 04 Sep 2026 11:48:43 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 7FFBB21567; Fri, 04 Sep 2026 11:48:43 +0200 (CEST) Content-Type: text/plain; charset=UTF-8 Date: Fri, 04 Sep 2026 11:48:38 +0200 Message-Id: Subject: Re: [PATCH proxmox 01/29] add new proxmox-match-expression crate From: "Lukas Wagner" To: "Arthur Bied-Charreton" , "Lukas Wagner" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260709115716.299836-1-l.wagner@proxmox.com> <20260709115716.299836-2-l.wagner@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788515314874 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.472 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 34CFI7TD2SSJGH3UOKMKS25SCWBQOK2B X-Message-ID-Hash: 34CFI7TD2SSJGH3UOKMKS25SCWBQOK2B X-MailFrom: l.wagner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pbs-devel@lists.proxmox.com, pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Fri Jul 24, 2026 at 8:46 AM CEST, Arthur Bied-Charreton wrote: > On Thu, Jul 09, 2026 at 01:56:48PM +0200, Lukas Wagner wrote: >> This new crate is a generic implementation of a serializable expression >> language. It supports basic combinators, such as AnyOf, OneOf and AllOf, >> constants, Not, as well as custom matcher leave nodes that are injected >> via a generic type parameter. >>=20 >> The first user of this implementation will be the notification stack. >>=20 > i really like this crate, great job!=20 Thanks! > beeing able to serialize evaluated > expressions is a cool feature for notifications history. some comments > inline.=20 [...] >> + >> +/// Evaluated expression resulting from [`Expression::evaluate`]. >> +/// >> +/// This type contains the original expression, augmented with per-node >> +/// results. This is useful for recording 'traces', e.g. for recording >> +/// exactly why a notification matcher matched a notification or not. >> +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] >> +pub struct EvaluatedExpressionWithResult { >> + /// The actual expression. >> + #[serde(flatten)] >> + expression: EvaluatedExpression, >> + /// The evaluated expression matched the notification. > since this crate is meant to be generic, we might want to drop > 'notification' in favor of 'input' or something like that >> + matches: bool, >> +} >> + >> +impl EvaluatedExpressionWithResult { >> + /// Return whether the expression matched the notification. > here as well Done, thanks. This was a leftover from when this was still inside proxmox-notify. >> + pub fn is_match(&self) -> bool { >> + self.matches >> + } >> + >> + /// Borrow the contained [`EvaluatedExpression`]. >> + pub fn expression(&self) -> &EvaluatedExpression { >> + &self.expression >> + } >> + >> + /// Return the contained [`EvaluatedExpression`], consuming `self`. >> + pub fn into_expression(self) -> EvaluatedExpression { >> + self.expression >> + } > nit: is there any reason why this is not an impl From for B? if not i > would personally find such an API more idiomatic Will add a From implementation in v2, but I will keep into_expression and have `from` call just that >> + >> + /// The direct child nodes, uniform across all combinator kinds. >> + /// >> + /// 'not' yields its single operand as a one-element slice; leaf no= des >> + /// ('constant', 'match') yield an empty slice. This lets a generic= tree >> + /// walker recurse without special-casing each variant. >> + pub fn children(&self) -> &[EvaluatedExpressionWithResult] { >> + match &self.expression { >> + EvaluatedExpression::AllOf(expressions) >> + | EvaluatedExpression::AnyOf(expressions) >> + | EvaluatedExpression::OneOf(expressions) =3D> expressions, >> + EvaluatedExpression::Not(expression) =3D> std::slice::from_= ref(&**expression), >> + EvaluatedExpression::Constant(_) | EvaluatedExpression::Mat= ch(_) =3D> &[], >> + } >> + } >> +} >> + >> +impl Expression >> +where >> + M: Clone + MatchExpression, >> +{ >> + /// Evaluate this expression against a provided variable. >> + /// >> + /// This function returns an `EvaluatedExpression`, replicating the= exact structure >> + /// of `self`, but annotated with a per-subexpression trace to reco= rd which of >> + /// the subexpressions matched or not. >> + /// >> + /// # Note >> + /// This evaluates the expression and *all* sub-expressions fully a= nd does not return >> + /// early if the final result is already determined by the already = evaluated sub-expressions >> + /// (for instance, [`Expression::AnyOf`] and the first sub-expressi= on matched). >> + /// This is done so that we can return the full trace via the [`Eva= luatedExpressionWithResult`] >> + /// type. >> + /// >> + /// If you need fast evaluation and do not care about the trace, fe= el free to add a >> + /// `evaluate_fast` (or similar), which does early returns as appro= priate and simply returns >> + /// `bool`. >> + pub fn evaluate(&self, data: &D) -> Result, E> { >> + let evaluated_expression =3D match self { >> + Expression::AllOf(expressions) =3D> { >> + if expressions.is_empty() { >> + // 'all-of' without any sub-expressions is *not* a = match >> + EvaluatedExpressionWithResult { >> + expression: EvaluatedExpression::AllOf(Vec::new= ()), >> + matches: false, >> + } > this probaly does not matter too much as long as it's documented, but > intuitively (and based on implementations for all-expressions in=20 > other languages like rust and python) i would expect all([]) to evaluate = to > true. Yeah, I see where you are coming from, but after some consideration I've decided that I'd like to keep it this way. For non-coders, all([]) being true is not very intuitive, and I'd prefer to just say "hey, all combinators eval to false if they have no children" in the docs. >> + } else { >> + let (matches, evaluated_expressions) =3D >> + Self::evaluate_subexpressions(data, expressions= , true, |a, b| a && b)?; >> + >> + EvaluatedExpressionWithResult { >> + expression: EvaluatedExpression::AllOf(evaluate= d_expressions), >> + matches, >> + } >> + } >> + } [...] >> + >> +#[cfg(test)] >> +mod test { >> + use super::*; >> + >> + #[derive(Serialize, Deserialize, Debug, Clone)] >> + #[serde(rename_all =3D "kebab-case", tag =3D "type")] >> + pub enum TestMatcher { >> + CustomFailure, >> + CustomTrue, >> + CustomFalse, >> + } > nit: this enum does not pass clippy because of the common prefix fixed, thanks! >> + >> + impl MatchExpression for TestMatcher { >> + type Data =3D (); >> + type Error =3D (); >> + >> + fn evaluate(&self, _data: &Self::Data) -> Result { >> + match self { >> + TestMatcher::CustomFailure =3D> Err(()), >> + TestMatcher::CustomTrue =3D> Ok(true), >> + TestMatcher::CustomFalse =3D> Ok(false), >> + } >> + } >> + } >> + > [...]