Merge pull request #1244 from zendesk/ktsanaktsidis/known_subclasses

Handle unknown subclasses of RedisClient::Error in clustering

cluster/lib/redis/cluster/client.rb

@@ -12,7 +12,7 @@ class Client < RedisClient::Cluster
         RedisClient::Cluster::ErrorCollection => Redis::Cluster::CommandErrorCollection,
         RedisClient::Cluster::Transaction::ConsistencyError => Redis::Cluster::TransactionConsistencyError,
         RedisClient::Cluster::NodeMightBeDown => Redis::Cluster::NodeMightBeDown,
-      ).freeze
+      )
 
       class << self
         def config(**kwargs)
@@ -22,6 +22,29 @@ def config(**kwargs)
         def sentinel(**kwargs)
           super(protocol: 2, **kwargs)
         end
+
+        def translate_error!(error, mapping: ERROR_MAPPING)
+          case error
+          when RedisClient::Cluster::ErrorCollection
+            error.errors.each do |_node, node_error|
+              if node_error.is_a?(RedisClient::AuthenticationError)
+                raise mapping.fetch(node_error.class), node_error.message, node_error.backtrace
+              end
+            end
+
+            remapped_node_errors = error.errors.map do |node_key, node_error|
+              remapped = mapping.fetch(node_error.class, node_error.class).new(node_error.message)
+              remapped.set_backtrace node_error.backtrace
+              [node_key, remapped]
+            end.to_h
+
+            raise(Redis::Cluster::CommandErrorCollection.new(remapped_node_errors, error.message).tap do |remapped|
+              remapped.set_backtrace error.backtrace
+            end)
+          else
+            Redis::Client.translate_error!(error, mapping: mapping)
+          end
+        end
       end
 
       def initialize(*)
@@ -79,22 +102,8 @@ def multi(watch: nil, &block)
 
       def handle_errors
         yield
-      rescue RedisClient::Cluster::ErrorCollection => error
-        error.errors.each do |_node, node_error|
-          if node_error.is_a?(RedisClient::AuthenticationError)
-            raise ERROR_MAPPING.fetch(node_error.class), node_error.message, node_error.backtrace
-          end
-        end
-        remapped_node_errors = error.errors.map do |node_key, node_error|
-          remapped = ERROR_MAPPING.fetch(node_error.class, node_error.class).new(node_error.message)
-          remapped.set_backtrace node_error.backtrace
-          [node_key, remapped]
-        end.to_h
-        raise(Redis::Cluster::CommandErrorCollection.new(remapped_node_errors, error.message).tap do |remapped|
-          remapped.set_backtrace error.backtrace
-        end)
       rescue ::RedisClient::Error => error
-        raise ERROR_MAPPING.fetch(error.class), error.message, error.backtrace
+        Redis::Cluster::Client.translate_error!(error)
       end
     end
   end

cluster/test/client_pipelining_test.rb

@@ -58,4 +58,22 @@ def test_pipelining_without_hash_tags
     end
     assert_equal 1.upto(6).map(&:to_s), result
   end
+
+  def test_pipeline_unmapped_errors_are_bubbled_up
+    ex = Class.new(StandardError)
+    assert_raises(ex) do
+      redis.pipelined do |_pipe|
+        raise ex, "boom"
+      end
+    end
+  end
+
+  def test_pipeline_error_subclasses_are_mapped
+    ex = Class.new(RedisClient::ConnectionError)
+    assert_raises(Redis::ConnectionError) do
+      redis.pipelined do |_pipe|
+        raise ex, "tick tock"
+      end
+    end
+  end
 end

lib/redis/client.rb

@@ -27,18 +27,18 @@ def sentinel(**kwargs)
         super(protocol: 2, **kwargs, client_implementation: ::RedisClient)
       end
 
-      def translate_error!(error)
-        redis_error = translate_error_class(error.class)
+      def translate_error!(error, mapping: ERROR_MAPPING)
+        redis_error = translate_error_class(error.class, mapping: mapping)
         raise redis_error, error.message, error.backtrace
       end
 
       private
 
-      def translate_error_class(error_class)
-        ERROR_MAPPING.fetch(error_class)
+      def translate_error_class(error_class, mapping: ERROR_MAPPING)
+        mapping.fetch(error_class)
       rescue IndexError
-        if (client_error = error_class.ancestors.find { |a| ERROR_MAPPING[a] })
-          ERROR_MAPPING[error_class] = ERROR_MAPPING[client_error]
+        if (client_error = error_class.ancestors.find { |a| mapping[a] })
+          mapping[error_class] = mapping[client_error]
         else
           raise
         end