-
Notifications
You must be signed in to change notification settings - Fork 123
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
feat: Default transaction isolation #1998
feat: Default transaction isolation #1998
Conversation
@gauravsnj: Can we add a description for the PR to add some context for the changes? Also, can we add a few tests for this? |
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.
LGTM (with a tiny request for a couple of extra tests)
"begin isolation level serializable not deferrable", "start isolation level serializable not deferrable", "begin transaction isolation level serializable not deferrable", "start transaction isolation level serializable not deferrable", "begin work isolation level serializable not deferrable", "start work isolation level serializable not deferrable", | ||
"begin isolation level default read write not deferrable", "start isolation level default read only not deferrable", "begin transaction isolation level default read only not deferrable", "start transaction isolation level default read write not deferrable", "begin work isolation level default read write not deferrable", "start work isolation level default read only not deferrable", | ||
"begin isolation level serializable read write not deferrable", "start isolation level serializable read write not deferrable", "begin transaction isolation level serializable read only not deferrable", "start transaction isolation level serializable read write not deferrable", "begin work isolation level serializable read write not deferrable", "start work isolation level serializable read only not deferrable", | ||
"begin isolation level serializable, read write, not deferrable", "start isolation level serializable, read write, not deferrable", "begin transaction isolation level serializable, read only, not deferrable", "start transaction isolation level serializable, read write, not deferrable", "begin work isolation level serializable, read write, not deferrable", "start work isolation level serializable, read only" |
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.
For completeness (and to prevent potential regressions in the future), could we also add a couple of tests with not deferrable
as not the last option. So for example:
begin not deferrable read write;
begin not deferrable isolation level default;
@@ -382,8 +382,9 @@ public Class<PgTransactionMode> getParameterClass() { | |||
public PgTransactionMode convert(String value) { | |||
PgTransactionMode mode = new PgTransactionMode(); | |||
// Transaction mode may contain multiple spaces. | |||
String valueWithoutNotDeferrable = value.replaceAll("(?i)(not\\s+deferrable)", " "); |
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.
nit: please check for a better variable name for valueWithoutNotDeferrable
, the double negative makes it slightly confusing to read.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️
If you write sample code, please follow the samples format.