rm TransactionError

This was always a fairly clunky API. Its main upside was that type
inference was able to figure out the error type in more cases than the
current implementation does. It has much larger downsides though. The
most common case is that you're just returning `diesel::result::Error`
anyway, so this enum was just an annoyance. If you weren't returning
that, you were probably returning an application specific error which
has a variant for `diesel::result::Error`, and now also had to handle
this additional type. It also turns out that
[`TransactionError<Box<::std::error::Error>>` doesn't implement
`::std::error::Error`](rust-lang/rust#39792).

The new API might require explicitly stating the error type in a few
more places (in particular `try!(conn.transaction(|| try!(query);
Ok(()))); Ok(...)` stopped inferring), but I think that this will end up
being more ergonomic for most uses.

Author: sgrif

CHANGELOG.md

@@ -31,6 +31,17 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
 
 [now-0.11.0]: http://docs.diesel.rs/diesel/expression/dsl/struct.now.html
 
+* [`Connection::transaction`][transaction-0.11.0] now returns your error
+  directly instead of wrapping it in `TransactionError`. It requires that the
+  error implement `From<diesel::result::Error>`
+
+[transaction-0.11.0]: http://docs.diesel.rs/diesel/connection/trait.Connection.html#method.transaction
+
+### Removed
+
+* `result::TransactionError`
+* `result::TransactionResult`
+
 ## [0.10.1] - 2017-02-08
 
 ### Fixed

diesel/src/connection/mod.rs

@@ -34,8 +34,9 @@ pub trait Connection: SimpleConnection + Sized {
     /// function returns an `Err`,
     /// [`TransactionError::UserReturnedError`](../result/enum.TransactionError.html#variant.UserReturnedError)
     /// will be returned wrapping that value.
-    fn transaction<T, E, F>(&self, f: F) -> TransactionResult<T, E> where
+    fn transaction<T, E, F>(&self, f: F) -> Result<T, E> where
         F: FnOnce() -> Result<T, E>,
+        E: From<Error>,
     {
         let transaction_manager = self.transaction_manager();
         try!(transaction_manager.begin_transaction(self));
@@ -46,7 +47,7 @@ pub trait Connection: SimpleConnection + Sized {
             },
             Err(e) => {
                 try!(transaction_manager.rollback_transaction(self));
-                Err(TransactionError::UserReturnedError(e))
+                Err(e)
             },
         }
     }
@@ -67,7 +68,7 @@ pub trait Connection: SimpleConnection + Sized {
         let mut user_result = None;
         let _ = self.transaction::<(), _, _>(|| {
             user_result = f().ok();
-            Err(())
+            Err(Error::RollbackTransaction)
         });
         user_result.expect("Transaction did not succeed")
     }

diesel/src/lib.rs

@@ -123,7 +123,7 @@ pub mod prelude {
     pub use insertable::Insertable;
     pub use query_dsl::*;
     pub use query_source::{QuerySource, Queryable, Table, Column, JoinTo};
-    pub use result::{QueryResult, TransactionError, TransactionResult, ConnectionError, ConnectionResult, OptionalExtension};
+    pub use result::{QueryResult, ConnectionError, ConnectionResult, OptionalExtension};
 }
 
 pub use prelude::*;

diesel/src/migrations/migration_error.rs

@@ -1,4 +1,4 @@
-use result::{self, TransactionError};
+use result;
 
 use std::convert::From;
 use std::{fmt, io};
@@ -99,13 +99,3 @@ impl From<io::Error> for RunMigrationsError {
         RunMigrationsError::MigrationError(e.into())
     }
 }
-
-impl From<TransactionError<RunMigrationsError>> for RunMigrationsError {
-    fn from(e: TransactionError<RunMigrationsError>) -> Self {
-        use result::TransactionError::*;
-        match e {
-            CouldntCreateTransaction(e) => RunMigrationsError::from(e),
-            UserReturnedError(e) => e,
-        }
-    }
-}

diesel/src/migrations/mod.rs

@@ -227,20 +227,19 @@ fn run_migration<Conn>(conn: &Conn, migration: &Migration, output: &mut Write)
         try!(migration.run(conn));
         try!(conn.insert_new_migration(migration.version()));
         Ok(())
