Repository: diesel-rs/diesel (language: rust)

Issue:
Invalid SQL when using union on a query that uses order_by or limit
<!--
If you want to report a bug, we added some points below which help us track down the problem faster.
-->

## Setup

### Versions

- **Rust:** 1.60.0
- **Diesel:** 2.0.0-rc.0
- **Database:** postgresql 13.6, mysql 8.0.28, sqlite 3.35.5
- **Operating System:** Ubuntu 21.10

### Feature Flags

- **diesel:** postgres, mysql, sqlite

## Problem Description
Diesel emits invalid SQL when using `union` on a query that uses `order_by` or `limit` 

### What are you trying to accomplish?
```rust
let users = users::table
        .order_by(users::id.asc())
        .limit(1)
        .union(users::table.order_by(users::id.desc()).limit(1))
        .load::<User>(conn)?;
```

### What is the expected output?
Up to 2 entries, with both the lowest id in table and the largest one

### What is the actual output?
 - Postgres 
 > DatabaseError(Unknown, "syntax error at or near \"UNION\"")

 - Mysql: 
 > DatabaseError(Unknown, "You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'UNION (SELECT `users`.`id`, `users`.`email` FROM `users` ORDER BY `users`.`id` D' at line 1")

 - Sqlite: 
 > DatabaseError(Unknown, "ORDER BY clause should come after UNION not before")


### Are you seeing any additional errors?
No

### Steps to reproduce

