Skip to content
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

Merged
merged 9 commits into from
Nov 27, 2024
2 changes: 2 additions & 0 deletions quaint/src/connector/queryable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be

Suggested change
#[cfg(any(feature = "mssql-native", feature = "postgresql-native", feature = "mysql-native"))]
#[cfg(any(feature = "sqlite-native", feature = "postgresql-native", feature = "mysql-native"))]

?

Copy link
Contributor Author

@jacek-prisma jacek-prisma Nov 27, 2024

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

macro_rules! impl_default_TransactionCapable {
($t:ty) => {
#[async_trait]
Expand All @@ -130,4 +131,5 @@ macro_rules! impl_default_TransactionCapable {
};
}

#[cfg(any(feature = "mssql-native", feature = "postgresql-native", feature = "mysql-native"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

pub(crate) use impl_default_TransactionCapable;
20 changes: 12 additions & 8 deletions quaint/src/connector/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand All @@ -29,6 +30,16 @@ pub(crate) struct TransactionOptions {
pub(crate) isolation_first: bool,
}

#[cfg(any(feature = "mssql-native", feature = "postgresql-native", feature = "mysql-native"))]
Copy link
Member

Choose a reason for hiding this comment

The 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.
///
Expand All @@ -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,
Expand Down Expand Up @@ -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,
}
}
}
14 changes: 11 additions & 3 deletions quaint/src/pooled/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
use crate::connector::{MakeTlsConnectorManager, PostgresNativeUrl};
use crate::{
ast,
connector::{self, impl_default_TransactionCapable, IsolationLevel, Queryable, Transaction, TransactionCapable},
connector::{self, IsolationLevel, Queryable, Transaction, TransactionCapable},
error::Error,
};

Expand All @@ -23,7 +23,15 @@
pub(crate) inner: MobcPooled<QuaintManager>,
}

impl_default_TransactionCapable!(PooledConnection);
#[async_trait]
impl TransactionCapable for PooledConnection {
async fn start_transaction<'a>(
&'a self,
isolation: Option<IsolationLevel>,
) -> crate::Result<Box<dyn Transaction + 'a>> {
self.inner.start_transaction(isolation).await
}
}

#[async_trait]
impl Queryable for PooledConnection {
Expand Down Expand Up @@ -104,7 +112,7 @@

#[async_trait]
impl Manager for QuaintManager {
type Connection = Box<dyn Queryable>;
type Connection = Box<dyn TransactionCapable>;
type Error = Error;

async fn connect(&self) -> crate::Result<Self::Connection> {
Expand All @@ -115,7 +123,7 @@

let conn = Sqlite::new(url)?;

Ok(Box::new(conn) as Self::Connection)

Check failure on line 126 in quaint/src/pooled/manager.rs

View workflow job for this annotation

GitHub Actions / React Native / Android build for commit 201474aea19ace7037d5d328ced352b5032f8869

the trait bound `connector::sqlite::native::Sqlite: queryable::TransactionCapable` is not satisfied
}

#[cfg(feature = "mysql-native")]
Expand Down
22 changes: 15 additions & 7 deletions quaint/src/single.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use crate::{
ast,
connector::{self, impl_default_TransactionCapable, ConnectionInfo, IsolationLevel, Queryable, TransactionCapable},
connector::{self, ConnectionInfo, IsolationLevel, Queryable, TransactionCapable},
};
use async_trait::async_trait;
use std::{fmt, sync::Arc};
Expand All @@ -16,7 +16,7 @@
/// The main entry point and an abstraction over a database connection.
#[derive(Clone)]
pub struct Quaint {
inner: Arc<dyn Queryable>,
inner: Arc<dyn TransactionCapable>,
aqrln marked this conversation as resolved.
Show resolved Hide resolved
aqrln marked this conversation as resolved.
Show resolved Hide resolved
connection_info: Arc<ConnectionInfo>,
}

Expand All @@ -26,7 +26,15 @@
}
}

impl_default_TransactionCapable!(Quaint);
#[async_trait]
impl TransactionCapable for Quaint {
async fn start_transaction<'a>(
&'a self,
isolation: Option<IsolationLevel>,
) -> crate::Result<Box<dyn connector::Transaction + 'a>> {
self.inner.start_transaction(isolation).await
}
}

