Proposal: a way to handle multi-DB ActiveRecord setups

[geoffharcourt]

Rails 6 provides connection switching to allow for applications with multiple databases. One of the supported behaviors is a primary/replica setup where the replica database is read-only. Devise users will run into some issues with the default Rails 6 connection switching, as it restricts writes to non-GET/HEAD/OPTIONS requests.

As a result, some requests will trigger ActiveRecord::ReadOnlyError as a result of hooks firing, etc. Some of the Devise methods can be worked around directly in models like this:

# app/models/user.rb
def forget_me!
  ActiveRecord::Base.connected_to(role: :writing) do
    super
  end
end

Unfortunately some of the cases where writes inside a GET request don't seem quite as straightforward to work around. Some of the Warden after_sign_in hooks call #save directly on the model. Since Warden hooks are additive, it's not easy to replace the hook with a monkey-patched one. We're currently experiencing some failures for lockable that we (believe we) are not able to code around without forking Devise. Example:

devise/lib/devise/hooks/lockable.rb

Lines 5 to 12 in f39c6fd

	Warden::Manager.after_set_user except: :fetch do |record, warden, options|
	if record.respond_to?(:failed_attempts) && warden.authenticated?(options[:scope])
	unless record.failed_attempts.to_i.zero?
	record.failed_attempts = 0
	record.save(validate: false)
	end
	end
	end

I'd like to propose a pull request to make the Rails 6 primary/replica functionality work without requiring any changes for users who are on older versions of Rails or who aren't leveraging ActiveRecord's connected_to functionality. There are two ways I can see to approach this:

1. Require users to wrap their model methods in a connected_to clause around super as described above. hooks/lockable would need to be updated to push its call to #save to a model method which could then be wrapped. Then we would document that this is the recommended approach for users leveraging primary/replica setups. This would be the smallest change, but would be less convenient for multiple-database users in Rails 6.

2. Add a new configuration option, something named like devise_database_role. If that role is set, then all model saves in model modules or hooks would get wrapped in a connected_to block using that role. If the setting is not defined, then all saves happen directly as now. This approach would be more comprehensive and be a one-stop change for primary/replica setups vs. needing to patch each relevant method, but requires more invasive changes across several model modules.

I'm excited to tackle either of these changes, but looking for feedback on the preferred approach.

Related: #5133

[rafaelfranca]

I think the solution 2 makes sense.

But devise is ORM agnostic, so I think we should probably take care of those connection switching methods should be in the active record specific adapter.

[geoffharcourt]

I suppose we could make the configuration option a Proc or lambda that would wrap all the saves, then folks could do anything they needed to for any ORM inside the block.

[trevorrjohn]

I suppose we could make the configuration option a Proc or lambda that would wrap all the saves, then folks could do anything they needed to for any ORM inside the block.

@geoffharcourt have you made progress on this? Happy to help as well...

[geoffharcourt]

I have a working change with a configurable proc (default is current behavior and existing tests pass), but I can't find any existing hooks tests and I'm not 100% sure how to build a test harness for those.

[trevorrjohn]

I only have bad ideas about that like inspecting Warder::Manager._after_set_user. Do you have a link to the branch?

[geoffharcourt]

Hi @trevorrjohn take a peek here: #5310

[geoffharcourt]

This change has been accomplished via 5d5636f. Thanks @carlosantoniodasilva!

[GuillaumeGillet]

Hi, facing the same issue, I struggle to understand how this patch have solved the multi-db issue. Is there any example or doc I can refer?

[trevorrjohn]

So nothing out of the box just works. I have a method in my user that takes a block and connects to the primary db (for writes) which I use, but mainly I wrote an around action which also connects to the primary db and use that in controllers that write to the db on GET requests.

…

On Thu, Aug 12, 2021, 5:13 AM GuillaumeGillet ***@***.***> wrote: Hi, facing the same issue, I struggle to understand how this patch have solved the multi-db issue. Is there any example or doc I can refer? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5264 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFEQLQ54QAZWCSLTVZ3IN3T4OGENANCNFSM4OR5B63A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[carlosantoniodasilva]

That's right, at the moment Devise doesn't have a way to automatically switch to a write connection (I have some ideas for that), so you have to override these hooks in your model and switch it yourself to the connection you want, then call super, otherwise it's just gonna use the current connection which might be a readonly one.

[GuillaumeGillet]

So based on what I read, I end up doing in my user model :

  def update_tracked_fields!(request)
    User.connected_to(role: :writing) do
      super
    end
  end

  def update_tracked_fields(request)
    User.connected_to(role: :writing) do
      super
    end
  end

  def remember_me!
    User.connected_to(role: :writing) do
      super
    end
  end

  def forget_me!
    User.connected_to(role: :writing) do
      super
    end
  end

So, so far, there is no global hook to do that. Am I right?

[trevorrjohn]

Yes, this is what I have too. Unless devise has added something in the past few months I don't believe there is another way.

…

On Thu, Aug 12, 2021, 7:51 AM GuillaumeGillet ***@***.***> wrote: So based on what I read, I end up doing in my user model : def update_tracked_fields!(request) User.connected_to(role: :writing) do super end end def update_tracked_fields(request) User.connected_to(role: :writing) do super end end def remember_me! User.connected_to(role: :writing) do super end end def forget_me! User.connected_to(role: :writing) do super end end So, so far, there is no global hook to do that. Am I right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5264 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFEQLVOOXLXEHIOHYMMMRLT4OYTXANCNFSM4OR5B63A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[GuillaumeGillet]

Ok, thx, I thought I was missing something.