From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id D4C2C1FF0E6 for ; Fri, 24 Jul 2026 08:47:29 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 95D062152D; Fri, 24 Jul 2026 08:47:22 +0200 (CEST) Date: Fri, 24 Jul 2026 08:46:45 +0200 From: Arthur Bied-Charreton To: Lukas Wagner Subject: Re: [PATCH proxmox 01/29] add new proxmox-match-expression crate Message-ID: References: <20260709115716.299836-1-l.wagner@proxmox.com> <20260709115716.299836-2-l.wagner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260709115716.299836-2-l.wagner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784875575879 X-SPAM-LEVEL: Spam detection results: 0 AWL 1.165 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) KAM_SHORT 0.001 Use of a URL Shortener for very short URL RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low 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: KJRZXHZUU6IF2B345VNPQNYAXOFQFBPU X-Message-ID-Hash: KJRZXHZUU6IF2B345VNPQNYAXOFQFBPU X-MailFrom: a.bied-charreton@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 VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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. > > The first user of this implementation will be the notification stack. > i really like this crate, great job! beeing able to serialize evaluated expressions is a cool feature for notifications history. some comments inline. > Signed-off-by: Lukas Wagner > --- > Cargo.toml | 2 + > proxmox-match-expression/Cargo.toml | 18 + > proxmox-match-expression/debian/changelog | 6 + > proxmox-match-expression/debian/control | 34 ++ > proxmox-match-expression/debian/copyright | 18 + > proxmox-match-expression/debian/debcargo.toml | 7 + > proxmox-match-expression/src/lib.rs | 511 ++++++++++++++++++ > 7 files changed, 596 insertions(+) > create mode 100644 proxmox-match-expression/Cargo.toml > create mode 100644 proxmox-match-expression/debian/changelog > create mode 100644 proxmox-match-expression/debian/control > create mode 100644 proxmox-match-expression/debian/copyright > create mode 100644 proxmox-match-expression/debian/debcargo.toml > create mode 100644 proxmox-match-expression/src/lib.rs > > diff --git a/Cargo.toml b/Cargo.toml > index ef407166..fcb06704 100644 > --- a/Cargo.toml > +++ b/Cargo.toml > @@ -31,6 +31,7 @@ members = [ > "proxmox-log", > "proxmox-login", > "proxmox-metrics", > + "proxmox-match-expression", > "proxmox-network-api", > "proxmox-network-types", > "proxmox-node-status", > @@ -177,6 +178,7 @@ proxmox-io = { version = "1.2.1", path = "proxmox-io" } > proxmox-lang = { version = "1.5", path = "proxmox-lang" } > proxmox-log = { version = "1.0.0", path = "proxmox-log" } > proxmox-login = { version = "1.0.0", path = "proxmox-login" } > +proxmox-match-expression = { version = "0.1", path = "proxmox-match-expression" } > proxmox-network-types = { version = "1.0.2", path = "proxmox-network-types" } > proxmox-parallel-handler = { version = "1.0.0", path = "proxmox-parallel-handler" } > proxmox-pgp = { version = "1.0.0", path = "proxmox-pgp" } > diff --git a/proxmox-match-expression/Cargo.toml b/proxmox-match-expression/Cargo.toml > new file mode 100644 > index 00000000..294537bc > --- /dev/null > +++ b/proxmox-match-expression/Cargo.toml > @@ -0,0 +1,18 @@ > +[package] > +name = "proxmox-match-expression" > +description = "evaluation logic for nested match expressions" > +version = "0.1.0" > + > +authors.workspace = true > +edition.workspace = true > +exclude.workspace = true > +homepage.workspace = true > +license.workspace = true > +repository.workspace = true > + > +[dependencies] > +serde = { workspace = true, features = ["derive"] } > + > +[dev-dependencies] > +serde_json.workspace = true > +pretty_assertions.workspace = true > diff --git a/proxmox-match-expression/debian/changelog b/proxmox-match-expression/debian/changelog > new file mode 100644 > index 00000000..44b20a10 > --- /dev/null > +++ b/proxmox-match-expression/debian/changelog > @@ -0,0 +1,6 @@ > +rust-proxmox-match-expression (0.1.0-1) unstable; urgency=medium > + > + * initial version. > + > + -- Proxmox Support Team Thu, 18 Jun 2026 14:02:03 +0200 > + > diff --git a/proxmox-match-expression/debian/control b/proxmox-match-expression/debian/control > new file mode 100644 > index 00000000..14bc8999 > --- /dev/null > +++ b/proxmox-match-expression/debian/control > @@ -0,0 +1,34 @@ > +Source: rust-proxmox-match-expression > +Section: rust > +Priority: optional > +Build-Depends: debhelper-compat (= 13), > + dh-sequence-cargo > +Build-Depends-Arch: cargo:native , > + rustc:native , > + libstd-rust-dev , > + librust-serde-1+default-dev , > + librust-serde-1+derive-dev > +Maintainer: Proxmox Support Team > +Standards-Version: 4.7.2 > +Vcs-Git: git://git.proxmox.com/git/proxmox.git > +Vcs-Browser: https://git.proxmox.com/?p=proxmox.git > +Homepage: https://proxmox.com > +X-Cargo-Crate: proxmox-match-expression > + > +Package: librust-proxmox-match-expression-dev > +Architecture: any > +Multi-Arch: same > +Depends: > + ${misc:Depends}, > + librust-serde-1+default-dev, > + librust-serde-1+derive-dev > +Provides: > + librust-proxmox-match-expression+default-dev (= ${binary:Version}), > + librust-proxmox-match-expression-0-dev (= ${binary:Version}), > + librust-proxmox-match-expression-0+default-dev (= ${binary:Version}), > + librust-proxmox-match-expression-0.1-dev (= ${binary:Version}), > + librust-proxmox-match-expression-0.1+default-dev (= ${binary:Version}), > + librust-proxmox-match-expression-0.1.0-dev (= ${binary:Version}), > + librust-proxmox-match-expression-0.1.0+default-dev (= ${binary:Version}) > +Description: Evaluation logic for nested match expressions - Rust source code > + Source code for Debianized Rust crate "proxmox-match-expression" > diff --git a/proxmox-match-expression/debian/copyright b/proxmox-match-expression/debian/copyright > new file mode 100644 > index 00000000..279c2689 > --- /dev/null > +++ b/proxmox-match-expression/debian/copyright > @@ -0,0 +1,18 @@ > +Format: https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/ > + > +Files: > + * > +Copyright: 2024 - 2026 Proxmox Server Solutions GmbH > +License: AGPL-3.0-or-later > + This program is free software: you can redistribute it and/or modify it under > + the terms of the GNU Affero General Public License as published by the Free > + Software Foundation, either version 3 of the License, or (at your option) any > + later version. > + . > + This program is distributed in the hope that it will be useful, but WITHOUT > + ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS > + FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more > + details. > + . > + You should have received a copy of the GNU Affero General Public License along > + with this program. If not, see . > diff --git a/proxmox-match-expression/debian/debcargo.toml b/proxmox-match-expression/debian/debcargo.toml > new file mode 100644 > index 00000000..b7864cdb > --- /dev/null > +++ b/proxmox-match-expression/debian/debcargo.toml > @@ -0,0 +1,7 @@ > +overlay = "." > +crate_src_path = ".." > +maintainer = "Proxmox Support Team " > + > +[source] > +vcs_git = "git://git.proxmox.com/git/proxmox.git" > +vcs_browser = "https://git.proxmox.com/?p=proxmox.git" > diff --git a/proxmox-match-expression/src/lib.rs b/proxmox-match-expression/src/lib.rs > new file mode 100644 > index 00000000..0b9b0208 > --- /dev/null > +++ b/proxmox-match-expression/src/lib.rs > @@ -0,0 +1,511 @@ > +//! De/serializable expression evaluation with custom leaf-nodes. > +//! [...] > +//! assert_str_eq!(serialized, expected); > +//! > +//! assert!(!evaluated_expression.children()[0].is_match()); > +//! assert!(evaluated_expression.children()[1].is_match()); > +//! > +//! assert_eq!(evaluated_expression.children()[1].children(), &[]); > +//! > +//! ``` > + thanks for the extensive docs :)) > +use serde::{Deserialize, Serialize}; > + > +/// Trait for custom expression leaf-nodes. > +pub trait MatchExpression { > + /// Use this type to specify the type of the data the expression is evaluated against. > + type Data; > + > + /// Error type returned when evaluating this custom expression. > + type Error; > + > + /// Evaluate this expression. > + /// > + /// If there are no errors, this should return `Ok(true)` or `Ok(false)`, depending on > + /// whether this leaf-node expression matched the provided data or not. > + fn evaluate(&self, data: &Self::Data) -> Result; > +} > + > +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] > +#[serde(rename_all = "kebab-case")] > +/// Match expression. > +pub enum Expression { > + /// Match if all of the sub-expression match. > + AllOf(Vec>), > + /// Match if any of the sub-expressions matches. > + AnyOf(Vec>), > + /// Match if exactly one of the sub-expressions matches. > + OneOf(Vec>), > + /// Match if the sub-expression does not match. > + Not(Box>), > + /// Constant value (true or false). > + Constant(bool), > + /// Custom matcher leaf-nodes. > + Match(M), > +} > + > +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] > +#[serde(rename_all = "kebab-case")] > +/// Evaluated match expression. > +pub enum EvaluatedExpression { > + /// Match if all of the sub-expressions match. > + AllOf(Vec>), > + /// Match if any of the sub-expressions matches. > + AnyOf(Vec>), > + /// Match if exactly one of the sub-expressions matches. > + OneOf(Vec>), > + /// Match if the sub-expression does not match. > + Not(Box>), > + /// Constant value (true or false). > + Constant(bool), > + /// Custom matcher leaf-nodes. > + Match(M), > +} > + > +/// 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 > + 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 > + > + /// The direct child nodes, uniform across all combinator kinds. > + /// > + /// 'not' yields its single operand as a one-element slice; leaf nodes > + /// ('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) => expressions, > + EvaluatedExpression::Not(expression) => std::slice::from_ref(&**expression), > + EvaluatedExpression::Constant(_) | EvaluatedExpression::Match(_) => &[], > + } > + } > +} > + > +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 record which of > + /// the subexpressions matched or not. > + /// > + /// # Note > + /// This evaluates the expression and *all* sub-expressions fully and 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-expression matched). > + /// This is done so that we can return the full trace via the [`EvaluatedExpressionWithResult`] > + /// type. > + /// > + /// If you need fast evaluation and do not care about the trace, feel free to add a > + /// `evaluate_fast` (or similar), which does early returns as appropriate and simply returns > + /// `bool`. > + pub fn evaluate(&self, data: &D) -> Result, E> { > + let evaluated_expression = match self { > + Expression::AllOf(expressions) => { > + 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 other languages like rust and python) i would expect all([]) to evaluate to true. > + } else { > + let (matches, evaluated_expressions) = > + Self::evaluate_subexpressions(data, expressions, true, |a, b| a && b)?; > + > + EvaluatedExpressionWithResult { > + expression: EvaluatedExpression::AllOf(evaluated_expressions), > + matches, > + } > + } > + } > + Expression::AnyOf(expressions) => { > + let (matches, evaluated_expressions) = > + Self::evaluate_subexpressions(data, expressions, false, |a, b| a || b)?; > + > + EvaluatedExpressionWithResult { > + expression: EvaluatedExpression::AnyOf(evaluated_expressions), > + matches, > + } > + } > + Expression::OneOf(expressions) => { > + let mut matched_count = 0; > + let mut evaluated_expressions = Vec::new(); > + > + for expression in expressions { > + let evaluated_expression = expression.evaluate(data)?; > + if evaluated_expression.is_match() { > + matched_count += 1; > + } > + evaluated_expressions.push(evaluated_expression) > + } > + > + EvaluatedExpressionWithResult { > + expression: EvaluatedExpression::OneOf(evaluated_expressions), > + matches: matched_count == 1, > + } > + } > + Expression::Not(expression) => { > + let evaluated_expression = expression.evaluate(data)?; > + let matches = !evaluated_expression.is_match(); > + EvaluatedExpressionWithResult { > + expression: EvaluatedExpression::Not(Box::new(evaluated_expression)), > + matches, > + } > + } > + Expression::Constant(value) => EvaluatedExpressionWithResult { > + expression: EvaluatedExpression::Constant(*value), > + matches: *value, > + }, > + Expression::Match(inner) => { > + let matches = inner.evaluate(data)?; > + EvaluatedExpressionWithResult { > + expression: EvaluatedExpression::Match(inner.clone()), > + matches, > + } > + } > + }; > + > + Ok(evaluated_expression) > + } > + > + fn evaluate_subexpressions( > + data: &D, > + expressions: &[Expression], > + initial: bool, > + op: impl Fn(bool, bool) -> bool, > + ) -> Result<(bool, Vec>), E> { > + let mut matches = initial; > + > + let mut evaluated_expressions = Vec::new(); > + > + for expression in expressions { > + let evaluated_expression = expression.evaluate(data)?; > + matches = op(matches, evaluated_expression.is_match()); > + evaluated_expressions.push(evaluated_expression); > + } > + > + Ok((matches, evaluated_expressions)) > + } > +} > + > +/// Convenience macro that can be used to create an 'AnyOf' expression. > +/// > +/// ``` > +/// use proxmox_match_expression::{any_of, Expression}; > +/// > +/// let x: Expression<()> = any_of![ > +/// Expression::Constant(true), > +/// ]; > +/// assert_eq!(x, Expression::AnyOf(vec![Expression::Constant(true)])); > +/// ``` > +/// > +#[macro_export] > +macro_rules! any_of { > + ($($x:expr),* $(,)?) => ( > + $crate::Expression::AnyOf(vec![$($x),*]) > + ); > +} > + > +/// Convenience macro that can be used to create an 'AllOf' expression. > +/// > +/// ``` > +/// use proxmox_match_expression::{all_of, Expression}; > +/// > +/// let x: Expression<()> = all_of![ > +/// Expression::Constant(true), > +/// ]; > +/// assert_eq!(x, Expression::AllOf(vec![Expression::Constant(true)])); > +/// ``` > +#[macro_export] > +macro_rules! all_of { > + ($($x:expr),* $(,)?) => ( > + $crate::Expression::AllOf(vec![$($x),*]) > + ); > +} > + > +/// Convenience macro that can be used to create an 'OneOf' expression. > +/// > +/// ``` > +/// use proxmox_match_expression::{one_of, Expression}; > +/// > +/// let x: Expression<()> = one_of![ > +/// Expression::Constant(true), > +/// ]; > +/// assert_eq!(x, Expression::OneOf(vec![Expression::Constant(true)])); > +/// ``` > +#[macro_export] > +macro_rules! one_of { > + ($($x:expr),* $(,)?) => ( > + $crate::Expression::OneOf(vec![$($x),*]) > + ); > +} > + > +/// Convenience macro that can be used to create a 'Not' expression; > +/// > +/// ``` > +/// use proxmox_match_expression::{not, Expression}; > +/// > +/// let x: Expression<()> = not!(Expression::Constant(true)); > +/// assert_eq!(x, Expression::Not(Box::new(Expression::Constant(true)))); > +/// ``` > +#[macro_export] > +macro_rules! not { > + ($e:expr) => { > + $crate::Expression::Not(Box::new($e)) > + }; > +} > + > +#[cfg(test)] > +mod test { > + use super::*; > + > + #[derive(Serialize, Deserialize, Debug, Clone)] > + #[serde(rename_all = "kebab-case", tag = "type")] > + pub enum TestMatcher { > + CustomFailure, > + CustomTrue, > + CustomFalse, > + } nit: this enum does not pass clippy because of the common prefix > + > + impl MatchExpression for TestMatcher { > + type Data = (); > + type Error = (); > + > + fn evaluate(&self, _data: &Self::Data) -> Result { > + match self { > + TestMatcher::CustomFailure => Err(()), > + TestMatcher::CustomTrue => Ok(true), > + TestMatcher::CustomFalse => Ok(false), > + } > + } > + } > + [...]