refactor: remove redundant or unused codes (#407)

Author: supercaracal

lib/redis_client/cluster/command.rb

@@ -17,7 +17,6 @@ class Command
       Detail = Struct.new(
         'RedisCommand',
         :first_key_position,
-        :last_key_position,
         :key_step,
         :write?,
         :readonly?,
@@ -54,7 +53,6 @@ def parse_command_reply(rows)
 
             acc[row[0].downcase] = ::RedisClient::Cluster::Command::Detail.new(
               first_key_position: row[3],
-              last_key_position: row[4],
               key_step: row[5],
               write?: row[2].include?('write'),
               readonly?: row[2].include?('readonly')
@@ -71,18 +69,7 @@ def extract_first_key(command)
         i = determine_first_key_position(command)
         return EMPTY_STRING if i == 0
 
-        (command[i].is_a?(Array) ? command[i].flatten.first : command[i]).to_s
-      end
-
-      def extract_all_keys(command)
-        keys_start = determine_first_key_position(command)
-        keys_end = determine_last_key_position(command, keys_start)
-        keys_step = determine_key_step(command)
-        return EMPTY_ARRAY if [keys_start, keys_end, keys_step].any?(&:zero?)
-
-        keys_end = [keys_end, command.size - 1].min
-        # use .. inclusive range because keys_end is a valid index.
-        (keys_start..keys_end).step(keys_step).map { |i| command[i] }
+        command[i]
       end
 
       def should_send_to_primary?(command)
@@ -116,45 +103,10 @@ def determine_first_key_position(command) # rubocop:disable Metrics/CyclomaticCo
         end
       end
 
-      # IMPORTANT: this determines the last key position INCLUSIVE of the last key -
-      # i.e. command[determine_last_key_position(command)] is a key.
-      # This is in line with what Redis returns from COMMANDS.
-      def determine_last_key_position(command, keys_start) # rubocop:disable Metrics/AbcSize
-        case name = ::RedisClient::Cluster::NormalizedCmdName.instance.get_by_command(command)
-        when 'eval', 'evalsha', 'zinterstore', 'zunionstore'
-          # EVALSHA sha1 numkeys [key [key ...]] [arg [arg ...]]
-          # ZINTERSTORE destination numkeys key [key ...] [WEIGHTS weight [weight ...]] [AGGREGATE <SUM | MIN | MAX>]
-          command[2].to_i + 2
-        when 'object', 'memory'
-          # OBJECT [ENCODING | FREQ | IDLETIME | REFCOUNT] key
-          # MEMORY USAGE key [SAMPLES count]
-          keys_start
-        when 'migrate'
-          # MIGRATE host port <key | ""> destination-db timeout [COPY] [REPLACE] [AUTH password | AUTH2 username password] [KEYS key [key ...]]
-          command[3].empty? ? (command.length - 1) : 3
-        when 'xread', 'xreadgroup'
-          # XREAD [COUNT count] [BLOCK milliseconds] STREAMS key [key ...] id [id ...]
-          keys_start + ((command.length - keys_start) / 2) - 1
-        else
-          # If there is a fixed, non-variable number of keys, don't iterate past that.
-          if @commands[name].last_key_position >= 0
-            @commands[name].last_key_position
-          else
-            command.length + @commands[name].last_key_position
-          end
-        end
-      end
-
-      def determine_optional_key_position(command, option_name) # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
-        idx = command&.flatten&.map(&:to_s)&.map(&:downcase)&.index(option_name&.downcase)
+      def determine_optional_key_position(command, option_name)
+        idx = command.map { |e| e.to_s.downcase }.index(option_name&.downcase)
         idx.nil? ? 0 : idx + 1
       end
-
-      def determine_key_step(command)
-        name = ::RedisClient::Cluster::NormalizedCmdName.instance.get_by_command(command)
-        # Some commands like EVALSHA have zero as the step in COMMANDS somehow.
-        @commands[name].key_step == 0 ? 1 : @commands[name].key_step
-      end
     end
   end
 end

lib/redis_client/cluster/errors.rb

@@ -34,11 +34,14 @@ def self.from_command(command)
     end
 
     class ErrorCollection < Error
+      EMPTY_HASH = {}.freeze
+
+      private_constant :EMPTY_HASH
       attr_reader :errors
 
       def self.with_errors(errors)
         if !errors.is_a?(Hash) || errors.empty?
-          new(errors.to_s).with_errors({})
+          new(errors.to_s).with_errors(EMPTY_HASH)
         else
           messages = errors.map { |node_key, error| "#{node_key}: (#{error.class}) #{error.message}" }
           new(messages.join(', ')).with_errors(errors)

lib/redis_client/cluster/router.rb

@@ -367,17 +367,20 @@ def send_watch_command(command)
         end
       end
 
-      MULTIPLE_KEYS_COMMAND_TO_SINGLE = {
-        'mget' => ['get', 1].freeze,
-        'mset' => ['set', 2].freeze,
-        'del' => ['del', 1].freeze
-      }.freeze
-
-      private_constant :MULTIPLE_KEYS_COMMAND_TO_SINGLE
-
       def send_multiple_keys_command(cmd, method, command, args, &block) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
         # This implementation is prioritized performance rather than readability or so.
-        single_key_cmd, keys_step = MULTIPLE_KEYS_COMMAND_TO_SINGLE.fetch(cmd)
+        case cmd
+        when 'mget'
+          single_key_cmd = 'get'
+          keys_step = 1
+        when 'mset'
+          single_key_cmd = 'set'
+          keys_step = 2
+        when 'del'
+          single_key_cmd = 'del'
+          keys_step = 1
+        else raise NotImplementedError, cmd
+        end
 
         return try_send(assign_node(command), method, command, args, &block) if command.size <= keys_step + 1 || ::RedisClient::Cluster::KeySlotConverter.hash_tag_included?(command[1])
 

test/redis_client/cluster/test_command.rb

@@ -53,16 +53,16 @@ def test_parse_command_reply
               ['set', -3, Set['write', 'denyoom', 'movablekeys'], 1, -1, 2, Set['@write', '@string', '@slow'], Set[], Set[], Set[]]
             ],
             want: {
-              'get' => { first_key_position: 1, last_key_position: -1, key_step: 1, write?: false, readonly?: true },
-              'set' => { first_key_position: 1, last_key_position: -1, key_step: 2, write?: true, readonly?: false }
+              'get' => { first_key_position: 1, key_step: 1, write?: false, readonly?: true },
+              'set' => { first_key_position: 1, key_step: 2, write?: true, readonly?: false }
             }
           },
           {
             rows: [
               ['GET', 2, Set['readonly', 'fast'], 1, -1, 1, Set['@read', '@string', '@fast'], Set[], Set[], Set[]]
             ],
             want: {
-              'get' => { first_key_position: 1, last_key_position: -1, key_step: 1, write?: false, readonly?: true }
+              'get' => { first_key_position: 1, key_step: 1, write?: false, readonly?: true }
             }
           },
           { rows: [[]], want: {} },
@@ -87,8 +87,6 @@ def test_extract_first_key
           { command: %w[GET foo{bar}baz], want: 'foo{bar}baz' },
           { command: %w[MGET foo bar baz], want: 'foo' },
           { command: %w[UNKNOWN foo bar], want: '' },
-          { command: [['GET'], 'foo'], want: 'foo' },
-          { command: ['GET', ['foo']], want: 'foo' },
           { command: [], want: '' },
           { command: nil, want: '' }
         ].each_with_index do |c, idx|
