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

Mysql2 -> Trilogy: ActiveRecord::ReadOnlyError replaced with ActiveRecord::StatementInvalid #72

Open
DaemonSpelledWrong opened this issue May 1, 2024 · 1 comment

Comments

@DaemonSpelledWrong
Copy link

DaemonSpelledWrong commented May 1, 2024

I recently swapped my company's database adapter over from mysql2 to trilogy and noticed that we were no longer rescuing ActiveRecord::ReadOnlyError exceptions because we started receiving ActiveRecord::StatementInvalid instead. Is it intended that Trilogy overwrites ActiveRecord::ReadOnlyError with ActiveRecord::StatementInvalid and, if so, may I ask why?

Further context:
My company uses Doorkeeper for Oauth2 in our Rails app. Doorkeeper utilizes oauth application credentials to create Doorkeeper::AccessToken objects with a token value users/APIs use when making requests to our own API. These token objects have a previous_refresh_token field that is "" by default and becomes populated with the previously used refresh token when you refresh your token.

On any given request, Doorkeeper will reset the previous_refresh_token column to "" if it is not currently "". So if you make a GET request with a token that was refreshed Doorkeeper will update the token and cause a write to the DB. This becomes a problem when all GET requests get routed to a reader connection. Previously we were capturing this case by rescuing ActiveRecord::ReadOnlyError but that changed and we were no longer handling these exceptions. I fixed it by additionally rescuing ActiveRecord::StatementInvalid but could've also fixed by simply executing all doorkeeper_token calls within a writer connection. However I am curious why this changed to begin with.

Edit: for more context, we're using Rails 6.1; not Rails 7+.

@composerinteralia
Copy link
Contributor

composerinteralia commented May 8, 2024

I don't think it's intentional. In Rails 6.1, the mysql2 adapter uses this #execute method to execute queries: https://github.com/rails/rails/blob/v6.1.0/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb#L41-L51 (which checks for readonly mode and raises the ReadOnlyError if needed), whereas this gem uses

def execute(sql, name = nil, **kwargs)
sql = transform_query(sql)
check_if_write_query(sql)
mark_transaction_written_if_write(sql)
result = raw_execute(sql, name, **kwargs)
ActiveRecord::Result.new(result.fields, result.to_a)
end
(which doesn't include the check at least on Rails 6.1).

So we might might need to add a similar readonly check to this gem to get the behavior to match. The easiest way to do that is probably to implement this method:

instead of writing an empty method. The implementation would look similar to what's in rails/rails@8d987c8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants