Add some test cases (#16)

supercaracal · web-flow · commit f3c46b2fd0cb · 2022-06-14T16:38:23.000+09:00
diff --git a/README.md b/README.md
@@ -7,7 +7,7 @@ Redis Cluster Client
 TODO
 
 ```ruby
-cli = RedisClient.cluster(nodes: %w[redis://127.0.0.1:7000]).new_client
+cli = RedisClient.cluster(nodes: %w[redis://127.0.0.1:6379]).new_client
 
 cli.call('PING')
 #=> PONG
diff --git a/Rakefile b/Rakefile
@@ -7,14 +7,15 @@ task default: :test
 Rake::TestTask.new :test do |t|
   t.libs << :test
   t.libs << :lib
-  t.test_files = Dir['test/**/test_*.rb']
+  t.test_files = ARGV.size == 1 ? Dir['test/**/test_*.rb'] : ARGV[1..]
+  t.options = '-v'
 end
 
 desc 'Wait for cluster to be ready'
 task :wait do
   $LOAD_PATH.unshift(File.expand_path('test', __dir__))
   require 'redis_client/cluster/controller'
-  nodes = (7000..7005).map { |port| "#{ENV.fetch('REDIS_SCHEME', 'redis')}://127.0.0.1:#{port}" }
+  nodes = (6379..6384).map { |port| "#{ENV.fetch('REDIS_SCHEME', 'redis')}://127.0.0.1:#{port}" }
   ctrl = ::RedisClient::Cluster::Controller.new(nodes)
   ctrl.wait_for_cluster_to_be_ready
 end
diff --git a/docker-compose.yaml b/docker-compose.yaml
@@ -16,27 +16,27 @@ services:
       timeout: "5s"
       retries: 10
     ports:
-      - "7000:6379"
+      - "6379:6379"
   node2:
     <<: *node
     ports:
-      - "7001:6379"
+      - "6380:6379"
   node3:
     <<: *node
     ports:
-      - "7002:6379"
+      - "6381:6379"
   node4:
     <<: *node
     ports:
-      - "7003:6379"
+      - "6382:6379"
   node5:
     <<: *node
     ports:
-      - "7004:6379"
+      - "6383:6379"
   node6:
     <<: *node
     ports:
-      - "7005:6379"
+      - "6384:6379"
   clustering:
     image: "redis:${REDIS_VERSION:-7}"
     command: >
diff --git a/lib/redis_client/cluster_config.rb b/lib/redis_client/cluster_config.rb
@@ -7,23 +7,26 @@
 
 class RedisClient
   class ClusterConfig
+    DEFAULT_HOST = '127.0.0.1'
+    DEFAULT_PORT = 6379
     DEFAULT_SCHEME = 'redis'
     SECURE_SCHEME = 'rediss'
+    DEFAULT_NODES = ["#{DEFAULT_SCHEME}://#{DEFAULT_HOST}:#{DEFAULT_PORT}"].freeze
     VALID_SCHEMES = [DEFAULT_SCHEME, SECURE_SCHEME].freeze
+    VALID_NODES_KEYS = %i[ssl username password host port db].freeze
+    MERGE_CONFIG_KEYS = %i[ssl username password].freeze
+
     InvalidClientConfigError = Class.new(::RedisClient::Error)
 
-    def initialize(nodes:, replica: false, fixed_hostname: nil, **client_config)
-      @replica = replica
-      @fixed_hostname = fixed_hostname
-      @client_config = client_config.dup
+    def initialize(nodes: DEFAULT_NODES, replica: false, fixed_hostname: '', **client_config)
+      @replica = true & replica
+      @fixed_hostname = fixed_hostname.to_s
       @node_configs = build_node_configs(nodes.dup)
-      add_common_node_config_if_needed(@client_config, @node_configs, :ssl)
-      add_common_node_config_if_needed(@client_config, @node_configs, :username)
-      add_common_node_config_if_needed(@client_config, @node_configs, :password)
+      @client_config = merge_generic_config(client_config, @node_configs)
     end
 
     def inspect
-      per_node_key.to_s
+      "#<#{self.class.name}: #{per_node_key.values}>"
     end
 
     def new_pool(size: 5, timeout: 5, **kwargs)
@@ -38,7 +41,7 @@ def per_node_key
       @node_configs.to_h do |config|
         node_key = ::RedisClient::Cluster::NodeKey.build_from_host_port(config[:host], config[:port])
         config = @client_config.merge(config)
-        config = config.merge(host: @fixed_hostname) if @fixed_hostname && !@fixed_hostname.empty?
+        config = config.merge(host: @fixed_hostname) unless @fixed_hostname.empty?
         [node_key, config]
       end
     end
@@ -62,9 +65,10 @@ def dup
     private
 
     def build_node_configs(addrs)
-      raise InvalidClientConfigError, 'Redis option of `cluster` must be an Array' unless addrs.is_a?(Array)
+      configs = Array[addrs].flatten.filter_map { |addr| parse_node_addr(addr) }
+      raise InvalidClientConfigError, '`nodes` option is empty' if configs.size.zero?
 
-      addrs.map { |addr| parse_node_addr(addr) }
+      configs
     end
 
     def parse_node_addr(addr)
@@ -74,41 +78,51 @@ def parse_node_addr(addr)
       when Hash
         parse_node_option(addr)
       else
-        raise InvalidClientConfigError, 'Redis option of `cluster` must includes String or Hash'
+        raise InvalidClientConfigError, "`nodes` option includes invalid type values: #{addr}"
       end
     end
 
-    def parse_node_url(addr) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength
+    def parse_node_url(addr) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength
+      return if addr.empty?
+
       uri = URI(addr)
-      raise InvalidClientConfigError, "Invalid uri scheme #{addr}" unless VALID_SCHEMES.include?(uri.scheme)
+      scheme = uri.scheme || DEFAULT_SCHEME
+      raise InvalidClientConfigError, "`nodes` option includes a invalid uri scheme: #{addr}" unless VALID_SCHEMES.include?(scheme)
 
-      db = uri.path.split('/')[1]&.to_i
       username = uri.user ? URI.decode_www_form_component(uri.user) : nil
       password = uri.password ? URI.decode_www_form_component(uri.password) : nil
+      host = uri.host || DEFAULT_HOST
+      port = uri.port || DEFAULT_PORT
+      db = uri.path.index('/').nil? ? uri.path : uri.path.split('/')[1]
+      db = db.nil? || db.empty? ? db : ensure_integer(db)
 
-      {
-        host: uri.host,
-        port: uri.port,
-        username: username,
-        password: password,
-        db: db,
-        ssl: uri.scheme == SECURE_SCHEME
-      }.reject { |_, v| v.nil? || v == '' }
+      { ssl: scheme == SECURE_SCHEME, username: username, password: password, host: host, port: port, db: db }
+        .reject { |_, v| v.nil? || v == '' || v == false }
     rescue URI::InvalidURIError => e
-      raise InvalidClientConfigError, e.message
+      raise InvalidClientConfigError, "#{e.message}: #{addr}"
     end
 
     def parse_node_option(addr)
+      return if addr.empty?
+
       addr = addr.transform_keys(&:to_sym)
-      raise InvalidClientConfigError, 'Redis option of `cluster` must includes `:host` and `:port` keys' if addr.values_at(:host, :port).any?(&:nil?)
+      addr[:host] ||= DEFAULT_HOST
+      addr[:port] = ensure_integer(addr[:port] || DEFAULT_PORT)
+      addr.select { |k, _| VALID_NODES_KEYS.include?(k) }
+    end
 
-      addr
+    def ensure_integer(value)
+      Integer(value)
+    rescue ArgumentError => e
+      raise InvalidClientConfigError, e.message
     end
 
-    def add_common_node_config_if_needed(client_config, node_configs, key)
-      return client_config if client_config[key].nil? && node_configs.first[key].nil?
+    def merge_generic_config(client_config, node_configs)
+      return client_config if node_configs.size.zero?
 
-      client_config[key] ||= node_configs.first[key]
+      cfg = node_configs.first
+      MERGE_CONFIG_KEYS.each { |k| client_config[k] = cfg[k] if cfg.key?(k) }
+      client_config
     end
   end
 end
diff --git a/test/redis_client/test_cluster_config.rb b/test/redis_client/test_cluster_config.rb
@@ -0,0 +1,150 @@
+# frozen_string_literal: true
+
+require 'testing_helper'
+require 'redis_client/cluster_config'
+
+class RedisClient
+  class TestClusterConfig < Minitest::Test
+    def test_inspect
+      want = '#<RedisClient::ClusterConfig: [{:host=>"127.0.0.1", :port=>6379}]>'
+      got = ::RedisClient::ClusterConfig.new.inspect
+      assert_equal(want, got)
+    end
+
+    def test_new_pool
+      assert_instance_of(::RedisClient::Cluster, ::RedisClient::ClusterConfig.new.new_pool)
+    end
+
+    def test_new_client
+      assert_instance_of(::RedisClient::Cluster, ::RedisClient::ClusterConfig.new.new_client)
+    end
+
+    def test_per_node_key
+      [
+        {
+          config: ::RedisClient::ClusterConfig.new,
+          want: {
+            '127.0.0.1:6379' => { host: '127.0.0.1', port: 6379 }
+          }
+        },
+        {
+          config: ::RedisClient::ClusterConfig.new(replica: true),
+          want: {
+            '127.0.0.1:6379' => { host: '127.0.0.1', port: 6379 }
+          }
+        },
+        {
+          config: ::RedisClient::ClusterConfig.new(fixed_hostname: 'endpoint.example.com'),
+          want: {
+            '127.0.0.1:6379' => { host: 'endpoint.example.com', port: 6379 }
+          }
+        },
+        {
+          config: ::RedisClient::ClusterConfig.new(timeout: 1),
+          want: {
+            '127.0.0.1:6379' => { host: '127.0.0.1', port: 6379, timeout: 1 }
+          }
+        }
+      ].each_with_index do |c, idx|
+        assert_equal(c[:want], c[:config].per_node_key, "Case: #{idx}")
+      end
+    end
+
+    def test_use_replica?
+      assert_predicate(::RedisClient::ClusterConfig.new(replica: true), :use_replica?)
+      refute_predicate(::RedisClient::ClusterConfig.new(replica: false), :use_replica?)
+      refute_predicate(::RedisClient::ClusterConfig.new, :use_replica?)
+    end
+
+    def test_update_node
+      config = ::RedisClient::ClusterConfig.new(nodes: %w[redis://127.0.0.1:6379])
+      assert_equal([{ host: '127.0.0.1', port: 6379 }], config.instance_variable_get(:@node_configs))
+      config.update_node(%w[redis://127.0.0.2:6380])
+      assert_equal([{ host: '127.0.0.2', port: 6380 }], config.instance_variable_get(:@node_configs))
+    end
+
+    def test_add_node
+      config = ::RedisClient::ClusterConfig.new(nodes: %w[redis://127.0.0.1:6379])
+      assert_equal([{ host: '127.0.0.1', port: 6379 }], config.instance_variable_get(:@node_configs))
+      config.add_node('127.0.0.2', 6380)
+      assert_equal([{ host: '127.0.0.1', port: 6379 }, { host: '127.0.0.2', port: 6380 }], config.instance_variable_get(:@node_configs))
+    end
+
+    def test_dup
+      orig = ::RedisClient::ClusterConfig.new
+      copy = orig.dup
+      refute_equal(orig.object_id, copy.object_id)
+    end
+
+    def test_build_node_configs
+      config = ::RedisClient::ClusterConfig.new
+      [
+        { addrs: %w[redis://127.0.0.1], want: [{ host: '127.0.0.1', port: 6379 }] },
+        { addrs: %w[redis://127.0.0.1:6379], want: [{ host: '127.0.0.1', port: 6379 }] },
+        { addrs: %w[redis://127.0.0.1:6379/1], want: [{ host: '127.0.0.1', port: 6379, db: 1 }] },
+        { addrs: %w[redis://127.0.0.1:6379 redis://127.0.0.2:6380], want: [{ host: '127.0.0.1', port: 6379 }, { host: '127.0.0.2', port: 6380 }] },
+        { addrs: %w[rediss://foo:bar@127.0.0.1:6379], want: [{ ssl: true, username: 'foo', password: 'bar', host: '127.0.0.1', port: 6379 }] },
+        { addrs: %w[redis://foo@127.0.0.1:6379], want: [{ host: '127.0.0.1', port: 6379, username: 'foo' }] },
+        { addrs: %w[redis://:bar@127.0.0.1:6379], want: [{ host: '127.0.0.1', port: 6379, password: 'bar' }] },
+        { addrs: [{ host: '127.0.0.1', port: 6379 }], want: [{ host: '127.0.0.1', port: 6379 }] },
+        { addrs: [{ host: '127.0.0.1', port: 6379 }, { host: '127.0.0.2', port: '6380' }], want: [{ host: '127.0.0.1', port: 6379 }, { host: '127.0.0.2', port: 6380 }] },
+        { addrs: [{ host: '127.0.0.1', port: 6379, username: 'foo', password: 'bar', ssl: true }], want: [{ ssl: true, username: 'foo', password: 'bar', host: '127.0.0.1', port: 6379 }] },
+        { addrs: [{ host: '127.0.0.1', port: 6379, db: 1 }], want: [{ host: '127.0.0.1', port: 6379, db: 1 }] },
+        { addrs: 'redis://127.0.0.1:6379', want: [{ host: '127.0.0.1', port: 6379 }] },
+        { addrs: { host: '127.0.0.1', port: 6379 }, want: [{ host: '127.0.0.1', port: 6379 }] },
+        { addrs: [{ host: '127.0.0.1' }], want: [{ host: '127.0.0.1', port: 6379 }] },
+        { addrs: %w[http://127.0.0.1:80], error: ::RedisClient::ClusterConfig::InvalidClientConfigError },
+        { addrs: [{ host: '127.0.0.1', port: 'foo' }], error: ::RedisClient::ClusterConfig::InvalidClientConfigError },
+        { addrs: %w[redis://127.0.0.1:foo], error: ::RedisClient::ClusterConfig::InvalidClientConfigError },
+        { addrs: [6379], error: ::RedisClient::ClusterConfig::InvalidClientConfigError },
+        { addrs: ['foo'], error: ::RedisClient::ClusterConfig::InvalidClientConfigError },
+        { addrs: [''], error: ::RedisClient::ClusterConfig::InvalidClientConfigError },
+        { addrs: [{}], error: ::RedisClient::ClusterConfig::InvalidClientConfigError },
+        { addrs: [], error: ::RedisClient::ClusterConfig::InvalidClientConfigError },
+        { addrs: {}, error: ::RedisClient::ClusterConfig::InvalidClientConfigError },
+        { addrs: '', error: ::RedisClient::ClusterConfig::InvalidClientConfigError },
+        { addrs: nil, error: ::RedisClient::ClusterConfig::InvalidClientConfigError }
+      ].each_with_index do |c, idx|
+        msg = "Case: #{idx}: #{c}"
+        got = -> { config.send(:build_node_configs, c[:addrs]) }
+        if c.key?(:error)
+          assert_raises(c[:error], msg, &got)
+        else
+          assert_equal(c.fetch(:want), got.call, msg)
+        end
+      end
+    end
+
+    def test_merge_generic_config
+      config = ::RedisClient::ClusterConfig.new
+      [
+        {
+          params: {
+            client_config: { ssl: false, username: 'foo', password: 'bar', timeout: 1 },
+            node_configs: [{ ssl: true, username: 'baz', password: 'zap', host: '127.0.0.1' }]
+          },
+          want: { ssl: true, username: 'baz', password: 'zap', timeout: 1 }
+        },
+        {
+          params: {
+            client_config: { ssl: false, timeout: 1 },
+            node_configs: [{ ssl: true, host: '127.0.0.1' }]
+          },
+          want: { ssl: true, timeout: 1 }
+        },
+        {
+          params: {
+            client_config: { timeout: 1 },
+            node_configs: [{ ssl: true }]
+          },
+          want: { ssl: true, timeout: 1 }
+        },
+        { params: { client_config: {}, node_configs: [], keys: [] }, want: {} }
+      ].each_with_index do |c, idx|
+        msg = "Case: #{idx}"
+        got = config.send(:merge_generic_config, c[:params][:client_config], c[:params][:node_configs])
+        assert_equal(c[:want], got, msg)
+      end
+    end
+  end
+end
diff --git a/test/test_redis_cluster_client.rb b/test/test_redis_cluster_client.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+require 'testing_helper'
+require 'redis_cluster_client'
+require 'redis_client/cluster_config'
+
+class TestRedisClient < Minitest::Test
+  def test_cluster
+    [
+      { kwargs: {}, error: nil },
+      { kwargs: { nodes: 'redis://127.0.0.1:6379' }, error: nil },
+      { kwargs: { nodes: { host: '127.0.0.1' } }, error: nil },
+      { kwargs: { nodes: { port: 6379 } }, error: nil },
+      { kwargs: { nodes: { host: '127.0.0.1', port: 6379 } }, error: nil },
+      { kwargs: { nodes: [{ host: '127.0.0.1', port: 6379 }] }, error: nil },
+      { kwargs: { nodes: %w[redis://127.0.0.1:6379] }, error: nil },
+      { kwargs: { nodes: %w[redis://127.0.0.1:6379], replica: true }, error: nil },
+      { kwargs: { nodes: %w[redis://127.0.0.1:6379], replica: true, fixed_hostname: 'endpoint.example.com' }, error: nil },
+      { kwargs: { nodes: %w[redis://127.0.0.1:6379], replica: 1, fixed_hostname: '' }, error: nil },
+      { kwargs: { nodes: %w[redis://127.0.0.1:6379], foo: 'bar' }, error: nil },
+      { kwargs: { nodes: 'http://127.0.0.1:80' }, error: ::RedisClient::ClusterConfig::InvalidClientConfigError },
+      { kwargs: { nodes: [] }, error: ::RedisClient::ClusterConfig::InvalidClientConfigError },
+      { kwargs: { nodes: nil }, error: ::RedisClient::ClusterConfig::InvalidClientConfigError }
+    ].each_with_index do |c, idx|
+      msg = "Case: #{idx}"
+      got = -> { ::RedisClient.cluster(**c[:kwargs]) }
+      if c[:error].nil?
+        assert_instance_of(::RedisClient::ClusterConfig, got.call, msg)
+      else
+        assert_raises(c[:error], msg, &got)
+      end
+    end
+  end
+end
diff --git a/test/testing_helper.rb b/test/testing_helper.rb
@@ -6,7 +6,7 @@
 class RedisClient
   module TestingHelper
     REDIS_SCHEME = ENV.fetch('REDIS_SCHEME', 'redis')
-    NODE_ADDRS = (7000..7005).map { |port| "#{REDIS_SCHEME}://127.0.0.1:#{port}" }.freeze
+    NODE_ADDRS = (6379..6384).map { |port| "#{REDIS_SCHEME}://127.0.0.1:#{port}" }.freeze
 
     def setup
       @raw_clients = NODE_ADDRS.map { |addr| ::RedisClient.config(url: addr).new_client }
