From 180282b93b0549b7f1dabb0cf6aa65d0d4c886fe Mon Sep 17 00:00:00 2001 From: Riccardo Busetti Date: Mon, 8 Jul 2024 09:05:21 +0200 Subject: [PATCH] feat(rule-condition): Add `any` and `all` loop conditions (#3791) --- CHANGELOG.md | 1 + relay-event-schema/src/protocol/event.rs | 21 +- relay-event-schema/src/protocol/exception.rs | 12 +- relay-protocol/src/condition.rs | 389 +++++++++++++++---- relay-protocol/src/traits.rs | 105 +++++ relay-protocol/src/value.rs | 1 + 6 files changed, 457 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c788b5bb64..97914c913b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ - Support extrapolation of metrics extracted from sampled data, as long as the sample rate is set in the DynamicSamplingContext. ([#3753](https://github.com/getsentry/relay/pull/3753)) - Extract thread ID and name in spans. ([#3771](https://github.com/getsentry/relay/pull/3771)) - Compute metrics summary on the extracted custom metrics. ([#3769](https://github.com/getsentry/relay/pull/3769)) +- Add support for `all` and `any` `RuleCondition`(s). ([#3791](https://github.com/getsentry/relay/pull/3791)) ## 24.6.0 diff --git a/relay-event-schema/src/protocol/event.rs b/relay-event-schema/src/protocol/event.rs index 3ce586f51b..e747ff736d 100644 --- a/relay-event-schema/src/protocol/event.rs +++ b/relay-event-schema/src/protocol/event.rs @@ -4,7 +4,9 @@ use std::str::FromStr; use relay_common::time; #[cfg(feature = "jsonschema")] use relay_jsonschema_derive::JsonSchema; -use relay_protocol::{Annotated, Array, Empty, FromValue, Getter, IntoValue, Object, Val, Value}; +use relay_protocol::{ + Annotated, Array, Empty, FromValue, Getter, GetterIter, IntoValue, Object, Val, Value, +}; #[cfg(feature = "jsonschema")] use schemars::{gen::SchemaGenerator, schema::Schema}; use sentry_release_parser::Release as ParsedRelease; @@ -805,6 +807,15 @@ impl Getter for Event { } }) } + + fn get_iter(&self, path: &str) -> Option> { + Some(match path.strip_prefix("event.")? { + "exception.values" => { + GetterIter::new_annotated(self.exceptions.value()?.values.value()?) + } + _ => return None, + }) + } } #[cfg(test)] @@ -1255,6 +1266,14 @@ mod tests { Some(Val::String("route")), event.get_value("event.transaction.source") ); + + let mut exceptions = event.get_iter("event.exception.values").unwrap(); + let exception = exceptions.next().unwrap(); + assert_eq!( + Some(Val::String("canvas.contentDocument")), + exception.get_value("value") + ); + assert!(exceptions.next().is_none()); } #[test] diff --git a/relay-event-schema/src/protocol/exception.rs b/relay-event-schema/src/protocol/exception.rs index 7fa07cc727..e6ef5fcfa0 100644 --- a/relay-event-schema/src/protocol/exception.rs +++ b/relay-event-schema/src/protocol/exception.rs @@ -1,6 +1,6 @@ #[cfg(feature = "jsonschema")] use relay_jsonschema_derive::JsonSchema; -use relay_protocol::{Annotated, Empty, FromValue, IntoValue, Object, Value}; +use relay_protocol::{Annotated, Empty, FromValue, Getter, IntoValue, Object, Val, Value}; use crate::processor::ProcessValue; use crate::protocol::{JsonLenientString, Mechanism, RawStacktrace, Stacktrace, ThreadId}; @@ -72,6 +72,16 @@ pub struct Exception { pub other: Object, } +impl Getter for Exception { + fn get_value(&self, path: &str) -> Option> { + Some(match path { + "ty" => self.ty.as_str()?.into(), + "value" => self.value.as_str()?.into(), + _ => return None, + }) + } +} + #[cfg(test)] mod tests { use relay_protocol::Map; diff --git a/relay-protocol/src/condition.rs b/relay-protocol/src/condition.rs index b35ede0933..4d8d40e893 100644 --- a/relay-protocol/src/condition.rs +++ b/relay-protocol/src/condition.rs @@ -307,6 +307,78 @@ impl NotCondition { } } +/// Applies the ANY operation to an array field. +/// +/// This condition matches if at least one of the elements of the array match with the +/// `inner` condition. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct AnyCondition { + /// Path of the field that should match the value. + pub name: String, + /// Inner rule to match on each element. + pub inner: Box, +} + +impl AnyCondition { + /// Creates a condition that matches any element in the array against the `inner` condition. + pub fn new(field: impl Into, inner: RuleCondition) -> Self { + Self { + name: field.into(), + inner: Box::new(inner), + } + } + + fn supported(&self) -> bool { + self.inner.supported() + } + fn matches(&self, instance: &T) -> bool + where + T: Getter + ?Sized, + { + let Some(mut getter_iter) = instance.get_iter(self.name.as_str()) else { + return false; + }; + + getter_iter.any(|g| self.inner.matches(g)) + } +} + +/// Applies the ALL operation to an array field. +/// +/// This condition matches if all the elements of the array match with the `inner` condition. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct AllCondition { + /// Path of the field that should match the value. + pub name: String, + /// Inner rule to match on each element. + pub inner: Box, +} + +impl AllCondition { + /// Creates a condition that matches all the elements in the array against the `inner` + /// condition. + pub fn new(field: impl Into, inner: RuleCondition) -> Self { + Self { + name: field.into(), + inner: Box::new(inner), + } + } + + fn supported(&self) -> bool { + self.inner.supported() + } + fn matches(&self, instance: &T) -> bool + where + T: Getter + ?Sized, + { + let Some(mut getter_iter) = instance.get_iter(self.name.as_str()) else { + return false; + }; + + getter_iter.all(|g| self.inner.matches(g)) + } +} + /// A condition that can be evaluated on structured data. /// /// The basic conditions are [`eq`](Self::eq), [`glob`](Self::glob), and the comparison operators. @@ -445,6 +517,33 @@ pub enum RuleCondition { /// ``` Not(NotCondition), + /// Loops over an array field and returns true if at least one element matches + /// the inner condition. + /// + /// # Example + /// + /// ``` + /// use relay_protocol::RuleCondition; + /// + /// let condition = RuleCondition::for_any("obj.exceptions", + /// RuleCondition::eq("name", "NullPointerException") + /// ); + /// ``` + Any(AnyCondition), + + /// Loops over an array field and returns true if all elements match the inner condition. + /// + /// # Example + /// + /// ``` + /// use relay_protocol::RuleCondition; + /// + /// let condition = RuleCondition::for_all("obj.exceptions", + /// RuleCondition::eq("name", "NullPointerException") + /// ); + /// ``` + All(AllCondition), + /// An unsupported condition for future compatibility. #[serde(other)] Unsupported, @@ -638,6 +737,36 @@ impl RuleCondition { } } + /// Creates an [`AnyCondition`]. + /// + /// # Example + /// + /// ``` + /// use relay_protocol::RuleCondition; + /// + /// let condition = RuleCondition::for_any("obj.exceptions", + /// RuleCondition::eq("name", "NullPointerException") + /// ); + /// ``` + pub fn for_any(field: impl Into, inner: RuleCondition) -> Self { + Self::Any(AnyCondition::new(field, inner)) + } + + /// Creates an [`AllCondition`]. + /// + /// # Example + /// + /// ``` + /// use relay_protocol::RuleCondition; + /// + /// let condition = RuleCondition::for_all("obj.exceptions", + /// RuleCondition::eq("name", "NullPointerException") + /// ); + /// ``` + pub fn for_all(field: impl Into, inner: RuleCondition) -> Self { + Self::All(AllCondition::new(field, inner)) + } + /// Checks if Relay supports this condition (in other words if the condition had any unknown configuration /// which was serialized as "Unsupported" (because the configuration is either faulty or was created for a /// newer relay that supports some other condition types) @@ -655,6 +784,8 @@ impl RuleCondition { RuleCondition::And(rules) => rules.supported(), RuleCondition::Or(rules) => rules.supported(), RuleCondition::Not(rule) => rule.supported(), + RuleCondition::Any(rule) => rule.supported(), + RuleCondition::All(rule) => rule.supported(), } } @@ -673,6 +804,8 @@ impl RuleCondition { RuleCondition::And(conditions) => conditions.matches(value), RuleCondition::Or(conditions) => conditions.matches(value), RuleCondition::Not(condition) => condition.matches(value), + RuleCondition::Any(condition) => condition.matches(value), + RuleCondition::All(condition) => condition.matches(value), RuleCondition::Unsupported => false, } } @@ -705,15 +838,31 @@ impl std::ops::Not for RuleCondition { #[cfg(test)] mod tests { use super::*; + use crate::GetterIter; + + #[derive(Debug)] + struct Exception { + name: String, + } + + impl Getter for Exception { + fn get_value(&self, path: &str) -> Option> { + Some(match path { + "name" => self.name.as_str().into(), + _ => return None, + }) + } + } - struct MockDSC { + struct Trace { transaction: String, release: String, environment: String, user_segment: String, + exceptions: Vec, } - impl Getter for MockDSC { + impl Getter for Trace { fn get_value(&self, path: &str) -> Option> { Some(match path.strip_prefix("trace.")? { "transaction" => self.transaction.as_str().into(), @@ -725,14 +874,29 @@ mod tests { } }) } + + fn get_iter(&self, path: &str) -> Option> { + Some(match path.strip_prefix("trace.")? { + "exceptions" => GetterIter::new(self.exceptions.iter()), + _ => return None, + }) + } } - fn mock_dsc() -> MockDSC { - MockDSC { + fn mock_trace() -> Trace { + Trace { transaction: "transaction1".to_string(), release: "1.1.1".to_string(), environment: "debug".to_string(), user_segment: "vip".to_string(), + exceptions: vec![ + Exception { + name: "NullPointerException".to_string(), + }, + Exception { + name: "NullUser".to_string(), + }, + ], } } @@ -780,76 +944,117 @@ mod tests { "name": "field_6", "value": ["3.*"] }] + }, + { + "op": "any", + "name": "obj.exceptions", + "inner": { + "op": "glob", + "name": "value", + "value": ["*Exception"] + } + }, + { + "op": "all", + "name": "obj.exceptions", + "inner": { + "op": "glob", + "name": "value", + "value": ["*Exception"] + } } ]"#; let rules: Result, _> = serde_json::from_str(serialized_rules); assert!(rules.is_ok()); let rules = rules.unwrap(); - insta::assert_ron_snapshot!(rules, @r#" - [ - EqCondition( - op: "eq", - name: "field_1", - value: [ - "UPPER", - "lower", - ], - options: EqCondOptions( - ignoreCase: true, - ), - ), - EqCondition( - op: "eq", - name: "field_2", - value: [ - "UPPER", - "lower", - ], - ), + insta::assert_ron_snapshot!(rules, @r###" + [ + EqCondition( + op: "eq", + name: "field_1", + value: [ + "UPPER", + "lower", + ], + options: EqCondOptions( + ignoreCase: true, + ), + ), + EqCondition( + op: "eq", + name: "field_2", + value: [ + "UPPER", + "lower", + ], + ), + GlobCondition( + op: "glob", + name: "field_3", + value: [ + "1.2.*", + "2.*", + ], + ), + NotCondition( + op: "not", + inner: GlobCondition( + op: "glob", + name: "field_4", + value: [ + "1.*", + ], + ), + ), + AndCondition( + op: "and", + inner: [ GlobCondition( op: "glob", - name: "field_3", + name: "field_5", value: [ - "1.2.*", "2.*", ], ), - NotCondition( - op: "not", - inner: GlobCondition( - op: "glob", - name: "field_4", - value: [ - "1.*", - ], - ), - ), - AndCondition( - op: "and", - inner: [ - GlobCondition( - op: "glob", - name: "field_5", - value: [ - "2.*", - ], - ), - ], - ), - OrCondition( - op: "or", - inner: [ - GlobCondition( - op: "glob", - name: "field_6", - value: [ - "3.*", - ], - ), + ], + ), + OrCondition( + op: "or", + inner: [ + GlobCondition( + op: "glob", + name: "field_6", + value: [ + "3.*", ], ), - ]"#); + ], + ), + AnyCondition( + op: "any", + name: "obj.exceptions", + inner: GlobCondition( + op: "glob", + name: "value", + value: [ + "*Exception", + ], + ), + ), + AllCondition( + op: "all", + name: "obj.exceptions", + inner: GlobCondition( + op: "glob", + name: "value", + value: [ + "*Exception", + ], + ), + ), + ] + "###); } #[test] @@ -939,11 +1144,11 @@ mod tests { ("string cmp", RuleCondition::gt("trace.transaction", "t")), ]; - let dsc = mock_dsc(); + let trace = mock_trace(); for (rule_test_name, condition) in conditions.iter() { let failure_name = format!("Failed on test: '{rule_test_name}'!!!"); - assert!(condition.matches(&dsc), "{failure_name}"); + assert!(condition.matches(&trace), "{failure_name}"); } } @@ -982,11 +1187,11 @@ mod tests { ("never", false, RuleCondition::never()), ]; - let dsc = mock_dsc(); + let trace = mock_trace(); for (rule_test_name, expected, condition) in conditions.iter() { let failure_name = format!("Failed on test: '{rule_test_name}'!!!"); - assert!(condition.matches(&dsc) == *expected, "{failure_name}"); + assert!(condition.matches(&trace) == *expected, "{failure_name}"); } } @@ -1025,11 +1230,11 @@ mod tests { ("all", true, RuleCondition::all()), ]; - let dsc = mock_dsc(); + let trace = mock_trace(); for (rule_test_name, expected, condition) in conditions.iter() { let failure_name = format!("Failed on test: '{rule_test_name}'!!!"); - assert!(condition.matches(&dsc) == *expected, "{failure_name}"); + assert!(condition.matches(&trace) == *expected, "{failure_name}"); } } @@ -1048,11 +1253,11 @@ mod tests { ), ]; - let dsc = mock_dsc(); + let trace = mock_trace(); for (rule_test_name, expected, condition) in conditions.iter() { let failure_name = format!("Failed on test: '{rule_test_name}'!!!"); - assert!(condition.matches(&dsc) == *expected, "{failure_name}"); + assert!(condition.matches(&trace) == *expected, "{failure_name}"); } } @@ -1086,11 +1291,55 @@ mod tests { ), ]; - let dsc = mock_dsc(); + let trace = mock_trace(); for (rule_test_name, condition) in conditions.iter() { let failure_name = format!("Failed on test: '{rule_test_name}'!!!"); - assert!(!condition.matches(&dsc), "{failure_name}"); + assert!(!condition.matches(&trace), "{failure_name}"); } } + + #[test] + fn test_any_condition_with_match() { + let condition = RuleCondition::for_any( + "trace.exceptions", + RuleCondition::glob("name", "*Exception"), + ); + + let trace = mock_trace(); + + assert!(condition.matches(&trace)); + } + + #[test] + fn test_any_condition_with_no_match() { + let condition = + RuleCondition::for_any("trace.exceptions", RuleCondition::glob("name", "Error")); + + let trace = mock_trace(); + + assert!(!condition.matches(&trace)); + } + + #[test] + fn test_all_condition() { + let condition = + RuleCondition::for_all("trace.exceptions", RuleCondition::glob("name", "Null*")); + + let trace = mock_trace(); + + assert!(condition.matches(&trace)); + } + + #[test] + fn test_all_condition_with_no_match() { + let condition = RuleCondition::for_all( + "trace.exceptions", + RuleCondition::glob("name", "*Exception"), + ); + + let trace = mock_trace(); + + assert!(!condition.matches(&trace)); + } } diff --git a/relay-protocol/src/traits.rs b/relay-protocol/src/traits.rs index 0658a038e8..ce9dbbed40 100644 --- a/relay-protocol/src/traits.rs +++ b/relay-protocol/src/traits.rs @@ -125,6 +125,104 @@ pub trait IntoValue: Debug + Empty { } } +/// A type-erased iterator over a collection of [`Getter`]s. +/// +/// This type is usually returned from [`Getter::get_iter`]. +/// +/// # Example +/// +/// ``` +/// use relay_protocol::{Getter, GetterIter, Val}; +/// +/// struct Nested { +/// nested_value: String, +/// } +/// +/// impl Getter for Nested { +/// fn get_value(&self, path: &str) -> Option> { +/// Some(match path { +/// "nested_value" => self.nested_value.as_str().into(), +/// _ => return None, +/// }) +/// } +/// } +/// +/// struct Root { +/// value_1: String, +/// value_2: Vec, +/// } +/// +/// impl Getter for Root { +/// fn get_value(&self, path: &str) -> Option> { +/// Some(match path.strip_prefix("root.")? { +/// "value_1" => self.value_1.as_str().into(), +/// _ => { +/// return None; +/// } +/// }) +/// } +/// +/// // `get_iter` should return a `GetterIter` that can be used for iterating on the +/// // `Getter`(s). +/// fn get_iter(&self, path: &str) -> Option> { +/// Some(match path.strip_prefix("root.")? { +/// "value_2" => GetterIter::new(self.value_2.iter()), +/// _ => return None, +/// }) +/// } +/// } +/// +/// // An example usage given an instance that implement `Getter`. +/// fn matches(instance: &T) -> bool +/// where T: Getter + ?Sized +/// { +/// let Some(mut getter_iter) = instance.get_iter("root.value_2") else { +/// return false; +/// }; +/// +/// for getter in getter_iter { +/// let nested_value = getter.get_value("nested_value"); +/// } +/// +/// true +/// } +/// ``` +pub struct GetterIter<'a> { + iter: Box + 'a>, +} + +impl<'a> GetterIter<'a> { + /// Creates a new [`GetterIter`] given an iterator of a type that implements [`Getter`]. + pub fn new(iterator: I) -> Self + where + I: Iterator + 'a, + T: Getter + 'a, + { + Self { + iter: Box::new(iterator.map(|v| v as &dyn Getter)), + } + } + + /// Creates a new [`GetterIter`] given a collection of + /// [`Annotated`]s whose type implement [`Getter`]. + pub fn new_annotated(iterator: I) -> Self + where + I: IntoIterator>, + I::IntoIter: 'a, + T: Getter + 'a, + { + Self::new(iterator.into_iter().filter_map(Annotated::value)) + } +} + +impl<'a> Iterator for GetterIter<'a> { + type Item = &'a dyn Getter; + + fn next(&mut self) -> Option { + self.iter.next() + } +} + /// A type that supports field access by paths. /// /// This is the runtime version of [`get_value!`](crate::get_value!) and additionally supports @@ -195,4 +293,11 @@ pub trait IntoValue: Debug + Empty { pub trait Getter { /// Returns the serialized value of a field pointed to by a `path`. fn get_value(&self, path: &str) -> Option>; + + /// Returns an iterator over the array pointed to by a `path`. + /// + /// If the path does not exist or is not an array, this returns `None`. Note that `get_value` may not return a value for paths that expose an iterator. + fn get_iter(&self, _path: &str) -> Option> { + None + } } diff --git a/relay-protocol/src/value.rs b/relay-protocol/src/value.rs index e5c7b9f5eb..2ae323326a 100644 --- a/relay-protocol/src/value.rs +++ b/relay-protocol/src/value.rs @@ -1,4 +1,5 @@ use std::collections::BTreeMap; +use std::fmt::Debug; use std::{fmt, str}; #[cfg(feature = "jsonschema")]