@@ -151,7 +149,6 @@ def test_determine_first_key_position
         cmd = ::RedisClient::Cluster::Command.load(@raw_clients)
         [
           { command: %w[EVAL "return ARGV[1]" 0 hello], want: 3 },
-          { command: [['EVAL'], '"return ARGV[1]"', 0, 'hello'], want: 3 },
           { command: %w[EVALSHA sha1 2 foo bar baz zap], want: 3 },
           { command: %w[MIGRATE host port key 0 5 COPY], want: 3 },
           { command: ['MIGRATE', 'host', 'port', '', '0', '5', 'COPY', 'KEYS', 'key'], want: 8 },
@@ -164,7 +161,7 @@ def test_determine_first_key_position
           { command: %w[XREADGROUP GROUP group consumer STREAMS key id], want: 5 },
           { command: %w[SET foo 1], want: 1 },
           { command: %w[set foo 1], want: 1 },
-          { command: [['SET'], 'foo', 1], want: 1 },
+          { command: ['SET', 'foo', 1], want: 1 },
           { command: %w[GET foo], want: 1 }
         ].each_with_index do |c, idx|
           msg = "Case: #{idx}"
@@ -179,62 +176,16 @@ def test_determine_optional_key_position
           { params: { command: %w[XREAD COUNT 2 STREAMS mystream writers 0-0 0-0], option_name: 'streams' }, want: 4 },
           { params: { command: %w[XREADGROUP GROUP group consumer STREAMS key id], option_name: 'streams' }, want: 5 },
           { params: { command: %w[GET foo], option_name: 'bar' }, want: 0 },
