Devise: 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:

https://github.com/heartcombo/devise/blob/f39c6fd92774cb66f96f546d8d5e8281542b4e78/lib/devise/hooks/lockable.rb#L5-L12

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: https://github.com/heartcombo/devise/pull/5310

[geoffharcourt]

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