Lockable module uses ActiveRecord-specific code (increment_counters), breaks compatibility with other ORMs

[tbelliard]

Environment

• Ruby any
• Rails any
• Devise 4.6.0+

Current behavior

Following issue #4981 resolved by PR #4996, commit 6270394 introduced a call to an ActiveRecord-specific method (increment_counters).
This call breaks compatibility with alternative ORMs like Sequel that rely on the interface provided by ActiveModel.

Expected behavior

Ideally Devise should remain as ORM-agnostic as possible and rely when possible on ActiveModel methods to interact with model classes. In this case that would probably mean having a (semi-)manual SQL call to get something equivalent to what is implemented in active record : failed_attempts = COALESCE(failed_attempts, 0) + 1 (see https://github.com/rails/rails/blob/32cfb82922bfc905b14d7d025287dac0b85b1668/activerecord/lib/active_record/counter_cache.rb#L63)

Honestly I am not totally sure whether there is a clean generic solution here to reach the desired result... even a semi-manual SQL call needs to rely to fairly low-level features of the ORM. If you already maintain some ORM-specific code in devise then I guess the best course of action might be to have alternative calls depending on whether the model is a ActiveRecord or Sequel or whatever else instance.

As a workaround for now I will implement a sequel-specific increment_counter method in my user model.

[zedtux]

Facing this issue too with the Nobrainer ORM.

[carlosantoniodasilva]

Looks like this was never caught because of the mongo ID support/tests not being there. That's something I plan to bring back, in which case I'll look into how we can change that update to be more agnostic.

Devise always aimed to be somewhat ORM agnostic, one of the reasons it uses orm_adapter:

devise/devise.gemspec

Line 23 in 45b831c

s.add_dependency("orm_adapter", "~> 0.1")

It looks like that's not been updated in a while though, and it doesn't seem to support either of the two ORMs you mentioned above, so I'm not sure how much we'll be able to rely on it going forward... we'll see.

[carlosantoniodasilva]

Actually, I just realized there are adapters for Sequel and Nobrainer available as separate gems:
https://rubygems.org/gems/orm_adapter-sequel
https://rubygems.org/gems/orm_adapter-nobrainer

So maybe it's a matter of making sure orm_adapter supports some sort of "increment counter" feature that each of those can implement, and Devise can use in a more agnostic way? 🤔

[brian-tanabe]

I wasn't able to figure out how to get this to work with Dynamoid either:
https://github.com/Dynamoid/Dynamoid