RUBY-276 - execution profile attributes should initially be :unspecified

* Moved execution profile validation logic out of Cassandra.validate_and_massage_options and into execution profile constructor. This will keep bad profiles from being created in the first place and report errors to the user sooner.

Author: stamhankar999

lib/cassandra.rb

@@ -517,25 +517,6 @@ def self.validate_and_massage_options(options)
                               ':execution_profiles must be a hash of <name,ExecutionProfile> entries.')
     end
 
-    if options.key?(:timeout)
-      timeout = options[:timeout]
-
-      unless timeout.nil?
-        Util.assert_instance_of(::Numeric, timeout, ":timeout must be a number of seconds, #{timeout.inspect} given")
-        Util.assert(timeout > 0, ":timeout must be greater than 0, #{timeout} given")
-      end
-    end
-    if options.key?(:execution_profiles)
-      options[:execution_profiles].each do |name, profile|
-        timeout = profile.timeout
-        next if timeout.nil?
-        Util.assert_instance_of(::Numeric, timeout,
-                                ":timeout of execution profile #{name} must be a number of seconds, " \
-                              "#{timeout.inspect} given")
-        Util.assert(timeout > 0, ":timeout of execution profile #{name} must be greater than 0, #{timeout} given")
-      end
-    end
-
     if options.key?(:heartbeat_interval)
       timeout = options[:heartbeat_interval]
 
@@ -584,28 +565,6 @@ def self.validate_and_massage_options(options)
       end
     end
 
-    if options.key?(:load_balancing_policy)
-      load_balancing_policy = options[:load_balancing_policy]
-      methods = [:host_up, :host_down, :host_found, :host_lost, :setup, :teardown,
-                 :distance, :plan]
-      Util.assert_responds_to_all(methods, load_balancing_policy) do
-        ":load_balancing_policy #{load_balancing_policy.inspect} must respond " \
-            "to #{methods.inspect}, but doesn't"
-      end
-    end
-    if options.key?(:execution_profiles)
-      methods = [:host_up, :host_down, :host_found, :host_lost, :setup, :teardown,
-                 :distance, :plan]
-      options[:execution_profiles].each do |name, profile|
-        load_balancing_policy = profile.load_balancing_policy
-        next if load_balancing_policy.nil?
-        Util.assert_responds_to_all(methods, load_balancing_policy,
-                                    ":load_balancing_policy in execution profile #{name} " \
-                                    "#{load_balancing_policy.inspect} must respond " \
-                                    "to #{methods.inspect}, but doesn't")
-      end
-    end
-
     if options.key?(:reconnection_policy)
       reconnection_policy = options[:reconnection_policy]
 
@@ -615,43 +574,11 @@ def self.validate_and_massage_options(options)
       end
     end
 
-    if options.key?(:retry_policy)
-      retry_policy = options[:retry_policy]
-      methods = [:read_timeout, :write_timeout, :unavailable]
-      Util.assert_responds_to_all(methods, retry_policy) do
-        ":retry_policy #{retry_policy.inspect} must respond to #{methods.inspect}, " \
-            "but doesn't"
-      end
-    end
-    if options.key?(:execution_profiles)
-      methods = [:read_timeout, :write_timeout, :unavailable]
-      options[:execution_profiles].each do |name, profile|
-        retry_policy = profile.retry_policy
-        next if retry_policy.nil?
-        Util.assert_responds_to_all(methods, retry_policy,
-                                    ":retry_policy in execution profile #{name} #{retry_policy.inspect} must " \
-                                    "respond to #{methods.inspect}, but doesn't")
-      end
-    end
+    # Validate options that go in an execution profile. Instantiating one
+    # causes validation automatically.
+    Cassandra::Execution::Profile.new(options)
 
     options[:listeners] = Array(options[:listeners]) if options.key?(:listeners)
-
-    if options.key?(:consistency)
-      consistency = options[:consistency]
-      Util.assert_one_of(CONSISTENCIES, consistency,
-                         ":consistency must be one of #{CONSISTENCIES.inspect}, " \
-                             "#{consistency.inspect} given")
-    end
-    if options.key?(:execution_profiles)
-      options[:execution_profiles].each do |name, profile|
-        consistency = profile.consistency
-        next if consistency.nil?
-        Util.assert_one_of(CONSISTENCIES, consistency,
-                           ":consistency in execution profile #{name} must be one of #{CONSISTENCIES.inspect}, " \
-                           "#{consistency.inspect} given")
-      end
-    end
-
     options[:nodelay] = !!options[:nodelay] if options.key?(:nodelay)
     options[:trace] = !!options[:trace] if options.key?(:trace)
     options[:shuffle_replicas] = !!options[:shuffle_replicas] if options.key?(:shuffle_replicas)

lib/cassandra/driver.rb

@@ -173,7 +173,7 @@ def self.let(name, &block)
     let(:page_size)                 { 10000 }
     let(:heartbeat_interval)        { 30 }
     let(:idle_timeout)              { 60 }
