Merge pull request #1061 from casperisfine/multi-deprecation

Deprecate calling commands on the original Redis instance in multi (block)

CHANGELOG.md

@@ -15,6 +15,20 @@
     pipeline.get("key")
   end
   ```
+* Deprecate calling commands on `Redis` inside `Redis#multi`. See #1059.
+  ```ruby
+  redis.multi do
+    redis.get("key")
+  end
+  ```
+
+  should be replaced by:
+
+  ```ruby
+  redis.multi do |transaction|
+    transaction.get("key")
+  end
+  ```
 * Deprecate `Redis#queue` and `Redis#commit`. See #1059.
 
 * Fix `zpopmax` and `zpopmin` when called inside a pipeline. See #1055.

README.md

@@ -200,9 +200,9 @@ the regular pipeline, the replies to the commands are returned by the
 `#multi` method.
 
 ```ruby
-redis.multi do
-  redis.set "foo", "bar"
-  redis.incr "baz"
+redis.multi do |transaction|
+  transaction.set "foo", "bar"
+  transaction.incr "baz"
 end
 # => ["OK", 1]
 ```

lib/redis.rb

@@ -107,7 +107,7 @@ def close
   end
   alias disconnect! close
 
-  # Queues a command for pipelining.
+  # @deprecated Queues a command for pipelining.
   #
   # Commands in the queue are executed with the Redis#commit method.
   #
@@ -124,7 +124,7 @@ def queue(*command)
     end
   end
 
-  # Sends all commands in the queue.
+  # @deprecated Sends all commands in the queue.
   #
   # See http://redis.io/topics/pipelining for more details.
   #
@@ -152,71 +152,10 @@ def _client
     @client
   end
 
-  # Watch the given keys to determine execution of the MULTI/EXEC block.
-  #
-  # Using a block is optional, but is necessary for thread-safety.
-  #
-  # An `#unwatch` is automatically issued if an exception is raised within the
-  # block that is a subclass of StandardError and is not a ConnectionError.
-  #
-  # @example With a block
-  #   redis.watch("key") do
-  #     if redis.get("key") == "some value"
-  #       redis.multi do |multi|
-  #         multi.set("key", "other value")
-  #         multi.incr("counter")
-  #       end
-  #     else
-  #       redis.unwatch
-  #     end
-  #   end
-  #     # => ["OK", 6]
-  #
-  # @example Without a block
-  #   redis.watch("key")
-  #     # => "OK"
-  #
-  # @param [String, Array<String>] keys one or more keys to watch
-  # @return [Object] if using a block, returns the return value of the block
-  # @return [String] if not using a block, returns `OK`
-  #
-  # @see #unwatch
-  # @see #multi
-  def watch(*keys)
-    synchronize do |client|
-      res = client.call([:watch, *keys])
-
-      if block_given?
-        begin
-          yield(self)
-        rescue ConnectionError
-          raise
-        rescue StandardError
-          unwatch
-          raise
-        end
-      else
-        res
-      end
-    end
-  end
-
-  # Forget about all watched keys.
-  #
-  # @return [String] `OK`
-  #
-  # @see #watch
-  # @see #multi
-  def unwatch
-    synchronize do |client|
-      client.call([:unwatch])
-    end
-  end
-
   def pipelined(&block)
     deprecation_displayed = false
     if block&.arity == 0
-      Pipeline.deprecation_warning(Kernel.caller_locations(1, 5))
+      Pipeline.deprecation_warning("pipelined", Kernel.caller_locations(1, 5))
       deprecation_displayed = true
     end
 
@@ -263,19 +202,27 @@ def pipelined(&block)
   #
   # @see #watch
   # @see #unwatch
-  def multi
-    synchronize do |prior_client|
-      if !block_given?
-        prior_client.call([:multi])
-      else
+  def multi(&block)
+    if block_given?
+      deprecation_displayed = false
+      if block&.arity == 0
+        Pipeline.deprecation_warning("multi", Kernel.caller_locations(1, 5))
+        deprecation_displayed = true
+      end
+
+      synchronize do |prior_client|
         begin
