-
Notifications
You must be signed in to change notification settings - Fork 16
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
Set DB transactions to read-only and provide messaging for non-read operations #21
Conversation
module PostgreSQLAdapter | ||
def begin_db_transaction | ||
super | ||
execute 'SET TRANSACTION 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.
What happens if someone tries to select for update in this mode?
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.
PG::ReadOnlySqlTransaction: ERROR: cannot execute SELECT FOR UPDATE in a read-only transaction
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.
Would that behavior break certain code-paths?
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.
Cool just wanted to ensure this covers locking in read statements also. I'm not aware of anything that would break.
Do you typically use this locally to manually verify your changes? If you run a couple commands that way and everything behaves properly I think that's enough code-path validation.
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.
Yep, I manually verify locally for in-house Salsify apps.
I could build out the feature tests here and have fancier rails fixtures, but it'd be extremely difficult to capture all the various use-cases that this should be catching. Would be a nice-to-have though.
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 fixes #18 and also fixes #19 , where transactions are always read-only in sandbox (read-only) mode. Clearer messaging is added when a user tries to make a non-read operation.
prime: @cjf2xn