-    let(:timeout)                   { Cassandra::Execution::Profile::DEFAULT_TIMEOUT }
+    let(:timeout)                   { 12 }
     let(:synchronize_schema)        { true }
     let(:schema_refresh_delay)      { 1 }
     let(:schema_refresh_timeout)    { 10 }

lib/cassandra/execution/profile.rb

@@ -46,28 +46,26 @@ class Profile
       attr_accessor :parent_name
 
       # @private
-      DEFAULT_OPTIONS = {load_balancing_policy: nil,
-                         retry_policy: nil,
-                         consistency: nil,
+      DEFAULT_OPTIONS = {load_balancing_policy: :unspecified,
+                         retry_policy: :unspecified,
+                         consistency: :unspecified,
                          timeout: :unspecified}.freeze
 
-      # @private
-      DEFAULT_TIMEOUT = 12
-
       # @private
       DEFAULT_PARENT_NAME = Cassandra::Execution::ProfileManager::DEFAULT_EXECUTION_PROFILE
 
       # @param options [Hash] hash of attributes. Unspecified attributes or attributes with nil values effectively
       #   fall back to the attributes in the default execution profile.
-      # @option options [Numeric] :timeout (12) Request execution timeout in
+      # @option options [Numeric] :timeout (:unspecified) Request execution timeout in
       #   seconds. Setting value to `nil` will remove request timeout.
-      # @option options [Cassandra::LoadBalancing::Policy] :load_balancing_policy (nil) Load-balancing policy that
-      #   determines which node will run the next statement.
-      # @option options [Cassandra::Retry::Policy] :retry_policy (nil) Retry policy that determines how request
-      #   retries should be handled for different failure modes.
-      # @option options [Symbol] :consistency (nil) Consistency level with which to run statements. Must be one
+      # @option options [Cassandra::LoadBalancing::Policy] :load_balancing_policy (:unspecified) Load-balancing policy
+      #   that determines which node will run the next statement.
+      # @option options [Cassandra::Retry::Policy] :retry_policy (:unspecified) Retry policy that determines how
+      #   request retries should be handled for different failure modes.
+      # @option options [Symbol] :consistency (:unspecified) Consistency level with which to run statements. Must be one
       #   of {Cassandra::CONSISTENCIES}.
       def initialize(options = {})
+        validate(options)
         options = DEFAULT_OPTIONS.merge(options)
         @load_balancing_policy = options[:load_balancing_policy]
         @retry_policy = options[:retry_policy]
@@ -76,17 +74,13 @@ def initialize(options = {})
         @parent_name = DEFAULT_PARENT_NAME
       end
 
-      def timeout
-        @timeout == :unspecified ? DEFAULT_TIMEOUT : @timeout
-      end
-
       # @private
       def to_h
         {
           load_balancing_policy: @load_balancing_policy,
           retry_policy: @retry_policy,
           consistency: @consistency,
-          timeout: timeout
+          timeout: @timeout
         }
       end
 
@@ -96,7 +90,7 @@ def eql?(other)
           @load_balancing_policy == other.load_balancing_policy && \
           @retry_policy == other.retry_policy && \
           @consistency == other.consistency && \
-          @timeout == other.instance_variable_get(:@timeout)
+          @timeout == other.timeout
       end
       alias == eql?
 
@@ -121,11 +115,49 @@ def inspect
       "timeout=#{@timeout.inspect}>"
       end
 
+      # @private
+      def validate(options)
+        if options.key?(:timeout)
+          timeout = options[:timeout]
+
+          unless timeout.nil?
+            Util.assert_instance_of(::Numeric, timeout, ":timeout must be a number of seconds, #{timeout.inspect} given")
+            Util.assert(timeout > 0, ":timeout must be greater than 0, #{timeout} given")
+          end
+        end
+
+        if options.key?(:load_balancing_policy)
+          load_balancing_policy = options[:load_balancing_policy]
+          methods = [:host_up, :host_down, :host_found, :host_lost, :setup, :teardown,
+                     :distance, :plan]
+          Util.assert_responds_to_all(methods, load_balancing_policy) do
+            ":load_balancing_policy #{load_balancing_policy.inspect} must respond " \
+            "to #{methods.inspect}, but doesn't"
+          end
+        end
+
+        if options.key?(:retry_policy)
+          retry_policy = options[:retry_policy]
+          methods = [:read_timeout, :write_timeout, :unavailable]
+          Util.assert_responds_to_all(methods, retry_policy) do
+            ":retry_policy #{retry_policy.inspect} must respond to #{methods.inspect}, " \
+            "but doesn't"
+          end
+        end
+
+        if options.key?(:consistency)
+          consistency = options[:consistency]
+          Util.assert_one_of(CONSISTENCIES, consistency,
+                             ":consistency must be one of #{CONSISTENCIES.inspect}, " \
+                             "#{consistency.inspect} given")
+        end
+      end
+
       # @private
       def merge_from(parent_profile)
-        @load_balancing_policy = parent_profile.load_balancing_policy if @load_balancing_policy.nil?
-        @retry_policy = parent_profile.retry_policy if @retry_policy.nil?
-        @consistency = parent_profile.consistency if @consistency.nil?
+        @load_balancing_policy = parent_profile.load_balancing_policy if @load_balancing_policy == :unspecified
+        @retry_policy = parent_profile.retry_policy if @retry_policy == :unspecified
+        @consistency = parent_profile.consistency if @consistency == :unspecified
         @timeout = parent_profile.timeout if @timeout == :unspecified
         self
       end

spec/cassandra/execution/options_spec.rb

@@ -22,20 +22,24 @@ module Cassandra
   module Execution
     describe(Options) do
       let(:load_balancing_policy) { double('lbp') }
+      let(:lbp2) { double('lbp2') }
       let(:retry_policy) { double('retry_policy') }
       let(:base_options) {
         Options.new(timeout: 10, consistency: :one, load_balancing_policy: load_balancing_policy,
                     retry_policy: retry_policy)
       }
+
       before do
-        allow(load_balancing_policy).to receive(:host_up)
-        allow(load_balancing_policy).to receive(:host_down)
-        allow(load_balancing_policy).to receive(:host_found)
-        allow(load_balancing_policy).to receive(:host_lost)
-        allow(load_balancing_policy).to receive(:setup)
-        allow(load_balancing_policy).to receive(:teardown)
-        allow(load_balancing_policy).to receive(:distance)
-        allow(load_balancing_policy).to receive(:plan)
+        [load_balancing_policy, lbp2].each do |policy|
+          allow(policy).to receive(:host_up)
+          allow(policy).to receive(:host_down)
+          allow(policy).to receive(:host_found)
+          allow(policy).to receive(:host_lost)
+          allow(policy).to receive(:setup)
+          allow(policy).to receive(:teardown)
+          allow(policy).to receive(:distance)
+          allow(policy).to receive(:plan)
+        end
 
         allow(retry_policy).to receive(:read_timeout)
         allow(retry_policy).to receive(:write_timeout)
@@ -76,9 +80,9 @@ module Execution
       end
 
       it 'should override with execution-profile and simple attribute' do
-        profile = Profile.new(timeout: 21, consistency: :quorum)
+        profile = Profile.new(load_balancing_policy: lbp2, retry_policy: retry_policy, timeout: 21, consistency: :quorum)
         result = base_options.override(profile, timeout: 42)
-        expect(result.load_balancing_policy).to be(load_balancing_policy)
+        expect(result.load_balancing_policy).to be(lbp2)
         expect(result.retry_policy).to be(retry_policy)
         expect(result.consistency).to be(:quorum)
         expect(result.timeout).to eq(42)

spec/cassandra/execution/profile_manager_spec.rb

@@ -41,24 +41,41 @@ module Execution
       }
 
       let(:default_profile) {
-        Profile.new(load_balancing_policy: lbp1, retry_policy: retry_policy, consistency: :quorum)
+        Profile.new(load_balancing_policy: lbp1, retry_policy: retry_policy, consistency: :quorum, timeout: 12)
       }
       let(:subject) {
         ProfileManager.new(default_profile, {p6: profile6, p1: profile1, p2: profile2, p3: profile3, p4: profile4, p5: profile5})
       }
 