[Reproduction repository](https://github.com/gobanos/diesel-union-bug)

<!--
Please include as much of your codebase as needed to reproduce the error.  If the relevant files are large, please consider linking to a public repository or a [Gist](https://gist.github.com/). This includes normally the following parts:

* The exact code where your hit the problem
* Relevant parts your schema, so any `table!` macro calls required for the
* Any other type definitions involved in the code, which produces your problem
-->

## Checklist

- [X] I have already looked over the [issue tracker](https://github.com/diesel-rs/diesel/issues)  and the [disussion forum](https://github.com/diesel-rs/diesel/discussions)  for similar possible closed issues.
<!--
If you are unsure if your issue is a duplicate of an existing issue please link the issue in question here
--> 
- [X] This issue can be reproduced on Rust's stable channel. (Your issue will be
  closed if this is not the case)
- [X] This issue can be reproduced without requiring a third party crate

<!--
Thank you for your submission!  You're helping make Diesel more robust 🎉

We'll try to respond as quickly as possible.
-->



Discussion / hints:
There is an "easy" fix for both Postgres and Mysql : wrap the source query with parenthesis.
Sadly this does not work for Sqlite, where parenthesis are forbidden around queries.

Fix patch:
diff --git a/diesel/src/query_builder/combination_clause.rs b/diesel/src/query_builder/combination_clause.rs
index 5435bd395e37..23a7002e0a6c 100644
--- a/diesel/src/query_builder/combination_clause.rs
+++ b/diesel/src/query_builder/combination_clause.rs
@@ -26,8 +26,8 @@ where
 pub struct CombinationClause<Combinator, Rule, Source, Rhs> {
     combinator: Combinator,
     duplicate_rule: Rule,
-    source: Source,
-    rhs: RhsParenthesisWrapper<Rhs>,
+    source: ParenthesisWrapper<Source>,
+    rhs: ParenthesisWrapper<Rhs>,
 }
 
 impl<Combinator, Rule, Source, Rhs> CombinationClause<Combinator, Rule, Source, Rhs> {
@@ -41,8 +41,8 @@ impl<Combinator, Rule, Source, Rhs> CombinationClause<Combinator, Rule, Source,
         CombinationClause {
             combinator,
             duplicate_rule,
-            source,
-            rhs: RhsParenthesisWrapper(rhs),
+            source: ParenthesisWrapper(source),
+            rhs: ParenthesisWrapper(rhs),
         }
     }
 }
@@ -129,8 +129,8 @@ impl<Combinator, Rule, Source, Rhs, DB: Backend> QueryFragment<DB>
 where
     Combinator: QueryFragment<DB>,
     Rule: QueryFragment<DB>,
-    Source: QueryFragment<DB>,
-    RhsParenthesisWrapper<Rhs>: QueryFragment<DB>,
+    ParenthesisWrapper<Source>: QueryFragment<DB>,
+    ParenthesisWrapper<Rhs>: QueryFragment<DB>,
     DB: Backend + SupportsCombinationClause<Combinator, Rule> + DieselReserveSpecialization,
 {
     fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, DB>) -> QueryResult<()> {
@@ -215,7 +215,7 @@ pub trait SupportsCombinationClause<Combinator, Rule> {}
 
 #[derive(Debug, Copy, Clone, QueryId)]
 /// Wrapper used to wrap rhs sql in parenthesis when supported by backend
-pub struct RhsParenthesisWrapper<T>(T);
+pub struct ParenthesisWrapper<T>(T);
 
 #[cfg(feature = "postgres")]
 mod postgres {
@@ -224,7 +224,7 @@ mod postgres {
     use crate::query_builder::{AstPass, QueryFragment};
     use crate::QueryResult;
 
-    impl<T: QueryFragment<Pg>> QueryFragment<Pg> for RhsParenthesisWrapper<T> {
+    impl<T: QueryFragment<Pg>> QueryFragment<Pg> for ParenthesisWrapper<T> {
         fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, Pg>) -> QueryResult<()> {
             out.push_sql("(");
             self.0.walk_ast(out.reborrow())?;
@@ -248,7 +248,7 @@ mod mysql {
     use crate::query_builder::{AstPass, QueryFragment};
     use crate::QueryResult;
 
-    impl<T: QueryFragment<Mysql>> QueryFragment<Mysql> for RhsParenthesisWrapper<T> {
+    impl<T: QueryFragment<Mysql>> QueryFragment<Mysql> for ParenthesisWrapper<T> {
         fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, Mysql>) -> QueryResult<()> {
             out.push_sql("(");
             self.0.walk_ast(out.reborrow())?;
@@ -268,9 +268,15 @@ mod sqlite {
     use crate::sqlite::Sqlite;
     use crate::QueryResult;
 
-    impl<T: QueryFragment<Sqlite>> QueryFragment<Sqlite> for RhsParenthesisWrapper<T> {
-        fn walk_ast<'b>(&'b self, out: AstPass<'_, 'b, Sqlite>) -> QueryResult<()> {
-            self.0.walk_ast(out) // SQLite does not support parenthesis around Ths
+    impl<T: QueryFragment<Sqlite>> QueryFragment<Sqlite> for ParenthesisWrapper<T> {
+        fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, Sqlite>) -> QueryResult<()> {
+            // SQLite does not support parenthesis around Ths
+            // we can emulate this by construct a fake outer
+            // SELECT * FROM (inner_query) statement
+            out.push_sql("SELECT * FROM (");
+            self.0.walk_ast(out.reborrow())?;
+            out.push_sql(")");
+            Ok(())
         }
     }
 


Test patch:
diff --git a/diesel_tests/tests/combination.rs b/diesel_tests/tests/combination.rs
index 04893e26535b..262fcde7ebb5 100644
--- a/diesel_tests/tests/combination.rs
+++ b/diesel_tests/tests/combination.rs
@@ -116,3 +116,33 @@ fn except() {
         .unwrap();
     assert_eq!(expected_data, data);
 }
+
+#[test]
+fn union_with_order() {
+    let conn = &mut connection();
+    let data = vec![
+        NewUser::new("Sean", None),
+        NewUser::new("Tess", None),
+        NewUser::new("Jim", None),
+    ];
+    insert_into(users::table)
+        .values(&data)
+        .execute(conn)
+        .unwrap();
+
+    let users = users::table
+        .select(users::name)
+        .order_by(users::id.asc())
+        .limit(1)
+        .union(
+            users::table
+                .order_by(users::id.desc())
+                .select(users::name)
+                .limit(1),
+        )
+        .positional_order_by(1)
+        .load::<String>(conn)
+        .unwrap();
+
+    assert_eq!(vec![String::from("Jim"), "Sean".into()], users);
+}