From 401161c236a102fc29cb207b131cfd1762e498fc Mon Sep 17 00:00:00 2001 From: Chris Tsang Date: Fri, 25 Aug 2023 19:09:38 +0100 Subject: [PATCH] Backport #675 --- .../meta_list_renaming_everything.rs | 2 +- .../tests/pass-static/simple_unit_struct.rs | 2 +- src/backend/mod.rs | 109 ++++++++++++++++ src/backend/mysql/mod.rs | 16 +++ src/backend/postgres/query.rs | 51 ++++++++ src/backend/query_builder.rs | 122 ++++++++++++++++-- src/backend/sqlite/mod.rs | 16 +++ src/expr.rs | 23 +--- src/lib.rs | 2 +- src/query/case.rs | 26 ++++ tests/mysql/query.rs | 8 +- tests/postgres/query.rs | 82 +++++++++++- tests/sqlite/query.rs | 8 +- 13 files changed, 421 insertions(+), 46 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/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..e1dc8f436 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); @@ -1378,11 +1388,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 +1405,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 +1524,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 +1559,50 @@ 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), 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(); + 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..ef7eb5cb1 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -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,17 +2488,6 @@ 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(_, _, _)) } 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/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"# + ); + } +} diff --git a/tests/mysql/query.rs b/tests/mysql/query.rs index fd656678b..ac6914f13 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(" ") diff --git a/tests/postgres/query.rs b/tests/postgres/query.rs index a99311142..0e73f84a4 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(" ") @@ -1912,3 +1912,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..6271767fb 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(" ")