-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Implement transaction options API #1827
Conversation
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
edf7ac2
to
2b09ca9
Compare
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.
This is good but I have a number of nits.
I still think something like the what I proposed here would be good in addition to this: #481 (comment) to allow for any options we don't currently support.
fn begin_with( | ||
&mut self, | ||
options: <<Self::Database as Database>::TransactionManager as TransactionManager>::Options, | ||
) -> BoxFuture<'_, Result<Transaction<'_, Self::Database>, Error>> |
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.
I'm sorry, I should have looked at this PR before releasing 0.6.0 because this is a breaking change.
While it's unlikely that any implementation of this trait exists outside SQLx, since we haven't sealed any of them it's not impossible.
@@ -14,8 +14,9 @@ pub struct MssqlTransactionManager; | |||
|
|||
impl TransactionManager for MssqlTransactionManager { | |||
type Database = Mssql; | |||
type Options = (); |
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.
I think I would prefer to see at least an empty MssqlTransactionOptions
type so that we can safely add things to it later without it being a breaking change.
@@ -10,27 +10,40 @@ pub struct AnyTransactionManager; | |||
|
|||
impl TransactionManager for AnyTransactionManager { | |||
type Database = Any; | |||
type Options = (); |
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.
Actually, I think this could be a struct containing a TransactionOptions
for each database, and then the implementation picks the corresponding one:
#[derive(Default)]
pub struct AnyTransactionOptions {
#[cfg(feature = "postgres")]
postgres: PgTransactionOptions
#[cfg(feature = "mysql")]
mysql: MySqlTransactionOptions,
// etc.
}
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.
This could then have setters to replace each type wholesale:
impl AnyTransactionOptions {
#[cfg(feature = "postgres")]
pub fn postgres(self, postgres: PgTransactionOptions) -> Self {
Self { postgres, ..self }
}
// etc.
}
Or even have setters for "best effort" options:
impl AnyTransactionOptions {
pub fn isolation_level_if_supported(self, ilvl: IsolationLevel) -> Self {
#[cfg(feature = "postgres")]
self.postgres = self.postgres.isolation_level(ilvl);
#[cfg(feature = "mysql")]
self.mysql = self.mysql.isolation_level(ilvl);
self
}
}
(Since PgIsolationLevel
and MySqlIsolationLevel
are identical, they don't need to be separate enums.)
This looks really awesome! As #481 indicates, there is currently no way to do a |
Thanks for the review @abonander, it sounds like this might need to wait for another minor release? I see there's another PR applying the feedback. |
Closing in favor of #1924 |
This adds
begin_with
methods for creating transactions with additional options. The exact options object varies depending upon the backend. For MS-SQL and AnyDatabase the options are currently just()
although this could be extended, with each backend transaction option struct potentially supportingFrom<AnyTransactionOptions>
.MySQL and Postgres both support the same ANSI isolation levels, although they are currently separate enums which could potentially be combined.
As much as possible, the tests try to verify that the correct transaction options have been applied.
Related to #481, #853, #851