diff --git a/README.md b/README.md index 6ae6e21..8f768c0 100644 --- a/README.md +++ b/README.md @@ -179,6 +179,13 @@ constructors `timestamp("...")` and `uuid("...")`. | `created > timestamp('2026-01-01T00:00:00Z')` | `(created) > ($1)`, bound as a timestamp | | `id == uuid('0123…cdef')` | `(id) = ($1)`, bound as a UUID | +A blank filter parses to the empty filter, as a blank ordering does to the empty +clause: it resolves against any mapping and renders as an empty `WhereClause`, +which leaves its slot empty — so a request's `filter` goes through the same +parse and resolve whether or not it says anything. An empty `WhereClause` is a +valid value throughout: `and` and `or` return the other side, and a query with +no `where` slot takes one without complaint. + Two deliberate choices: `== null` renders as `IS NULL`, because `= NULL` is never true and so never what was meant; and both sides of a binary operator are always parenthesized, so there is no precedence table to get wrong. diff --git a/crates/sqlx-query-cel/src/filter.rs b/crates/sqlx-query-cel/src/filter.rs index 894a660..6ea4ffe 100644 --- a/crates/sqlx-query-cel/src/filter.rs +++ b/crates/sqlx-query-cel/src/filter.rs @@ -36,9 +36,6 @@ use sqlx_query::{QueryResolver, Value, WhereClause}; /// Errors [`FilterClause::parse`]/[`FilterClause::resolve`] can return. #[derive(Debug, thiserror::Error, PartialEq)] pub enum FilterClauseError { - #[error("a filter cannot be blank")] - Blank, - #[error("{0}")] Parse(String), @@ -56,9 +53,14 @@ pub enum FilterClauseError { /// A parsed, not-yet-resolved CEL filter condition. See the module docs /// for the parse -> resolve -> render pipeline. -#[derive(Debug, Clone)] +/// +/// The default is the empty filter — what a blank string parses to — which +/// has no condition: it resolves to itself and renders as an empty +/// [`WhereClause`], so a composer leaves its slot empty. +#[derive(Debug, Clone, Default)] pub struct FilterClause { - expr: cel::IdedExpr, + /// `None` for the empty filter. + expr: Option, } impl FilterClause { @@ -68,21 +70,19 @@ impl FilterClause { /// `status == null` is written as `IS NULL` rather than `= NULL`, /// which in SQL is never true and so never what was meant. /// - /// Unlike [`OrderByClause::parse`](sqlx_query::OrderByClause::parse), - /// blank is refused rather than read as "no condition": an ordering - /// is a list and can be empty, a condition is one expression and - /// cannot. Skip the call instead. + /// An empty or all-whitespace string is the empty filter, no condition + /// at all — as [`OrderByClause::parse`](sqlx_query::OrderByClause::parse) + /// reads a blank ordering, and as AIP-160 reads a blank `filter`. /// /// # Errors /// - /// [`FilterClauseError::Blank`] on an empty/all-whitespace string; /// [`FilterClauseError::Parse`] if it isn't valid CEL; /// [`FilterClauseError::Unsupported`] if it parses as CEL with no /// reading as a condition (a macro, a map literal, a function call /// other than the operators above, ...). pub fn parse(filter: &str) -> Result { if filter.trim().is_empty() { - return Err(FilterClauseError::Blank); + return Ok(FilterClause::default()); } let parsed = cel::parser::Parser::new() @@ -92,14 +92,14 @@ impl FilterClause { // Fails fast on anything with no reading as a condition. Self::validate(&parsed)?; - Ok(FilterClause { expr: parsed }) + Ok(FilterClause { expr: Some(parsed) }) } /// This condition as a [`WhereClause`], with every literal bound to /// a `$N` placeholder rather than spliced into the text — matches /// [`Cursor::to_where_clause`](sqlx_query::Cursor::to_where_clause)'s /// naming, since both types answer "what's your WHERE-clause form?" - /// the same way. + /// the same way. The empty filter is the empty clause. /// /// # Panics /// @@ -108,9 +108,13 @@ impl FilterClause { /// `FilterClause` exists to call this on. #[must_use] pub fn to_where_clause(&self) -> WhereClause { + let Some(expr) = &self.expr else { + return WhereClause::default(); + }; + let mut values = Vec::new(); - let sql = Self::render(&self.expr, &mut values) - .expect("parse() already validated this tree renders"); + let sql = + Self::render(expr, &mut values).expect("parse() already validated this tree renders"); values .into_iter() .fold(WhereClause::new(sql), WhereClause::bind_value) @@ -487,7 +491,9 @@ impl QueryResolver for FilterClause { /// offered. Same allow-list shape as /// [`OrderByClause::resolve`](sqlx_query::OrderByClause::resolve). fn resolve(mut self, columns: &HashMap<&str, &str>) -> Result { - Self::rename(&mut self.expr, columns)?; + if let Some(expr) = &mut self.expr { + Self::rename(expr, columns)?; + } Ok(self) } } @@ -568,16 +574,22 @@ mod tests { ); } + /// A blank filter is no condition: it resolves against any mapping, the + /// empty one included, and renders as the empty clause. + #[test] + fn a_blank_filter_is_the_empty_one() { + for blank in ["", " ", "\n\t"] { + let filter = FilterClause::parse(blank) + .and_then(|filter| filter.resolve(&HashMap::new())) + .expect("a blank filter parses and resolves"); + + assert!(filter.to_where_clause().is_empty(), "{blank:?}"); + } + assert!(FilterClause::default().to_where_clause().is_empty()); + } + #[test] fn what_has_no_reading_as_a_condition_is_refused() { - assert!(matches!( - FilterClause::parse(""), - Err(FilterClauseError::Blank) - )); - assert!(matches!( - FilterClause::parse(" "), - Err(FilterClauseError::Blank) - )); assert!(refuses("size(name) > 3")); assert!(refuses("name.size() > 3")); assert!(refuses("[1, 2].all(x, x > 0)")); diff --git a/crates/sqlx-query-cel/tests/execute.rs b/crates/sqlx-query-cel/tests/execute.rs index 322a6ee..d936e50 100644 --- a/crates/sqlx-query-cel/tests/execute.rs +++ b/crates/sqlx-query-cel/tests/execute.rs @@ -72,6 +72,14 @@ async fn a_pattern_matches_a_prefix_a_suffix_and_a_substring() { assert_eq!(sqlite("name.contains('cer')").await.len(), 2); } +/// A blank filter is no filter: every row, straight through the composer +/// that would otherwise have `AND`ed a condition in. +#[tokio::test] +async fn a_blank_filter_selects_every_row() { + assert_eq!(sqlite("").await.len(), NAMES.len()); + assert_eq!(sqlite(" ").await.len(), NAMES.len()); +} + /// `None` when there is no server to talk to -- see `sqlx-query`'s /// `execute_postgres.rs` for why reachability, not the variable, decides. async fn postgres() -> Option { diff --git a/crates/sqlx-query/src/clauses/where.rs b/crates/sqlx-query/src/clauses/where.rs index b9b3091..bf56ab2 100644 --- a/crates/sqlx-query/src/clauses/where.rs +++ b/crates/sqlx-query/src/clauses/where.rs @@ -22,6 +22,13 @@ pub struct WhereClause { values: Vec, } +/// The clause that says nothing: see [`WhereClause::is_empty`]. +impl Default for WhereClause { + fn default() -> Self { + WhereClause::new("") + } +} + impl WhereClause { /// A `WhereClause` with no bind values — most hand-written filters /// (e.g. `"deleted_at IS NULL"`) don't reference any. Attach values @@ -59,6 +66,16 @@ impl WhereClause { &self.values } + /// Whether this clause says nothing — no SQL text, or only whitespace. + /// An empty clause is a valid value: [`and`](Self::and) and + /// [`or`](Self::or) return the other side unchanged, and + /// [`QueryComposer`](crate::QueryComposer) leaves its slot empty, the + /// way it does for an empty [`OrderByClause`](crate::OrderByClause). + #[must_use] + pub fn is_empty(&self) -> bool { + self.sql.as_str().trim().is_empty() + } + /// This clause's SQL text. Matches /// [`OrderByClause::sql`](crate::OrderByClause::sql)'s return type — /// both clause types answer "what's your SQL text?" as [`SqlStr`], the @@ -83,6 +100,9 @@ impl WhereClause { /// reference one value twice (a [`Cursor`](crate::Cursor)'s tuple /// comparison does exactly that), and shifting by the count would then /// land `other` on top of a number `self` is still using. + /// + /// An [empty](Self::is_empty) side contributes nothing: the result is + /// the other side, as it was. #[must_use] pub fn and(self, other: WhereClause) -> WhereClause { self.combine(other, "AND") @@ -93,13 +113,21 @@ impl WhereClause { /// [`and`](Self::and) does. /// /// Both sides are parenthesised, so there's no precedence to reason - /// about when mixing this with `and`. + /// about when mixing this with `and`. An [empty](Self::is_empty) side + /// contributes nothing, as for `and`. #[must_use] pub fn or(self, other: WhereClause) -> WhereClause { self.combine(other, "OR") } fn combine(self, other: WhereClause, connective: &str) -> WhereClause { + if other.is_empty() { + return self; + } + if self.is_empty() { + return other; + } + let lexer = QueryLexer::standard(); let offset = lexer.max_placeholder_number(self.sql().as_str()); let other_sql = lexer.shift_placeholder_numbers(other.sql().as_str(), offset); @@ -159,6 +187,20 @@ mod tests { ); } + #[test] + fn an_empty_side_contributes_nothing() { + let status = WhereClause::new("status = $1").bind_value("ACTIVE"); + + assert!(WhereClause::default().is_empty()); + assert!(WhereClause::new(" ").is_empty()); + assert_eq!(WhereClause::default().and(status.clone()), status); + assert_eq!(status.clone().and(WhereClause::default()), status); + assert_eq!(status.clone().or(WhereClause::new(" ")), status); + assert!(WhereClause::default() + .and(WhereClause::default()) + .is_empty()); + } + #[test] fn and_with_no_values_on_either_side_needs_no_shift() { let a = WhereClause::new("deleted_at IS NULL"); diff --git a/crates/sqlx-query/src/composer.rs b/crates/sqlx-query/src/composer.rs index 046474b..dbf44ba 100644 --- a/crates/sqlx-query/src/composer.rs +++ b/crates/sqlx-query/src/composer.rs @@ -418,7 +418,7 @@ impl QueryComposer { .cloned() .chain(self.cursor.as_ref().map(Cursor::to_where_clause)) .reduce(WhereClause::and) - .filter(|w| !w.sql().as_str().is_empty()) + .filter(|w| !w.is_empty()) .map(|w| w.shift(offset)); // Parenthesised whole, whatever it holds. The slot's connective joins diff --git a/crates/sqlx-query/tests/composer.rs b/crates/sqlx-query/tests/composer.rs index 34ff8a9..9f66eca 100644 --- a/crates/sqlx-query/tests/composer.rs +++ b/crates/sqlx-query/tests/composer.rs @@ -427,6 +427,50 @@ fn cursor_and_filter_are_combined_with_and() { ); } +/// An empty filter -- what a blank one parses to -- is no filter: the slot +/// drops exactly as if nothing had been pushed. +#[test] +fn an_empty_filter_drops_the_where_slot() { + let sql = "SELECT id FROM users WHERE /* query.where AND */ id = $1"; + + let mut pushed = QueryComposer::::new(sql); + pushed.bind_value("007").push_where(WhereClause::default()); + let mut untouched = QueryComposer::::new(sql); + untouched.bind_value("007"); + + assert_eq!( + pushed.compose().unwrap().sql(), + untouched.compose().unwrap().sql() + ); +} + +/// Nor does an empty filter need a slot to go in: there is nothing to +/// splice, so a query without one is not an error. +#[test] +fn an_empty_filter_needs_no_where_slot() { + let mut query = QueryComposer::::new("SELECT id FROM users"); + query.push_where(WhereClause::default()); + + assert_eq!(query.compose().unwrap().sql(), "SELECT id FROM users"); +} + +/// Beside a cursor, an empty filter leaves the cursor's comparison as it +/// is, rather than `AND`ing it with `()`. +#[test] +fn an_empty_filter_leaves_a_cursor_alone() { + let sql = "SELECT * FROM t WHERE /* query.where AND */ TRUE"; + let mut query = QueryComposer::::new(sql); + query + .push_where(WhereClause::default()) + .with_cursor(rank_cursor()); + + let statement = query.compose().unwrap(); + let (sql, values) = (statement.sql(), values(&statement)); + + assert!(sql.contains("WHERE ((rank < $1) OR (rank = $1 AND id > $2)) AND TRUE")); + assert_eq!(values, vec![Value::Int(42), Value::Int(7)]); +} + /// The reason everything is numbered internally: a `?` binds by where it /// sits in the text, so a slot *ahead* of the base query's own markers /// binds ahead of them too. A value list built as base-then-fragments —