Skip to content

feat: support multiple PostgreSQL transaction options #1949

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

Merged
merged 8 commits into from
Jul 22, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
fix: support on/off yes/no 1/0
  • Loading branch information
olavloite committed Jul 21, 2022
commit 93f6f5a819f4abbe4212dd2cf7cb4d3826d6b08f
Original file line number Diff line number Diff line change
Expand Up @@ -111,14 +111,24 @@ public Boolean convert(String value) {
if ("true".equalsIgnoreCase(value)
Copy link
Contributor

@gauravsnj gauravsnj Jul 21, 2022

Choose a reason for hiding this comment

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

I think we should also trim the string to consider the cases like ' true '

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(See also above). PostgreSQL does not consider that to be a valid boolean value. When the value is quoted with either single or double quotes, it may not contain any spaces.

|| "tru".equalsIgnoreCase(value)
|| "tr".equalsIgnoreCase(value)
|| "t".equalsIgnoreCase(value)) {
|| "t".equalsIgnoreCase(value)
|| "on".equalsIgnoreCase(value)
|| "1".equalsIgnoreCase(value)
|| "yes".equalsIgnoreCase(value)
|| "ye".equalsIgnoreCase(value)
|| "y".equalsIgnoreCase(value)) {
return Boolean.TRUE;
}
if ("false".equalsIgnoreCase(value)
|| "fals".equalsIgnoreCase(value)
Copy link
Contributor

@gauravsnj gauravsnj Jul 21, 2022

Choose a reason for hiding this comment

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

I don't think that the values like "tru", "fal", "tr" are supported without single quotes. So, tru or fal shouldn't work

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strangely enough, they are. I've tried a multitude of different statements with a real PG instance. See below for the results:

knut-test-db=# set default_transaction_read_only to "on";
SET

knut-test-db=# set default_transaction_read_only to tr;
SET

knut-test-db=# set default_transaction_read_only to ' true ';
ERROR:  parameter "default_transaction_read_only" requires a Boolean value

knut-test-db=# set default_transaction_read_only to "true";
SET

knut-test-db=# set default_transaction_read_only to 'true';
SET

knut-test-db=# set default_transaction_read_only to ye;
SET

knut-test-db=# set default_transaction_read_only to 'ye';
SET

So TLDR:

  • Both single and double quotes are allowed (which is strange, as double quotes would normally be used to quote identifiers)
  • Unique prefixes like tr and n are allowed both with and without quotes.
  • Spaces inside quotes are not allowed, so ' true ' is not a valid value.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's strange because once I tested inserting boolean values in some table and the results were totally opposite :) Btw thanks for clarifying

|| "fal".equalsIgnoreCase(value)
Copy link
Contributor

@gauravsnj gauravsnj Jul 21, 2022

Choose a reason for hiding this comment

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

Can we add support for 'ON', 'OFF', 'yes', 'no', 0 and 1 while converting the boolean values? For the Psycopg2, adding only 'ON' and 'OFF' will do but we can add 'yes', 'no', 0 and 1 to make it fully generic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, added for:

  • on/off
  • 1/0
  • yes/no

Including all unique prefixes (i.e. ye is also valid as true).

|| "fa".equalsIgnoreCase(value)
|| "f".equalsIgnoreCase(value)) {
|| "f".equalsIgnoreCase(value)
|| "off".equalsIgnoreCase(value)
|| "of".equalsIgnoreCase(value)
|| "0".equalsIgnoreCase(value)
|| "no".equalsIgnoreCase(value)
|| "n".equalsIgnoreCase(value)) {
return Boolean.FALSE;
}
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@
}
},
{
"name": "SET DEFAULT_TRANSACTION_READ_ONLY = TRUE|FALSE",
"name": "SET DEFAULT_TRANSACTION_READ_ONLY =|TO TRUE|FALSE",
"executorName": "ClientSideStatementSetExecutor",
"resultType": "NO_RESULT",
"statementType": "SET_READONLY",
Expand All @@ -393,7 +393,15 @@
"set default_transaction_read_only = t",
"set default_transaction_read_only = f",
"set default_transaction_read_only to 't'",
"set default_transaction_read_only to \"f\""
"set default_transaction_read_only to \"f\"",
"set default_transaction_read_only = on",
"set default_transaction_read_only = off",
"set default_transaction_read_only = 1",
"set default_transaction_read_only = 0",
"set default_transaction_read_only = yes",
"set default_transaction_read_only = no",
"set default_transaction_read_only = y",
"set default_transaction_read_only = n"
],
"setStatement": {
"propertyName": "DEFAULT_TRANSACTION_READ_ONLY",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,11 @@ public void testDefaultTransactionReadOnlyTrue() {
"set default_transaction_read_only to tru",
"set default_transaction_read_only to 'tr'",
"set default_transaction_read_only to \"t\"",
"set default_transaction_read_only = on",
"set default_transaction_read_only = 1",
"set default_transaction_read_only = yes",
"set default_transaction_read_only = ye",
"set default_transaction_read_only = y",
};

for (String sql : statements) {
Expand Down Expand Up @@ -204,6 +209,11 @@ public void testDefaultTransactionReadOnlyFalse() {
"set default_transaction_read_only to fal",
"set default_transaction_read_only to 'fa'",
"set default_transaction_read_only to \"f\"",
"set default_transaction_read_only = off",
"set default_transaction_read_only = of",
"set default_transaction_read_only = 0",
"set default_transaction_read_only = no",
"set default_transaction_read_only = n",
};

for (String sql : statements) {
Expand Down
Loading