Merge pull request #3486 from weiznich/backport_fixes

Backport fixes
diesel-rs · Jan 24, 2023 · eedc141 · eedc141
2 parents a02fcfb + c11a7c9
commit eedc141
Show file tree

Hide file tree

Showing 15 changed files with 468 additions and 142 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -51,13 +51,6 @@ jobs:
           echo "RUSTFLAGS=--cfg doc_cfg" >> $GITHUB_ENV
           echo "RUSTDOCFLAGS=--cfg doc_cfg" >> $GITHUB_ENV
 
-      - name: Set environment variables
-        shell: bash
-        if: matrix.rust != 'nightly'
-        run: |
-          echo "RUSTFLAGS=-D warnings" >> $GITHUB_ENV
-          echo "RUSTDOCFLAGS=-D warnings" >> $GITHUB_ENV
-
       - name: Install postgres (Linux)
         if: runner.os == 'Linux' && matrix.backend == 'postgres'
         run: |
@@ -130,14 +123,12 @@ jobs:
       - name: Install sqlite (MacOS)
         if: runner.os == 'macOS' && matrix.backend == 'sqlite'
         run: |
-          brew update
           brew install sqlite
           echo "SQLITE_DATABASE_URL=/tmp/test.db" >> $GITHUB_ENV
 
       - name: Install mysql (MacOS)
         if: runner.os == 'macOS' && matrix.backend == 'mysql'
         run: |
-          brew update
           brew install mariadb@10.5
           /usr/local/opt/mariadb@10.5/bin/mysql_install_db
           /usr/local/opt/mariadb@10.5/bin/mysql.server start
@@ -333,11 +324,6 @@ jobs:
         run: |
           sudo apt-get update
           sudo apt-get -y install libsqlite3-dev libpq-dev libmysqlclient-dev
-      - name: Set environment variables
-        shell: bash
-        run: |
-          echo "RUSTFLAGS=-D warnings" >> $GITHUB_ENV
-          echo "RUSTDOCFLAGS=-D warnings" >> $GITHUB_ENV
 
       - name: Remove potential newer clippy.toml from dependencies
         run: |

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,13 @@ Increasing the minimal supported Rust version will always be coupled at least wi
 
 ## Unreleased 
 
+## [2.0.3] 2023-01-24
+
+## Fixed
+
+* Fixed a bug with our transaction manager implementation that caused by marking transactions as broken which could be recovered.
+* Fixed an issue with the combination of `BoxableExpression` and order clauses
+
 ## [2.0.2] 2022-10-11
 
 ### Fixed

diff --git a/diesel/Cargo.toml b/diesel/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "diesel"
-version = "2.0.2"
+version = "2.0.3"
 license = "MIT OR Apache-2.0"
 description = "A safe, extensible ORM and Query Builder for PostgreSQL, SQLite, and MySQL"
 readme = "README.md"

diff --git a/diesel/src/connection/transaction_manager.rs b/diesel/src/connection/transaction_manager.rs
diff --git a/diesel/src/mysql/connection/mod.rs b/diesel/src/mysql/connection/mod.rs
@@ -171,7 +171,7 @@ fn update_transaction_manager_status<T>(
     if let Err(Error::DatabaseError(DatabaseErrorKind::SerializationFailure, _)) = query_result {
         transaction_manager
             .status
-            .set_top_level_transaction_requires_rollback()
+            .set_requires_rollback_maybe_up_to_top_level(true)
     }
     query_result
 }

