nateware / redis-objects

Map Redis types directly to Ruby objects
Artistic License 2.0
2.09k stars 229 forks source link

The `lock` method conflicts with ActiveRecord::Querying#lock #196

Closed UniIsland closed 2 years ago

UniIsland commented 8 years ago

Class method lock defined in Redis::Objects::Locks::ClassMethods conflicts with lock in ActiveRecord::Querying#lock, which makes it impossible to enable ActiveRecord Pessimistic Locking by Account.lock.find(1).

We can still chain the method like this Account.wherer(id:1).lock.take. but that is dirty and better be avoided.

This issue could be part of #153 .

nateware commented 7 years ago

This is a tough one, because the design philosophy of Redis::Objects is that database locking is a terrible idea. On the other hand, I can see the argument that there could be edge cases where you would want to use ActiveRecord locking. I don't have a strong opinion other than I'm worried at this point that it would break backwards compatibility. I would be ok with changing this to something like rlock or redis_lock if we bumped the version to 2.0

khiav223577 commented 6 years ago

It'll also break with_lock method if you include Redis::Objects in an ActiveRecord EX:

class User < ActiveRecord::Base
  include Redis::Objects
  counter :your_counter
end

I solved the problem by moving it to a service object

class CounterService
  include Redis::Objects
  attr_reader :id # for redis

  def initialize(id)
    @id = id
  end

  counter :your_counter
end
class User < ActiveRecord::Base
  def your_counter
    CounterService.new(id).your_counter
  end
end
nateware commented 4 years ago

CC @tmsrjs - another issue related to lock that may be worth looking at in version 2.0.0

chbonser commented 2 years ago

@nateware I think this issue needs a fresh look. In particular, the fact that this collision causes Active Record's with_lock to silently fail to acquire a lock is super surprising and very hard to detect.

At a minimum, while this method name collision exists, I recommend updating the README to discourage mixing Redis::Objects into ActiveRecord models. @khiav223577's pattern plus adding method delegators is a fine workaround to recommend instead.

nateware commented 2 years ago

What is the consensus on this issues and the similar #191? I'm ok at this point if we want to rename the method to redis_lock or rlock (or similar) and bump the version to 2.0.0

nateware commented 2 years ago

This has been fixed in 2.0.0.alpha and pushed to rubygems