From ae2c01d3700cfd5199468fd77217ba11157367d9 Mon Sep 17 00:00:00 2001 From: Magnus Bakken <10287813+magbak@users.noreply.github.com> Date: Wed, 23 Aug 2023 21:47:16 +0200 Subject: [PATCH 1/2] Issue #674: Fix issues with semantics of parenthesis removal (#675) * Issue #674: Fix issues with semantics of parenthesis removal * Fix comment * #674 Revamp how conditions are written to SQL - now uniformly handled as SimpleExpr:Binary * #674 Fix clippy * Issue #674 Move methods for deciding associativity and precedence to trait * Added extra testcase from Issue #674 * Issue #674: Follow up on comments in code review --- src/backend/mod.rs | 109 ++++++++++++++++++++++ src/backend/mysql/mod.rs | 16 ++++ src/backend/postgres/query.rs | 51 ++++++++++ src/backend/query_builder.rs | 170 ++++++++++++++++++++++------------ src/backend/sqlite/mod.rs | 16 ++++ src/expr.rs | 45 ++------- src/lib.rs | 2 +- src/query/condition.rs | 33 ++++++- tests/mysql/query.rs | 17 ++-- tests/postgres/query.rs | 91 ++++++++++++++++-- tests/sqlite/query.rs | 17 ++-- 11 files changed, 438 insertions(+), 129 deletions(-) diff --git a/src/backend/mod.rs b/src/backend/mod.rs index 03c3ec307..b918e8544 100644 --- a/src/backend/mod.rs +++ b/src/backend/mod.rs @@ -85,3 +85,112 @@ pub trait EscapeBuilder { output } } + +pub trait PrecedenceDecider { + // This method decides which precedence relations should lead to dropped parentheses. + // There will be more fine grained precedence relations than the ones represented here, + // but dropping parentheses due to these relations can be confusing for readers. + fn inner_expr_well_known_greater_precedence( + &self, + inner: &SimpleExpr, + outer_oper: &Oper, + ) -> bool; +} + +pub trait OperLeftAssocDecider { + // This method decides if the left associativity of an operator should lead to dropped parentheses. + // Not all known left associative operators are necessarily included here, + // as dropping them may in some cases be confusing to readers. + fn well_known_left_associative(&self, op: &BinOper) -> bool; +} + +#[derive(Debug, PartialEq)] +pub enum Oper { + UnOper(UnOper), + BinOper(BinOper), +} + +impl From for Oper { + fn from(value: UnOper) -> Self { + Oper::UnOper(value) + } +} + +impl From for Oper { + fn from(value: BinOper) -> Self { + Oper::BinOper(value) + } +} + +impl Oper { + pub(crate) fn is_logical(&self) -> bool { + matches!( + self, + Oper::UnOper(UnOper::Not) | Oper::BinOper(BinOper::And) | Oper::BinOper(BinOper::Or) + ) + } + + pub(crate) fn is_between(&self) -> bool { + matches!( + self, + Oper::BinOper(BinOper::Between) | Oper::BinOper(BinOper::NotBetween) + ) + } + + pub(crate) fn is_like(&self) -> bool { + matches!( + self, + Oper::BinOper(BinOper::Like) | Oper::BinOper(BinOper::NotLike) + ) + } + + pub(crate) fn is_in(&self) -> bool { + matches!( + self, + Oper::BinOper(BinOper::In) | Oper::BinOper(BinOper::NotIn) + ) + } + + pub(crate) fn is_is(&self) -> bool { + matches!( + self, + Oper::BinOper(BinOper::Is) | Oper::BinOper(BinOper::IsNot) + ) + } + + pub(crate) fn is_shift(&self) -> bool { + matches!( + self, + Oper::BinOper(BinOper::LShift) | Oper::BinOper(BinOper::RShift) + ) + } + + pub(crate) fn is_arithmetic(&self) -> bool { + match self { + Oper::BinOper(b) => { + matches!( + b, + BinOper::Mul | BinOper::Div | BinOper::Mod | BinOper::Add | BinOper::Sub + ) + } + _ => false, + } + } + + pub(crate) fn is_comparison(&self) -> bool { + match self { + Oper::BinOper(b) => { + matches!( + b, + BinOper::SmallerThan + | BinOper::SmallerThanOrEqual + | BinOper::Equal + | BinOper::GreaterThanOrEqual + | BinOper::GreaterThan + | BinOper::NotEqual + ) + } + _ => false, + } + } +} diff --git a/src/backend/mysql/mod.rs b/src/backend/mysql/mod.rs index 33a6e6100..4f972e9ed 100644 --- a/src/backend/mysql/mod.rs +++ b/src/backend/mysql/mod.rs @@ -26,3 +26,19 @@ impl QuotedBuilder for MysqlQueryBuilder { impl EscapeBuilder for MysqlQueryBuilder {} impl TableRefBuilder for MysqlQueryBuilder {} + +impl PrecedenceDecider for MysqlQueryBuilder { + fn inner_expr_well_known_greater_precedence( + &self, + inner: &SimpleExpr, + outer_oper: &Oper, + ) -> bool { + common_inner_expr_well_known_greater_precedence(inner, outer_oper) + } +} + +impl OperLeftAssocDecider for MysqlQueryBuilder { + fn well_known_left_associative(&self, op: &BinOper) -> bool { + common_well_known_left_associative(op) + } +} diff --git a/src/backend/postgres/query.rs b/src/backend/postgres/query.rs index 90b1c0ef0..2970f4442 100644 --- a/src/backend/postgres/query.rs +++ b/src/backend/postgres/query.rs @@ -1,6 +1,38 @@ use super::*; use crate::extension::postgres::*; +impl OperLeftAssocDecider for PostgresQueryBuilder { + fn well_known_left_associative(&self, op: &BinOper) -> bool { + let common_answer = common_well_known_left_associative(op); + let pg_specific_answer = matches!(op, BinOper::PgOperator(PgBinOper::Concatenate)); + common_answer || pg_specific_answer + } +} + +impl PrecedenceDecider for PostgresQueryBuilder { + fn inner_expr_well_known_greater_precedence( + &self, + inner: &SimpleExpr, + outer_oper: &Oper, + ) -> bool { + let common_answer = common_inner_expr_well_known_greater_precedence(inner, outer_oper); + let pg_specific_answer = match inner { + SimpleExpr::Binary(_, inner_bin_oper, _) => { + let inner_oper: Oper = (*inner_bin_oper).into(); + if inner_oper.is_arithmetic() || inner_oper.is_shift() { + is_ilike(inner_bin_oper) + } else if is_pg_comparison(inner_bin_oper) { + outer_oper.is_logical() + } else { + false + } + } + _ => false, + }; + common_answer || pg_specific_answer + } +} + impl QueryBuilder for PostgresQueryBuilder { fn placeholder(&self) -> (&str, bool) { ("$", true) @@ -136,3 +168,22 @@ impl QueryBuilder for PostgresQueryBuilder { "COALESCE" } } + +fn is_pg_comparison(b: &BinOper) -> bool { + matches!( + b, + BinOper::PgOperator(PgBinOper::Contained) + | BinOper::PgOperator(PgBinOper::Contains) + | BinOper::PgOperator(PgBinOper::Similarity) + | BinOper::PgOperator(PgBinOper::WordSimilarity) + | BinOper::PgOperator(PgBinOper::StrictWordSimilarity) + | BinOper::PgOperator(PgBinOper::Matches) + ) +} + +fn is_ilike(b: &BinOper) -> bool { + matches!( + b, + BinOper::PgOperator(PgBinOper::ILike) | BinOper::PgOperator(PgBinOper::NotILike) + ) +} diff --git a/src/backend/query_builder.rs b/src/backend/query_builder.rs index 30e934275..bb0d06f05 100644 --- a/src/backend/query_builder.rs +++ b/src/backend/query_builder.rs @@ -3,7 +3,9 @@ use std::ops::Deref; const QUOTE: Quote = Quote(b'"', b'"'); -pub trait QueryBuilder: QuotedBuilder + EscapeBuilder + TableRefBuilder { +pub trait QueryBuilder: + QuotedBuilder + EscapeBuilder + TableRefBuilder + OperLeftAssocDecider + PrecedenceDecider +{ /// The type of placeholder the builder uses for values, and whether it is numbered. fn placeholder(&self) -> (&str, bool) { ("?", false) @@ -296,7 +298,15 @@ pub trait QueryBuilder: QuotedBuilder + EscapeBuilder + TableRefBuilder { SimpleExpr::Unary(op, expr) => { self.prepare_un_oper(op, sql); write!(sql, " ").unwrap(); + let drop_expr_paren = + self.inner_expr_well_known_greater_precedence(expr, &(*op).into()); + if !drop_expr_paren { + write!(sql, "(").unwrap(); + } self.prepare_simple_expr(expr, sql); + if !drop_expr_paren { + write!(sql, ")").unwrap(); + } } SimpleExpr::FunctionCall(func) => { self.prepare_function(&func.func, sql); @@ -1261,53 +1271,8 @@ pub trait QueryBuilder: QuotedBuilder + EscapeBuilder + TableRefBuilder { #[doc(hidden)] /// Translate part of a condition to part of a "WHERE" clause. fn prepare_condition_where(&self, condition: &Condition, sql: &mut dyn SqlWriter) { - if condition.negate { - write!(sql, "NOT ").unwrap() - } - - if condition.is_empty() { - match condition.condition_type { - ConditionType::All => self.prepare_constant_true(sql), - ConditionType::Any => self.prepare_constant_false(sql), - } - return; - } - - if condition.negate { - write!(sql, "(").unwrap(); - } - condition.conditions.iter().fold(true, |first, cond| { - if !first { - match condition.condition_type { - ConditionType::Any => write!(sql, " OR ").unwrap(), - ConditionType::All => write!(sql, " AND ").unwrap(), - } - } - match cond { - ConditionExpression::Condition(c) => { - if condition.conditions.len() > 1 { - write!(sql, "(").unwrap(); - } - self.prepare_condition_where(c, sql); - if condition.conditions.len() > 1 { - write!(sql, ")").unwrap(); - } - } - ConditionExpression::SimpleExpr(e) => { - if condition.conditions.len() > 1 && (e.is_logical() || e.is_between()) { - write!(sql, "(").unwrap(); - } - self.prepare_simple_expr(e, sql); - if condition.conditions.len() > 1 && (e.is_logical() || e.is_between()) { - write!(sql, ")").unwrap(); - } - } - } - false - }); - if condition.negate { - write!(sql, ")").unwrap(); - } + let simple_expr = condition.to_simple_expr(); + self.prepare_simple_expr(&simple_expr, sql); } #[doc(hidden)] @@ -1378,11 +1343,16 @@ pub trait QueryBuilder: QuotedBuilder + EscapeBuilder + TableRefBuilder { right: &SimpleExpr, sql: &mut dyn SqlWriter, ) { - let no_paren = matches!(op, BinOper::Equal | BinOper::NotEqual); - let left_paren = left.need_parentheses() - && left.is_binary() - && op != left.get_bin_oper().unwrap() - && !no_paren; + // If left has higher precedence than op, we can drop parentheses around left + let drop_left_higher_precedence = + self.inner_expr_well_known_greater_precedence(left, &(*op).into()); + + // Figure out if left associativity rules allow us to drop the left parenthesis + let drop_left_assoc = left.is_binary() + && op == left.get_bin_oper().unwrap() + && self.well_known_left_associative(op); + + let left_paren = !drop_left_higher_precedence && !drop_left_assoc; if left_paren { write!(sql, "(").unwrap(); } @@ -1390,17 +1360,33 @@ pub trait QueryBuilder: QuotedBuilder + EscapeBuilder + TableRefBuilder { if left_paren { write!(sql, ")").unwrap(); } + write!(sql, " ").unwrap(); self.prepare_bin_oper(op, sql); write!(sql, " ").unwrap(); - let no_right_paren = matches!( - op, - BinOper::Between | BinOper::NotBetween | BinOper::Like | BinOper::NotLike - ); - let right_paren = (right.need_parentheses() - || right.is_binary() && op != left.get_bin_oper().unwrap()) - && !no_right_paren - && !no_paren; + + // If right has higher precedence than op, we can drop parentheses around right + let drop_right_higher_precedence = + self.inner_expr_well_known_greater_precedence(right, &(*op).into()); + + let op_as_oper = Oper::BinOper(*op); + // Due to representation of trinary op between as nested binary ops. + let drop_right_between_hack = op_as_oper.is_between() + && right.is_binary() + && matches!(right.get_bin_oper(), Some(&BinOper::And)); + + // Due to representation of trinary op like/not like with optional arg escape as nested binary ops. + let drop_right_escape_hack = op_as_oper.is_like() + && right.is_binary() + && matches!(right.get_bin_oper(), Some(&BinOper::Escape)); + + // Due to custom representation of casting AS datatype + let drop_right_as_hack = (op == &BinOper::As) && matches!(right, SimpleExpr::Custom(_)); + + let right_paren = !drop_right_higher_precedence + && !drop_right_escape_hack + && !drop_right_between_hack + && !drop_right_as_hack; if right_paren { write!(sql, "(").unwrap(); } @@ -1493,6 +1479,22 @@ impl SubQueryStatement { pub(crate) struct CommonSqlQueryBuilder; +impl OperLeftAssocDecider for CommonSqlQueryBuilder { + fn well_known_left_associative(&self, op: &BinOper) -> bool { + common_well_known_left_associative(op) + } +} + +impl PrecedenceDecider for CommonSqlQueryBuilder { + fn inner_expr_well_known_greater_precedence( + &self, + inner: &SimpleExpr, + outer_oper: &Oper, + ) -> bool { + common_inner_expr_well_known_greater_precedence(inner, outer_oper) + } +} + impl QueryBuilder for CommonSqlQueryBuilder { fn prepare_query_statement(&self, query: &SubQueryStatement, sql: &mut dyn SqlWriter) { query.prepare_statement(self, sql); @@ -1512,3 +1514,49 @@ impl QuotedBuilder for CommonSqlQueryBuilder { impl EscapeBuilder for CommonSqlQueryBuilder {} impl TableRefBuilder for CommonSqlQueryBuilder {} + +pub(crate) fn common_inner_expr_well_known_greater_precedence( + inner: &SimpleExpr, + outer_oper: &Oper, +) -> bool { + match inner { + // We only consider the case where an inner expression is contained in either a + // unary or binary expression (with an outer_oper). + // We do not need to wrap with parentheses: + // Columns, tuples (already wrapped), constants, function calls, values, + // keywords, subqueries (already wrapped) + SimpleExpr::Column(_) + | SimpleExpr::Tuple(_) + | SimpleExpr::Constant(_) + | SimpleExpr::FunctionCall(_) + | SimpleExpr::Value(_) + | SimpleExpr::Keyword(_) + | SimpleExpr::SubQuery(_, _) => true, + SimpleExpr::Binary(_, inner_oper, _) => { + let inner_oper: Oper = (*inner_oper).into(); + if inner_oper.is_arithmetic() || inner_oper.is_shift() { + outer_oper.is_comparison() + || outer_oper.is_between() + || outer_oper.is_in() + || outer_oper.is_like() + || outer_oper.is_logical() + } else if inner_oper.is_comparison() + || inner_oper.is_in() + || inner_oper.is_like() + || inner_oper.is_is() + { + outer_oper.is_logical() + } else { + false + } + } + _ => false, + } +} + +pub(crate) fn common_well_known_left_associative(op: &BinOper) -> bool { + matches!( + op, + BinOper::And | BinOper::Or | BinOper::Add | BinOper::Sub | BinOper::Mul | BinOper::Mod + ) +} diff --git a/src/backend/sqlite/mod.rs b/src/backend/sqlite/mod.rs index a768a29d9..b884cc5ed 100644 --- a/src/backend/sqlite/mod.rs +++ b/src/backend/sqlite/mod.rs @@ -32,3 +32,19 @@ impl EscapeBuilder for SqliteQueryBuilder { } impl TableRefBuilder for SqliteQueryBuilder {} + +impl PrecedenceDecider for SqliteQueryBuilder { + fn inner_expr_well_known_greater_precedence( + &self, + inner: &SimpleExpr, + outer_oper: &Oper, + ) -> bool { + common_inner_expr_well_known_greater_precedence(inner, outer_oper) + } +} + +impl OperLeftAssocDecider for SqliteQueryBuilder { + fn well_known_left_associative(&self, op: &BinOper) -> bool { + common_well_known_left_associative(op) + } +} diff --git a/src/expr.rs b/src/expr.rs index 48b72b2f7..ed846c15f 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -359,11 +359,11 @@ impl Expr { /// /// assert_eq!( /// query.to_string(MysqlQueryBuilder), - /// r#"SELECT `character`, `size_w`, `size_h` FROM `character` WHERE `id` = 1 AND 6 = 2 * 3"# + /// r#"SELECT `character`, `size_w`, `size_h` FROM `character` WHERE `id` = 1 AND (6 = 2 * 3)"# /// ); /// assert_eq!( /// query.to_string(SqliteQueryBuilder), - /// r#"SELECT "character", "size_w", "size_h" FROM "character" WHERE "id" = 1 AND 6 = 2 * 3"# + /// r#"SELECT "character", "size_w", "size_h" FROM "character" WHERE "id" = 1 AND (6 = 2 * 3)"# /// ); /// ``` /// ``` @@ -378,7 +378,7 @@ impl Expr { /// /// assert_eq!( /// query.to_string(PostgresQueryBuilder), - /// r#"SELECT "character", "size_w", "size_h" FROM "character" WHERE "id" = 1 AND 6 = 2 * 3"# + /// r#"SELECT "character", "size_w", "size_h" FROM "character" WHERE "id" = 1 AND (6 = 2 * 3)"# /// ); /// ``` /// ``` @@ -2127,15 +2127,15 @@ impl SimpleExpr { /// /// assert_eq!( /// query.to_string(MysqlQueryBuilder), - /// r#"SELECT `character`, `size_w`, `size_h` FROM `character` WHERE ((`size_w` = 1) AND (`size_h` = 2)) OR ((`size_w` = 3) AND (`size_h` = 4))"# + /// r#"SELECT `character`, `size_w`, `size_h` FROM `character` WHERE (`size_w` = 1 AND `size_h` = 2) OR (`size_w` = 3 AND `size_h` = 4)"# /// ); /// assert_eq!( /// query.to_string(PostgresQueryBuilder), - /// r#"SELECT "character", "size_w", "size_h" FROM "character" WHERE (("size_w" = 1) AND ("size_h" = 2)) OR (("size_w" = 3) AND ("size_h" = 4))"# + /// r#"SELECT "character", "size_w", "size_h" FROM "character" WHERE ("size_w" = 1 AND "size_h" = 2) OR ("size_w" = 3 AND "size_h" = 4)"# /// ); /// assert_eq!( /// query.to_string(SqliteQueryBuilder), - /// r#"SELECT "character", "size_w", "size_h" FROM "character" WHERE (("size_w" = 1) AND ("size_h" = 2)) OR (("size_w" = 3) AND ("size_h" = 4))"# + /// r#"SELECT "character", "size_w", "size_h" FROM "character" WHERE ("size_w" = 1 AND "size_h" = 2) OR ("size_w" = 3 AND "size_h" = 4)"# /// ); /// ``` pub fn and(self, right: SimpleExpr) -> Self { @@ -2158,15 +2158,15 @@ impl SimpleExpr { /// /// assert_eq!( /// query.to_string(MysqlQueryBuilder), - /// r#"SELECT `character`, `size_w`, `size_h` FROM `character` WHERE ((`size_w` = 1) OR (`size_h` = 2)) AND ((`size_w` = 3) OR (`size_h` = 4))"# + /// r#"SELECT `character`, `size_w`, `size_h` FROM `character` WHERE (`size_w` = 1 OR `size_h` = 2) AND (`size_w` = 3 OR `size_h` = 4)"# /// ); /// assert_eq!( /// query.to_string(PostgresQueryBuilder), - /// r#"SELECT "character", "size_w", "size_h" FROM "character" WHERE (("size_w" = 1) OR ("size_h" = 2)) AND (("size_w" = 3) OR ("size_h" = 4))"# + /// r#"SELECT "character", "size_w", "size_h" FROM "character" WHERE ("size_w" = 1 OR "size_h" = 2) AND ("size_w" = 3 OR "size_h" = 4)"# /// ); /// assert_eq!( /// query.to_string(SqliteQueryBuilder), - /// r#"SELECT "character", "size_w", "size_h" FROM "character" WHERE (("size_w" = 1) OR ("size_h" = 2)) AND (("size_w" = 3) OR ("size_h" = 4))"# + /// r#"SELECT "character", "size_w", "size_h" FROM "character" WHERE ("size_w" = 1 OR "size_h" = 2) AND ("size_w" = 3 OR "size_h" = 4)"# /// ); /// ``` pub fn or(self, right: SimpleExpr) -> Self { @@ -2488,37 +2488,10 @@ impl SimpleExpr { self.like_like(BinOper::NotLike, like.into_like_expr()) } - pub(crate) fn need_parentheses(&self) -> bool { - match self { - Self::Binary(left, oper, _) => !matches!( - (left.as_ref(), oper), - (Self::Binary(_, BinOper::And, _), BinOper::And) - | (Self::Binary(_, BinOper::Or, _), BinOper::Or) - ), - _ => false, - } - } - pub(crate) fn is_binary(&self) -> bool { matches!(self, Self::Binary(_, _, _)) } - pub(crate) fn is_logical(&self) -> bool { - match self { - Self::Binary(_, op, _) => { - matches!(op, BinOper::And | BinOper::Or) - } - _ => false, - } - } - - pub(crate) fn is_between(&self) -> bool { - matches!( - self, - Self::Binary(_, BinOper::Between, _) | Self::Binary(_, BinOper::NotBetween, _) - ) - } - pub(crate) fn get_bin_oper(&self) -> Option<&BinOper> { match self { Self::Binary(_, oper, _) => Some(oper), diff --git a/src/lib.rs b/src/lib.rs index 390c75a3d..17987430c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -260,7 +260,7 @@ //! r#"SELECT "character" FROM "character""#, //! r#"WHERE ("size_w" + 1) * 2 = ("size_h" / 2) - 1"#, //! r#"AND "size_w" IN (SELECT ln(2.4 ^ 1.2))"#, -//! r#"AND (("character" LIKE 'D') AND ("character" LIKE 'E'))"#, +//! r#"AND ("character" LIKE 'D' AND "character" LIKE 'E')"#, //! ] //! .join(" ") //! ); diff --git a/src/query/condition.rs b/src/query/condition.rs index 0bd94e4f6..421ee11c2 100644 --- a/src/query/condition.rs +++ b/src/query/condition.rs @@ -62,7 +62,7 @@ impl Condition { /// .to_string(PostgresQueryBuilder); /// assert_eq!( /// statement, - /// r#"SELECT "id" FROM "glyph" WHERE (NOT ("aspect" = 0)) AND (NOT ("id" = 0))"# + /// r#"SELECT "id" FROM "glyph" WHERE (NOT "aspect" = 0) AND (NOT "id" = 0)"# /// ); /// ``` #[allow(clippy::should_implement_trait)] @@ -260,6 +260,37 @@ impl Condition { pub fn len(&self) -> usize { self.conditions.len() } + + pub(crate) fn to_simple_expr(&self) -> SimpleExpr { + let mut inner_exprs = vec![]; + for ce in &self.conditions { + inner_exprs.push(match ce { + ConditionExpression::Condition(c) => c.to_simple_expr(), + ConditionExpression::SimpleExpr(e) => e.clone(), + }); + } + let mut inner_exprs_into_iter = inner_exprs.into_iter(); + let expr = if let Some(first_expr) = inner_exprs_into_iter.next() { + let mut out_expr = first_expr; + for e in inner_exprs_into_iter { + out_expr = match self.condition_type { + ConditionType::Any => out_expr.or(e), + ConditionType::All => out_expr.and(e), + }; + } + out_expr + } else { + SimpleExpr::Constant(match self.condition_type { + ConditionType::Any => false.into(), + ConditionType::All => true.into(), + }) + }; + if self.negate { + expr.not() + } else { + expr + } + } } impl From for ConditionExpression { diff --git a/tests/mysql/query.rs b/tests/mysql/query.rs index fd656678b..f8273b018 100644 --- a/tests/mysql/query.rs +++ b/tests/mysql/query.rs @@ -138,7 +138,7 @@ fn select_10() { .and(Expr::col((Char::Table, Char::FontId)).equals((Font::Table, Font::Id))) ) .to_string(MysqlQueryBuilder), - "SELECT `character` FROM `character` LEFT JOIN `font` ON (`character`.`font_id` = `font`.`id`) AND (`character`.`font_id` = `font`.`id`)" + "SELECT `character` FROM `character` LEFT JOIN `font` ON `character`.`font_id` = `font`.`id` AND `character`.`font_id` = `font`.`id`" ); } @@ -326,7 +326,7 @@ fn select_22() { ) ) .to_string(MysqlQueryBuilder), - "SELECT `character` FROM `character` WHERE (`character` LIKE 'C' OR ((`character` LIKE 'D') AND (`character` LIKE 'E'))) AND ((`character` LIKE 'F') OR (`character` LIKE 'G'))" + "SELECT `character` FROM `character` WHERE (`character` LIKE 'C' OR (`character` LIKE 'D' AND `character` LIKE 'E')) AND (`character` LIKE 'F' OR `character` LIKE 'G')" ); } @@ -500,8 +500,8 @@ fn select_34a() { .to_string(MysqlQueryBuilder), [ "SELECT `aspect`, MAX(`image`) FROM `glyph` GROUP BY `aspect`", - "HAVING ((`aspect` > 2) OR (`aspect` < 8))", - "OR ((`aspect` > 12) AND (`aspect` < 18))", + "HAVING `aspect` > 2 OR `aspect` < 8", + "OR (`aspect` > 12 AND `aspect` < 18)", "OR `aspect` > 32", ] .join(" ") @@ -546,10 +546,7 @@ fn select_37() { .cond_where(Cond::any().add(Cond::all()).add(Cond::any())) .build(MysqlQueryBuilder); - assert_eq!( - statement, - r#"SELECT `id` FROM `glyph` WHERE (TRUE) OR (FALSE)"# - ); + assert_eq!(statement, r#"SELECT `id` FROM `glyph` WHERE TRUE OR FALSE"#); assert_eq!(values.0, vec![]); } @@ -688,7 +685,7 @@ fn select_44() { assert_eq!( statement, - r#"SELECT `id` FROM `glyph` WHERE NOT (`aspect` < 8)"# + r#"SELECT `id` FROM `glyph` WHERE NOT `aspect` < 8"# ); } @@ -725,7 +722,7 @@ fn select_46() { assert_eq!( statement, - r#"SELECT `id` FROM `glyph` WHERE NOT (`aspect` < 8)"# + r#"SELECT `id` FROM `glyph` WHERE NOT `aspect` < 8"# ); } diff --git a/tests/postgres/query.rs b/tests/postgres/query.rs index a99311142..64c571a1f 100644 --- a/tests/postgres/query.rs +++ b/tests/postgres/query.rs @@ -142,7 +142,7 @@ fn select_10() { .and(Expr::col((Char::Table, Char::FontId)).equals((Font::Table, Font::Id))) ) .to_string(PostgresQueryBuilder), - r#"SELECT "character" FROM "character" LEFT JOIN "font" ON ("character"."font_id" = "font"."id") AND ("character"."font_id" = "font"."id")"# + r#"SELECT "character" FROM "character" LEFT JOIN "font" ON "character"."font_id" = "font"."id" AND "character"."font_id" = "font"."id""# ); } @@ -315,7 +315,7 @@ fn select_22() { ) ) .to_string(PostgresQueryBuilder), - r#"SELECT "character" FROM "character" WHERE ("character" LIKE 'C' OR (("character" LIKE 'D') AND ("character" LIKE 'E'))) AND (("character" LIKE 'F') OR ("character" LIKE 'G'))"# + r#"SELECT "character" FROM "character" WHERE ("character" LIKE 'C' OR ("character" LIKE 'D' AND "character" LIKE 'E')) AND ("character" LIKE 'F' OR "character" LIKE 'G')"# ); } @@ -484,8 +484,8 @@ fn select_34a() { .to_string(PostgresQueryBuilder), [ r#"SELECT "aspect", MAX("image") FROM "glyph" GROUP BY "aspect""#, - r#"HAVING (("aspect" > 2) OR ("aspect" < 8))"#, - r#"OR (("aspect" > 12) AND ("aspect" < 18))"#, + r#"HAVING "aspect" > 2 OR "aspect" < 8"#, + r#"OR ("aspect" > 12 AND "aspect" < 18)"#, r#"OR "aspect" > 32"#, ] .join(" ") @@ -530,10 +530,7 @@ fn select_37() { .cond_where(Cond::any().add(Cond::all()).add(Cond::any())) .build(PostgresQueryBuilder); - assert_eq!( - statement, - r#"SELECT "id" FROM "glyph" WHERE (TRUE) OR (FALSE)"# - ); + assert_eq!(statement, r#"SELECT "id" FROM "glyph" WHERE TRUE OR FALSE"#); assert_eq!(values.0, vec![]); } @@ -672,7 +669,7 @@ fn select_44() { assert_eq!( statement, - r#"SELECT "id" FROM "glyph" WHERE NOT ("aspect" < 8)"# + r#"SELECT "id" FROM "glyph" WHERE NOT "aspect" < 8"# ); } @@ -709,7 +706,7 @@ fn select_46() { assert_eq!( statement, - r#"SELECT "id" FROM "glyph" WHERE NOT ("aspect" < 8)"# + r#"SELECT "id" FROM "glyph" WHERE NOT "aspect" < 8"# ); } @@ -1912,3 +1909,77 @@ fn regex_case_insensitive_bin_oper() { ) ); } + +#[test] +fn test_issue_674_nested_logical() { + let t = SimpleExpr::Value(true.into()); + let f = SimpleExpr::Value(false.into()); + + let x_op_y = |x, op, y| SimpleExpr::Binary(Box::new(x), op, Box::new(y)); + let t_or_t = x_op_y(t.clone(), BinOper::Or, t.clone()); + let t_or_t_or_f = x_op_y(t_or_t, BinOper::Or, f); + let t_or_t_or_t_and_t = x_op_y(t_or_t_or_f.clone(), BinOper::And, t); + + assert_eq!( + Query::select() + .columns([Char::Character]) + .from(Char::Table) + .and_where(t_or_t_or_t_and_t) + .to_string(PostgresQueryBuilder), + r#"SELECT "character" FROM "character" WHERE (TRUE OR TRUE OR FALSE) AND TRUE"# + ); +} + +#[test] +fn test_issue_674_nested_comparison() { + let int100 = SimpleExpr::Value(100i32.into()); + let int0 = SimpleExpr::Value(0i32.into()); + let int1 = SimpleExpr::Value(1i32.into()); + + let x_op_y = |x, op, y| SimpleExpr::Binary(Box::new(x), op, Box::new(y)); + let t_smaller_than_t = x_op_y(int100, BinOper::SmallerThan, int0); + let t_smaller_than_t_smaller_than_f = x_op_y(t_smaller_than_t, BinOper::SmallerThan, int1); + + assert_eq!( + Query::select() + .columns([Char::Character]) + .from(Char::Table) + .and_where(t_smaller_than_t_smaller_than_f) + .to_string(PostgresQueryBuilder), + r#"SELECT "character" FROM "character" WHERE (100 < 0) < 1"# + ); +} + +#[test] +fn test_issue_674_and_inside_not() { + let t = SimpleExpr::Value(true.into()); + let f = SimpleExpr::Value(false.into()); + + let op_x = |op, x| SimpleExpr::Unary(op, Box::new(x)); + let x_op_y = |x, op, y| SimpleExpr::Binary(Box::new(x), op, Box::new(y)); + let f_and_t = x_op_y(f, BinOper::And, t); + let not_f_and_t = op_x(UnOper::Not, f_and_t); + + assert_eq!( + Query::select() + .columns([Char::Character]) + .from(Char::Table) + .and_where(not_f_and_t) + .to_string(PostgresQueryBuilder), + r#"SELECT "character" FROM "character" WHERE NOT (FALSE AND TRUE)"# + ); +} + +#[test] +fn test_issue_674_nested_logical_panic() { + let e = SimpleExpr::from(true).and(SimpleExpr::from(true).and(true.into()).and(true.into())); + + assert_eq!( + Query::select() + .columns([Char::Character]) + .from(Char::Table) + .and_where(e) + .to_string(PostgresQueryBuilder), + r#"SELECT "character" FROM "character" WHERE TRUE AND (TRUE AND TRUE AND TRUE)"# + ); +} diff --git a/tests/sqlite/query.rs b/tests/sqlite/query.rs index a6300fb8c..d898010fe 100644 --- a/tests/sqlite/query.rs +++ b/tests/sqlite/query.rs @@ -142,7 +142,7 @@ fn select_10() { .and(Expr::col((Char::Table, Char::FontId)).equals((Font::Table, Font::Id))), ) .to_string(SqliteQueryBuilder), - r#"SELECT "character" FROM "character" LEFT JOIN "font" ON ("character"."font_id" = "font"."id") AND ("character"."font_id" = "font"."id")"# + r#"SELECT "character" FROM "character" LEFT JOIN "font" ON "character"."font_id" = "font"."id" AND "character"."font_id" = "font"."id""# ); } @@ -315,7 +315,7 @@ fn select_22() { ) ) .to_string(SqliteQueryBuilder), - r#"SELECT "character" FROM "character" WHERE ("character" LIKE 'C' OR (("character" LIKE 'D') AND ("character" LIKE 'E'))) AND (("character" LIKE 'F') OR ("character" LIKE 'G'))"# + r#"SELECT "character" FROM "character" WHERE ("character" LIKE 'C' OR ("character" LIKE 'D' AND "character" LIKE 'E')) AND ("character" LIKE 'F' OR "character" LIKE 'G')"# ); } @@ -484,8 +484,8 @@ fn select_34a() { .to_string(SqliteQueryBuilder), [ r#"SELECT "aspect", MAX("image") FROM "glyph" GROUP BY "aspect""#, - r#"HAVING (("aspect" > 2) OR ("aspect" < 8))"#, - r#"OR (("aspect" > 12) AND ("aspect" < 18))"#, + r#"HAVING "aspect" > 2 OR "aspect" < 8"#, + r#"OR ("aspect" > 12 AND "aspect" < 18)"#, r#"OR "aspect" > 32"#, ] .join(" ") @@ -530,10 +530,7 @@ fn select_37() { .cond_where(Cond::any().add(Cond::all()).add(Cond::any())) .build(SqliteQueryBuilder); - assert_eq!( - statement, - r#"SELECT "id" FROM "glyph" WHERE (TRUE) OR (FALSE)"# - ); + assert_eq!(statement, r#"SELECT "id" FROM "glyph" WHERE TRUE OR FALSE"#); assert_eq!(values.0, vec![]); } @@ -672,7 +669,7 @@ fn select_44() { assert_eq!( statement, - r#"SELECT "id" FROM "glyph" WHERE NOT ("aspect" < 8)"# + r#"SELECT "id" FROM "glyph" WHERE NOT "aspect" < 8"# ); } @@ -709,7 +706,7 @@ fn select_46() { assert_eq!( statement, - r#"SELECT "id" FROM "glyph" WHERE NOT ("aspect" < 8)"# + r#"SELECT "id" FROM "glyph" WHERE NOT "aspect" < 8"# ); } From e993f9ab2bf10de8577c6303b0694fdb22af1629 Mon Sep 17 00:00:00 2001 From: Chris Tsang Date: Fri, 25 Aug 2023 18:47:23 +0100 Subject: [PATCH 2/2] Fix double parens for CASE --- .../meta_list_renaming_everything.rs | 2 +- .../tests/pass-static/simple_unit_struct.rs | 2 +- src/backend/query_builder.rs | 3 ++- src/query/case.rs | 26 +++++++++++++++++++ 4 files changed, 30 insertions(+), 3 deletions(-) diff --git a/sea-query-derive/tests/pass-static/meta_list_renaming_everything.rs b/sea-query-derive/tests/pass-static/meta_list_renaming_everything.rs index 2266949c6..5f9667cc2 100644 --- a/sea-query-derive/tests/pass-static/meta_list_renaming_everything.rs +++ b/sea-query-derive/tests/pass-static/meta_list_renaming_everything.rs @@ -1,4 +1,4 @@ -use sea_query::{Iden, IdenStatic, Quote}; +use sea_query::{Iden, IdenStatic}; use strum::{EnumIter, IntoEnumIterator}; #[derive(IdenStatic, EnumIter, Copy, Clone)] diff --git a/sea-query-derive/tests/pass-static/simple_unit_struct.rs b/sea-query-derive/tests/pass-static/simple_unit_struct.rs index 48ecef9f3..35a73aec2 100644 --- a/sea-query-derive/tests/pass-static/simple_unit_struct.rs +++ b/sea-query-derive/tests/pass-static/simple_unit_struct.rs @@ -1,4 +1,4 @@ -use sea_query::{Iden, IdenStatic, Quote}; +use sea_query::{Iden, IdenStatic}; #[derive(Copy, Clone, IdenStatic)] pub struct SomeType; diff --git a/src/backend/query_builder.rs b/src/backend/query_builder.rs index bb0d06f05..fd8c7677d 100644 --- a/src/backend/query_builder.rs +++ b/src/backend/query_builder.rs @@ -1524,13 +1524,14 @@ pub(crate) fn common_inner_expr_well_known_greater_precedence( // unary or binary expression (with an outer_oper). // We do not need to wrap with parentheses: // Columns, tuples (already wrapped), constants, function calls, values, - // keywords, subqueries (already wrapped) + // keywords, subqueries (already wrapped), case (already wrapped) SimpleExpr::Column(_) | SimpleExpr::Tuple(_) | SimpleExpr::Constant(_) | SimpleExpr::FunctionCall(_) | SimpleExpr::Value(_) | SimpleExpr::Keyword(_) + | SimpleExpr::Case(_) | SimpleExpr::SubQuery(_, _) => true, SimpleExpr::Binary(_, inner_oper, _) => { let inner_oper: Oper = (*inner_oper).into(); diff --git a/src/query/case.rs b/src/query/case.rs index 22e7833d0..f1b3baf4c 100644 --- a/src/query/case.rs +++ b/src/query/case.rs @@ -132,3 +132,29 @@ impl Into for CaseStatement { SimpleExpr::Case(Box::new(self)) } } + +#[cfg(test)] +mod test { + use crate::*; + + #[test] + #[cfg(feature = "backend-postgres")] + fn test_where_case_eq() { + let case_statement: SimpleExpr = Expr::case( + Expr::col(Alias::new("col")).lt(5), + Expr::col(Alias::new("othercol")), + ) + .finally(Expr::col(Alias::new("finalcol"))) + .into(); + + let result = Query::select() + .column(Asterisk) + .from(Alias::new("tbl")) + .and_where(case_statement.eq(10)) + .to_string(PostgresQueryBuilder); + assert_eq!( + result, + r#"SELECT * FROM "tbl" WHERE (CASE WHEN ("col" < 5) THEN "othercol" ELSE "finalcol" END) = 10"# + ); + } +}