impl Quaint {
/// Create a new connection to the database. The connection string
Expand Down Expand Up @@ -137,28 +145,28 @@
let params = connector::SqliteParams::try_from(s)?;
let sqlite = connector::Sqlite::new(&params.file_path)?;

Arc::new(sqlite) as Arc<dyn Queryable>
Arc::new(sqlite) as Arc<dyn TransactionCapable>

Check failure on line 148 in quaint/src/single.rs

View workflow job for this annotation

GitHub Actions / React Native / Android build for commit 201474aea19ace7037d5d328ced352b5032f8869

the trait bound `connector::sqlite::native::Sqlite: queryable::TransactionCapable` is not satisfied
}
#[cfg(feature = "mysql-native")]
s if s.starts_with("mysql") => {
let url = connector::MysqlUrl::new(url::Url::parse(s)?)?;
let mysql = connector::Mysql::new(url).await?;

Arc::new(mysql) as Arc<dyn Queryable>
Arc::new(mysql) as Arc<dyn TransactionCapable>
}
#[cfg(feature = "postgresql-native")]
s if s.starts_with("postgres") || s.starts_with("postgresql") => {
let url = connector::PostgresNativeUrl::new(url::Url::parse(s)?)?;
let tls_manager = connector::MakeTlsConnectorManager::new(url.clone());
let psql = connector::PostgreSql::new(url, &tls_manager).await?;
Arc::new(psql) as Arc<dyn Queryable>
Arc::new(psql) as Arc<dyn TransactionCapable>
}
#[cfg(feature = "mssql-native")]
s if s.starts_with("jdbc:sqlserver") | s.starts_with("sqlserver") => {
let url = connector::MssqlUrl::new(s)?;
let psql = connector::Mssql::new(url).await?;

Arc::new(psql) as Arc<dyn Queryable>
Arc::new(psql) as Arc<dyn TransactionCapable>
}
_ => unimplemented!("Supported url schemes: file or sqlite, mysql, postgresql or jdbc:sqlserver."),
};
Expand All @@ -175,7 +183,7 @@
use crate::connector::sqlite::DEFAULT_SQLITE_DATABASE;

Ok(Quaint {
inner: Arc::new(connector::Sqlite::new_in_memory()?),

Check failure on line 186 in quaint/src/single.rs

View workflow job for this annotation

GitHub Actions / React Native / Android build for commit 201474aea19ace7037d5d328ced352b5032f8869

the trait bound `connector::sqlite::native::Sqlite: queryable::TransactionCapable` is not satisfied
connection_info: Arc::new(ConnectionInfo::Native(NativeConnectionInfo::InMemorySqlite {
db_name: DEFAULT_SQLITE_DATABASE.to_owned(),
})),
Expand Down
34 changes: 34 additions & 0 deletions quaint/src/tests/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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<()> {
Expand Down
1 change: 1 addition & 0 deletions quaint/src/tests/test_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,6 @@ pub trait TestApi {
fn autogen_id(&self, name: &str) -> String;
fn conn(&self) -> &crate::single::Quaint;
async fn create_additional_connection(&self) -> crate::Result<crate::single::Quaint>;
fn create_pool(&self) -> crate::Result<crate::pooled::Quaint>;
fn get_name(&mut self) -> String;
}
8 changes: 8 additions & 0 deletions quaint/src/tests/test_api/mssql.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ impl<'a> MsSql<'a> {
let names = Generator::default();
let conn = Quaint::new(&CONN_STR).await?;

// snapshot isolation enables us to test isolation levels easily
conn.raw_cmd("ALTER DATABASE tempdb SET ALLOW_SNAPSHOT_ISOLATION ON")
.await?;

Ok(Self { names, conn })
}
}
Expand Down Expand Up @@ -76,6 +80,10 @@ impl<'a> TestApi for MsSql<'a> {
Quaint::new(&CONN_STR).await
}

fn create_pool(&self) -> crate::Result<crate::pooled::Quaint> {
Ok(crate::pooled::Quaint::builder(&CONN_STR)?.build())
}

fn render_create_table(&mut self, table_name: &str, columns: &str) -> (String, String) {
let table_name = format!("##{table_name}");
let create = format!(
Expand Down
4 changes: 4 additions & 0 deletions quaint/src/tests/test_api/mysql.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,10 @@ impl<'a> TestApi for MySql<'a> {
Quaint::new(&self.conn_str).await
}

fn create_pool(&self) -> crate::Result<crate::pooled::Quaint> {
Ok(crate::pooled::Quaint::builder(&CONN_STR)?.build())
}

fn unique_constraint(&mut self, column: &str) -> String {
format!("UNIQUE({column})")
}
Expand Down
4 changes: 4 additions & 0 deletions quaint/src/tests/test_api/postgres.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ impl<'a> TestApi for PostgreSql<'a> {
Quaint::new(&CONN_STR).await
}

fn create_pool(&self) -> crate::Result<crate::pooled::Quaint> {
Ok(crate::pooled::Quaint::builder(&CONN_STR)?.build())
}

fn unique_constraint(&mut self, column: &str) -> String {
format!("UNIQUE({column})")
}
Expand Down
4 changes: 4 additions & 0 deletions quaint/src/tests/test_api/sqlite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Member

@aqrln aqrln Nov 27, 2024

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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})")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ mod metrics {

match runner.connector_version() {
Sqlite(_) => assert_eq!(total_queries, 2),
SqlServer(_) => assert_eq!(total_queries, 10),
SqlServer(_) => assert_eq!(total_queries, 12),
MongoDb(_) => assert_eq!(total_queries, 5),
CockroachDb(_) => assert_eq!(total_queries, 2),
MySql(_) => assert_eq!(total_queries, 9),
Expand Down
Loading