Updating User model with GET requests is not compatible with Rails 6 read/write splitting

[vitobotta]

Environment

• Ruby 2.6.4
• Rails 6.0.0
• Devise 4.7.0

Current behavior

Not sure of where to post this. I am trying to set up read/writing splitting of SQL queries with Rails 6 since it now has support for this thanks to the multiple database features, and am having problems with some Devise actions. Rails' new auto connection switching sends writes to a primary database and reads to replicas and whether or not writes are allowed depends on the HTTP request. If it's a GET request for example, only reads are allowed by default and sent to the replicas, while for POST/PUT/DELETE queries are sent to the master/primary.

In some cases Devise performs writes even with GET requests, for example when user follows the link in the confirmation email which confirms the user using a token. In this case Devise updates confirmed_at and updated_at columns of the user but because it's a GET request, the write is blocked by Rails 6 using the automatic connection switching based on the HTTP verb.

Expected behavior

In theory Devise should follow HTTP semantics and not make changes to data with GET requests, but I am not sure of how to work around this. I am thinking that I could load the confirmation page and then submit a POST request with JavaScript that actually confirms the user, but there are other cases where Devise makes changes with GET requests.

Any help/idea much appreciated.

[colinross]

I don't necessarily want to get into a discussion about the rails 6 read/write stuff, but I don't (personally) think that is it that black and white, especially when dealing with the meta-record like the logged in user, assuming you want to track their actions. I would also argue that we are talking about RESTful semantics, and not HTTP.

Specifically with the confirm request, you can't do a POST from inside an email, at least directly. Also, semantically, it is the show action of the 'confirmation' resource.

That all said, I believe the cleanest way of to overcome this situation would be to always use the write db for devise actions, or maybe just the ones you know have problems. Again, I'm not sure of how granular you can configure the

Alternatively you could override the pertinent methods on the model (pseudo code, I don't know what the final DSL for read/write looked like at the moment being an RC)

# User
def confirm
  User.connected_to(hander: :writing) do
    super
  end
end

I don't think this is a bug with devise, at a certain point there is a difference between 'written for rails' and 'written to support every possible configuration of rails'.

It is clearly a personal opinion, but I would prefer devise to be a solid, tested, and customizable 90% solution rather than muck it up with large amounts of functionality not required by 95+% of installations.

[vitobotta]

Hi @colinross , in the meantime I ended up doing something similar with an around filter for all the Devise actions just to be sure:

  around_action :ensure_primary_database, if: :devise_controller?
...
    def ensure_primary_database
      ActiveRecord::Base.connected_to(role: :writing) do
        ActiveRecord::Base.connection_handler.while_preventing_writes(false) do
          yield
        end
      end
    end

This solved my original problem, so I had forgotten about it. I agree with what you said about Devise, perhaps it's a non-issue so I'm closing. Thank you for your reply.

[colinross]

Sounds good, although for the next person looking at this, I think that your filter overmatches a bit. It seems would prevent using the read replica even for application-level defined controller actions (in the case that you are extending a devise controller). YYMV

[vitobotta]

OK, I can use that thing only where needed. Apart from the login and confirmation actions, is there anywhere else Devise writes with GET requests?

[colinross]

I wouldn't' want to speak authoritatively on that, as it depends on what modules you have enabled and your configuration. Is there a specific exception that gets thrown when this is attempted that you/we could rescue/track?

…

On Mon, Sep 23, 2019 at 12:09 PM Vito Botta ***@***.***> wrote: OK, I can use that thing only where needed. Apart from the login and confirmation actions, is there anywhere else Devise writes with GET requests? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5133>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABNFT3XYUYE2T7BINIQZYLQLEH5DANCNFSM4ITVSEHQ> .

[vitobotta]

It complains that it can't write to the database while in read only mode, because the GET request forces the usage of the replicas. Luckily Rails prevents writes from going to the replicas :)

[colinross]

Just spitballing here, but could you run the test suite under the same type of read/write configuration?

…

On Mon, Sep 23, 2019 at 12:25 PM Vito Botta ***@***.***> wrote: It complains that it can't write to the database while in read only mode, because the GET request forces the usage of the replicas. Luckily Rails prevents writes from going to the replicas :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5133>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABNFTYT5YZXDPWBBMD2DBDQLEJ3NANCNFSM4ITVSEHQ> .

[geoffharcourt]

@colinross it's extremely difficult to test read/write behavior in a typical Rails test suite because any suite that uses transactions to wrap all the tests won't use a replica database (I think to avoid sync issues). We've had a lot of trouble trying to write application-level tests that verify reader or writer use authoritatively.

[nodunayo]

I found this thread last night after having the same issue with Devise.

I realised that the trackable module was causing the ActiveRecord::ReadOnlyError given this was the query that was being attempted:

Write query attempted while in readonly mode: UPDATE "users" SET "current_sign_in_at" = $1, "last_sign_in_at" = $2, "current_sign_in_ip" = $3, "last_sign_in_ip" = $4, "sign_in_count" = $5, "updated_at" = $6 WHERE "users"."id" = $7

While @vitobotta's general solution above did not work for me, I managed to solve the problem by forcing the write connection in the following methods:

class User < ApplicationRecord
  devise :database_authenticatable, :registerable, :recoverable, :rememberable, :trackable, :validatable

...
    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
...
end

I suspect the second patch isn't needed as update_tracked_fields always seems to be called within update_tracked_fields!, but I'll leave it there for now. 😅

[geoffharcourt]

@nodunayo we did something similar, but also patched #remember_me! and #forget_me!.

[geoffharcourt]

Running into an issue with this now with the after_sign_in Warden callback in lockable. I can't override it because callbacks are additive. I'm wondering if the best long-term solution here would be to have an optional symbol you could specify in the Devise config, and if present, all of these methods would wrap (including the hooks).

[nodunayo]

@geoffharcourt A few days ago I decommissioned my readonly follower DB, so I no longer have this issue at all.

Hope you manage to solve this for the long-term. I suspect we will eventually set up a follower again.