diff --git a/README.md b/README.md index 94f145ce..5e55ce68 100644 --- a/README.md +++ b/README.md @@ -572,10 +572,11 @@ class MyLoggingHook end ``` -Hooks can read their inputs but not change them: +Hooks can read the hook context and hints but not change them: -- Only `before` hooks can change the evaluation context: return an `EvaluationContext` for the SDK to merge in, or assign `hook_context.evaluation_context`. By the time `after`, `error` and `finally` run, it's a frozen copy, so assigning it or writing to its `fields` raises `FrozenError`. Nested field values aren't frozen, because they belong to the caller. +- Only `before` hooks can change the evaluation context: return an `EvaluationContext` for the SDK to merge in, or assign `hook_context.evaluation_context`. By the time `after`, `error` and `finally` run, it's a deep-frozen copy, so assigning it or changing its values raises `FrozenError`. Your own context isn't frozen. - The flag key, the default value, the hints, and the client and provider metadata are frozen copies. Your own objects aren't frozen. +- Only the spec's value types are copied: strings, numbers, booleans, times, hashes and arrays. Other objects, such as a user or a record you put in the context, are passed to hooks as they are. A time is copied but left unfrozen, so `utc` and `localtime` still work, and a hash's default proc isn't copied. > Built a new hook? [Let us know](https://github.com/open-feature/openfeature.dev/issues/new?assignees=&labels=hook&projects=&template=document-hook.yaml&title=%5BHook%5D%3A+) so we can add it to the docs! diff --git a/lib/open_feature/sdk/client.rb b/lib/open_feature/sdk/client.rb index 841f7b8a..eb48ffd0 100644 --- a/lib/open_feature/sdk/client.rb +++ b/lib/open_feature/sdk/client.rb @@ -58,7 +58,8 @@ def track(tracking_event_name, evaluation_context: nil, tracking_event_details: built_context = build_evaluation_context(evaluation_context) provider.track(tracking_event_name, evaluation_context: built_context, tracking_event_details: tracking_event_details) - rescue StandardError, NotImplementedError + nil # Spec 6.1.1.1: returns nothing + rescue *RECOVERABLE_ERRORS nil end @@ -138,8 +139,8 @@ def fetch_details(type:, flag_key:, default_value:, evaluation_context: nil, inv evaluate_flag(provider: provider, type: type, flag_key: flag_key, default_value: default_value, evaluation_context: hctx.evaluation_context) end end - rescue StandardError, NotImplementedError => e - # Spec 1.4.10 and 1.4.11: report through the details, never raise or log. NotImplementedError is the abstract-method idiom. + rescue *RECOVERABLE_ERRORS => e + # Spec 1.4.10 and 1.4.11: report through the details, never raise or log resolution = Provider::ResolutionDetails.new( value: default_value, error_code: EvaluationError.code_for(e), diff --git a/lib/open_feature/sdk/configuration.rb b/lib/open_feature/sdk/configuration.rb index f6c7e84e..60bee6e9 100644 --- a/lib/open_feature/sdk/configuration.rb +++ b/lib/open_feature/sdk/configuration.rb @@ -110,7 +110,7 @@ def shutdown @provider_state_registry.set_initial_state(prov, ProviderState::NOT_READY) prov.send(:detach) if prov.is_a?(Provider::EventEmitter) prov.shutdown if prov.respond_to?(:shutdown) - rescue => e + rescue *RECOVERABLE_ERRORS => e @logger&.warn("Error shutting down provider #{prov&.class&.name || "unknown"}: #{e.message}") end @@ -165,7 +165,8 @@ def set_provider_internal(provider, domain:, wait_for_init:) # First, so a provider registered with another API (1.8.4) leaves no state behind provider.send(:attach, self) if provider.is_a?(Provider::EventEmitter) attempt = @init_attempts[provider] = InitAttempt.new(:running, nil, Thread::Queue.new) - @provider_state_registry.set_initial_state(provider) + # Spec 2.8.5.1: a provider without init is READY from registration + @provider_state_registry.set_initial_state(provider, has_init ? ProviderState::NOT_READY : ProviderState::READY) end # Publish last: clients read @providers without the lock, and must not see an untracked provider @@ -184,30 +185,42 @@ def set_provider_internal(provider, domain:, wait_for_init:) end end - # Shutdown old provider outside mutex to avoid blocking other operations - if needs_shutdown - begin - old_provider.shutdown if old_provider.respond_to?(:shutdown) - rescue => e - @logger&.warn("Error shutting down previous provider #{old_provider&.class&.name || "unknown"}: #{e.message}") + begin + # Spec 2.8.5.1: a provider without init is READY from registration, so its READY handlers run now, before the + # old provider's shutdown; after it, a handler added during the shutdown would run twice + unless has_init + if needs_init + init_provider(provider, context_for_init, domain:, attempt:, raise_on_error: wait_for_init) + raise incomplete_init_error(provider, "initialization did not complete") if wait_for_init && attempt.state != :succeeded + elsif !old_provider.equal?(provider) + # Each new binding runs its domain's READY handlers + dispatch_provider_event(provider, ProviderEvent::PROVIDER_READY, token: attempt) + end + end + ensure + # Shutdown old provider outside mutex to avoid blocking other operations. Checked again, since a READY + # handler above may have bound it again (a rollback) + if needs_shutdown && !provider_bound?(old_provider) + begin + old_provider.shutdown if old_provider.respond_to?(:shutdown) + rescue *RECOVERABLE_ERRORS => e + @logger&.warn("Error shutting down previous provider #{old_provider&.class&.name || "unknown"}: #{e.message}") + end end end + return unless has_init # Initialize provider outside the mutex to avoid blocking other operations if needs_init - # Spec 2.8.5.1: a provider without init is READY, and its handlers have run, when set_provider returns - if wait_for_init || !has_init - init_provider(provider, context_for_init, domain:, attempt:, raise_on_error: wait_for_init) + if wait_for_init + init_provider(provider, context_for_init, domain:, attempt:, raise_on_error: true) # Replaced or shut down while init ran: fail like the waiters on this attempt did - raise incomplete_init_error(provider, "initialization did not complete") if wait_for_init && attempt.state != :succeeded + raise incomplete_init_error(provider, "initialization did not complete") if attempt.state != :succeeded else init_thread = Thread.new do init_provider(provider, context_for_init, domain:, attempt:, raise_on_error: false) end end - elsif !has_init - # Spec 2.8.5.1: each new binding of a provider without init runs its domain's READY handlers - dispatch_provider_event(provider, ProviderEvent::PROVIDER_READY, token: attempt) unless old_provider.equal?(provider) elsif wait_for_init # Already registered: wait for an init still in flight, then report its outcome. # From inside that init (e.g. a handler of an event it emits) the wait could never end. @@ -232,7 +245,7 @@ def init_provider(provider, context, domain:, attempt:, raise_on_error: false) # An abandoned attempt (provider replaced or shut down meanwhile) reports no READY. # Spec 2.8.2/2.8.3: a provider that emits its own lifecycle events gets no synthetic READY or ERROR dispatch_provider_event(provider, ProviderEvent::PROVIDER_READY, token: attempt) if finish_init_attempt(attempt, :succeeded) && !emits_lifecycle_events - rescue => e + rescue *RECOVERABLE_ERRORS => e # Spec 1.7.5: PROVIDER_FATAL from init makes the provider FATAL error_code = EvaluationError.code_for(e) error = ProviderInitializationError.new( @@ -250,7 +263,7 @@ def init_provider(provider, context, domain:, attempt:, raise_on_error: false) raise error if raise_on_error ensure - # Reached unsettled only when init raised something other than a StandardError + # Reached unsettled only when init raised something not in RECOVERABLE_ERRORS, such as an Interrupt finish_init_attempt(attempt, :failed) end @@ -296,7 +309,7 @@ def dispatch_provider_event(provider, event_type, details = {}, token: nil) until queue.empty? begin deliver_provider_event(provider, *queue.shift) - rescue => e + rescue *RECOVERABLE_ERRORS => e # Its own emitter already returned, so log rather than fail the event that was dispatching @logger&.error("Queued provider event failed: #{e.message}") end @@ -405,7 +418,7 @@ def run_handlers_for_provider(provider, event_type, event_details) handlers.each do |handler| handler.call(event_details) - rescue => e + rescue *RECOVERABLE_ERRORS => e @logger&.error("Client event handler failed: #{e.message}") end end @@ -430,7 +443,7 @@ def run_immediate_handler(event_type, handler, client) begin handler.call(event_details) - rescue => e + rescue *RECOVERABLE_ERRORS => e @logger&.error("Immediate API event handler failed: #{e.message}") end end @@ -447,7 +460,7 @@ def run_immediate_handler(event_type, handler, client) begin handler.call(event_details) - rescue => e + rescue *RECOVERABLE_ERRORS => e @logger&.error("Immediate client event handler failed: #{e.message}") end end diff --git a/lib/open_feature/sdk/evaluation_error.rb b/lib/open_feature/sdk/evaluation_error.rb index 28c4c25e..53e71e7b 100644 --- a/lib/open_feature/sdk/evaluation_error.rb +++ b/lib/open_feature/sdk/evaluation_error.rb @@ -4,6 +4,10 @@ module OpenFeature module SDK + # What the SDK reports or logs, rather than raises, when provider, hook or handler code fails. + # NotImplementedError isn't a StandardError, but it's Ruby's abstract-method idiom; Interrupt and the rest still propagate. + RECOVERABLE_ERRORS = [StandardError, NotImplementedError].freeze + # Raised by a provider to signal an evaluation error with an OpenFeature # error code (spec 2.2.7). Any exception responding to +error_code+ is # honored the same way. diff --git a/lib/open_feature/sdk/event_dispatcher.rb b/lib/open_feature/sdk/event_dispatcher.rb index 4b59478d..c97f742a 100644 --- a/lib/open_feature/sdk/event_dispatcher.rb +++ b/lib/open_feature/sdk/event_dispatcher.rb @@ -55,7 +55,7 @@ def trigger_event(event_type, event_details = {}) # Call handlers outside of mutex to avoid deadlocks handlers_to_call.each do |handler| handler.call(event_details) - rescue => e + rescue *RECOVERABLE_ERRORS => e @logger&.warn "Event handler failed for #{event_type}: #{e.message}\n#{e.backtrace.join("\n")}" end end diff --git a/lib/open_feature/sdk/frozen_copy.rb b/lib/open_feature/sdk/frozen_copy.rb new file mode 100644 index 00000000..549ca530 --- /dev/null +++ b/lib/open_feature/sdk/frozen_copy.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module OpenFeature + module SDK + # Deep-frozen copies of the spec's value types (spec 3.1.2), so whoever gets one can't change it and the original is + # never frozen (spec 1.4.15.1, 4.1.3, 4.1.4.1, 4.2.2) + module FrozenCopy + def self.of(value, seen = nil) + return value if defined?(Ractor) && Ractor.shareable?(value) + + case value + when String then value.frozen? ? value : value.dup.freeze + when Time then value.dup # Left unfrozen: utc and localtime convert in place, and on a copy they touch nothing else + when Hash, Array + seen ||= {}.compare_by_identity + return seen[value] if seen.key?(value) + + # Registered before its items, so a cycle or a shared structure maps to this one copy + copy = seen[value] = value.dup + if copy.is_a?(Hash) + if copy.default_proc + copy.default_proc = nil # Behavior rather than data, and on a frozen copy one that stores would raise + elsif !copy.default.nil? + copy.default = of(copy.default, seen) + end + copy.transform_values! { |item| of(item, seen) } + else + copy.map! { |item| of(item, seen) } + end + copy.freeze + else + value # Anything else, like a user object or a record, is passed through as is + end + end + end + end +end diff --git a/lib/open_feature/sdk/hooks/frozen_copy.rb b/lib/open_feature/sdk/hooks/frozen_copy.rb deleted file mode 100644 index 9b8e3d3b..00000000 --- a/lib/open_feature/sdk/hooks/frozen_copy.rb +++ /dev/null @@ -1,14 +0,0 @@ -# frozen_string_literal: true - -module OpenFeature - module SDK - module Hooks - # A deep-frozen copy of a hook input, so hooks can't change it and the caller's object isn't frozen (spec 4.1.3, 4.2.2) - def self.frozen_copy(value) - Ractor.make_shareable(value, copy: true) - rescue - value.dup.freeze # shortcut: when something inside can't be copied (a Mutex, Queue or Proc), only the top level is frozen - end - end - end -end diff --git a/lib/open_feature/sdk/hooks/hints.rb b/lib/open_feature/sdk/hooks/hints.rb index 6ce0e727..726dfd03 100644 --- a/lib/open_feature/sdk/hooks/hints.rb +++ b/lib/open_feature/sdk/hooks/hints.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "delegate" -require_relative "frozen_copy" +require_relative "../frozen_copy" module OpenFeature module SDK @@ -14,7 +14,7 @@ def initialize(hash = {}) assert_allowed_key(key) assert_allowed_value(value) end - super(Hooks.frozen_copy(hash)) + super(FrozenCopy.of(hash)) freeze end diff --git a/lib/open_feature/sdk/hooks/hook_context.rb b/lib/open_feature/sdk/hooks/hook_context.rb index eda378ec..a4de59f6 100644 --- a/lib/open_feature/sdk/hooks/hook_context.rb +++ b/lib/open_feature/sdk/hooks/hook_context.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require_relative "frozen_copy" +require_relative "../frozen_copy" module OpenFeature module SDK @@ -13,33 +13,36 @@ module Hooks # - evaluation_context is mutable only in before hooks; HookExecutor freezes the hook context after them (4.1.4.1) class HookContext attr_reader :flag_key, :flag_value_type, :client_metadata, :provider_metadata - attr_accessor :evaluation_context + attr_writer :evaluation_context def initialize(flag_key:, flag_value_type:, default_value:, evaluation_context:, client_metadata: nil, provider_metadata: nil) - @flag_key = Hooks.frozen_copy(flag_key) + @flag_key = FrozenCopy.of(flag_key) @flag_value_type = flag_value_type.freeze @source_default_value = default_value @evaluation_context = evaluation_context @client_metadata = client_metadata - @provider_metadata = Hooks.frozen_copy(provider_metadata) + @provider_metadata = frozen_metadata(provider_metadata) @hook_data = {} @copies = {} end # Copied on first read: most hooks never read it, and the copy grows with the value def default_value - @copies.fetch(:default_value) { @copies[:default_value] = Hooks.frozen_copy(@source_default_value) } + @copies.fetch(:default_value) { @copies[:default_value] = FrozenCopy.of(@source_default_value) } end - # Swaps in a frozen copy of the evaluation context, so neither the caller's nor a before hook's object is frozen. - # Top-level strings are copied too, so a hook can't edit a targeting key in place. - def freeze - return self if frozen? - @evaluation_context &&= @evaluation_context.dup.tap do |copy| - copy.fields.transform_values! { |value| (value.is_a?(String) && !value.frozen?) ? value.dup.freeze : value } - end.freeze - super + # Live during the before stage; once frozen, a deep-frozen copy made on first read, so neither the caller's + # context nor one a before hook assigned is frozen + def evaluation_context + return @evaluation_context unless frozen? + + @copies.fetch(:evaluation_context) do + @copies[:evaluation_context] = @evaluation_context&.dup&.tap do |copy| + seen = {}.compare_by_identity # One map for every field, so a structure two fields share is copied once + copy.fields.transform_values! { |value| FrozenCopy.of(value, seen) } + end&.freeze + end end # Returns a mutable hash scoped to the given hook instance. @@ -48,6 +51,15 @@ def freeze def hook_data_for(hook) @hook_data[hook.object_id] ||= {} end + + private + + # FrozenCopy passes Structs through, so ProviderMetadata's members are copied here (spec 4.2.2.3) + def frozen_metadata(metadata) + return metadata if !metadata.is_a?(Struct) || (defined?(Ractor) && Ractor.shareable?(metadata)) + + metadata.dup.tap { |copy| copy.each_pair { |member, value| copy[member] = FrozenCopy.of(value) } }.freeze + end end end end diff --git a/lib/open_feature/sdk/hooks/hook_executor.rb b/lib/open_feature/sdk/hooks/hook_executor.rb index 7f534095..f7b5af39 100644 --- a/lib/open_feature/sdk/hooks/hook_executor.rb +++ b/lib/open_feature/sdk/hooks/hook_executor.rb @@ -58,7 +58,7 @@ def execute(ordered_hooks:, hook_context:, hints:, default_value:, &evaluate_blo else run_after_hooks(ordered_hooks, hook_context, evaluation_details, hints) end - rescue StandardError, NotImplementedError => e + rescue *RECOVERABLE_ERRORS => e run_error_hooks(ordered_hooks, hook_context, e, hints) evaluation_details = EvaluationDetails.new( @@ -109,7 +109,7 @@ def run_error_hooks(hooks, hook_context, exception, hints) hooks.reverse_each do |hook| next unless hook.respond_to?(:error) hook.error(hook_context: hook_context, exception: exception, hints: hints) - rescue => e + rescue *RECOVERABLE_ERRORS => e @logger&.error("Error hook #{hook.class.name} failed: #{e.message}") end end @@ -121,7 +121,7 @@ def run_finally_hooks(hooks, hook_context, evaluation_details, hints) hooks.reverse_each do |hook| next unless hook.respond_to?(:finally) hook.finally(hook_context: hook_context, evaluation_details: evaluation_details, hints: hints) - rescue => e + rescue *RECOVERABLE_ERRORS => e @logger&.error("Finally hook #{hook.class.name} failed: #{e.message}") end end diff --git a/lib/open_feature/sdk/provider/resolution_details.rb b/lib/open_feature/sdk/provider/resolution_details.rb index 1a918999..2b9a78ec 100644 --- a/lib/open_feature/sdk/provider/resolution_details.rb +++ b/lib/open_feature/sdk/provider/resolution_details.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require_relative "../frozen_copy" + module OpenFeature module SDK module Provider @@ -7,14 +9,7 @@ module Provider ResolutionDetails = Struct.new(:value, :reason, :variant, :error_code, :error_message, :flag_metadata, keyword_init: true) do def flag_metadata - raw = self[:flag_metadata] - if raw.nil? - EMPTY_FLAG_METADATA - elsif raw.frozen? - raw - else - raw.dup.freeze - end + FrozenCopy.of(self[:flag_metadata] || EMPTY_FLAG_METADATA) end end end diff --git a/sig/open_feature/sdk/evaluation_error.rbs b/sig/open_feature/sdk/evaluation_error.rbs index 092fd3bb..1b8d0a17 100644 --- a/sig/open_feature/sdk/evaluation_error.rbs +++ b/sig/open_feature/sdk/evaluation_error.rbs @@ -1,5 +1,7 @@ module OpenFeature module SDK + RECOVERABLE_ERRORS: Array[singleton(Exception)] + class EvaluationError < StandardError attr_reader error_code: String diff --git a/sig/open_feature/sdk/frozen_copy.rbs b/sig/open_feature/sdk/frozen_copy.rbs new file mode 100644 index 00000000..6f6e27ed --- /dev/null +++ b/sig/open_feature/sdk/frozen_copy.rbs @@ -0,0 +1,7 @@ +module OpenFeature + module SDK + module FrozenCopy + def self.of: (untyped value, ?Hash[untyped, untyped]? seen) -> untyped + end + end +end diff --git a/sig/open_feature/sdk/hooks/frozen_copy.rbs b/sig/open_feature/sdk/hooks/frozen_copy.rbs deleted file mode 100644 index 014fd8ec..00000000 --- a/sig/open_feature/sdk/hooks/frozen_copy.rbs +++ /dev/null @@ -1,7 +0,0 @@ -module OpenFeature - module SDK - module Hooks - def self.frozen_copy: [T] (T value) -> T - end - end -end diff --git a/sig/open_feature/sdk/hooks/hook_context.rbs b/sig/open_feature/sdk/hooks/hook_context.rbs index 61d36f62..304cf46f 100644 --- a/sig/open_feature/sdk/hooks/hook_context.rbs +++ b/sig/open_feature/sdk/hooks/hook_context.rbs @@ -6,15 +6,19 @@ module OpenFeature attr_reader flag_value_type: Symbol attr_reader client_metadata: ClientMetadata? attr_reader provider_metadata: Provider::ProviderMetadata? - attr_accessor evaluation_context: EvaluationContext? + attr_writer evaluation_context: EvaluationContext? def initialize: (flag_key: String, flag_value_type: Symbol, default_value: untyped, evaluation_context: EvaluationContext?, ?client_metadata: ClientMetadata?, ?provider_metadata: Provider::ProviderMetadata?) -> void def default_value: () -> untyped - def freeze: () -> self + def evaluation_context: () -> EvaluationContext? def hook_data_for: (untyped hook) -> Hash[untyped, untyped] + + private + + def frozen_metadata: (untyped metadata) -> untyped end end end diff --git a/spec/open_feature/sdk/frozen_copy_spec.rb b/spec/open_feature/sdk/frozen_copy_spec.rb new file mode 100644 index 00000000..9b4195f1 --- /dev/null +++ b/spec/open_feature/sdk/frozen_copy_spec.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe OpenFeature::SDK::FrozenCopy do + it "returns a deep-frozen copy without freezing the original" do + source = {"list" => [+"a"]} + copy = described_class.of(source) + + expect(copy["list"].first).to be_frozen + expect(source["list"]).not_to be_frozen + end + + it "copies what it can, item by item, when something inside can't be copied" do + queue = Thread::Queue.new + source = {"list" => [+"a"], "queue" => queue, "lookup" => Hash.new { |hash, key| hash[key] = key }} + copy = described_class.of(source) + + expect(copy).to be_frozen + expect(copy["list"]).to be_frozen + expect(copy["list"].first).to be_frozen + expect(copy["lookup"]).to be_frozen + expect(copy["queue"]).to equal(queue) + expect(source["list"]).not_to be_frozen + end + + it "copies a cycle as a cycle of copies" do + source = [Thread::Queue.new] + source << source + copy = described_class.of(source) + + expect(copy).to be_frozen + expect(copy[1]).to equal(copy) + end + + it "copies a structure shared in two places once, without handing back the original" do + inner = [Thread::Queue.new, +"s"] + copy = described_class.of({"a" => inner, "b" => inner}) + + expect(copy["b"]).not_to equal(inner) + expect(copy["b"]).to equal(copy["a"]) + expect(copy["b"]).to be_frozen + end + + it "passes objects that aren't spec value types through untouched" do + user = Struct.new(:name).new(+"Ada") + copy = described_class.of({"user" => user}) + + expect(copy["user"]).to equal(user) + expect(user).not_to be_frozen + end + + it "gives a Time its own copy, left unfrozen so utc and localtime still work without touching the original" do + time = Time.new(2026, 1, 1, 12, 0, 0, "+05:00") + copy = described_class.of(time) + copy.utc + + expect(copy).not_to equal(time) + expect(time.utc_offset).to eq(5 * 3600) + end + + it "drops a Hash's default proc, which a frozen copy couldn't run" do + copy = described_class.of(Hash.new { |hash, key| hash[key] = [] }.merge("a" => 1)) + + expect(copy["missing"]).to be_nil + expect(copy["a"]).to eq(1) + end + + it "copies a Hash's default value too" do + regions = [+"us"] + copy = described_class.of(Hash.new(regions)) + + expect { copy["missing"] << "eu" }.to raise_error(FrozenError) + expect(regions).to eq(["us"]) + end + + it "works on a Ruby without Ractor" do + hide_const("Ractor") + source = {"list" => [+"a"]} + + expect(described_class.of(source)["list"].first).to be_frozen + expect(source["list"]).not_to be_frozen + end +end diff --git a/spec/open_feature/sdk/hooks/hints_spec.rb b/spec/open_feature/sdk/hooks/hints_spec.rb index 72bd61fe..6b28fc0e 100644 --- a/spec/open_feature/sdk/hooks/hints_spec.rb +++ b/spec/open_feature/sdk/hooks/hints_spec.rb @@ -46,13 +46,13 @@ expect(hint_hash[:nested][:key]).not_to be_frozen end - it "freezes only the top level when a nested value can't be copied" do - source = {key: {lock: Mutex.new}} + it "still freezes nested values when one of them can't be copied" do + source = {key: {lock: Mutex.new, list: []}} hints = described_class.new(source) - expect(hints[:key]).to eq(source[:key]) - expect { hints[:other] = 1 }.to raise_error(FrozenError) - expect(source).not_to be_frozen + expect(hints[:key]).to be_frozen + expect(hints[:key][:list]).to be_frozen + expect(source[:key]).not_to be_frozen end end end diff --git a/spec/open_feature/sdk/hooks/hook_context_spec.rb b/spec/open_feature/sdk/hooks/hook_context_spec.rb index 245529ea..72a94570 100644 --- a/spec/open_feature/sdk/hooks/hook_context_spec.rb +++ b/spec/open_feature/sdk/hooks/hook_context_spec.rb @@ -102,6 +102,15 @@ expect(ctx.evaluation_context.tenant).to eq("acme") end + it "copies a structure shared by two fields once" do + shared = [+"x"] + ctx = described_class.new(flag_key: "flag", flag_value_type: :boolean, default_value: false, + evaluation_context: OpenFeature::SDK::EvaluationContext.new(a: shared, b: shared)).freeze + + expect(ctx.evaluation_context.field("a")).to equal(ctx.evaluation_context.field("b")) + expect(ctx.evaluation_context.field("a")).not_to equal(shared) + end + it "is a frozen copy once the hook context is frozen" do hook_context.freeze @@ -114,13 +123,14 @@ describe "values that can't be copied" do [Mutex.new, Thread::Queue.new, -> {}].each do |uncopyable| - it "freezes a shallow copy when it holds a #{uncopyable.class}" do - default = {held: uncopyable} + it "still freezes a copy when it holds a #{uncopyable.class}" do + default = {held: uncopyable, list: []} ctx = described_class.new(flag_key: "flag", flag_value_type: :object, default_value: default, evaluation_context: nil) - expect(ctx.default_value).to eq(default) expect(ctx.default_value).to be_frozen + expect(ctx.default_value[:list]).to be_frozen expect(default).not_to be_frozen + expect(default[:list]).not_to be_frozen end end end diff --git a/spec/specification/events_spec.rb b/spec/specification/events_spec.rb index b63196d0..e23bd5a5 100644 --- a/spec/specification/events_spec.rb +++ b/spec/specification/events_spec.rb @@ -405,6 +405,28 @@ def init(_evaluation_context) end context "Requirement 5.2.5" do + specify "a handler raising NotImplementedError doesn't stop the other API or client handlers" do + called = [] + client = OpenFeature::SDK.build_client + ready = OpenFeature::SDK::ProviderEvent::PROVIDER_READY + OpenFeature::SDK.add_handler(ready, ->(_) { raise NotImplementedError, "api" }) + OpenFeature::SDK.add_handler(ready, ->(_) { called << :api }) + client.add_handler(ready, ->(_) { raise NotImplementedError, "client" }) + client.add_handler(ready, ->(_) { called << :client }) + called.clear # the NoOp provider from the before block is already READY, so each handler ran once on registration + + expect { OpenFeature::SDK.set_provider_and_wait(OpenFeature::SDK::Provider::InMemoryProvider.new) }.not_to raise_error + expect(called).to eq([:api, :client]) + end + + specify "a handler that runs immediately and raises NotImplementedError doesn't escape add_handler" do + ready = OpenFeature::SDK::ProviderEvent::PROVIDER_READY + OpenFeature::SDK.set_provider_and_wait(OpenFeature::SDK::Provider::InMemoryProvider.new) + + expect { OpenFeature::SDK.add_handler(ready, ->(_) { raise NotImplementedError, "api" }) }.not_to raise_error + expect { OpenFeature::SDK.build_client.add_handler(ready, ->(_) { raise NotImplementedError, "client" }) }.not_to raise_error + end + specify "If an event handler function terminates abnormally, other handler functions MUST still be invoked" do handler1_called = false handler2_called = false diff --git a/spec/specification/flag_evaluation_api_spec.rb b/spec/specification/flag_evaluation_api_spec.rb index 4a920b4b..39a5f061 100644 --- a/spec/specification/flag_evaluation_api_spec.rb +++ b/spec/specification/flag_evaluation_api_spec.rb @@ -835,6 +835,15 @@ def before(hook_context:, hints:) end context "Requirement 1.4.15.1" do + specify "flag_metadata values are frozen too, and the provider's hash is left alone" do + metadata = {"owner" => +"team-a"} + resolution = OpenFeature::SDK::Provider::ResolutionDetails.new(value: true, flag_metadata: metadata) + + expect { resolution.flag_metadata["owner"] << "!" }.to raise_error(FrozenError) + expect(metadata["owner"]).to eq("team-a") + expect(metadata["owner"]).not_to be_frozen + end + specify "flag_metadata is frozen when not provided" do resolution = OpenFeature::SDK::Provider::ResolutionDetails.new(value: true) expect(resolution.flag_metadata).to be_frozen @@ -862,6 +871,20 @@ def before(hook_context:, hints:) expect(OpenFeature::SDK).to respond_to(:shutdown) end + specify "a provider whose shutdown raises NotImplementedError doesn't stop the other shutdowns, or the reset (1.6.2)" do + failing = OpenFeature::SDK::Provider::InMemoryProvider.new + failing.define_singleton_method(:shutdown) { raise NotImplementedError, "shutdown" } + other = OpenFeature::SDK::Provider::InMemoryProvider.new + allow(other).to receive(:shutdown) + OpenFeature::SDK.set_provider_and_wait(failing, domain: "a") + OpenFeature::SDK.set_provider_and_wait(other, domain: "b") + OpenFeature::SDK.configure { |config| config.hooks << Class.new { include OpenFeature::SDK::Hooks::Hook }.new } + + expect { OpenFeature::SDK.shutdown }.not_to raise_error + expect(other).to have_received(:shutdown) + expect(OpenFeature::SDK.hooks).to be_empty + end + specify "When a shutdown function is called, the API invokes the shutdown function on the registered provider." do provider1 = OpenFeature::SDK::Provider::InMemoryProvider.new provider2 = OpenFeature::SDK::Provider::InMemoryProvider.new diff --git a/spec/specification/hooks_spec.rb b/spec/specification/hooks_spec.rb index 03398418..f730d753 100644 --- a/spec/specification/hooks_spec.rb +++ b/spec/specification/hooks_spec.rb @@ -241,6 +241,75 @@ def context_probe(*stages) expect(targeting_key).not_to be_frozen end + specify "after hooks can't change nested values in the caller's evaluation context" do + regions = ["us"] + outcome = nil + hook = Class.new do + include OpenFeature::SDK::Hooks::Hook + + define_method(:after) do |hook_context:, **| + hook_context.evaluation_context.field("regions") << "eu" + outcome = :changed + rescue FrozenError + outcome = :frozen + end + end.new + OpenFeature::SDK.configure { |config| config.evaluation_context = OpenFeature::SDK::EvaluationContext.new(regions:) } + + OpenFeature::SDK.build_client.fetch_boolean_value(flag_key: "flag-1", default_value: false, hooks: [hook]) + + expect(outcome).to eq(:frozen) + expect(regions).to eq(["us"]) + expect(regions).not_to be_frozen + end + + specify "objects in the evaluation context reach after hooks as they are, so reading them can't fail" do + user_class = Class.new do + def name = @name ||= +"Ada" + end + hook = Class.new do + include OpenFeature::SDK::Hooks::Hook + + define_method(:after) { |hook_context:, **| hook_context.evaluation_context.field("user").name } + end.new + + details = OpenFeature::SDK.build_client.fetch_boolean_details(flag_key: "flag-1", default_value: false, + evaluation_context: OpenFeature::SDK::EvaluationContext.new(user: user_class.new), hooks: [hook]) + + expect(details.value).to be(true) + expect(details.error_code).to be_nil + end + + specify "an after hook can convert a Time in the evaluation context, and the caller's Time is unchanged" do + signup = Time.new(2026, 1, 1, 12, 0, 0, "+05:00") + hook = Class.new do + include OpenFeature::SDK::Hooks::Hook + + define_method(:after) { |hook_context:, **| hook_context.evaluation_context.field("signup").utc } + end.new + + details = OpenFeature::SDK.build_client.fetch_boolean_details(flag_key: "flag-1", default_value: false, + evaluation_context: OpenFeature::SDK::EvaluationContext.new(signup:), hooks: [hook]) + + expect(details.error_code).to be_nil + expect(signup.utc_offset).to eq(5 * 3600) + end + + specify "an after hook can read a missing key of a Hash with a default proc in the evaluation context" do + lookup = Hash.new { |hash, key| hash[key] = [] } + hook = Class.new do + include OpenFeature::SDK::Hooks::Hook + + define_method(:after) { |hook_context:, **| hook_context.evaluation_context.field("lookup")["missing"] } + end.new + + details = OpenFeature::SDK.build_client.fetch_boolean_details(flag_key: "flag-1", default_value: false, + evaluation_context: OpenFeature::SDK::EvaluationContext.new(lookup:), hooks: [hook]) + + expect(details.error_code).to be_nil + expect(lookup).to be_empty + end + specify "the evaluation context is frozen in error and finally hooks when a before hook raises" do failing_before = Class.new do include OpenFeature::SDK::Hooks::Hook @@ -677,6 +746,26 @@ def before(hook_context:, hints:) expect(call_log).to include("surviving_finally") end + + specify "a finally hook raising NotImplementedError doesn't stop the others or change the result" do + call_log = [] + failing_hook = Class.new do + include OpenFeature::SDK::Hooks::Hook + + define_method(:finally) { |**| raise NotImplementedError, "finally" } + end.new + surviving_hook = Class.new do + include OpenFeature::SDK::Hooks::Hook + + define_method(:finally) { |**| call_log << "surviving_finally" } + end.new + + details = OpenFeature::SDK.build_client.fetch_boolean_details(flag_key: "flag-1", default_value: false, hooks: [surviving_hook, failing_hook]) + + expect(call_log).to eq(["surviving_finally"]) + expect(details.value).to be(true) + expect(details.error_code).to be_nil + end end context "Requirement 4.4.4" do @@ -706,6 +795,28 @@ def before(hook_context:, hints:) expect(call_log).to include("surviving_error") end + + specify "an error hook raising NotImplementedError doesn't stop the others, and runs once" do + call_log = [] + failing_hook = Class.new do + include OpenFeature::SDK::Hooks::Hook + + define_method(:error) do |**| + call_log << "failing_error" + raise NotImplementedError, "error hook" + end + end.new + surviving_hook = Class.new do + include OpenFeature::SDK::Hooks::Hook + + define_method(:error) { |**| call_log << "surviving_error" } + end.new + + details = OpenFeature::SDK.build_client.fetch_boolean_details(flag_key: "missing-flag", default_value: false, hooks: [surviving_hook, failing_hook]) + + expect(call_log).to eq(["failing_error", "surviving_error"]) + expect(details.error_code).to eq(OpenFeature::SDK::Provider::ErrorCode::FLAG_NOT_FOUND) + end end context "Requirement 4.4.5" do diff --git a/spec/specification/provider_spec.rb b/spec/specification/provider_spec.rb index 47809de7..b5e85975 100644 --- a/spec/specification/provider_spec.rb +++ b/spec/specification/provider_spec.rb @@ -235,6 +235,33 @@ def state_of(provider) = OpenFeature::SDK.configuration.provider_state(provider) end end + context "an init that raises NotImplementedError" do + let(:provider) do + Class.new(OpenFeature::SDK::Provider::InMemoryProvider) do + def init(_evaluation_context = nil) = raise(NotImplementedError, "init") + end.new + end + + specify "fails initialization: set_provider_and_wait raises, the status is ERROR, and one PROVIDER_ERROR fires" do + errors = 0 + OpenFeature::SDK.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_ERROR, ->(_) { errors += 1 }) + + expect { OpenFeature::SDK.set_provider_and_wait(provider) }.to raise_error(OpenFeature::SDK::ProviderInitializationError) + expect(OpenFeature::SDK.build_client.provider_status).to eq(OpenFeature::SDK::ProviderState::ERROR) + expect(errors).to eq(1) + end + + specify "fails initialization through set_provider too" do + errored = Queue.new + OpenFeature::SDK.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_ERROR, ->(_) { errored << true }) + + OpenFeature::SDK.set_provider(provider) + + expect(errored.pop(timeout: 2)).to be(true) + expect(OpenFeature::SDK.build_client.provider_status).to eq(OpenFeature::SDK::ProviderState::ERROR) + end + end + context "Condition 2.8.5" do specify "2.8.5.1: a provider without init is READY when set_provider returns" do provider = OpenFeature::SDK::Provider::NoOpProvider.new @@ -245,6 +272,66 @@ def state_of(provider) = OpenFeature::SDK.configuration.provider_state(provider) expect(state_of(provider)).to eq(OpenFeature::SDK::ProviderState::READY) end + specify "2.8.5.1: a provider without init is READY while the provider it replaces shuts down" do + in_shutdown = Queue.new + release = Queue.new + old_provider = OpenFeature::SDK::Provider::InMemoryProvider.new + old_provider.define_singleton_method(:shutdown) do + in_shutdown << true + release.pop + end + OpenFeature::SDK.set_provider_and_wait(old_provider) + + replacing = Thread.new { OpenFeature::SDK.set_provider(OpenFeature::SDK::Provider::InMemoryProvider.new({"flag" => true})) } + expect(in_shutdown.pop(timeout: 2)).to be(true) + status = OpenFeature::SDK.build_client.provider_status + release << true + replacing.join(2) + + expect(status).to eq(OpenFeature::SDK::ProviderState::READY) + end + + specify "2.8.5.1: a READY handler added while the replaced provider shuts down runs once" do + in_shutdown = Queue.new + release = Queue.new + old_provider = OpenFeature::SDK::Provider::InMemoryProvider.new + old_provider.define_singleton_method(:shutdown) do + in_shutdown << true + release.pop + end + OpenFeature::SDK.set_provider_and_wait(old_provider) + client = OpenFeature::SDK.build_client + runs = Hash.new(0) + + replacing = Thread.new { OpenFeature::SDK.set_provider(OpenFeature::SDK::Provider::InMemoryProvider.new) } + expect(in_shutdown.pop(timeout: 2)).to be(true) + OpenFeature::SDK.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_READY, ->(_) { runs[:api] += 1 }) + client.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_READY, ->(_) { runs[:client] += 1 }) + release << true + replacing.join(2) + + expect(runs).to eq(api: 1, client: 1) + end + + specify "2.8.5.1: a READY handler that rolls back to the old provider doesn't get it shut down" do + shut_down = false + old_provider = OpenFeature::SDK::Provider::InMemoryProvider.new + old_provider.define_singleton_method(:shutdown) { shut_down = true } + OpenFeature::SDK.set_provider_and_wait(old_provider) + rolled_back = false + OpenFeature::SDK.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_READY, ->(_) do + next if rolled_back || OpenFeature::SDK.provider.equal?(old_provider) + + rolled_back = true + OpenFeature::SDK.set_provider(old_provider) + end) + + OpenFeature::SDK.set_provider(OpenFeature::SDK::Provider::InMemoryProvider.new) + + expect(OpenFeature::SDK.provider).to equal(old_provider) + expect(shut_down).to be(false) + end + specify "2.8.5.1: READY handlers run for a provider without init" do ready_calls = calls_for(OpenFeature::SDK::ProviderEvent::PROVIDER_READY) expect(Thread).not_to receive(:new) diff --git a/spec/specification/tracking_spec.rb b/spec/specification/tracking_spec.rb index a8652aed..5d1ed006 100644 --- a/spec/specification/tracking_spec.rb +++ b/spec/specification/tracking_spec.rb @@ -7,6 +7,16 @@ OpenFeature::SDK::API.instance.send(:configuration).send(:reset) end + context "Conditional Requirement 6.1.1.1: returns nothing" do + specify "track returns nil even when the provider's track returns a value" do + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + provider.define_singleton_method(:track) { |*, **| :provider_return_value } + OpenFeature::SDK.set_provider_and_wait(provider) + + expect(OpenFeature::SDK.build_client.track("event")).to be_nil + end + end + context "6.1 - Tracking API" do context "Condition 6.1.1.1" do specify "The client MUST define a function for tracking with parameters: tracking event name (required), evaluation context (optional), and tracking event details (optional)" do