-          @client = Pipeline::Multi.new(prior_client)
-          yield(self)
-          prior_client.call_pipeline(@client)
+          pipeline = Pipeline::Multi.new(prior_client)
+          @client = deprecation_displayed ? pipeline : DeprecatedMulti.new(pipeline)
+          pipelined_connection = PipelinedConnection.new(pipeline)
+          yield pipelined_connection
+          prior_client.call_pipeline(pipeline)
         ensure
           @client = prior_client
         end
       end
+    else
+      send_command([:multi])
     end
   end
 

lib/redis/commands/transactions.rb

@@ -3,6 +3,65 @@
 class Redis
   module Commands
     module Transactions
+      # Watch the given keys to determine execution of the MULTI/EXEC block.
+      #
+      # Using a block is optional, but is necessary for thread-safety.
+      #
+      # An `#unwatch` is automatically issued if an exception is raised within the
+      # block that is a subclass of StandardError and is not a ConnectionError.
+      #
+      # @example With a block
+      #   redis.watch("key") do
+      #     if redis.get("key") == "some value"
+      #       redis.multi do |multi|
+      #         multi.set("key", "other value")
+      #         multi.incr("counter")
+      #       end
+      #     else
+      #       redis.unwatch
+      #     end
+      #   end
+      #     # => ["OK", 6]
+      #
+      # @example Without a block
+      #   redis.watch("key")
+      #     # => "OK"
+      #
+      # @param [String, Array<String>] keys one or more keys to watch
+      # @return [Object] if using a block, returns the return value of the block
+      # @return [String] if not using a block, returns `OK`
+      #
+      # @see #unwatch
+      # @see #multi
+      def watch(*keys)
+        synchronize do |client|
+          res = client.call([:watch, *keys])
+
+          if block_given?
+            begin
+              yield(self)
+            rescue ConnectionError
+              raise
+            rescue StandardError
+              unwatch
+              raise
+            end
+          else
+            res
+          end
+        end
+      end
+
+      # Forget about all watched keys.
+      #
+      # @return [String] `OK`
+      #
+      # @see #watch
+      # @see #multi
+      def unwatch
+        send_command([:unwatch])
+      end
+
       # Execute all commands issued after MULTI.
       #
       # Only call this method when `#multi` was called **without** a block.

lib/redis/pipeline.rb

@@ -44,19 +44,19 @@ class Pipeline
     STDLIB_PATH = File.expand_path("..", MonitorMixin.instance_method(:synchronize).source_location.first).freeze
 
     class << self
-      def deprecation_warning(caller_locations) # :nodoc:
+      def deprecation_warning(method, caller_locations) # :nodoc:
         callsite = caller_locations.find { |l| !l.path.start_with?(REDIS_INTERNAL_PATH, STDLIB_PATH) }
         callsite ||= caller_locations.last # The caller_locations should be large enough, but just in case.
         ::Redis.deprecate! <<~MESSAGE
           Pipelining commands on a Redis instance is deprecated and will be removed in Redis 5.0.0.
 
-          redis.pipelined do
+          redis.#{method} do
             redis.get("key")
           end
 
           should be replaced by
 
-          redis.pipelined do |pipeline|
+          redis.#{method} do |pipeline|
             pipeline.get("key")
           end
 
@@ -187,15 +187,32 @@ def commands
     end
   end
 
-  DeprecatedPipeline = DelegateClass(Pipeline) do
+  DeprecatedPipeline = DelegateClass(Pipeline)
+  class DeprecatedPipeline
     def initialize(pipeline)
       super(pipeline)
       @deprecation_displayed = false
     end
 
     def __getobj__
       unless @deprecation_displayed
-        Pipeline.deprecation_warning(Kernel.caller_locations(1, 10))
+        Pipeline.deprecation_warning("pipelined", Kernel.caller_locations(1, 10))
+        @deprecation_displayed = true
+      end
+      @delegate_dc_obj
+    end
+  end
+
+  DeprecatedMulti = DelegateClass(Pipeline::Multi)
+  class DeprecatedMulti
+    def initialize(pipeline)
+      super(pipeline)
+      @deprecation_displayed = false
+    end
+
+    def __getobj__
+      unless @deprecation_displayed
+        Pipeline.deprecation_warning("multi", Kernel.caller_locations(1, 10))
         @deprecation_displayed = true
       end
       @delegate_dc_obj