+      before do
+        [lbp1, lbp2, lbp3].each do |policy|
+          allow(policy).to receive(:host_up)
+          allow(policy).to receive(:host_down)
+          allow(policy).to receive(:host_found)
+          allow(policy).to receive(:host_lost)
+          allow(policy).to receive(:setup)
+          allow(policy).to receive(:teardown)
+          allow(policy).to receive(:distance)
+          allow(policy).to receive(:plan)
+        end
+
+        allow(retry_policy).to receive(:read_timeout)
+        allow(retry_policy).to receive(:write_timeout)
+        allow(retry_policy).to receive(:unavailable)
+      end
+
       it 'should fill out profiles with values from default profile' do
         subject
         expect(profile1).to eq(Profile.new(load_balancing_policy: lbp1, retry_policy: retry_policy,
-                                     consistency: :quorum, timeout: Profile::DEFAULT_TIMEOUT))
+                                     consistency: :quorum, timeout: 12))
         expect(profile2).to eq(Profile.new(load_balancing_policy: lbp2, retry_policy: retry_policy,
-                                           consistency: :quorum, timeout: Profile::DEFAULT_TIMEOUT))
+                                           consistency: :quorum, timeout: 12))
         expect(profile3).to eq(Profile.new(load_balancing_policy: lbp3, retry_policy: retry_policy,
-                                           consistency: :quorum, timeout: Profile::DEFAULT_TIMEOUT))
+                                           consistency: :quorum, timeout: 12))
         expect(profile4).to eq(Profile.new(load_balancing_policy: lbp1, retry_policy: retry_policy,
-                                           consistency: :quorum, timeout: Profile::DEFAULT_TIMEOUT))
+                                           consistency: :quorum, timeout: 12))
         expect(profile5).to eq(Profile.new(load_balancing_policy: lbp1, retry_policy: retry_policy,
-                                           consistency: :quorum, timeout: Profile::DEFAULT_TIMEOUT))
+                                           consistency: :quorum, timeout: 12))
         expect(profile6).to eq(profile3)
       end