fix: respect isolation level parameter and defaults #5056
Conversation
|
|
CodSpeed Performance ReportMerging #5056 will not alter performanceComparing Summary
|
jacek-prisma
changed the title
quaint/src/connector/queryable.rs
Outdated
| @@ -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"))] | |||
There was a problem hiding this comment.
Should this be
| #[cfg(any(feature = "mssql-native", feature = "postgresql-native", feature = "mysql-native"))] | |
| #[cfg(any(feature = "sqlite-native", feature = "postgresql-native", feature = "mysql-native"))] |
?
There was a problem hiding this comment.
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
quaint/src/connector/queryable.rs
Outdated
| @@ -130,4 +131,5 @@ macro_rules! impl_default_TransactionCapable { | |||
| }; | |||
| } | |||
|
|
|||
| #[cfg(any(feature = "mssql-native", feature = "postgresql-native", feature = "mysql-native"))] | |||
quaint/src/connector/transaction.rs
Outdated
| @@ -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.
this should probably be enabled for all four connectors
| @@ -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.
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 create_additional_connection and other methods, I'm just thinking out loud and wondering if it might be worth it to de-duplicate a bunch of stuff here generally.
There was a problem hiding this comment.
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
| // 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?), | ||
| ] { |
…ines#5056 Signed-off-by: Jacek Malec <[email protected]>
…ines#5056 (#25745) * chore(deps): update engines to 5.23.0-21.0160cc2f56997459423712af8dfa8cf1ad97704b * fix: update tests to account for a sqlserver fix in prisma/prisma-engines#5056 Signed-off-by: Jacek Malec <[email protected]> * fix: further fixes related to mssql isolation level * refactor: minor test cleanups * fix: revert deletion of eslint-disable line * refactor: minor test cleanup * fix: re-add eslint-disable and update the lint stage commit not to write files --------- Signed-off-by: Jacek Malec <[email protected]> Co-authored-by: Prismo <[email protected]>
Fixes a few things:
start_transactionwas not being dispatched correctly in a few places becauseQuainthad animplthat was bypassing the impl of the inner typemssql_transaction_isolation_leveltest to capture that the isolation level parameter is respected and that isolation level changes do not linger for more than 1 transaction when left unspecified