-          { params: { command: ['FOO', ['BAR'], 'BAZ'], option_name: 'bar' }, want: 2 },
+          { params: { command: %w[FOO BAR BAZ], option_name: 'bar' }, want: 2 },
           { params: { command: %w[FOO BAR BAZ], option_name: 'BAR' }, want: 2 },
           { params: { command: [], option_name: nil }, want: 0 },
-          { params: { command: [], option_name: '' }, want: 0 },
-          { params: { command: nil, option_name: nil }, want: 0 }
+          { params: { command: [], option_name: '' }, want: 0 }
         ].each_with_index do |c, idx|
           msg = "Case: #{idx}"
           got = cmd.send(:determine_optional_key_position, c[:params][:command], c[:params][:option_name])
           assert_equal(c[:want], got, msg)
         end
       end
-
-      def test_determine_key_step
-        cmd = ::RedisClient::Cluster::Command.load(@raw_clients)
-        [
-          { name: 'MSET', want: 2 },
-          { name: 'MGET', want: 1 },
-          { name: 'DEL', want: 1 },
-          { name: 'EVALSHA', want: 1 }
-        ].each_with_index do |c, idx|
-          msg = "Case: #{idx}"
-          got = cmd.send(:determine_key_step, c[:name])
-          assert_equal(c[:want], got, msg)
-        end
-      end
-
-      def test_extract_all_keys
-        cmd = ::RedisClient::Cluster::Command.load(@raw_clients)
-        [
-          { command: ['EVAL', 'return ARGV[1]', '0', 'hello'], want: [] },
-          { command: ['EVAL', 'return ARGV[1]', '3', 'key1', 'key2', 'key3', 'arg1', 'arg2'], want: %w[key1 key2 key3] },
-          { command: [['EVAL'], '"return ARGV[1]"', 0, 'hello'], want: [] },
-          { command: %w[EVALSHA sha1 2 foo bar baz zap], want: %w[foo bar] },
-          { command: %w[MIGRATE host port key 0 5 COPY], want: %w[key] },
-          { command: ['MIGRATE', 'host', 'port', '', '0', '5', 'COPY', 'KEYS', 'key1'], want: %w[key1] },
-          { command: ['MIGRATE', 'host', 'port', '', '0', '5', 'COPY', 'KEYS', 'key1', 'key2'], want: %w[key1 key2] },
-          { command: %w[ZINTERSTORE out 2 zset1 zset2 WEIGHTS 2 3], want: %w[zset1 zset2] },
-          { command: %w[ZUNIONSTORE out 2 zset1 zset2 WEIGHTS 2 3], want: %w[zset1 zset2] },
-          { command: %w[OBJECT HELP], want: [] },
-          { command: %w[MEMORY HELP], want: [] },
-          { command: %w[MEMORY USAGE key], want: %w[key] },
-          { command: %w[XREAD COUNT 2 STREAMS mystream writers 0-0 0-0], want: %w[mystream writers] },
-          { command: %w[XREADGROUP GROUP group consumer STREAMS key id], want: %w[key] },
-          { command: %w[SET foo 1], want: %w[foo] },
-          { command: %w[set foo 1], want: %w[foo] },
-          { command: [['SET'], 'foo', 1], want: %w[foo] },
-          { command: %w[GET foo], want: %w[foo] },
-          { command: %w[MGET foo bar baz], want: %w[foo bar baz] },
-          { command: %w[MSET foo val bar val baz val], want: %w[foo bar baz] },
-          { command: %w[BLPOP foo bar 0], want: %w[foo bar] }
-        ].each_with_index do |c, idx|
-          msg = "Case: #{idx}"
-          got = cmd.send(:extract_all_keys, c[:command])
-          assert_equal(c[:want], got, msg)
-        end
-      end
     end
   end
 end