From 8067af635e7adc726656ecf75d4395a30cb1e359 Mon Sep 17 00:00:00 2001 From: "Matthew M. Keeler" Date: Tue, 2 Apr 2024 13:07:31 -0400 Subject: [PATCH] fix: Adjust migration variation and hook interaction (#264) --- .../impl/evaluation_with_hook_result.rb | 34 ++++++++++++ lib/ldclient-rb/interfaces.rb | 2 +- lib/ldclient-rb/ldclient.rb | 54 +++++++++++-------- spec/ldclient_hooks_spec.rb | 47 ++++++++++++++++ 4 files changed, 114 insertions(+), 23 deletions(-) create mode 100644 lib/ldclient-rb/impl/evaluation_with_hook_result.rb diff --git a/lib/ldclient-rb/impl/evaluation_with_hook_result.rb b/lib/ldclient-rb/impl/evaluation_with_hook_result.rb new file mode 100644 index 00000000..b092bc06 --- /dev/null +++ b/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 diff --git a/lib/ldclient-rb/interfaces.rb b/lib/ldclient-rb/interfaces.rb index 956a5f6a..3aa6fe68 100644 --- a/lib/ldclient-rb/interfaces.rb +++ b/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 diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index 57e0c52b..f3bb311a 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/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] # 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 # diff --git a/spec/ldclient_hooks_spec.rb b/spec/ldclient_hooks_spec.rb index 887bd829..4c403981 100644 --- a/spec/ldclient_hooks_spec.rb +++ b/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