diff --git a/Cargo.lock b/Cargo.lock index 676f1d1ce5..726ebeeed2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5519,6 +5519,7 @@ dependencies = [ "serde_with", "sha1 0.10.6", "sha2", + "smallvec", "spdx", "sqlx", "sqlx-tracing", diff --git a/Cargo.toml b/Cargo.toml index 5b2287ccf1..2d99c43e1b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -193,6 +193,7 @@ sha1 = "0.10.6" sha1_smol = { version = "1.0.1", features = ["std"] } sha2 = "0.10.9" shlex = "1.3.0" +smallvec = "1.15.1" spdx = "0.12.0" sqlx = { version = "0.8.6", default-features = false } sqlx-tracing = { path = "packages/sqlx-tracing" } diff --git a/apps/labrinth/Cargo.toml b/apps/labrinth/Cargo.toml index 6635b37830..1081074d9a 100644 --- a/apps/labrinth/Cargo.toml +++ b/apps/labrinth/Cargo.toml @@ -115,6 +115,7 @@ serde_json = { workspace = true } serde_with = { workspace = true } sha1 = { workspace = true } sha2 = { workspace = true } +smallvec = { workspace = true } spdx = { workspace = true, features = ["text"] } sqlx = { workspace = true, features = [ "chrono", diff --git a/apps/labrinth/src/search/backend/typesense/filter.rs b/apps/labrinth/src/search/backend/typesense/filter.rs index 75e128015c..14b36b7b1f 100644 --- a/apps/labrinth/src/search/backend/typesense/filter.rs +++ b/apps/labrinth/src/search/backend/typesense/filter.rs @@ -158,6 +158,9 @@ fn lower(filter: &FilterExpr, version: bool) -> Result> { validate_predicate(predicate)?; Ok(TypesenseFilter::Predicate { predicate, version }) } + FilterExpr::Not(_) => { + Err(eyre!("search filter contains an unnormalized negation")) + } } } @@ -199,6 +202,7 @@ fn filter_scope(filter: &FilterExpr) -> FilterScope { FilterScope::Mixed } } + FilterExpr::Not(expression) => filter_scope(expression), } } @@ -214,7 +218,7 @@ fn to_dnf(filter: &FilterExpr) -> Result>> { FilterExpr::Predicate(predicate) => Ok(vec![vec![predicate]]), FilterExpr::Or(expressions) => { let mut clauses = Vec::new(); - for expression in expressions { + for expression in expressions.iter() { clauses.extend(to_dnf(expression)?); if clauses.len() > MAX_DNF_CLAUSES { return Err(eyre!( @@ -226,7 +230,7 @@ fn to_dnf(filter: &FilterExpr) -> Result>> { } FilterExpr::And(expressions) => { let mut clauses = vec![Vec::new()]; - for expression in expressions { + for expression in expressions.iter() { let right = to_dnf(expression)?; if clauses.len().saturating_mul(right.len()) > MAX_DNF_CLAUSES { return Err(eyre!( @@ -246,6 +250,9 @@ fn to_dnf(filter: &FilterExpr) -> Result>> { } Ok(clauses) } + FilterExpr::Not(_) => { + Err(eyre!("search filter contains an unnormalized negation")) + } } } @@ -270,6 +277,10 @@ fn filter_complexity(filter: &FilterExpr) -> (usize, usize) { }, ) } + FilterExpr::Not(expression) => { + let (nodes, depth) = filter_complexity(expression); + (nodes + 1, depth + 1) + } } } @@ -437,6 +448,16 @@ mod tests { ); } + #[test] + fn serializes_legacy_unary_not_filters() { + assert_eq!( + serialize( + r#"NOT"project_id"="8xOSkvVU" AND NOT"project_id"="DRol93FL""# + ), + "project_id:!=`8xOSkvVU` && project_id:!=`DRol93FL`" + ); + } + #[test] fn cartesian_version_filter_uses_one_join() { let filter = serialize( diff --git a/apps/labrinth/src/search/filter/ast.rs b/apps/labrinth/src/search/filter/ast.rs index d7ab2b4d7e..8cc8cbd8f7 100644 --- a/apps/labrinth/src/search/filter/ast.rs +++ b/apps/labrinth/src/search/filter/ast.rs @@ -1,26 +1,31 @@ +use smallvec::SmallVec; + #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] pub enum FilterExpr { - And(Vec), - Or(Vec), + And(Box>), + Or(Box>), + Not(Box), Predicate(FilterPredicate), } impl FilterExpr { pub fn and(expressions: impl IntoIterator) -> Option { - let mut expressions = expressions.into_iter().collect::>(); + let mut expressions = + expressions.into_iter().collect::>(); match expressions.len() { 0 => None, 1 => expressions.pop(), - _ => Some(Self::And(expressions)), + _ => Some(Self::And(Box::new(expressions))), } } pub fn or(expressions: impl IntoIterator) -> Option { - let mut expressions = expressions.into_iter().collect::>(); + let mut expressions = + expressions.into_iter().collect::>(); match expressions.len() { 0 => None, 1 => expressions.pop(), - _ => Some(Self::Or(expressions)), + _ => Some(Self::Or(Box::new(expressions))), } } } diff --git a/apps/labrinth/src/search/filter/normalize.rs b/apps/labrinth/src/search/filter/normalize.rs index 8d3899c47e..2644fabfa3 100644 --- a/apps/labrinth/src/search/filter/normalize.rs +++ b/apps/labrinth/src/search/filter/normalize.rs @@ -1,5 +1,7 @@ use std::collections::{BTreeMap, BTreeSet}; +use smallvec::SmallVec; + use super::{ FilterComparison, FilterCondition, FilterExpr, FilterField, FilterLiteral, FilterPredicate, @@ -7,12 +9,70 @@ use super::{ pub fn normalize(expression: FilterExpr) -> FilterExpr { match expression { - FilterExpr::And(expressions) => normalize_and(expressions), - FilterExpr::Or(expressions) => normalize_or(expressions), + FilterExpr::And(expressions) => normalize_and(*expressions), + FilterExpr::Or(expressions) => normalize_or(*expressions), + FilterExpr::Not(expression) => normalize_not(*expression), FilterExpr::Predicate(predicate) => normalize_predicate(predicate), } } +fn normalize_not(expression: FilterExpr) -> FilterExpr { + match expression { + FilterExpr::And(expressions) => normalize_or( + (*expressions) + .into_iter() + .map(|expression| FilterExpr::Not(Box::new(expression))) + .collect(), + ), + FilterExpr::Or(expressions) => normalize_and( + (*expressions) + .into_iter() + .map(|expression| FilterExpr::Not(Box::new(expression))) + .collect(), + ), + FilterExpr::Not(expression) => normalize(*expression), + FilterExpr::Predicate(mut predicate) => { + predicate.condition = match predicate.condition { + FilterCondition::Compare { comparison, value } => { + FilterCondition::Compare { + comparison: match comparison { + FilterComparison::Equal => { + FilterComparison::NotEqual + } + FilterComparison::NotEqual => { + FilterComparison::Equal + } + FilterComparison::GreaterThan => { + FilterComparison::LessThanOrEqual + } + FilterComparison::GreaterThanOrEqual => { + FilterComparison::LessThan + } + FilterComparison::LessThan => { + FilterComparison::GreaterThanOrEqual + } + FilterComparison::LessThanOrEqual => { + FilterComparison::GreaterThan + } + }, + value, + } + } + FilterCondition::In { values, negated } => { + FilterCondition::In { + values, + negated: !negated, + } + } + FilterCondition::Exists { negated } => { + FilterCondition::Exists { negated: !negated } + } + }; + normalize_predicate(predicate) + } + } +} + fn normalize_predicate(predicate: FilterPredicate) -> FilterExpr { if predicate.field.as_str() == "minecraft_java_server.ping.data" && let FilterCondition::Exists { negated } = predicate.condition @@ -29,30 +89,34 @@ fn normalize_predicate(predicate: FilterPredicate) -> FilterExpr { FilterExpr::Predicate(predicate) } -fn normalize_and(expressions: Vec) -> FilterExpr { - let mut normalized = expressions - .into_iter() - .map(normalize) - .flat_map(|expression| match expression { - FilterExpr::And(children) => children, - expression => vec![expression], - }) - .collect::>(); +fn normalize_and(expressions: SmallVec<[FilterExpr; 4]>) -> FilterExpr { + let mut normalized = expressions.into_iter().map(normalize).fold( + SmallVec::<[_; 4]>::new(), + |mut flattened, expression| { + match expression { + FilterExpr::And(children) => flattened.extend(*children), + expression => flattened.push(expression), + } + flattened + }, + ); normalized.sort(); normalized.dedup(); FilterExpr::and(normalized).expect("an AND expression is non-empty") } -fn normalize_or(expressions: Vec) -> FilterExpr { - let mut normalized = expressions - .into_iter() - .map(normalize) - .flat_map(|expression| match expression { - FilterExpr::Or(children) => children, - expression => vec![expression], - }) - .collect::>(); +fn normalize_or(expressions: SmallVec<[FilterExpr; 4]>) -> FilterExpr { + let mut normalized = expressions.into_iter().map(normalize).fold( + SmallVec::<[_; 4]>::new(), + |mut flattened, expression| { + match expression { + FilterExpr::Or(children) => flattened.extend(*children), + expression => flattened.push(expression), + } + flattened + }, + ); normalized.sort(); normalized.dedup(); @@ -210,7 +274,7 @@ fn predicate_clause( _ => None, }) .collect(), - FilterExpr::Or(_) => None, + FilterExpr::Or(_) | FilterExpr::Not(_) => None, } } @@ -323,4 +387,22 @@ mod tests { assert_eq!(normalized, expected); } + + #[test] + fn normalizes_unary_not() { + let normalized = normalize( + parse_expression( + r#"NOT"project_id"="8xOSkvVU" AND NOT (downloads > 100 OR open_source = true)"#, + ) + .unwrap(), + ); + let expected = normalize( + parse_expression( + r#"project_id != "8xOSkvVU" AND downloads <= 100 AND open_source != true"#, + ) + .unwrap(), + ); + + assert_eq!(normalized, expected); + } } diff --git a/apps/labrinth/src/search/filter/parse.rs b/apps/labrinth/src/search/filter/parse.rs index 8dc28c6ec1..3d7973f038 100644 --- a/apps/labrinth/src/search/filter/parse.rs +++ b/apps/labrinth/src/search/filter/parse.rs @@ -18,16 +18,25 @@ pub struct FilterParseError { fn keyword( keyword: &'static str, ) -> BoxedParser<'static, char, (), Simple> { - keyword - .chars() - .fold(empty().ignored().boxed(), |parser, character| { - parser - .then_ignore(one_of([ - character.to_ascii_lowercase(), - character.to_ascii_uppercase(), - ])) - .boxed() - }) + let keyword = + keyword + .chars() + .fold(empty().ignored().boxed(), |parser, character| { + parser + .then_ignore(one_of([ + character.to_ascii_lowercase(), + character.to_ascii_uppercase(), + ])) + .boxed() + }); + let boundary = filter(|character: &char| { + !character.is_ascii_alphanumeric() && !"_.".contains(*character) + }) + .rewind() + .ignored() + .or(end()); + + keyword.then_ignore(boundary).boxed() } fn quoted_literal( @@ -63,8 +72,8 @@ fn literal_parser() -> BoxedParser<'static, char, FilterLiteral, Simple> { quoted.or(bare).padded().boxed() } -fn parser() -> impl Parser> { - let field = filter(|character: &char| { +fn field_name_parser() -> BoxedParser<'static, char, String, Simple> { + filter(|character: &char| { character.is_ascii_alphabetic() || "_.".contains(*character) }) .then( @@ -73,9 +82,19 @@ fn parser() -> impl Parser> { }) .repeated(), ) - .map(|(first, rest)| { - FilterField::new(std::iter::once(first).chain(rest).collect::()) - }) + .map(|(first, rest)| std::iter::once(first).chain(rest).collect::()) + .boxed() +} + +fn parser() -> impl Parser> { + let field_name = field_name_parser(); + let field = choice(( + field_name.clone(), + field_name.clone().delimited_by(just('"'), just('"')), + field_name.clone().delimited_by(just('\''), just('\'')), + field_name.delimited_by(just('`'), just('`')), + )) + .map(FilterField::new) .padded(); let literal = literal_parser(); @@ -131,9 +150,16 @@ fn parser() -> impl Parser> { .clone() .or(expression.delimited_by(just('(').padded(), just(')').padded())) .padded(); - let and = atom + let unary = keyword("NOT").padded().repeated().then(atom).map( + |(operators, expression)| { + operators.into_iter().fold(expression, |expression, ()| { + FilterExpr::Not(Box::new(expression)) + }) + }, + ); + let and = unary .clone() - .then(keyword("AND").padded().ignore_then(atom).repeated()) + .then(keyword("AND").padded().ignore_then(unary).repeated()) .map(|(first, rest)| { FilterExpr::and(std::iter::once(first).chain(rest)) .expect("an expression always contains one operand") @@ -167,7 +193,9 @@ pub fn parse_expression(input: &str) -> Result { #[cfg(test)] mod tests { use super::parse_expression; - use crate::search::filter::{FilterCondition, FilterExpr, FilterLiteral}; + use crate::search::filter::{ + FilterComparison, FilterCondition, FilterExpr, FilterLiteral, + }; #[test] fn parses_boolean_precedence() { @@ -204,4 +232,35 @@ mod tests { } if value == "value with spaces" )); } + + #[test] + fn parses_unary_not_with_quoted_field() { + let expression = + parse_expression(r#"NOT"project_id"="8xOSkvVU""#).unwrap(); + + let FilterExpr::Not(expression) = expression else { + panic!("expected a NOT expression"); + }; + let FilterExpr::Predicate(predicate) = *expression else { + panic!("expected a predicate"); + }; + assert_eq!(predicate.field.as_str(), "project_id"); + assert!(matches!( + predicate.condition, + FilterCondition::Compare { + comparison: FilterComparison::Equal, + value: FilterLiteral::String(value), + } if value == "8xOSkvVU" + )); + } + + #[test] + fn does_not_parse_not_prefix_in_field_name_as_operator() { + let expression = parse_expression("notification = true").unwrap(); + + let FilterExpr::Predicate(predicate) = expression else { + panic!("expected a predicate"); + }; + assert_eq!(predicate.field.as_str(), "notification"); + } }