fix: Adjust migration variation and hook interaction (#264)

Author: keelerm84

lib/ldclient-rb/impl/evaluation_with_hook_result.rb

@@ -0,0 +1,34 @@
+module LaunchDarkly
+  module Impl
+    #
+    # Simple helper class for returning formatted data.
+    #
+    # The variation methods make use of the new hook support. Those methods all need to return an evaluation detail, and
+    # some other unstructured bit of data.
+    #
+    class EvaluationWithHookResult
+      #
+      # Return the evaluation detail that was generated as part of the evaluation.
+      #
+      # @return [LaunchDarkly::EvaluationDetail]
+      #
+      attr_reader :evaluation_detail
+
+      #
+      # All purpose container for additional return values from the wrapping method
+      #
+      # @return [any]
+      #
+      attr_reader :results
+
+      #
+      # @param evaluation_detail [LaunchDarkly::EvaluationDetail]
+      # @param results [any]
+      #
+      def initialize(evaluation_detail, results = nil)
+        @evaluation_detail = evaluation_detail
+        @results = results
+      end
+    end
+  end
+end

lib/ldclient-rb/interfaces.rb

@@ -920,7 +920,7 @@ def before_evaluation(evaluation_series_context, data)
         end
 
         #
-        # The after method is called during the execution of the variation method # after the flag value has been
+        # The after method is called during the execution of the variation method after the flag value has been
         # determined. The method is executed synchronously.
         #
         # @param evaluation_series_context [EvaluationSeriesContext] Contains read-only information about the evaluation

lib/ldclient-rb/ldclient.rb

@@ -4,6 +4,7 @@
 require "ldclient-rb/impl/data_store"
 require "ldclient-rb/impl/diagnostic_events"
 require "ldclient-rb/impl/evaluator"
+require "ldclient-rb/impl/evaluation_with_hook_result"
 require "ldclient-rb/impl/flag_tracker"
 require "ldclient-rb/impl/store_client_wrapper"
 require "ldclient-rb/impl/migrations/tracker"
@@ -217,8 +218,13 @@ def initialized?
     # @return the variation for the provided context, or the default value if there's an error
     #
     def variation(key, context, default)
-      detail, _, _, = variation_with_flag(key, context, default)
-      detail.value
+      context = Impl::Context::make_context(context)
+      result = evaluate_with_hooks(key, context, default, :variation) do
+        detail, _, _ = variation_with_flag(key, context, default)
+        LaunchDarkly::Impl::EvaluationWithHookResult.new(detail)
+      end
+
+      result.evaluation_detail.value
     end
 
     #
@@ -246,11 +252,12 @@ def variation(key, context, default)
     #
     def variation_detail(key, context, default)
       context = Impl::Context::make_context(context)
-      detail, _, _ = evaluate_with_hooks(key, context, default, :variation_detail) do
-        evaluate_internal(key, context, default, true)
+      result = evaluate_with_hooks(key, context, default, :variation_detail) do
+        detail, _, _ = evaluate_internal(key, context, default, true)
+        LaunchDarkly::Impl::EvaluationWithHookResult.new(detail)
       end
 
-      detail
+      result.evaluation_detail
     end
 
     #
@@ -270,15 +277,17 @@ def variation_detail(key, context, default)
     # @param method [Symbol]
     # @param &block [#call] Implicit passed block
     #
+    # @return [LaunchDarkly::Impl::EvaluationWithHookResult]
+    #
     private def evaluate_with_hooks(key, context, default, method)
       return yield if @hooks.empty?
 
       hooks, evaluation_series_context = prepare_hooks(key, context, default, method)
       hook_data = execute_before_evaluation(hooks, evaluation_series_context)
-      evaluation_detail, flag, error = yield
-      execute_after_evaluation(hooks, evaluation_series_context, hook_data, evaluation_detail)
+      evaluation_result = yield
+      execute_after_evaluation(hooks, evaluation_series_context, hook_data, evaluation_result.evaluation_detail)
 
-      [evaluation_detail, flag, error]
+      evaluation_result
     end
 
     #
@@ -375,20 +384,24 @@ def migration_variation(key, context, default_stage)
       end
 
       context = Impl::Context::make_context(context)
-      detail, flag, _ = variation_with_flag(key, context, default_stage.to_s)
+      result = evaluate_with_hooks(key, context, default_stage, :migration_variation) do
+        detail, flag, _ = variation_with_flag(key, context, default_stage.to_s)
+
+        stage = detail.value
+        stage = stage.to_sym if stage.respond_to? :to_sym
 
-      stage = detail.value
-      stage = stage.to_sym if stage.respond_to? :to_sym
+        if Migrations::VALID_STAGES.include?(stage)
+          tracker = Impl::Migrations::OpTracker.new(@config.logger, key, flag, context, detail, default_stage)
+          next LaunchDarkly::Impl::EvaluationWithHookResult.new(detail, {stage: stage, tracker: tracker})
+        end
 
-      if Migrations::VALID_STAGES.include?(stage)
+        detail = LaunchDarkly::Impl::Evaluator.error_result(LaunchDarkly::EvaluationReason::ERROR_WRONG_TYPE, default_stage.to_s)
         tracker = Impl::Migrations::OpTracker.new(@config.logger, key, flag, context, detail, default_stage)
-        return stage, tracker
-      end
 
-      detail = LaunchDarkly::Impl::Evaluator.error_result(LaunchDarkly::EvaluationReason::ERROR_WRONG_TYPE, default_stage.to_s)
-      tracker = Impl::Migrations::OpTracker.new(@config.logger, key, flag, context, detail, default_stage)
+        LaunchDarkly::Impl::EvaluationWithHookResult.new(detail, {stage: default_stage, tracker: tracker})
+      end
 
-      [default_stage, tracker]
+      [result.results[:stage], result.results[:tracker]]
     end
 
     #
@@ -628,16 +641,13 @@ def create_default_data_source(sdk_key, config, diagnostic_accumulator)
 
     #
     # @param key [String]
-    # @param context [Hash, LDContext]
+    # @param context [LDContext]
     # @param default [Object]
     #
     # @return [Array<EvaluationDetail, [LaunchDarkly::Impl::Model::FeatureFlag, nil], [String, nil]>]
     #
     def variation_with_flag(key, context, default)
-      context = Impl::Context::make_context(context)
-      evaluate_with_hooks(key, context, default, :variation_detail) do
-        evaluate_internal(key, context, default, false)
-      end
+      evaluate_internal(key, context, default, false)
     end
 
     #

spec/ldclient_hooks_spec.rb

@@ -148,5 +148,52 @@ module LaunchDarkly
         end
       end
     end
+
+    context "migration variation" do
+      it "EvaluationDetail contains stage value" do
+        td = Integrations::TestData.data_source
+        td.update(td.flag("flagkey").variations("off").variation_for_all(0))
+
+        detail = nil
+        config_hook = MockHook.new(->(_, _) { }, ->(_, _, d) { detail = d })
+        with_client(test_config(data_source: td, hooks: [config_hook])) do |client|
+          client.migration_variation("flagkey", basic_context, LaunchDarkly::Migrations::STAGE_LIVE)
+
+          expect(detail.value).to eq LaunchDarkly::Migrations::STAGE_OFF.to_s
+          expect(detail.variation_index).to eq 0
+          expect(detail.reason).to eq EvaluationReason::fallthrough
+        end
+      end
+
+      it "EvaluationDetail gets default if flag doesn't evaluate to stage" do
+        td = Integrations::TestData.data_source
+        td.update(td.flag("flagkey").variations("nonstage").variation_for_all(0))
+
+        detail = nil
+        config_hook = MockHook.new(->(_, _) { }, ->(_, _, d) { detail = d })
+        with_client(test_config(data_source: td, hooks: [config_hook])) do |client|
+          client.migration_variation("flagkey", basic_context, LaunchDarkly::Migrations::STAGE_LIVE)
+
+          expect(detail.value).to eq LaunchDarkly::Migrations::STAGE_LIVE.to_s
+          expect(detail.variation_index).to eq nil
+          expect(detail.reason).to eq EvaluationReason.error(EvaluationReason::ERROR_WRONG_TYPE)
+        end
+      end
+
+      it "EvaluationDetail default gets converted to off if invalid" do
+        td = Integrations::TestData.data_source
+        td.update(td.flag("flagkey").variations("nonstage").variation_for_all(0))
+
+        detail = nil
+        config_hook = MockHook.new(->(_, _) { }, ->(_, _, d) { detail = d })
+        with_client(test_config(data_source: td, hooks: [config_hook])) do |client|
+          client.migration_variation("flagkey", basic_context, :invalid)
+
+          expect(detail.value).to eq LaunchDarkly::Migrations::STAGE_OFF.to_s
+          expect(detail.variation_index).to eq nil
+          expect(detail.reason).to eq EvaluationReason.error(EvaluationReason::ERROR_WRONG_TYPE)
+        end
+      end
+    end
   end
 end