diff --git a/diesel/src/pg/connection/mod.rs b/diesel/src/pg/connection/mod.rs
@@ -239,37 +239,54 @@ fn update_transaction_manager_status<T>(
     query_result: QueryResult<T>,
     conn: &mut ConnectionAndTransactionManager,
 ) -> QueryResult<T> {
-    if let Err(Error::DatabaseError { .. }) = query_result {
-        /// avoid monomorphizing for every result type - this part will not be inlined
-        fn non_generic_inner(conn: &mut ConnectionAndTransactionManager) {
-            let raw_conn: &mut RawConnection = &mut conn.raw_connection;
-            let tm: &mut AnsiTransactionManager = &mut conn.transaction_state;
-            if tm.status.is_not_broken_and_in_transaction() {
-                // libpq keeps track of the transaction status internally, and that is accessible
-                // via `transaction_status`. We can use that to update the AnsiTransactionManager
-                // status
-                match raw_conn.transaction_status() {
-                    PgTransactionStatus::InError => {
-                        tm.status.set_top_level_transaction_requires_rollback()
-                    }
-                    PgTransactionStatus::Unknown => tm.status.set_in_error(),
-                    PgTransactionStatus::Idle => {
-                        // This may repair the transaction manager
-                        tm.status = TransactionManagerStatus::Valid(Default::default())
-                    }
-                    PgTransactionStatus::InTransaction => {
-                        let transaction_status = &mut tm.status;
-                        if !matches!(transaction_status, TransactionManagerStatus::Valid(valid_tm) if valid_tm.transaction_depth().is_some())
-                        {
-                            transaction_status.set_in_error()
-                        }
+    /// avoid monomorphizing for every result type - this part will not be inlined
+    fn non_generic_inner(conn: &mut ConnectionAndTransactionManager, is_err: bool) {
+        let raw_conn: &mut RawConnection = &mut conn.raw_connection;
+        let tm: &mut AnsiTransactionManager = &mut conn.transaction_state;
+        // libpq keeps track of the transaction status internally, and that is accessible
+        // via `transaction_status`. We can use that to update the AnsiTransactionManager
+        // status
+        match raw_conn.transaction_status() {
+            PgTransactionStatus::InError => {
+                tm.status.set_requires_rollback_maybe_up_to_top_level(true)
+            }
+            PgTransactionStatus::Unknown => tm.status.set_in_error(),
+            PgTransactionStatus::Idle => {
+                // This is useful in particular for commit attempts (even
+                // if `COMMIT` errors it still exits transaction)
+
+                // This may repair the transaction manager
+                tm.status = TransactionManagerStatus::Valid(Default::default())
+            }
+            PgTransactionStatus::InTransaction => {
+                let transaction_status = &mut tm.status;
+                // If we weren't an error, it is possible that we were a transaction start
+                // -> we should tolerate any state
+                if is_err {
+                    // An error may not have us enter a transaction, so if we weren't in one
+                    // we may not be in one now
+                    if !matches!(transaction_status, TransactionManagerStatus::Valid(valid_tm) if valid_tm.transaction_depth().is_some())
+                    {
+                        // -> transaction manager is broken
+                        transaction_status.set_in_error()
                     }
-                    PgTransactionStatus::Active => {}
+                } else {
+                    // If transaction was InError before, but now it's not (because we attempted
+                    // a rollback), we may pretend it's fixed because
+                    // if it isn't Postgres *will* tell us again.
+
+                    // Fun fact: if we have not received an `InTransaction` status however,
+                    // postgres will *not* warn us that transaction is broken when attempting to
+                    // commit, so we may think that commit has succeeded but in fact it hasn't.
+                    tm.status.set_requires_rollback_maybe_up_to_top_level(false)
                 }
             }
+            PgTransactionStatus::Active => {
+                // This is a transient state for libpq - nothing we can deduce here.
+            }
         }
-        non_generic_inner(conn)
     }
+    non_generic_inner(conn, query_result.is_err());
     query_result
 }
 

diff --git a/diesel/src/pg/connection/raw.rs b/diesel/src/pg/connection/raw.rs
@@ -108,6 +108,8 @@ impl RawConnection {
         RawResult::new(ptr, self)
     }
 
+    /// This is reasonably inexpensive as it just accesses variables internal to the connection
+    /// that are kept up to date by the `ReadyForQuery` messages from the PG server
     pub(super) fn transaction_status(&self) -> PgTransactionStatus {
         unsafe { PQtransactionStatus(self.internal_connection.as_ptr()) }.into()
     }

diff --git a/diesel/src/query_builder/select_statement/boxed.rs b/diesel/src/query_builder/select_statement/boxed.rs
@@ -389,9 +389,12 @@ where
     }
 }
 
-impl<'a, ST, QS, DB, Order, GB> OrderDsl<Order> for BoxedSelectStatement<'a, ST, QS, DB, GB>
+// no impls for `NoFromClause` here because order is not really supported there yet
+impl<'a, ST, QS, DB, Order, GB> OrderDsl<Order>
+    for BoxedSelectStatement<'a, ST, FromClause<QS>, DB, GB>
 where
     DB: Backend,
+    QS: QuerySource,
     Order: QueryFragment<DB> + AppearsOnTable<QS> + Send + 'a,
 {
     type Output = Self;
@@ -402,9 +405,11 @@ where
     }
 }
 
-impl<'a, ST, QS, DB, Order, GB> ThenOrderDsl<Order> for BoxedSelectStatement<'a, ST, QS, DB, GB>
+impl<'a, ST, QS, DB, Order, GB> ThenOrderDsl<Order>
+    for BoxedSelectStatement<'a, ST, FromClause<QS>, DB, GB>
 where
     DB: Backend + 'a,
+    QS: QuerySource,
     Order: QueryFragment<DB> + AppearsOnTable<QS> + Send + 'a,
 {
     type Output = Self;

diff --git a/diesel/src/query_builder/select_statement/dsl_impls.rs b/diesel/src/query_builder/select_statement/dsl_impls.rs
@@ -261,15 +261,18 @@ where
     }
 }
 
+// no impls for `NoFromClause` here because order is not really supported there yet
 impl<ST, F, S, D, W, O, LOf, G, H, LC, Expr> OrderDsl<Expr>
-    for SelectStatement<F, S, D, W, O, LOf, G, H, LC>
+    for SelectStatement<FromClause<F>, S, D, W, O, LOf, G, H, LC>
 where
+    F: QuerySource,
     Expr: AppearsOnTable<F>,
     Self: SelectQuery<SqlType = ST>,
-    SelectStatement<F, S, D, W, OrderClause<Expr>, LOf, G, H, LC>: SelectQuery<SqlType = ST>,
+    SelectStatement<FromClause<F>, S, D, W, OrderClause<Expr>, LOf, G, H, LC>:
+        SelectQuery<SqlType = ST>,
     OrderClause<Expr>: ValidOrderingForDistinct<D>,
 {
-    type Output = SelectStatement<F, S, D, W, OrderClause<Expr>, LOf, G, H, LC>;
+    type Output = SelectStatement<FromClause<F>, S, D, W, OrderClause<Expr>, LOf, G, H, LC>;
 
     fn order(self, expr: Expr) -> Self::Output {
         let order = OrderClause(expr);
@@ -288,11 +291,12 @@ where
 }
 
 impl<F, S, D, W, O, LOf, G, H, LC, Expr> ThenOrderDsl<Expr>
-    for SelectStatement<F, S, D, W, OrderClause<O>, LOf, G, H, LC>
+    for SelectStatement<FromClause<F>, S, D, W, OrderClause<O>, LOf, G, H, LC>
 where
+    F: QuerySource,
     Expr: AppearsOnTable<F>,
 {
-    type Output = SelectStatement<F, S, D, W, OrderClause<(O, Expr)>, LOf, G, H, LC>;
+    type Output = SelectStatement<FromClause<F>, S, D, W, OrderClause<(O, Expr)>, LOf, G, H, LC>;
 
     fn then_order_by(self, expr: Expr) -> Self::Output {
         SelectStatement::new(

diff --git a/diesel/src/result.rs b/diesel/src/result.rs
@@ -73,8 +73,7 @@ pub enum Error {
     RollbackErrorOnCommit {
         /// The error that was encountered when attempting the rollback
         rollback_error: Box<Error>,
-        /// If the rollback attempt resulted from a failed attempt to commit the transaction,
-        /// you will find the related error here.
+        /// The error that was encountered during the failed commit attempt
         commit_error: Box<Error>,
     },
 

diff --git a/diesel_bench/Cargo.toml b/diesel_bench/Cargo.toml
@@ -15,7 +15,7 @@ sqlx = {version = "0.6", features = ["runtime-tokio-rustls"], optional = true}
 tokio = {version = "1", optional = true}
 rusqlite = {version = "0.27", optional = true}
 rust_postgres = {version = "0.19", optional = true, package = "postgres"}
-rust_mysql = {version = "22.1", optional = true, package = "mysql"}
+rust_mysql = {version = "23.0", optional = true, package = "mysql"}
 rustorm = {version = "0.20", optional = true}
 rustorm_dao = {version = "0.20", optional = true}
 quaint = {version = "=0.2.0-alpha.13", optional = true}

diff --git a/diesel_compile_tests/Cargo.lock b/diesel_compile_tests/Cargo.lock
diff --git a/diesel_compile_tests/tests/fail/boxed_queries_require_selectable_expression_for_order.stderr b/diesel_compile_tests/tests/fail/boxed_queries_require_selectable_expression_for_order.stderr
@@ -1,11 +1,10 @@
-error[E0277]: the trait bound `FromClause<users::table>: AppearsInFromClause<posts::table>` is not satisfied
+error[E0271]: type mismatch resolving `<users::table as AppearsInFromClause<posts::table>>::Count == diesel::query_source::Once`
   --> tests/fail/boxed_queries_require_selectable_expression_for_order.rs:21:37
    |
 21 |     users::table.into_boxed::<Pg>().order(posts::title.desc());
-   |                                     ^^^^^ the trait `AppearsInFromClause<posts::table>` is not implemented for `FromClause<users::table>`
+   |                                     ^^^^^ expected struct `diesel::query_source::Never`, found struct `diesel::query_source::Once`
    |
-   = help: the trait `AppearsInFromClause<QS1>` is implemented for `FromClause<QS2>`
-note: required because of the requirements on the impl of `AppearsOnTable<FromClause<users::table>>` for `posts::columns::title`
+note: required because of the requirements on the impl of `AppearsOnTable<users::table>` for `posts::columns::title`
   --> tests/fail/boxed_queries_require_selectable_expression_for_order.rs:13:1
    |
 13 | / table! {
@@ -16,6 +15,35 @@ note: required because of the requirements on the impl of `AppearsOnTable<FromCl
 18 | | }
    | |_^
    = note: 1 redundant requirement hidden
-   = note: required because of the requirements on the impl of `AppearsOnTable<FromClause<users::table>>` for `diesel::expression::operators::Desc<posts::columns::title>`
+   = note: required because of the requirements on the impl of `AppearsOnTable<users::table>` for `diesel::expression::operators::Desc<posts::columns::title>`
+   = note: required because of the requirements on the impl of `OrderDsl<diesel::expression::operators::Desc<posts::columns::title>>` for `BoxedSelectStatement<'_, (diesel::sql_types::Integer, diesel::sql_types::Text), FromClause<users::table>, Pg>`
+   = note: this error originates in the macro `$crate::__diesel_column` which comes from the expansion of the macro `table` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0277]: the trait bound `users::table: TableNotEqual<posts::table>` is not satisfied
+  --> tests/fail/boxed_queries_require_selectable_expression_for_order.rs:21:37
+   |
+21 |     users::table.into_boxed::<Pg>().order(posts::title.desc());
+   |                                     ^^^^^ the trait `TableNotEqual<posts::table>` is not implemented for `users::table`
+   |
+   = help: the following other types implement trait `TableNotEqual<T>`:
+             <Only<pg::metadata_lookup::pg_namespace::table> as TableNotEqual<pg::metadata_lookup::pg_type::table>>
+             <Only<pg::metadata_lookup::pg_type::table> as TableNotEqual<pg::metadata_lookup::pg_namespace::table>>
+             <pg::metadata_lookup::pg_namespace::table as TableNotEqual<Only<pg::metadata_lookup::pg_type::table>>>
+             <pg::metadata_lookup::pg_namespace::table as TableNotEqual<pg::metadata_lookup::pg_type::table>>
+             <pg::metadata_lookup::pg_type::table as TableNotEqual<Only<pg::metadata_lookup::pg_namespace::table>>>
+             <pg::metadata_lookup::pg_type::table as TableNotEqual<pg::metadata_lookup::pg_namespace::table>>
+   = note: required because of the requirements on the impl of `AppearsInFromClause<posts::table>` for `users::table`
+note: required because of the requirements on the impl of `AppearsOnTable<users::table>` for `posts::columns::title`
+  --> tests/fail/boxed_queries_require_selectable_expression_for_order.rs:13:1
+   |
+13 | / table! {
+14 | |     posts {
+15 | |         id -> Integer,
+16 | |         title -> VarChar,
+17 | |     }
+18 | | }
+   | |_^
+   = note: 1 redundant requirement hidden
+   = note: required because of the requirements on the impl of `AppearsOnTable<users::table>` for `diesel::expression::operators::Desc<posts::columns::title>`
    = note: required because of the requirements on the impl of `OrderDsl<diesel::expression::operators::Desc<posts::columns::title>>` for `BoxedSelectStatement<'_, (diesel::sql_types::Integer, diesel::sql_types::Text), FromClause<users::table>, Pg>`
    = note: this error originates in the macro `$crate::__diesel_column` which comes from the expansion of the macro `table` (in Nightly builds, run with -Z macro-backtrace for more info)
diff --git a/diesel_compile_tests/tests/fail/order_requires_column_from_same_table.stderr b/diesel_compile_tests/tests/fail/order_requires_column_from_same_table.stderr
@@ -1,11 +1,36 @@
-error[E0277]: the trait bound `FromClause<users::table>: AppearsInFromClause<posts::table>` is not satisfied
+error[E0271]: type mismatch resolving `<users::table as AppearsInFromClause<posts::table>>::Count == diesel::query_source::Once`
   --> tests/fail/order_requires_column_from_same_table.rs:18:31
    |
 18 |     let source = users::table.order(posts::id);
-   |                               ^^^^^ the trait `AppearsInFromClause<posts::table>` is not implemented for `FromClause<users::table>`
+   |                               ^^^^^ expected struct `diesel::query_source::Never`, found struct `diesel::query_source::Once`
    |
-   = help: the trait `AppearsInFromClause<QS1>` is implemented for `FromClause<QS2>`
-note: required because of the requirements on the impl of `AppearsOnTable<FromClause<users::table>>` for `posts::columns::id`
+note: required because of the requirements on the impl of `AppearsOnTable<users::table>` for `posts::columns::id`
+  --> tests/fail/order_requires_column_from_same_table.rs:11:1
+   |
+11 | / table! {
+12 | |     posts {
+13 | |         id -> Integer,
+14 | |     }
+15 | | }
+   | |_^
+   = note: required because of the requirements on the impl of `OrderDsl<posts::columns::id>` for `SelectStatement<FromClause<users::table>>`
+   = note: this error originates in the macro `$crate::__diesel_column` which comes from the expansion of the macro `table` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0277]: the trait bound `users::table: TableNotEqual<posts::table>` is not satisfied
+  --> tests/fail/order_requires_column_from_same_table.rs:18:31
+   |
+18 |     let source = users::table.order(posts::id);
+   |                               ^^^^^ the trait `TableNotEqual<posts::table>` is not implemented for `users::table`
+   |
+   = help: the following other types implement trait `TableNotEqual<T>`:
+             <Only<pg::metadata_lookup::pg_namespace::table> as TableNotEqual<pg::metadata_lookup::pg_type::table>>
+             <Only<pg::metadata_lookup::pg_type::table> as TableNotEqual<pg::metadata_lookup::pg_namespace::table>>
+             <pg::metadata_lookup::pg_namespace::table as TableNotEqual<Only<pg::metadata_lookup::pg_type::table>>>
+             <pg::metadata_lookup::pg_namespace::table as TableNotEqual<pg::metadata_lookup::pg_type::table>>
+             <pg::metadata_lookup::pg_type::table as TableNotEqual<Only<pg::metadata_lookup::pg_namespace::table>>>
+             <pg::metadata_lookup::pg_type::table as TableNotEqual<pg::metadata_lookup::pg_namespace::table>>
+   = note: required because of the requirements on the impl of `AppearsInFromClause<posts::table>` for `users::table`
+note: required because of the requirements on the impl of `AppearsOnTable<users::table>` for `posts::columns::id`
   --> tests/fail/order_requires_column_from_same_table.rs:11:1
    |
 11 | / table! {