-    }).map_err(|e| e.into())
+    })
 }
 
 fn revert_migration<Conn: Connection>(conn: &Conn, migration: Box<Migration>, output: &mut Write)
     -> Result<(), RunMigrationsError>
 {
-    try!(conn.transaction(|| {
+    conn.transaction(|| {
         try!(writeln!(output, "Rolling back migration {}", migration.version()));
         try!(migration.revert(conn));
         let target = __diesel_schema_migrations.filter(version.eq(migration.version()));
         try!(::delete(target).execute(conn));
         Ok(())
-    }));
-    Ok(())
+    })
 }
 
 /// Returns the directory containing migrations. Will look at for

diesel/src/result.rs

@@ -15,6 +15,11 @@ pub enum Error {
     QueryBuilderError(Box<StdError+Send+Sync>),
     DeserializationError(Box<StdError+Send+Sync>),
     SerializationError(Box<StdError+Send+Sync>),
+    /// You can return this variant inside of a transaction when you want to
+    /// roll it back, but have no actual error to return. Diesel will never
+    /// return this variant unless you gave it to us, and it can be safely
+    /// ignored in error handling.
+    RollbackTransaction,
     #[doc(hidden)]
     __Nonexhaustive,
 }
@@ -68,15 +73,8 @@ pub enum ConnectionError {
     __Nonexhaustive, // Match against _ instead, more variants may be added in the future
 }
 
-#[derive(Debug, PartialEq)]
-pub enum TransactionError<E> {
-    CouldntCreateTransaction(Error),
-    UserReturnedError(E),
-}
-
 pub type QueryResult<T> = Result<T, Error>;
 pub type ConnectionResult<T> = Result<T, ConnectionError>;
-pub type TransactionResult<T, E> = Result<T, TransactionError<E>>;
 
 pub trait OptionalExtension<T> {
     fn optional(self) -> Result<Option<T>, Error>;
@@ -104,21 +102,6 @@ impl From<NulError> for Error {
     }
 }
 
-impl<E> From<Error> for TransactionError<E> {
-    fn from(e: Error) -> Self {
-        TransactionError::CouldntCreateTransaction(e)
-    }
-}
-
-impl From<TransactionError<Error>> for Error {
-    fn from(e: TransactionError<Error>) -> Self {
-        match e {
-            TransactionError::CouldntCreateTransaction(e) |
-            TransactionError::UserReturnedError(e) => e,
-        }
-    }
-}
-
 impl Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
@@ -128,6 +111,7 @@ impl Display for Error {
             Error::QueryBuilderError(ref e) => e.fmt(f),
             Error::DeserializationError(ref e) => e.fmt(f),
             Error::SerializationError(ref e) => e.fmt(f),
+            Error::RollbackTransaction => write!(f, "{}", self.description()),
             Error::__Nonexhaustive => unreachable!(),
         }
     }
@@ -142,6 +126,7 @@ impl StdError for Error {
             Error::QueryBuilderError(ref e) => e.description(),
             Error::DeserializationError(ref e) => e.description(),
             Error::SerializationError(ref e) => e.description(),
+            Error::RollbackTransaction => "The current transaction was aborted",
             Error::__Nonexhaustive => unreachable!(),
         }
     }
@@ -169,24 +154,6 @@ impl StdError for ConnectionError {
     }
 }
 
