Merge pull request #3486 from weiznich/backport_fixes
Backport fixes
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 7202323..8e4d0ee 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -51,13 +51,6 @@
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 @@
- 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 @@
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
index 1a0d724..870da02 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,6 +10,13 @@
## 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
index a01c25f..66b21b9 100644
--- 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
index cc3c688..74a00bb 100644
--- a/diesel/src/connection/transaction_manager.rs
+++ b/diesel/src/connection/transaction_manager.rs
@@ -135,13 +135,11 @@
}
}
- #[cfg(any(
- feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes",
- feature = "postgres",
- ))]
+ #[cfg(feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes")]
#[diesel_derives::__diesel_public_if(
feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"
)]
+ #[deprecated(note = "Removed without replacement")]
/// Whether we may be interested in calling
/// `set_top_level_transaction_requires_rollback_if_not_broken`
///
@@ -153,6 +151,17 @@
}
}
+ #[cfg(feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes")]
+ #[diesel_derives::__diesel_public_if(
+ feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"
+ )]
+ #[deprecated(note = "Use `set_requires_rollback_maybe_up_to_top_level` instead")]
+ /// If in transaction and transaction manager is not broken, registers that the
+ /// connection can not be used anymore until top-level transaction is rolled back
+ pub(crate) fn set_top_level_transaction_requires_rollback(&mut self) {
+ self.set_requires_rollback_maybe_up_to_top_level(true);
+ }
+
#[cfg(any(
feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes",
feature = "postgres",
@@ -162,18 +171,21 @@
#[diesel_derives::__diesel_public_if(
feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"
)]
- /// If in transaction and transaction manager is not broken, registers that the
- /// connection can not be used anymore until top-level transaction is rolled back
- pub(crate) fn set_top_level_transaction_requires_rollback(&mut self) {
+ /// If in transaction and transaction manager is not broken, registers that it's possible that
+ /// the connection can not be used anymore until top-level transaction is rolled back.
+ ///
+ /// If that is registered, savepoints rollbacks will still be attempted, but failure to do so
+ /// will not result in an error. (Some may succeed, some may not.)
+ pub(crate) fn set_requires_rollback_maybe_up_to_top_level(&mut self, to: bool) {
if let TransactionManagerStatus::Valid(ValidTransactionManagerStatus {
in_transaction:
Some(InTransactionStatus {
- top_level_transaction_requires_rollback,
+ requires_rollback_maybe_up_to_top_level,
..
}),
}) = self
{
- *top_level_transaction_requires_rollback = true;
+ *requires_rollback_maybe_up_to_top_level = to;
}
}
@@ -213,7 +225,9 @@
#[derive(Debug)]
struct InTransactionStatus {
transaction_depth: NonZeroU32,
- top_level_transaction_requires_rollback: bool,
+ /// If that is registered, savepoints rollbacks will still be attempted, but failure to do so
+ /// will not result in an error. (Some may succeed, some may not.)
+ requires_rollback_maybe_up_to_top_level: bool,
test_transaction: bool,
}
@@ -252,7 +266,7 @@
(None, TransactionDepthChange::IncreaseDepth) => {
self.in_transaction = Some(InTransactionStatus {
transaction_depth: NonZeroU32::new(1).expect("1 is non-zero"),
- top_level_transaction_requires_rollback: false,
+ requires_rollback_maybe_up_to_top_level: false,
test_transaction: false,
});
Ok(())
@@ -332,40 +346,38 @@
fn rollback_transaction(conn: &mut Conn) -> QueryResult<()> {
let transaction_state = Self::get_transaction_state(conn)?;
- let rollback_sql = match transaction_state.in_transaction {
- Some(ref mut in_transaction) => {
+ let (
+ (rollback_sql, rolling_back_top_level),
+ requires_rollback_maybe_up_to_top_level_before_execute,
+ ) = match transaction_state.in_transaction {
+ Some(ref in_transaction) => (
match in_transaction.transaction_depth.get() {
- 1 => Cow::Borrowed("ROLLBACK"),
- depth_gt1 => {
- if in_transaction.top_level_transaction_requires_rollback {
- // There's no point in *actually* rolling back this one
- // because we won't be able to do anything until top-level
- // is rolled back.
-
- // To make it easier on the user (that they don't have to really look
- // at actual transaction depth and can just rely on the number of
- // times they have called begin/commit/rollback) we don't mark the
- // transaction manager as out of the savepoints as soon as we
- // realize there is that issue, but instead we still decrement here:
- in_transaction.transaction_depth = NonZeroU32::new(depth_gt1 - 1)
- .expect("Depth was checked to be > 1");
- return Ok(());
- } else {
- Cow::Owned(format!(
- "ROLLBACK TO SAVEPOINT diesel_savepoint_{}",
- depth_gt1 - 1
- ))
- }
- }
- }
- }
+ 1 => (Cow::Borrowed("ROLLBACK"), true),
+ depth_gt1 => (
+ Cow::Owned(format!(
+ "ROLLBACK TO SAVEPOINT diesel_savepoint_{}",
+ depth_gt1 - 1
+ )),
+ false,
+ ),
+ },
+ in_transaction.requires_rollback_maybe_up_to_top_level,
+ ),
None => return Err(Error::NotInTransaction),
};
match conn.batch_execute(&*rollback_sql) {
Ok(()) => {
- Self::get_transaction_state(conn)?
- .change_transaction_depth(TransactionDepthChange::DecreaseDepth)?;
+ match Self::get_transaction_state(conn)?
+ .change_transaction_depth(TransactionDepthChange::DecreaseDepth)
+ {
+ Ok(()) => {}
+ Err(Error::NotInTransaction) if rolling_back_top_level => {
+ // Transaction exit may have already been detected by connection
+ // implementation. It's fine.
+ }
+ Err(e) => return Err(e),
+ }
Ok(())
}
Err(rollback_error) => {
@@ -375,17 +387,33 @@
in_transaction:
Some(InTransactionStatus {
transaction_depth,
- top_level_transaction_requires_rollback,
+ requires_rollback_maybe_up_to_top_level,
..
}),
- }) if transaction_depth.get() > 1
- && !*top_level_transaction_requires_rollback =>
- {
+ }) if transaction_depth.get() > 1 => {
// A savepoint failed to rollback - we may still attempt to repair
- // the connection by rolling back top-level transaction.
+ // the connection by rolling back higher levels.
+
+ // To make it easier on the user (that they don't have to really
+ // look at actual transaction depth and can just rely on the number
+ // of times they have called begin/commit/rollback) we still
+ // decrement here:
*transaction_depth = NonZeroU32::new(transaction_depth.get() - 1)
.expect("Depth was checked to be > 1");
- *top_level_transaction_requires_rollback = true;
+ *requires_rollback_maybe_up_to_top_level = true;
+ if requires_rollback_maybe_up_to_top_level_before_execute {
+ // In that case, we tolerate that savepoint releases fail
+ // -> we should ignore errors
+ return Ok(());
+ }
+ }
+ TransactionManagerStatus::Valid(ValidTransactionManagerStatus {
+ in_transaction: None,
+ }) => {
+ // we would have returned `NotInTransaction` if that was already the state
+ // before we made our call
+ // => Transaction manager status has been fixed by the underlying connection
+ // so we don't need to set_in_error
}
_ => tm_status.set_in_error(),
}
@@ -402,53 +430,50 @@
fn commit_transaction(conn: &mut Conn) -> QueryResult<()> {
let transaction_state = Self::get_transaction_state(conn)?;
let transaction_depth = transaction_state.transaction_depth();
- let commit_sql = match transaction_depth {
+ let (commit_sql, committing_top_level) = match transaction_depth {
None => return Err(Error::NotInTransaction),
- Some(transaction_depth) if transaction_depth.get() == 1 => Cow::Borrowed("COMMIT"),
- Some(transaction_depth) => Cow::Owned(format!(
- "RELEASE SAVEPOINT diesel_savepoint_{}",
- transaction_depth.get() - 1
- )),
+ Some(transaction_depth) if transaction_depth.get() == 1 => {
+ (Cow::Borrowed("COMMIT"), true)
+ }
+ Some(transaction_depth) => (
+ Cow::Owned(format!(
+ "RELEASE SAVEPOINT diesel_savepoint_{}",
+ transaction_depth.get() - 1
+ )),
+ false,
+ ),
};
match conn.batch_execute(&*commit_sql) {
Ok(()) => {
- Self::get_transaction_state(conn)?
- .change_transaction_depth(TransactionDepthChange::DecreaseDepth)?;
+ match Self::get_transaction_state(conn)?
+ .change_transaction_depth(TransactionDepthChange::DecreaseDepth)
+ {
+ Ok(()) => {}
+ Err(Error::NotInTransaction) if committing_top_level => {
+ // Transaction exit may have already been detected by connection.
+ // It's fine
+ }
+ Err(e) => return Err(e),
+ }
Ok(())
}
Err(commit_error) => {
if let TransactionManagerStatus::Valid(ValidTransactionManagerStatus {
in_transaction:
Some(InTransactionStatus {
- ref mut transaction_depth,
- top_level_transaction_requires_rollback: true,
+ requires_rollback_maybe_up_to_top_level: true,
..
}),
}) = conn.transaction_state().status
{
- match transaction_depth.get() {
- 1 => match Self::rollback_transaction(conn) {
- Ok(()) => {}
- Err(rollback_error) => {
- conn.transaction_state().status.set_in_error();
- return Err(Error::RollbackErrorOnCommit {
- rollback_error: Box::new(rollback_error),
- commit_error: Box::new(commit_error),
- });
- }
- },
- depth_gt1 => {
- // There's no point in *actually* rolling back this one
- // because we won't be able to do anything until top-level
- // is rolled back.
-
- // To make it easier on the user (that they don't have to really look
- // at actual transaction depth and can just rely on the number of
- // times they have called begin/commit/rollback) we don't mark the
- // transaction manager as out of the savepoints as soon as we
- // realize there is that issue, but instead we still decrement here:
- *transaction_depth = NonZeroU32::new(depth_gt1 - 1)
- .expect("Depth was checked to be > 1");
+ match Self::rollback_transaction(conn) {
+ Ok(()) => {}
+ Err(rollback_error) => {
+ conn.transaction_state().status.set_in_error();
+ return Err(Error::RollbackErrorOnCommit {
+ rollback_error: Box::new(rollback_error),
+ commit_error: Box::new(commit_error),
+ });
}
}
}
@@ -490,7 +515,7 @@
if self.top_level_requires_rollback_after_next_batch_execute {
self.transaction_state
.status
- .set_top_level_transaction_requires_rollback();
+ .set_requires_rollback_maybe_up_to_top_level(true);
}
res
}
@@ -1010,4 +1035,181 @@
).transaction_depth().expect("Transaction depth")
);
}
+
+ // regression test for #3470
+ // crates.io depends on this behaviour
+ #[test]
+ #[cfg(feature = "postgres")]
+ fn some_libpq_failures_are_recoverable_by_rolling_back_the_savepoint_only() {
+ use crate::connection::{AnsiTransactionManager, TransactionManager};
+ use crate::prelude::*;
+ use crate::sql_query;
+
+ crate::table! {
+ rollback_test (id) {
+ id -> Int4,
+ value -> Int4,
+ }
+ }
+
+ let conn = &mut crate::test_helpers::pg_connection_no_transaction();
+ assert_eq!(
+ None,
+ <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+ conn
+ ).transaction_depth().expect("Transaction depth")
+ );
+
+ let res = conn.transaction(|conn| {
+ sql_query(
+ "CREATE TABLE IF NOT EXISTS rollback_test (id INT PRIMARY KEY, value INT NOT NULL)",
+ )
+ .execute(conn)?;
+ conn.transaction(|conn| {
+ sql_query("SET TRANSACTION READ ONLY").execute(conn)?;
+ crate::update(rollback_test::table)
+ .set(rollback_test::value.eq(0))
+ .execute(conn)
+ })
+ .map(|_| {
+ panic!("Should use the `or_else` branch");
+ })
+ .or_else(|_| sql_query("SELECT 1").execute(conn))
+ .map(|_| ())
+ });
+ assert!(res.is_ok());
+
+ assert_eq!(
+ None,
+ <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+ conn
+ ).transaction_depth().expect("Transaction depth")
+ );
+ }
+
+ #[test]
+ #[cfg(feature = "postgres")]
+ fn other_libpq_failures_are_not_recoverable_by_rolling_back_the_savepoint_only() {
+ use crate::connection::{AnsiTransactionManager, TransactionManager};
+ use crate::prelude::*;
+ use crate::sql_query;
+ use std::num::NonZeroU32;
+ use std::sync::{Arc, Barrier};
+
+ crate::table! {
+ rollback_test2 (id) {
+ id -> Int4,
+ value -> Int4,
+ }
+ }
+ let conn = &mut crate::test_helpers::pg_connection_no_transaction();
+
+ sql_query(
+ "CREATE TABLE IF NOT EXISTS rollback_test2 (id INT PRIMARY KEY, value INT NOT NULL)",
+ )
+ .execute(conn)
+ .unwrap();
+
+ let start_barrier = Arc::new(Barrier::new(2));
+ let commit_barrier = Arc::new(Barrier::new(2));
+
+ let other_start_barrier = start_barrier.clone();
+ let other_commit_barrier = commit_barrier.clone();
+
+ let t1 = std::thread::spawn(move || {
+ let conn = &mut crate::test_helpers::pg_connection_no_transaction();
+ assert_eq!(
+ None,
+ <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+ conn
+ ).transaction_depth().expect("Transaction depth")
+ );
+ let r = conn.build_transaction().serializable().run::<_, crate::result::Error, _>(|conn| {
+ assert_eq!(
+ NonZeroU32::new(1),
+ <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+ conn
+ ).transaction_depth().expect("Transaction depth")
+ );
+ rollback_test2::table.load::<(i32, i32)>(conn)?;
+ crate::insert_into(rollback_test2::table)
+ .values((rollback_test2::id.eq(1), rollback_test2::value.eq(42)))
+ .execute(conn)?;
+ let r = conn.transaction(|conn| {
+ assert_eq!(
+ NonZeroU32::new(2),
+ <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+ conn
+ ).transaction_depth().expect("Transaction depth")
+ );
+ start_barrier.wait();
+ commit_barrier.wait();
+ let r = rollback_test2::table.load::<(i32, i32)>(conn);
+ assert!(r.is_err());
+ Err::<(), _>(crate::result::Error::RollbackTransaction)
+ });
+ assert_eq!(
+ NonZeroU32::new(1),
+ <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+ conn
+ ).transaction_depth().expect("Transaction depth")
+ );
+ assert!(
+ matches!(r, Err(crate::result::Error::RollbackTransaction)),
+ "rollback failed (such errors should be ignored by transaction manager): {}",
+ r.unwrap_err()
+ );
+ let r = rollback_test2::table.load::<(i32, i32)>(conn);
+ assert!(r.is_err());
+ // fun fact: if hitting "commit" after receiving a serialization failure, PG
+ // returns that the commit has succeeded, but in fact it was actually rolled back.
+ // soo.. one should avoid doing that
+ r
+ });
+ assert!(r.is_err());
+ assert_eq!(
+ None,
+ <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+ conn
+ ).transaction_depth().expect("Transaction depth")
+ );
+ });
+
+ let t2 = std::thread::spawn(move || {
+ other_start_barrier.wait();
+ let conn = &mut crate::test_helpers::pg_connection_no_transaction();
+ assert_eq!(
+ None,
+ <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+ conn
+ ).transaction_depth().expect("Transaction depth")
+ );
+ let r = conn.build_transaction().serializable().run::<_, crate::result::Error, _>(|conn| {
+ assert_eq!(
+ NonZeroU32::new(1),
+ <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+ conn
+ ).transaction_depth().expect("Transaction depth")
+ );
+ let _ = rollback_test2::table.load::<(i32, i32)>(conn)?;
+ crate::insert_into(rollback_test2::table)
+ .values((rollback_test2::id.eq(23), rollback_test2::value.eq(42)))
+ .execute(conn)?;
+ Ok(())
+ });
+ other_commit_barrier.wait();
+ assert!(r.is_ok(), "{:?}", r.unwrap_err());
+ assert_eq!(
+ None,
+ <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+ conn
+ ).transaction_depth().expect("Transaction depth")
+ );
+ });
+ crate::sql_query("DELETE FROM rollback_test2")
+ .execute(conn)
+ .unwrap();
+ t1.join().unwrap();
+ t2.join().unwrap();
+ }
}
diff --git a/diesel/src/mysql/connection/mod.rs b/diesel/src/mysql/connection/mod.rs
index 00cec1d..7b520cc 100644
--- a/diesel/src/mysql/connection/mod.rs
+++ b/diesel/src/mysql/connection/mod.rs
@@ -171,7 +171,7 @@
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
index fd2130d..d1f7cfc 100644
--- a/diesel/src/pg/connection/mod.rs
+++ b/diesel/src/pg/connection/mod.rs
@@ -239,37 +239,54 @@
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()
+ /// 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::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()
- }
- }
- 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
index a7483cf..3696230 100644
--- a/diesel/src/pg/connection/raw.rs
+++ b/diesel/src/pg/connection/raw.rs
@@ -108,6 +108,8 @@
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
index 5dc2835..c6935c9 100644
--- a/diesel/src/query_builder/select_statement/boxed.rs
+++ b/diesel/src/query_builder/select_statement/boxed.rs
@@ -389,9 +389,12 @@
}
}
-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 @@
}
}
-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
index 579f464..8d475dc 100644
--- a/diesel/src/query_builder/select_statement/dsl_impls.rs
+++ b/diesel/src/query_builder/select_statement/dsl_impls.rs
@@ -261,15 +261,18 @@
}
}
+// 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 @@
}
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
index ce8b1ad..2a9ff4e 100644
--- a/diesel/src/result.rs
+++ b/diesel/src/result.rs
@@ -73,8 +73,7 @@
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
index 971f2b9..795696a 100644
--- a/diesel_bench/Cargo.toml
+++ b/diesel_bench/Cargo.toml
@@ -15,7 +15,7 @@
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
index 0da3f61..ee009f5 100644
--- a/diesel_compile_tests/Cargo.lock
+++ b/diesel_compile_tests/Cargo.lock
@@ -58,7 +58,7 @@
[[package]]
name = "diesel"
-version = "2.0.0"
+version = "2.0.2"
dependencies = [
"bigdecimal",
"bitflags",
@@ -92,7 +92,7 @@
[[package]]
name = "diesel_derives"
-version = "2.0.0"
+version = "2.0.1"
dependencies = [
"proc-macro-error",
"proc-macro2",
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
index a6f50f5..c5d6cd2 100644
--- 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 @@
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
index 0d1cf53..ee30be6 100644
--- 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! {
diff --git a/diesel_tests/tests/order.rs b/diesel_tests/tests/order.rs
index a65f257..9addd26 100644
--- a/diesel_tests/tests/order.rs
+++ b/diesel_tests/tests/order.rs
@@ -78,3 +78,54 @@
let data: Vec<_> = users.order(name.desc()).load(conn).unwrap();
assert_eq!(expected_data, data);
}
+
+// regression test for #3412
+#[test]
+fn dynamic_order() {
+ use crate::schema::users;
+ use diesel::expression::expression_types::NotSelectable;
+
+ let conn = &mut connection_with_sean_and_tess_in_users_table();
+ let expected = &["Tess", "Sean"] as &[_];
+
+ let order_field: Box<dyn BoxableExpression<users::table, _, SqlType = NotSelectable>> =
+ Box::new(users::id.desc());
+
+ let result = users::table
+ .select(users::name)
+ .order(order_field)
+ .load::<String>(conn)
+ .unwrap();
+ assert_eq!(expected, &result);
+
+ let order_field: Box<dyn BoxableExpression<users::table, _, SqlType = NotSelectable>> =
+ Box::new(users::id.desc());
+
+ let result = users::table
+ .select(users::name)
+ .then_order_by(order_field)
+ .load::<String>(conn)
+ .unwrap();
+ assert_eq!(expected, &result);
+
+ let order_field: Box<dyn BoxableExpression<users::table, _, SqlType = NotSelectable>> =
+ Box::new(users::id.desc());
+ let result = users::table
+ .select(users::name)
+ .into_boxed()
+ .order(order_field)
+ .load::<String>(conn)
+ .unwrap();
+ assert_eq!(expected, &result);
+
+ let order_field: Box<dyn BoxableExpression<users::table, _, SqlType = NotSelectable>> =
+ Box::new(users::id.desc());
+
+ let result = users::table
+ .select(users::name)
+ .into_boxed()
+ .then_order_by(order_field)
+ .load::<String>(conn)
+ .unwrap();
+ assert_eq!(expected, &result);
+}
