trilogy-libraries / activerecord-trilogy-adapter

Active Record adapter for the Trilogy database client for Rails v6.0 - v7.0.
https://github.com/trilogy-libraries/trilogy
MIT License
167 stars 16 forks source link

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

Open DaemonSpelledWrong opened 2 months ago

DaemonSpelledWrong commented 2 months ago

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 commented 1 month ago

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 https://github.com/trilogy-libraries/activerecord-trilogy-adapter/blob/93f5a0c3f697f3845ec1cd41738a3cd08b96c9f6/lib/active_record/connection_adapters/trilogy/database_statements.rb#L15-L22 (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: https://github.com/trilogy-libraries/activerecord-trilogy-adapter/blob/93f5a0c3f697f3845ec1cd41738a3cd08b96c9f6/lib/active_record/connection_adapters/trilogy/database_statements.rb#L98 instead of writing an empty method. The implementation would look similar to what's in https://github.com/rails/rails/commit/8d987c83345aafb02ab7bb9a2231d44529bb76f3.