-impl<E: Display> Display for TransactionError<E> {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match *self {
-            TransactionError::CouldntCreateTransaction(ref e) => e.fmt(f),
-            TransactionError::UserReturnedError(ref e) => e.fmt(f),
-        }
-    }
-}
-
-impl<E: StdError> StdError for TransactionError<E> {
-    fn description(&self) -> &str {
-        match *self {
-            TransactionError::CouldntCreateTransaction(ref e) => e.description(),
-            TransactionError::UserReturnedError(ref e) => e.description(),
-        }
-    }
-}
-
 impl PartialEq for Error {
     fn eq(&self, other: &Error) -> bool {
         match (self, other) {

diesel_tests/tests/transactions.rs

@@ -1,14 +1,6 @@
 use schema::*;
 use diesel::*;
-
-macro_rules! try_no_coerce {
-    ($e:expr) => ({
-        match $e {
-            Ok(e) => e,
-            Err(e) => return Err(e),
-        }
-    })
-}
+use diesel::result::Error;
 
 #[test]
 #[cfg(not(feature = "sqlite"))] // FIXME: This test is only valid when operating on a file and not :memory:
@@ -19,10 +11,10 @@ fn transaction_executes_fn_in_a_sql_transaction() {
     setup_test_table(&conn1, test_name);
     let get_count = |conn| count_test_table(conn, test_name);
 
-    conn1.transaction(|| {
+    conn1.transaction::<_, Error, _>(|| {
         assert_eq!(0, get_count(&conn1));
         assert_eq!(0, get_count(&conn2));
-        try_no_coerce!(conn1.execute(&format!("INSERT INTO {} DEFAULT VALUES", test_name)));
+        try!(conn1.execute(&format!("INSERT INTO {} DEFAULT VALUES", test_name)));
         assert_eq!(1, get_count(&conn1));
         assert_eq!(0, get_count(&conn2));
         Ok(())
@@ -38,7 +30,7 @@ fn transaction_executes_fn_in_a_sql_transaction() {
 fn transaction_returns_the_returned_value() {
     let conn1 = connection_without_transaction();
 
-    assert_eq!(Ok(1), conn1.transaction::<_, (), _>(|| Ok(1)));
+    assert_eq!(Ok(1), conn1.transaction::<_, Error, _>(|| Ok(1)));
 }
 
 #[test]
@@ -48,9 +40,9 @@ fn transaction_is_rolled_back_when_returned_an_error() {
     setup_test_table(&connection, test_name);
     let get_count = || count_test_table(&connection, test_name);
 
-    let _ = connection.transaction::<(), (), _>(|| {
+    let _ = connection.transaction::<(), _, _>(|| {
         connection.execute(&format!("INSERT INTO {} DEFAULT VALUES", test_name)).unwrap();
-        Err(())
+        Err(Error::RollbackTransaction)
     });
     assert_eq!(0, get_count());
 
@@ -64,22 +56,22 @@ fn transactions_can_be_nested() {
     setup_test_table(&connection, test_name);
     let get_count = || count_test_table(&connection, test_name);
 
-    let _ = connection.transaction::<(), (), _>(|| {
+    let _ = connection.transaction::<(), _, _>(|| {
         connection.execute(&format!("INSERT INTO {} DEFAULT VALUES", test_name)).unwrap();
         assert_eq!(1, get_count());
-        let _ = connection.transaction::<(), (), _>(|| {
+        let _ = connection.transaction::<(), _, _>(|| {
             connection.execute(&format!("INSERT INTO {} DEFAULT VALUES", test_name)).unwrap();
             assert_eq!(2, get_count());
-            Err(())
+            Err(Error::RollbackTransaction)
         });
         assert_eq!(1, get_count());
-        let _ = connection.transaction::<(), (), _>(|| {
+        let _ = connection.transaction::<(), Error, _>(|| {
             connection.execute(&format!("INSERT INTO {} DEFAULT VALUES", test_name)).unwrap();
             assert_eq!(2, get_count());
             Ok(())
         });
         assert_eq!(2, get_count());
-        Err(())
+        Err(Error::RollbackTransaction)
     });
     assert_eq!(0, get_count());
 
@@ -92,8 +84,8 @@ fn test_transaction_always_rolls_back() {
     let test_name = "test_transaction_always_rolls_back";
     setup_test_table(&connection, test_name);
 
-    let result = connection.test_transaction(|| {
-        try_no_coerce!(connection.execute(&format!("INSERT INTO {} DEFAULT VALUES", test_name)));
+    let result = connection.test_transaction::<_, Error, _>(|| {
+        try!(connection.execute(&format!("INSERT INTO {} DEFAULT VALUES", test_name)));
         assert_eq!(1, count_test_table(&connection, test_name));
         Ok("success")
     });