-
Notifications
You must be signed in to change notification settings - Fork 240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: respect isolation level parameter and defaults #5056
Changes from 7 commits
d90ca61
018eae3
5a7c078
ab8a010
b5ee534
6560c62
a3d3e91
2de72ae
7098ead
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,6 +112,7 @@ pub trait TransactionCapable: Queryable { | |
) -> crate::Result<Box<dyn Transaction + 'a>>; | ||
} | ||
|
||
#[cfg(any(feature = "mssql-native", feature = "postgresql-native", feature = "mysql-native"))] | ||
macro_rules! impl_default_TransactionCapable { | ||
($t:ty) => { | ||
#[async_trait] | ||
|
@@ -130,4 +131,5 @@ macro_rules! impl_default_TransactionCapable { | |
}; | ||
} | ||
|
||
#[cfg(any(feature = "mssql-native", feature = "postgresql-native", feature = "mysql-native"))] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
pub(crate) use impl_default_TransactionCapable; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ pub trait Transaction: Queryable { | |
fn as_queryable(&self) -> &dyn Queryable; | ||
} | ||
|
||
#[cfg(any(feature = "mssql-native", feature = "postgresql-native", feature = "mysql-native"))] | ||
pub(crate) struct TransactionOptions { | ||
/// The isolation level to use. | ||
pub(crate) isolation_level: Option<IsolationLevel>, | ||
|
@@ -29,6 +30,16 @@ pub(crate) struct TransactionOptions { | |
pub(crate) isolation_first: bool, | ||
} | ||
|
||
#[cfg(any(feature = "mssql-native", feature = "postgresql-native", feature = "mysql-native"))] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should probably be enabled for all four connectors |
||
impl TransactionOptions { | ||
pub fn new(isolation_level: Option<IsolationLevel>, isolation_first: bool) -> Self { | ||
Self { | ||
isolation_level, | ||
isolation_first, | ||
} | ||
} | ||
} | ||
|
||
/// A default representation of an SQL database transaction. If not commited, a | ||
/// transaction will be rolled back by default when dropped. | ||
/// | ||
|
@@ -40,6 +51,7 @@ pub struct DefaultTransaction<'a> { | |
} | ||
|
||
impl<'a> DefaultTransaction<'a> { | ||
#[cfg(any(feature = "mssql-native", feature = "postgresql-native", feature = "mysql-native"))] | ||
pub(crate) async fn new( | ||
inner: &'a dyn Queryable, | ||
begin_stmt: &str, | ||
|
@@ -196,11 +208,3 @@ impl FromStr for IsolationLevel { | |
} | ||
} | ||
} | ||
impl TransactionOptions { | ||
pub fn new(isolation_level: Option<IsolationLevel>, isolation_first: bool) -> Self { | ||
Self { | ||
isolation_level, | ||
isolation_first, | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,6 +115,40 @@ async fn transactions_with_isolation_works(api: &mut dyn TestApi) -> crate::Resu | |
Ok(()) | ||
} | ||
|
||
#[test_each_connector(tags("mssql"))] | ||
async fn mssql_transaction_isolation_level(api: &mut dyn TestApi) -> crate::Result<()> { | ||
let table = api.create_temp_table("id int, value int").await?; | ||
|
||
let conn_a = api.conn(); | ||
// Start a transaction with the default isolation level, which in tests is | ||
// set to READ UNCOMMITED via the DB url and insert a row, but do not commit the transaction. | ||
let tx_a = conn_a.start_transaction(None).await?; | ||
let insert = Insert::single_into(&table).value("value", 3).value("id", 4); | ||
let rows_affected = tx_a.execute(insert.into()).await?; | ||
assert_eq!(1, rows_affected); | ||
|
||
// We want to verify that pooled connection behaves the same way, so we test both cases. | ||
let pool = api.create_pool()?; | ||
for conn_b in [ | ||
Box::new(pool.check_out().await?) as Box<dyn TransactionCapable>, | ||
Box::new(api.create_additional_connection().await?), | ||
] { | ||
Comment on lines
+130
to
+135
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice! |
||
// Start a transaction that explicitly sets the isolation level to SNAPSHOT and query the table | ||
// expecting to see the old state. | ||
let tx_b = conn_b.start_transaction(Some(IsolationLevel::Snapshot)).await?; | ||
let res = tx_b.query(Select::from_table(&table).into()).await?; | ||
assert_eq!(0, res.len()); | ||
|
||
// Start a transaction without an explicit isolation level, it should be run with the default | ||
// again, which is set to READ UNCOMMITED here. | ||
let tx_c = conn_b.start_transaction(None).await?; | ||
let res = tx_c.query(Select::from_table(&table).into()).await?; | ||
assert_eq!(1, res.len()); | ||
} | ||
|
||
Ok(()) | ||
} | ||
|
||
// SQLite only supports serializable. | ||
#[test_each_connector(tags("sqlite"))] | ||
async fn sqlite_serializable_tx(api: &mut dyn TestApi) -> crate::Result<()> { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,6 +83,10 @@ impl<'a> TestApi for Sqlite<'a> { | |
Quaint::new(CONN_STR).await | ||
} | ||
|
||
fn create_pool(&self) -> crate::Result<crate::pooled::Quaint> { | ||
Ok(crate::pooled::Quaint::builder(CONN_STR)?.build()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could probably add a method that returns the connection string and move the implementation to the trait itself to reduce some duplication. No need to change anything, it is totally fine as is and is consistent with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've had the same idea, but I wasn't sure if putting the connection string on the trait leaks too much of an internal detail |
||
} | ||
|
||
fn unique_constraint(&mut self, column: &str) -> String { | ||
format!("UNIQUE({column})") | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I think it should have all the connectors, I was testing with the WASM build specifically, but I think not including sqlite here causes errors in other platform builds