Missing parenthesis lead to broken SQL queries #2140
Comments
|
To be clear Beside of that I see two possible solutions to fix this:
As you've already noticed it is not as simple as adding just parenthesis everywhere the first solution would involve a quite heavy rework of our internal query constructing workflow. Additionally even than there may be expressions where it is just not decidable which variant is the right one. In that light I'm in favour of keeping the rules as simple as possible and just add parenthesis in as few cases as possible and expose the functionality to insert parenthesis manually. |
Yes, I understand that.
Could you please give an example? From what I can see, if we just wrap all binary operations in parenthesis, the resulting SQL AST will be exactly the same as the original Rust AST (which is what library user want, as I see it). Considering the example of And, assuming we put parenthesis around every binary operation, SQL syntax tree is exactly the same. It will result in As to using It may sound stupid or maybe overly naïve, but what if we change the public api of update(posts).set(body, "updated");
update(users).set(login_attempts, login_attempts + 1);
// or
update(posts).set(body.assign("updated"));This way we could freely add parenthesis around all binary operators and be sure that Rust and SQL expressions have the same meaning. |
What about the following, pathological example? true.into_sql::<Bool>.eq(false).eq(false)That would generate the following query
Changing the public API of |
Well I think syntactically this is the tree below, no ambiguity here. So if we properly wrap every expression in brackets, it will render as If one wants
Makes total sense. Speaking of keeping backward compatibility. I was also playing with the idea of changing the way We can use it on one single place then, like this: diff --git a/diesel/src/query_builder/update_statement/mod.rs b/diesel/src/query_builder/update_statement/mod.rs
index 53ec7870..faeb7c70 100644
--- a/diesel/src/query_builder/update_statement/mod.rs
+++ b/diesel/src/query_builder/update_statement/mod.rs
@@ -207,7 +207,7 @@ where
out.push_sql("UPDATE ");
self.table.from_clause().walk_ast(out.reborrow())?;
out.push_sql(" SET ");
- self.values.walk_ast(out.reborrow())?;
+ Ungrouped(self.values).walk_ast(out.reborrow())?;
self.where_clause.walk_ast(out.reborrow())?;
self.returning.walk_ast(out.reborrow())?;
Ok(())Hmmm, actually, looking at the code, I'm not even sure anymore that So, bottom line is: I still believe that the right way to go is to wrap all operator expressions in parenthesis there diesel/diesel/src/expression/operators.rs Lines 109 to 111 in b649a86 When I have more time, I'll play with the code. I want to see what is going to be broken by this change as I don't really see it now. |
|
I just made a little test and ran Almost everything passed with one exception: |
|
First of all: Passing all tests in Additionally I would like to hear the opinion of the other @diesel-rs/core members here. |
Include parenthesis around every infix operation. This requires making `BETWEEN` and `NOT BETWEEN` manually implemented query nodes instead of relaying on that ones defined by `infix_operator!` because `x BETWEEN (l AND u)` is invalid SQL.
Include parenthesis around every infix operation provided by one of the
`ExpressionMethod` traits.
It is not possible to include this directly into the `infix_operator!`
macro due several SQL constructs:
* `a LIKE b` can have optional `ESCAPE` clauses (same for `NOT LIKE`,
`ILIKE` eg). Unconditionally putting a parenthesis around `(a LIKE b)`
is no valid SQL
* `a BETWEEN l AND u` is implemented a set of two infix operators,
`BETWEEN` and `AND`, so putting the parenthesis directly into the
operator macro would lead to `x BETWEEN (l AND u)` which is not valid
SQL
Instead I've opted for adding explicit parenthesis in the corresponding
`ExpressionMethod` (and similar traits` implementations. As part of this
change I've changed all the return types there to use some type from
`diesel::dsl`, so that potential users have an easier time to figure out
what types to use if they have to write some where clause or something
like that.
While changing the return types to `diesel::dsl` types I noticed and
fixed the following issues:
* Not all methods had an equivalent type in `diesel::dsl`, sometimes
because it was missing, sometimes because the postgres specific types
weren't correctly exported there. Both is fixed now
* Some types were not anymore compatible with the actual methods. A
notable example is `dsl::And` and `BoolExpressionMethods::and` where the
later returned a potentially nullable type depending on the input types,
while the first was only valid for not nullable right sides. Fixing that
was a bit more complicated:
* I've opted for making the return type of `BoolExpressionMethods::and`
(and `or`) unconditonally nullable. This prevents having an explicit
`ST` parameter on `dsl::And`
* This in turn requires that we accept a non nullable expression in
places where a nullable expression is expected. Technically this
was already possible, but would require explicitly calling
`.nullable()` on affected expressions. Given that `.and()` and
`.or()` are really common, that seemed not to be a good solution.
As result I've changed the `AsExpression` impl for `T: Expression`
in such a way that for any `T` with `T::SqlType: SqlType<IsNull =
is_nullable::NotNull>` there is now an impl
`AsExpression<Nullable<T::SqlType>>` so that such expressions are
also accepted in nullable contexts.
Include parenthesis around every infix operation provided by one of the
`ExpressionMethod` traits.
It is not possible to include this directly into the `infix_operator!`
macro due several SQL constructs:
* `a LIKE b` can have optional `ESCAPE` clauses (same for `NOT LIKE`,
`ILIKE` eg). Unconditionally putting a parenthesis around `(a LIKE b)`
is no valid SQL
* `a BETWEEN l AND u` is implemented a set of two infix operators,
`BETWEEN` and `AND`, so putting the parenthesis directly into the
operator macro would lead to `x BETWEEN (l AND u)` which is not valid
SQL
Instead I've opted for adding explicit parenthesis in the corresponding
`ExpressionMethod` (and similar traits` implementations. As part of this
change I've changed all the return types there to use some type from
`diesel::dsl`, so that potential users have an easier time to figure out
what types to use if they have to write some where clause or something
like that.
While changing the return types to `diesel::dsl` types I noticed and
fixed the following issues:
* Not all methods had an equivalent type in `diesel::dsl`, sometimes
because it was missing, sometimes because the postgres specific types
weren't correctly exported there. Both is fixed now
* Some types were not anymore compatible with the actual methods. A
notable example is `dsl::And` and `BoolExpressionMethods::and` where the
later returned a potentially nullable type depending on the input types,
while the first was only valid for not nullable right sides. Fixing that
was a bit more complicated:
* I've opted for making the return type of `BoolExpressionMethods::and`
(and `or`) unconditonally nullable. This prevents having an explicit
`ST` parameter on `dsl::And`
* This in turn requires that we accept a non nullable expression in
places where a nullable expression is expected. Technically this
was already possible, but would require explicitly calling
`.nullable()` on affected expressions. Given that `.and()` and
`.or()` are really common, that seemed not to be a good solution.
As result I've changed the `AsExpression` impl for `T: Expression`
in such a way that for any `T` with `T::SqlType: SqlType<IsNull =
is_nullable::NotNull>` there is now an impl
`AsExpression<Nullable<T::SqlType>>` so that such expressions are
also accepted in nullable contexts.
Include parenthesis around every infix operation provided by one of the
`ExpressionMethod` traits.
It is not possible to include this directly into the `infix_operator!`
macro due several SQL constructs:
* `a LIKE b` can have optional `ESCAPE` clauses (same for `NOT LIKE`,
`ILIKE` eg). Unconditionally putting a parenthesis around `(a LIKE b)`
is no valid SQL
* `a BETWEEN l AND u` is implemented a set of two infix operators,
`BETWEEN` and `AND`, so putting the parenthesis directly into the
operator macro would lead to `x BETWEEN (l AND u)` which is not valid
SQL
Instead I've opted for adding explicit parenthesis in the corresponding
`ExpressionMethod` (and similar traits` implementations. As part of this
change I've changed all the return types there to use some type from
`diesel::dsl`, so that potential users have an easier time to figure out
what types to use if they have to write some where clause or something
like that.
While changing the return types to `diesel::dsl` types I noticed and
fixed the following issues:
* Not all methods had an equivalent type in `diesel::dsl`, sometimes
because it was missing, sometimes because the postgres specific types
weren't correctly exported there. Both is fixed now
* Some types were not anymore compatible with the actual methods. A
notable example is `dsl::And` and `BoolExpressionMethods::and` where the
later returned a potentially nullable type depending on the input types,
while the first was only valid for not nullable right sides. Fixing that
was a bit more complicated:
* I've opted for making the return type of `BoolExpressionMethods::and`
(and `or`) unconditonally nullable. This prevents having an explicit
`ST` parameter on `dsl::And`
* This in turn requires that we accept a non nullable expression in
places where a nullable expression is expected. Technically this
was already possible, but would require explicitly calling
`.nullable()` on affected expressions. Given that `.and()` and
`.or()` are really common, that seemed not to be a good solution.
As result I've changed the `AsExpression` impl for `T: Expression`
in such a way that for any `T` with `T::SqlType: SqlType<IsNull =
is_nullable::NotNull>` there is now an impl
`AsExpression<Nullable<T::SqlType>>` so that such expressions are
also accepted in nullable contexts.
Include parenthesis around every infix operation provided by one of the
`ExpressionMethod` traits.
It is not possible to include this directly into the `infix_operator!`
macro due several SQL constructs:
* `a LIKE b` can have optional `ESCAPE` clauses (same for `NOT LIKE`,
`ILIKE` eg). Unconditionally putting a parenthesis around `(a LIKE b)`
is no valid SQL
* `a BETWEEN l AND u` is implemented a set of two infix operators,
`BETWEEN` and `AND`, so putting the parenthesis directly into the
operator macro would lead to `x BETWEEN (l AND u)` which is not valid
SQL
Instead I've opted for adding explicit parenthesis in the corresponding
`ExpressionMethod` (and similar traits` implementations. As part of this
change I've changed all the return types there to use some type from
`diesel::dsl`, so that potential users have an easier time to figure out
what types to use if they have to write some where clause or something
like that.
While changing the return types to `diesel::dsl` types I noticed and
fixed the following issues:
* Not all methods had an equivalent type in `diesel::dsl`, sometimes
because it was missing, sometimes because the postgres specific types
weren't correctly exported there. Both is fixed now
* Some types were not anymore compatible with the actual methods. A
notable example is `dsl::And` and `BoolExpressionMethods::and` where the
later returned a potentially nullable type depending on the input types,
while the first was only valid for not nullable right sides. Fixing that
was a bit more complicated:
* I've opted for making the return type of `BoolExpressionMethods::and`
(and `or`) unconditonally nullable. This prevents having an explicit
`ST` parameter on `dsl::And`
* This in turn requires that we accept a non nullable expression in
places where a nullable expression is expected. Technically this
was already possible, but would require explicitly calling
`.nullable()` on affected expressions. Given that `.and()` and
`.or()` are really common, that seemed not to be a good solution.
As result I've changed the `AsExpression` impl for `T: Expression`
in such a way that for any `T` with `T::SqlType: SqlType<IsNull =
is_nullable::NotNull>` there is now an impl
`AsExpression<Nullable<T::SqlType>>` so that such expressions are
also accepted in nullable contexts.
Include parenthesis around every infix operation provided by one of the
`ExpressionMethod` traits.
It is not possible to include this directly into the `infix_operator!`
macro due several SQL constructs:
* `a LIKE b` can have optional `ESCAPE` clauses (same for `NOT LIKE`,
`ILIKE` eg). Unconditionally putting a parenthesis around `(a LIKE b)`
is no valid SQL
* `a BETWEEN l AND u` is implemented a set of two infix operators,
`BETWEEN` and `AND`, so putting the parenthesis directly into the
operator macro would lead to `x BETWEEN (l AND u)` which is not valid
SQL
Instead I've opted for adding explicit parenthesis in the corresponding
`ExpressionMethod` (and similar traits` implementations. As part of this
change I've changed all the return types there to use some type from
`diesel::dsl`, so that potential users have an easier time to figure out
what types to use if they have to write some where clause or something
like that.
While changing the return types to `diesel::dsl` types I noticed and
fixed the following issues:
* Not all methods had an equivalent type in `diesel::dsl`, sometimes
because it was missing, sometimes because the postgres specific types
weren't correctly exported there. Both is fixed now
* Some types were not anymore compatible with the actual methods. A
notable example is `dsl::And` and `BoolExpressionMethods::and` where the
later returned a potentially nullable type depending on the input types,
while the first was only valid for not nullable right sides. Fixing that
was a bit more complicated:
* I've opted for making the return type of `BoolExpressionMethods::and`
(and `or`) unconditonally nullable. This prevents having an explicit
`ST` parameter on `dsl::And`
* This in turn requires that we accept a non nullable expression in
places where a nullable expression is expected. Technically this
was already possible, but would require explicitly calling
`.nullable()` on affected expressions. Given that `.and()` and
`.or()` are really common, that seemed not to be a good solution.
As result I've changed the `AsExpression` impl for `T: Expression`
in such a way that for any `T` with `T::SqlType: SqlType<IsNull =
is_nullable::NotNull>` there is now an impl
`AsExpression<Nullable<T::SqlType>>` so that such expressions are
also accepted in nullable contexts.
Include parenthesis around every infix operation provided by one of the
`ExpressionMethod` traits.
It is not possible to include this directly into the `infix_operator!`
macro due several SQL constructs:
* `a LIKE b` can have optional `ESCAPE` clauses (same for `NOT LIKE`,
`ILIKE` eg). Unconditionally putting a parenthesis around `(a LIKE b)`
is no valid SQL
* `a BETWEEN l AND u` is implemented a set of two infix operators,
`BETWEEN` and `AND`, so putting the parenthesis directly into the
operator macro would lead to `x BETWEEN (l AND u)` which is not valid
SQL
Instead I've opted for adding explicit parenthesis in the corresponding
`ExpressionMethod` (and similar traits` implementations. As part of this
change I've changed all the return types there to use some type from
`diesel::dsl`, so that potential users have an easier time to figure out
what types to use if they have to write some where clause or something
like that.
While changing the return types to `diesel::dsl` types I noticed and
fixed the following issues:
* Not all methods had an equivalent type in `diesel::dsl`, sometimes
because it was missing, sometimes because the postgres specific types
weren't correctly exported there. Both is fixed now
* Some types were not anymore compatible with the actual methods. A
notable example is `dsl::And` and `BoolExpressionMethods::and` where the
later returned a potentially nullable type depending on the input types,
while the first was only valid for not nullable right sides. Fixing that
was a bit more complicated:
* I've opted for making the return type of `BoolExpressionMethods::and`
(and `or`) unconditonally nullable. This prevents having an explicit
`ST` parameter on `dsl::And`
* This in turn requires that we accept a non nullable expression in
places where a nullable expression is expected. Technically this
was already possible, but would require explicitly calling
`.nullable()` on affected expressions. Given that `.and()` and
`.or()` are really common, that seemed not to be a good solution.
As result I've changed the `AsExpression` impl for `T: Expression`
in such a way that for any `T` with `T::SqlType: SqlType<IsNull =
is_nullable::NotNull>` there is now an impl
`AsExpression<Nullable<T::SqlType>>` so that such expressions are
also accepted in nullable contexts.
Setup
Versions
Feature Flags
Problem Description
Missing parentheses around equality operator lead to malformed SQL queries in the best case and wrong SQL queries silently returning incorrect results in the worst case.
What are you trying to accomplish?
Build a query which corresponds to something like this:
Translating this to diesel, the code will look like this:
Full runnable test can be found in my
operator-precedence-bugbranch of diesel. Here is the full test DNNX/diesel@master...operator-precedence-bug which can be run with(cd diesel_tests && cargo test --features "postgres" --no-default-features expressions::test_operator_precedence).What is the expected output?
Green test.
What is the actual output?
Are you seeing any additional errors?
No.
Steps to reproduce
Additional notes
I already raised this issue in #2133 (comment), but I probably failed to express myself very clearly.
The big problem with this bug is that it may result in incorrectly working SQL queries, in particular in Mysql. Mysql is less strict about queries, so it parses things like
select 2=2=2=2just fine, whereas Postgres rejects such queries.I tried to fix the bug myself, but I don't even know where to start. I think adding parentheses around every operator might be a solution, but there's one subtle issue with it: diesel also uses
eqto generateSETpart of theUPDATEstatements - and there having parentheses aroundx = <expr>inUPDATE foo SET x = <expr>will be a problem.Checklist
closed if this is not the case)
The text was updated successfully, but these errors were encountered: