Skip to content

Commit

Permalink
Wrap all math operations in parens
Browse files Browse the repository at this point in the history
Previously if you used local variables or parens in Rust to override the
default order of operations, we would completely lose that information
in the generated SQL. This would at best result in an unexpected value,
but at worse would cause a query like `now + 2.minutes() * (some_int +
1)` to result in a runtime error complaining that there's no operator
for `interval + integer`

I intend to backport this to 1.4

Fixes #2017
  • Loading branch information
sgrif committed Mar 19, 2019
1 parent 57e17f2 commit d40d263
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 0 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
have been. All types in Diesel are now correctly only considered
non-aggregate if their parts are.

* Parenthesis are now inserted around all mathematical operations. This means
that `(2.into_sql() + 3) * 4` will correctly evaluate to 20 as expected.
Previously we would generate SQL that evaluated to 14. This could even result
in runtime errors if multiple types were involved (for example, `interval *
(integer + 1)`)

### Deprecated

* `diesel_(prefix|postfix|infix)_operator!` have been deprecated. These macros
Expand Down
2 changes: 2 additions & 0 deletions diesel/src/expression/ops/numeric.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,11 @@ macro_rules! numeric_operation {
Rhs: QueryFragment<DB>,
{
fn walk_ast(&self, mut out: AstPass<DB>) -> QueryResult<()> {
out.push_sql("(");
self.lhs.walk_ast(out.reborrow())?;
out.push_sql($op);
self.rhs.walk_ast(out.reborrow())?;
out.push_sql(")");
Ok(())
}
}
Expand Down
10 changes: 10 additions & 0 deletions diesel_tests/tests/expressions/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ fn test_dividing_nullables() {
let data = nullable_table.select(value / value).load(&connection);
assert_eq!(Ok(expected_data), data);
}

#[test]
fn mix_and_match_all_numeric_ops() {
use schema::users::dsl::*;
Expand All @@ -190,3 +191,12 @@ fn mix_and_match_all_numeric_ops() {
let data = users.select(id * 3 / 2 + 4 - 1).load(&connection);
assert_eq!(Ok(expected_data), data);
}

#[test]
fn precedence_with_parens_is_maintained() {
use diesel::sql_types::Integer;

let x = select((2.into_sql::<Integer>() + 3) * 4)
.get_result::<i32>(&connection());
assert_eq!(Ok(20), x);
}

0 comments on commit d40d263

Please sign in to comment.