From f5092af8b94a58c09ab74f7d3870229a6424776e Mon Sep 17 00:00:00 2001 From: Andrew Shannon Brown Date: Mon, 23 Jul 2018 17:09:29 -0700 Subject: [PATCH 01/21] Remove @ashanbrown from codeowners --- CODEOWNERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CODEOWNERS b/CODEOWNERS index 44429ee1..8b137891 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -1 +1 @@ -* @ashanbrown + From fd63b2b84cd7806bbbacb094b0ac3ce2502fe94f Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 1 Aug 2018 17:40:01 -0700 Subject: [PATCH 02/21] log exception stacktraces at debug level --- lib/ldclient-rb/events.rb | 4 ++-- lib/ldclient-rb/ldclient.rb | 12 +++--------- lib/ldclient-rb/util.rb | 5 +++++ spec/ldclient_spec.rb | 11 ----------- 4 files changed, 10 insertions(+), 22 deletions(-) diff --git a/lib/ldclient-rb/events.rb b/lib/ldclient-rb/events.rb index 0c9a0ece..202fc235 100644 --- a/lib/ldclient-rb/events.rb +++ b/lib/ldclient-rb/events.rb @@ -142,7 +142,7 @@ def main_loop(queue, buffer, flush_workers) message.completed end rescue => e - @config.logger.warn { "[LDClient] Unexpected error in event processor: #{e.inspect}. \nTrace: #{e.backtrace}" } + Util.log_exception(@config.logger, "Unexpected error in event processor", e) end end end @@ -226,7 +226,7 @@ def trigger_flush(buffer, flush_workers) resp = EventPayloadSendTask.new.run(@sdk_key, @config, @client, payload, @formatter) handle_response(resp) if !resp.nil? rescue => e - @config.logger.warn { "[LDClient] Unexpected error in event processor: #{e.inspect}. \nTrace: #{e.backtrace}" } + Util.log_exception(@config.logger, "Unexpected error in event processor", e) end end buffer.clear if success # Reset our internal state, these events now belong to the flush worker diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index 5c0e872d..3f0f6d9a 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -162,7 +162,7 @@ def variation(key, user, default) @event_processor.add_event(make_feature_event(feature, user, res[:variation], value, default)) return value rescue => exn - @config.logger.warn { "[LDClient] Error evaluating feature flag: #{exn.inspect}. \nTrace: #{exn.backtrace}" } + Util.log_exception(@config.logger, "Error evaluating feature flag", exn) @event_processor.add_event(make_feature_event(feature, user, nil, default, default)) return default end @@ -210,7 +210,7 @@ def all_flags(user) # TODO rescue if necessary Hash[features.map{ |k, f| [k, evaluate(f, user, @store, @config.logger)[:value]] }] rescue => exn - @config.logger.warn { "[LDClient] Error evaluating all flags: #{exn.inspect}. \nTrace: #{exn.backtrace}" } + Util.log_exception(@config.logger, "Error evaluating all flags", exn) return Hash.new end end @@ -226,12 +226,6 @@ def close @store.stop end - def log_exception(caller, exn) - error_traceback = "#{exn.inspect} #{exn}\n\t#{exn.backtrace.join("\n\t")}" - error = "[LDClient] Unexpected exception in #{caller}: #{error_traceback}" - @config.logger.error { error } - end - def sanitize_user(user) if user[:key] user[:key] = user[:key].to_s @@ -252,7 +246,7 @@ def make_feature_event(flag, user, variation, value, default) } end - private :evaluate, :log_exception, :sanitize_user, :make_feature_event + private :evaluate, :sanitize_user, :make_feature_event end # diff --git a/lib/ldclient-rb/util.rb b/lib/ldclient-rb/util.rb index 6ba70dbc..99ee2477 100644 --- a/lib/ldclient-rb/util.rb +++ b/lib/ldclient-rb/util.rb @@ -1,6 +1,11 @@ module LaunchDarkly module Util + def self.log_exception(logger, message, exc) + logger.warn { "[LDClient] #{message}: #{exc.inspect}" } + logger.debug { "[LDClient] Exception trace: #{exc.backtrace}" } + end + def self.http_error_recoverable?(status) if status >= 400 && status < 500 status == 400 || status == 408 || status == 429 diff --git a/spec/ldclient_spec.rb b/spec/ldclient_spec.rb index 8e4b5eb5..68c57166 100644 --- a/spec/ldclient_spec.rb +++ b/spec/ldclient_spec.rb @@ -130,17 +130,6 @@ def event_processor end end - describe '#log_exception' do - it "log error data" do - expect(client.instance_variable_get(:@config).logger).to receive(:error) - begin - raise StandardError.new 'asdf' - rescue StandardError => exn - client.send(:log_exception, 'caller', exn) - end - end - end - describe 'with send_events: false' do let(:config) { LaunchDarkly::Config.new({offline: true, send_events: false, update_processor: update_processor}) } let(:client) { subject.new("secret", config) } From d4be186ed2026056dd9768fd2b265181f9353c72 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 1 Aug 2018 17:48:15 -0700 Subject: [PATCH 03/21] re-add minimal unit test --- spec/util_spec.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 spec/util_spec.rb diff --git a/spec/util_spec.rb b/spec/util_spec.rb new file mode 100644 index 00000000..25881aaa --- /dev/null +++ b/spec/util_spec.rb @@ -0,0 +1,17 @@ +require "spec_helper" + +describe LaunchDarkly::Util do + describe 'log_exception' do + let(:logger) { double() } + + it "logs error data" do + expect(logger).to receive(:warn) + expect(logger).to receive(:debug) + begin + raise StandardError.new 'asdf' + rescue StandardError => exn + LaunchDarkly::Util.log_exception(logger, "message", exn) + end + end + end +end From d73d66c19c03511905aa9eef827bb656b19791be Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 1 Aug 2018 17:51:32 -0700 Subject: [PATCH 04/21] log exceptions at error level --- lib/ldclient-rb/util.rb | 2 +- spec/util_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ldclient-rb/util.rb b/lib/ldclient-rb/util.rb index 99ee2477..707ba3ce 100644 --- a/lib/ldclient-rb/util.rb +++ b/lib/ldclient-rb/util.rb @@ -2,7 +2,7 @@ module LaunchDarkly module Util def self.log_exception(logger, message, exc) - logger.warn { "[LDClient] #{message}: #{exc.inspect}" } + logger.error { "[LDClient] #{message}: #{exc.inspect}" } logger.debug { "[LDClient] Exception trace: #{exc.backtrace}" } end diff --git a/spec/util_spec.rb b/spec/util_spec.rb index 25881aaa..50a72f76 100644 --- a/spec/util_spec.rb +++ b/spec/util_spec.rb @@ -5,7 +5,7 @@ let(:logger) { double() } it "logs error data" do - expect(logger).to receive(:warn) + expect(logger).to receive(:error) expect(logger).to receive(:debug) begin raise StandardError.new 'asdf' From ca15234e9214701061528d3ce702c20d34d3a9a9 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 17 Aug 2018 16:30:19 -0700 Subject: [PATCH 05/21] add new version of all_flags that captures more metadata --- lib/ldclient-rb.rb | 1 + lib/ldclient-rb/flags_state.rb | 51 +++++++++++++++++++ lib/ldclient-rb/ldclient.rb | 44 ++++++++++++---- spec/ldclient_spec.rb | 91 ++++++++++++++++++++++++++++++++++ 4 files changed, 176 insertions(+), 11 deletions(-) create mode 100644 lib/ldclient-rb/flags_state.rb diff --git a/lib/ldclient-rb.rb b/lib/ldclient-rb.rb index ce9d0307..7264b220 100644 --- a/lib/ldclient-rb.rb +++ b/lib/ldclient-rb.rb @@ -1,6 +1,7 @@ require "ldclient-rb/version" require "ldclient-rb/util" require "ldclient-rb/evaluation" +require "ldclient-rb/flags_state" require "ldclient-rb/ldclient" require "ldclient-rb/cache_store" require "ldclient-rb/expiring_cache" diff --git a/lib/ldclient-rb/flags_state.rb b/lib/ldclient-rb/flags_state.rb new file mode 100644 index 00000000..f68dc20b --- /dev/null +++ b/lib/ldclient-rb/flags_state.rb @@ -0,0 +1,51 @@ + +module LaunchDarkly + # + # A snapshot of the state of all feature flags with regard to a specific user, generated by + # calling the client's all_flags_state method. + # + class FeatureFlagsState + def initialize(valid) + @flag_values = {} + @flag_metadata = {} + @valid = valid + end + + # Used internally to build the state map. + def add_flag(flag, value, variation) + key = flag[:key] + @flag_values[key] = value + meta = { version: flag[:version], trackEvents: flag[:trackEvents] } + meta[:variation] = variation if !variation.nil? + meta[:debugEventsUntilDate] = flag[:debugEventsUntilDate] if flag[:debugEventsUntilDate] + @flag_metadata[key] = meta + end + + # Returns true if this object contains a valid snapshot of feature flag state, or false if the + # state could not be computed (for instance, because the client was offline or there was no user). + def valid? + @valid + end + + # Returns the value of an individual feature flag at the time the state was recorded. + # Returns nil if the flag returned the default value, or if there was no such flag. + def flag_value(key) + @flag_values[key] + end + + # Returns a map of flag keys to flag values. If a flag would have evaluated to the default value, + # its value will be nil. + def values_map + @flag_values + end + + # Returns a JSON string representation of the entire state map, in the format used by the + # LaunchDarkly JavaScript SDK. Use this method if you are passing data to the front end that + # will be used to "bootstrap" the JavaScript client. + def json_string + ret = @flag_values.clone + ret['$flagsState'] = @flag_metadata + ret.to_json + end + end +end diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index 3f0f6d9a..5c64b7e7 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -193,26 +193,48 @@ def track(event_name, user, data) end # - # Returns all feature flag values for the given user + # Returns all feature flag values for the given user. This method is deprecated - please use + # all_flags_state instead. Current versions of the client-side SDK (2.0.0 and later) will not + # generate analytics events correctly if you pass the result of all_flags. # def all_flags(user) - sanitize_user(user) - return Hash.new if @config.offline? + all_flags_state(user).values_map + end - unless user - @config.logger.error { "[LDClient] Must specify user in all_flags" } - return Hash.new + # + # Returns a FeatureFlagsState object that encapsulates the state of all feature flags for a given user, + # including the flag values and also metadata that can be used on the front end. This method does not + # send analytics events back to LaunchDarkly. + # + def all_flags_state(user) + return FeatureFlagsState.new(false) if @config.offline? + + unless user && !user[:key].nil? + @config.logger.error { "[LDClient] User and user key must be specified in all_flags_state" } + return FeatureFlagsState.new(false) end + sanitize_user(user) + begin features = @store.all(FEATURES) - - # TODO rescue if necessary - Hash[features.map{ |k, f| [k, evaluate(f, user, @store, @config.logger)[:value]] }] rescue => exn - Util.log_exception(@config.logger, "Error evaluating all flags", exn) - return Hash.new + Util.log_exception(@config.logger, "Unable to read flags for all_flags_state", exn) + return FeatureFlagsState.new(false) + end + + state = FeatureFlagsState.new(true) + features.each do |k, f| + begin + result = evaluate(f, user, @store, @config.logger) + state.add_flag(f, result[:value], result[:variation]) + rescue => exn + Util.log_exception(@config.logger, "Error evaluating flag \"#{k}\" in all_flags_state", exn) + state.add_flag(f, nil, nil) + end end + + state end # diff --git a/spec/ldclient_spec.rb b/spec/ldclient_spec.rb index 68c57166..9d13dee0 100644 --- a/spec/ldclient_spec.rb +++ b/spec/ldclient_spec.rb @@ -99,6 +99,97 @@ def event_processor end end + describe '#all_flags' do + let(:flag1) { { key: "key1", offVariation: 0, variations: [ 'value1' ] } } + let(:flag2) { { key: "key2", offVariation: 0, variations: [ 'value2' ] } } + + it "returns flag values" do + config.feature_store.init({ LaunchDarkly::FEATURES => { 'key1' => flag1, 'key2' => flag2 } }) + + result = client.all_flags({ key: 'userkey' }) + expect(result).to eq({ 'key1' => 'value1', 'key2' => 'value2' }) + end + + it "returns empty map for nil user" do + config.feature_store.init({ LaunchDarkly::FEATURES => { 'key1' => flag1, 'key2' => flag2 } }) + + result = client.all_flags(nil) + expect(result).to eq({}) + end + + it "returns empty map for nil user key" do + config.feature_store.init({ LaunchDarkly::FEATURES => { 'key1' => flag1, 'key2' => flag2 } }) + + result = client.all_flags({}) + expect(result).to eq({}) + end + + it "returns empty map if offline" do + offline_config.feature_store.init({ LaunchDarkly::FEATURES => { 'key1' => flag1, 'key2' => flag2 } }) + + result = offline_client.all_flags(nil) + expect(result).to eq({}) + end + end + + describe '#all_flags_state' do + let(:flag1) { { key: "key1", version: 100, offVariation: 0, variations: [ 'value1' ], trackEvents: false } } + let(:flag2) { { key: "key2", version: 200, offVariation: 1, variations: [ 'x', 'value2' ], trackEvents: true, debugEventsUntilDate: 1000 } } + + it "returns flags state" do + config.feature_store.init({ LaunchDarkly::FEATURES => { 'key1' => flag1, 'key2' => flag2 } }) + + state = client.all_flags_state({ key: 'userkey' }) + expect(state.valid?).to be true + + values = state.values_map + expect(values).to eq({ 'key1' => 'value1', 'key2' => 'value2' }) + + result = JSON.parse(state.json_string) + expect(result).to eq({ + 'key1' => 'value1', + 'key2' => 'value2', + '$flagsState' => { + 'key1' => { + 'variation' => 0, + 'version' => 100, + 'trackEvents' => false + }, + 'key2' => { + 'variation' => 1, + 'version' => 200, + 'trackEvents' => true, + 'debugEventsUntilDate' => 1000 + } + } + }) + end + + it "returns empty state for nil user" do + config.feature_store.init({ LaunchDarkly::FEATURES => { 'key1' => flag1, 'key2' => flag2 } }) + + state = client.all_flags_state(nil) + expect(state.valid?).to be false + expect(state.values_map).to eq({}) + end + + it "returns empty state for nil user key" do + config.feature_store.init({ LaunchDarkly::FEATURES => { 'key1' => flag1, 'key2' => flag2 } }) + + state = client.all_flags_state({}) + expect(state.valid?).to be false + expect(state.values_map).to eq({}) + end + + it "returns empty state if offline" do + offline_config.feature_store.init({ LaunchDarkly::FEATURES => { 'key1' => flag1, 'key2' => flag2 } }) + + state = offline_client.all_flags_state({ key: 'userkey' }) + expect(state.valid?).to be false + expect(state.values_map).to eq({}) + end + end + describe '#secure_mode_hash' do it "will return the expected value for a known message and secret" do result = client.secure_mode_hash({key: :Message}) From ed19523fd0d93306204929248e179945fdabf10f Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 17 Aug 2018 16:37:43 -0700 Subject: [PATCH 06/21] add tests for FeatureFlagsState --- spec/flags_state_spec.rb | 56 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 spec/flags_state_spec.rb diff --git a/spec/flags_state_spec.rb b/spec/flags_state_spec.rb new file mode 100644 index 00000000..9241028d --- /dev/null +++ b/spec/flags_state_spec.rb @@ -0,0 +1,56 @@ +require "spec_helper" + +describe LaunchDarkly::FeatureFlagsState do + subject { LaunchDarkly::FeatureFlagsState } + + it "can get flag value" do + state = subject.new(true) + flag = { key: 'key' } + state.add_flag(flag, 'value', 1) + + expect(state.flag_value('key')).to eq 'value' + end + + it "returns nil for unknown flag" do + state = subject.new(true) + + expect(state.flag_value('key')).to be nil + end + + it "can be converted to values map" do + state = subject.new(true) + flag1 = { key: 'key1' } + flag2 = { key: 'key2' } + state.add_flag(flag1, 'value1', 0) + state.add_flag(flag2, 'value2', 1) + + expect(state.values_map).to eq({ 'key1' => 'value1', 'key2' => 'value2' }) + end + + it "can be converted to JSON string" do + state = subject.new(true) + flag1 = { key: "key1", version: 100, offVariation: 0, variations: [ 'value1' ], trackEvents: false } + flag2 = { key: "key2", version: 200, offVariation: 1, variations: [ 'x', 'value2' ], trackEvents: true, debugEventsUntilDate: 1000 } + state.add_flag(flag1, 'value1', 0) + state.add_flag(flag2, 'value2', 1) + + result = JSON.parse(state.json_string) + expect(result).to eq({ + 'key1' => 'value1', + 'key2' => 'value2', + '$flagsState' => { + 'key1' => { + 'variation' => 0, + 'version' => 100, + 'trackEvents' => false + }, + 'key2' => { + 'variation' => 1, + 'version' => 200, + 'trackEvents' => true, + 'debugEventsUntilDate' => 1000 + } + } + }) + end +end From 73f2d892fa166b5ccf2b68f268f77c04a49462ee Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Mon, 20 Aug 2018 12:59:10 -0700 Subject: [PATCH 07/21] provide as_json method that returns a hash instead of just a string --- lib/ldclient-rb/flags_state.rb | 22 +++++++++++++++++----- lib/ldclient-rb/ldclient.rb | 4 ++-- spec/flags_state_spec.rb | 30 +++++++++++++++++++++--------- spec/ldclient_spec.rb | 16 ++++++++-------- 4 files changed, 48 insertions(+), 24 deletions(-) diff --git a/lib/ldclient-rb/flags_state.rb b/lib/ldclient-rb/flags_state.rb index f68dc20b..a5af6c5a 100644 --- a/lib/ldclient-rb/flags_state.rb +++ b/lib/ldclient-rb/flags_state.rb @@ -1,3 +1,4 @@ +require 'json' module LaunchDarkly # @@ -35,17 +36,28 @@ def flag_value(key) # Returns a map of flag keys to flag values. If a flag would have evaluated to the default value, # its value will be nil. + # + # Do not use this method if you are passing data to the front end to "bootstrap" the JavaScript client. + # Instead, use as_json. def values_map @flag_values end - # Returns a JSON string representation of the entire state map, in the format used by the - # LaunchDarkly JavaScript SDK. Use this method if you are passing data to the front end that - # will be used to "bootstrap" the JavaScript client. - def json_string + # Returns a hash that can be used as a JSON representation of the entire state map, in the format + # used by the LaunchDarkly JavaScript SDK. Use this method if you are passing data to the front end + # in order to "bootstrap" the JavaScript client. + # + # Do not rely on the exact shape of this data, as it may change in future to support the needs of + # the JavaScript client. + def as_json(*) # parameter is unused, but may be passed if we're using the json gem ret = @flag_values.clone ret['$flagsState'] = @flag_metadata - ret.to_json + ret + end + + # Same as as_json, but converts the JSON structure into a string. + def to_json(*a) + as_json.to_json(a) end end end diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index 5c64b7e7..c8addbca 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -194,8 +194,8 @@ def track(event_name, user, data) # # Returns all feature flag values for the given user. This method is deprecated - please use - # all_flags_state instead. Current versions of the client-side SDK (2.0.0 and later) will not - # generate analytics events correctly if you pass the result of all_flags. + # all_flags_state instead. Current versions of the client-side SDK will not generate analytics + # events correctly if you pass the result of all_flags. # def all_flags(user) all_flags_state(user).values_map diff --git a/spec/flags_state_spec.rb b/spec/flags_state_spec.rb index 9241028d..e6e1c17c 100644 --- a/spec/flags_state_spec.rb +++ b/spec/flags_state_spec.rb @@ -27,30 +27,42 @@ expect(state.values_map).to eq({ 'key1' => 'value1', 'key2' => 'value2' }) end - it "can be converted to JSON string" do + it "can be converted to JSON structure" do state = subject.new(true) flag1 = { key: "key1", version: 100, offVariation: 0, variations: [ 'value1' ], trackEvents: false } flag2 = { key: "key2", version: 200, offVariation: 1, variations: [ 'x', 'value2' ], trackEvents: true, debugEventsUntilDate: 1000 } state.add_flag(flag1, 'value1', 0) state.add_flag(flag2, 'value2', 1) - result = JSON.parse(state.json_string) + result = state.as_json expect(result).to eq({ 'key1' => 'value1', 'key2' => 'value2', '$flagsState' => { 'key1' => { - 'variation' => 0, - 'version' => 100, - 'trackEvents' => false + :variation => 0, + :version => 100, + :trackEvents => false }, 'key2' => { - 'variation' => 1, - 'version' => 200, - 'trackEvents' => true, - 'debugEventsUntilDate' => 1000 + :variation => 1, + :version => 200, + :trackEvents => true, + :debugEventsUntilDate => 1000 } } }) end + + it "can be converted to JSON string" do + state = subject.new(true) + flag1 = { key: "key1", version: 100, offVariation: 0, variations: [ 'value1' ], trackEvents: false } + flag2 = { key: "key2", version: 200, offVariation: 1, variations: [ 'x', 'value2' ], trackEvents: true, debugEventsUntilDate: 1000 } + state.add_flag(flag1, 'value1', 0) + state.add_flag(flag2, 'value2', 1) + + object = state.as_json + str = state.to_json + expect(object.to_json).to eq(str) + end end diff --git a/spec/ldclient_spec.rb b/spec/ldclient_spec.rb index 9d13dee0..b5939ea1 100644 --- a/spec/ldclient_spec.rb +++ b/spec/ldclient_spec.rb @@ -145,21 +145,21 @@ def event_processor values = state.values_map expect(values).to eq({ 'key1' => 'value1', 'key2' => 'value2' }) - result = JSON.parse(state.json_string) + result = state.as_json expect(result).to eq({ 'key1' => 'value1', 'key2' => 'value2', '$flagsState' => { 'key1' => { - 'variation' => 0, - 'version' => 100, - 'trackEvents' => false + :variation => 0, + :version => 100, + :trackEvents => false }, 'key2' => { - 'variation' => 1, - 'version' => 200, - 'trackEvents' => true, - 'debugEventsUntilDate' => 1000 + :variation => 1, + :version => 200, + :trackEvents => true, + :debugEventsUntilDate => 1000 } } }) From ab896b1e801f944166c5525e6aa1d00cf333da0b Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Mon, 20 Aug 2018 20:01:32 -0700 Subject: [PATCH 08/21] state can be serialized with JSON.generate --- lib/ldclient-rb/flags_state.rb | 5 ++++- spec/flags_state_spec.rb | 16 +++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/lib/ldclient-rb/flags_state.rb b/lib/ldclient-rb/flags_state.rb index a5af6c5a..09f88975 100644 --- a/lib/ldclient-rb/flags_state.rb +++ b/lib/ldclient-rb/flags_state.rb @@ -3,7 +3,9 @@ module LaunchDarkly # # A snapshot of the state of all feature flags with regard to a specific user, generated by - # calling the client's all_flags_state method. + # calling the client's all_flags_state method. Serializing this object to JSON using + # JSON.generate (or the to_json method) will produce the appropriate data structure for + # bootstrapping the LaunchDarkly JavaScript client. # class FeatureFlagsState def initialize(valid) @@ -52,6 +54,7 @@ def values_map def as_json(*) # parameter is unused, but may be passed if we're using the json gem ret = @flag_values.clone ret['$flagsState'] = @flag_metadata + ret['$valid'] = @valid ret end diff --git a/spec/flags_state_spec.rb b/spec/flags_state_spec.rb index e6e1c17c..3d21029b 100644 --- a/spec/flags_state_spec.rb +++ b/spec/flags_state_spec.rb @@ -1,4 +1,5 @@ require "spec_helper" +require "json" describe LaunchDarkly::FeatureFlagsState do subject { LaunchDarkly::FeatureFlagsState } @@ -50,7 +51,8 @@ :trackEvents => true, :debugEventsUntilDate => 1000 } - } + }, + '$valid' => true }) end @@ -65,4 +67,16 @@ str = state.to_json expect(object.to_json).to eq(str) end + + it "uses our custom serializer with JSON.generate" do + state = subject.new(true) + flag1 = { key: "key1", version: 100, offVariation: 0, variations: [ 'value1' ], trackEvents: false } + flag2 = { key: "key2", version: 200, offVariation: 1, variations: [ 'x', 'value2' ], trackEvents: true, debugEventsUntilDate: 1000 } + state.add_flag(flag1, 'value1', 0) + state.add_flag(flag2, 'value2', 1) + + stringFromToJson = state.to_json + stringFromGenerate = JSON.generate(state) + expect(stringFromGenerate).to eq(stringFromToJson) + end end From 00347c66ae17167910d316617e061d85f6793681 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Mon, 20 Aug 2018 20:02:41 -0700 Subject: [PATCH 09/21] add $valid --- spec/ldclient_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/ldclient_spec.rb b/spec/ldclient_spec.rb index b5939ea1..5dbb8195 100644 --- a/spec/ldclient_spec.rb +++ b/spec/ldclient_spec.rb @@ -161,7 +161,8 @@ def event_processor :trackEvents => true, :debugEventsUntilDate => 1000 } - } + }, + '$valid' => true }) end From bdac27e1cf37e2c95c4455d705a99aaa2a948b28 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Tue, 21 Aug 2018 11:46:14 -0700 Subject: [PATCH 10/21] add ability to filter for only client-side flags --- lib/ldclient-rb/ldclient.rb | 17 +++++++++++++++-- spec/ldclient_spec.rb | 16 ++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index c8addbca..e9873679 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -194,9 +194,12 @@ def track(event_name, user, data) # # Returns all feature flag values for the given user. This method is deprecated - please use - # all_flags_state instead. Current versions of the client-side SDK will not generate analytics + # {#all_flags_state} instead. Current versions of the client-side SDK will not generate analytics # events correctly if you pass the result of all_flags. # + # @param user [Hash] The end user requesting the feature flags + # @return [Hash] a hash of feature flag keys to values + # def all_flags(user) all_flags_state(user).values_map end @@ -206,7 +209,13 @@ def all_flags(user) # including the flag values and also metadata that can be used on the front end. This method does not # send analytics events back to LaunchDarkly. # - def all_flags_state(user) + # @param user [Hash] The end user requesting the feature flags + # @param options={} [Hash] Optional parameters to control how the state is generated + # @option options [Boolean] :client_side_only (false) True if only flags marked for use with the + # client-side SDK should be included in the state. By default, all flags are included. + # @return [FeatureFlagsState] a FeatureFlagsState object which can be serialized to JSON + # + def all_flags_state(user, options={}) return FeatureFlagsState.new(false) if @config.offline? unless user && !user[:key].nil? @@ -224,7 +233,11 @@ def all_flags_state(user) end state = FeatureFlagsState.new(true) + client_only = options[:client_side_only] || false features.each do |k, f| + if client_only && !f[:clientSide] + next + end begin result = evaluate(f, user, @store, @config.logger) state.add_flag(f, result[:value], result[:variation]) diff --git a/spec/ldclient_spec.rb b/spec/ldclient_spec.rb index 5dbb8195..ae76a678 100644 --- a/spec/ldclient_spec.rb +++ b/spec/ldclient_spec.rb @@ -166,6 +166,22 @@ def event_processor }) end + it "can be filtered for only client-side flags" do + flag1 = { key: "server-side-1", offVariation: 0, variations: [ 'a' ], clientSide: false } + flag2 = { key: "server-side-2", offVariation: 0, variations: [ 'b' ], clientSide: false } + flag3 = { key: "client-side-1", offVariation: 0, variations: [ 'value1' ], clientSide: true } + flag4 = { key: "client-side-2", offVariation: 0, variations: [ 'value2' ], clientSide: true } + config.feature_store.init({ LaunchDarkly::FEATURES => { + flag1[:key] => flag1, flag2[:key] => flag2, flag3[:key] => flag3, flag4[:key] => flag4 + }}) + + state = client.all_flags_state({ key: 'userkey' }, client_side_only: true) + expect(state.valid?).to be true + + values = state.values_map + expect(values).to eq({ 'client-side-1' => 'value1', 'client-side-2' => 'value2' }) + end + it "returns empty state for nil user" do config.feature_store.init({ LaunchDarkly::FEATURES => { 'key1' => flag1, 'key2' => flag2 } }) From cee4c18aa0a6330cd3e24f6c9b11914cae57d34d Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 22 Aug 2018 19:58:42 -0700 Subject: [PATCH 11/21] implement evaluation with explanations --- lib/ldclient-rb/evaluation.rb | 185 +++++++++++++++++---------- lib/ldclient-rb/events.rb | 1 + lib/ldclient-rb/flags_state.rb | 3 +- lib/ldclient-rb/ldclient.rb | 124 ++++++++++-------- spec/evaluation_spec.rb | 221 +++++++++++++++++++-------------- spec/ldclient_spec.rb | 98 ++++++++++++--- 6 files changed, 403 insertions(+), 229 deletions(-) diff --git a/lib/ldclient-rb/evaluation.rb b/lib/ldclient-rb/evaluation.rb index b4dd796c..b803f4a2 100644 --- a/lib/ldclient-rb/evaluation.rb +++ b/lib/ldclient-rb/evaluation.rb @@ -2,6 +2,31 @@ require "semantic" module LaunchDarkly + # An object returned by `LDClient.variation_detail`, combining the result of a flag evaluation with + # an explanation of how it was calculated. + class EvaluationDetail + def initialize(value, variation, reason) + @value = value + @variation = variation + @reason = reason + end + + # @return [Object] The result of the flag evaluation. This will be either one of the flag's + # variations or the default value that was passed to the `variation` method. + attr_reader :value + + # @return [int|nil] The index of the returned value within the flag's list of variations, e.g. + # 0 for the first variation - or `nil` if the default value was returned. + attr_reader :variation + + # @return [Hash] An object describing the main factor that influenced the flag evaluation value. + attr_reader :reason + + def ==(other) + @value == other.value && @variation == other.variation && @reason == other.reason + end + end + module Evaluation BUILTINS = [:key, :ip, :country, :email, :firstName, :lastName, :avatar, :name, :anonymous] @@ -110,101 +135,109 @@ def self.comparator(converter) class EvaluationError < StandardError end - # Evaluates a feature flag, returning a hash containing the evaluation result and any events - # generated during prerequisite evaluation. Raises EvaluationError if the flag is not well-formed - # Will return nil, but not raise an exception, indicating that the rules (including fallthrough) did not match - # In that case, the caller should return the default value. - def evaluate(flag, user, store, logger) - if flag.nil? - raise EvaluationError, "Flag does not exist" - end + # Used internally to hold an evaluation result and the events that were generated from prerequisites. + EvalResult = Struct.new(:detail, :events) + + def error_result(errorKind, value = nil) + EvaluationDetail.new(value, nil, { kind: 'ERROR', errorKind: errorKind }) + end + # Evaluates a feature flag and returns an EvalResult. The result.value will be nil if the flag returns + # the default value. Error conditions produce a result with an error reason, not an exception. + def evaluate(flag, user, store, logger) if user.nil? || user[:key].nil? - raise EvaluationError, "Invalid user" + return EvalResult.new(error_result('USER_NOT_SPECIFIED'), []) end events = [] if flag[:on] - res = eval_internal(flag, user, store, events, logger) - if !res.nil? - res[:events] = events - return res + detail = eval_internal(flag, user, store, events, logger) + return EvalResult.new(detail, events) + end + + return EvalResult.new(get_off_value(flag, { kind: 'OFF' }), events) + end + + + def eval_internal(flag, user, store, events, logger) + prereq_failure_reason = check_prerequisites(flag, user, store, events, logger) + if !prereq_failure_reason.nil? + return get_off_value(flag, prereq_failure_reason) + end + + # Check user target matches + (flag[:targets] || []).each do |target| + (target[:values] || []).each do |value| + if value == user[:key] + return get_variation(flag, target[:variation], { kind: 'TARGET_MATCH' }) + end + end + end + + # Check custom rules + rules = flag[:rules] || [] + rules.each_index do |i| + rule = rules[i] + if rule_match_user(rule, user, store) + return get_value_for_variation_or_rollout(flag, rule, user, + { kind: 'RULE_MATCH', ruleIndex: i, ruleId: rule[:id] }, logger) end end - offVariation = flag[:offVariation] - if !offVariation.nil? && offVariation < flag[:variations].length - value = flag[:variations][offVariation] - return { variation: offVariation, value: value, events: events } + # Check the fallthrough rule + if !flag[:fallthrough].nil? + return get_value_for_variation_or_rollout(flag, flag[:fallthrough], user, + { kind: 'FALLTHROUGH' }, logger) end - { variation: nil, value: nil, events: events } + return EvaluationDetail.new(nil, nil, { kind: 'FALLTHROUGH' }) end - def eval_internal(flag, user, store, events, logger) - failed_prereq = false - # Evaluate prerequisites, if any + def check_prerequisites(flag, user, store, events, logger) + failed_prereqs = [] + (flag[:prerequisites] || []).each do |prerequisite| - prereq_flag = store.get(FEATURES, prerequisite[:key]) + prereq_ok = true + prereq_key = prerequisite[:key] + prereq_flag = store.get(FEATURES, prereq_key) if prereq_flag.nil? || !prereq_flag[:on] - failed_prereq = true + logger.error { "[LDClient] Could not retrieve prerequisite flag \"#{prereq_key}\" when evaluating \"#{flag[:key]}\"" } + prereq_ok = false + elsif !prereq_flag[:on] + prereq_ok = false else begin prereq_res = eval_internal(prereq_flag, user, store, events, logger) event = { kind: "feature", - key: prereq_flag[:key], - variation: prereq_res.nil? ? nil : prereq_res[:variation], - value: prereq_res.nil? ? nil : prereq_res[:value], + key: prereq_key, + variation: prereq_res.variation, + value: prereq_res.value, version: prereq_flag[:version], prereqOf: flag[:key], trackEvents: prereq_flag[:trackEvents], debugEventsUntilDate: prereq_flag[:debugEventsUntilDate] } events.push(event) - if prereq_res.nil? || prereq_res[:variation] != prerequisite[:variation] - failed_prereq = true + if prereq_res.variation != prerequisite[:variation] + prereq_ok = false end rescue => exn - logger.error { "[LDClient] Error evaluating prerequisite: #{exn.inspect}" } - failed_prereq = true + Util.log_exception(logger, "Error evaluating prerequisite flag \"#{prereq_key}\" for flag \"{flag[:key]}\"", exn) + prereq_ok = false end end - end - - if failed_prereq - return nil - end - # The prerequisites were satisfied. - # Now walk through the evaluation steps and get the correct - # variation index - eval_rules(flag, user, store) - end - - def eval_rules(flag, user, store) - # Check user target matches - (flag[:targets] || []).each do |target| - (target[:values] || []).each do |value| - if value == user[:key] - return { variation: target[:variation], value: get_variation(flag, target[:variation]) } - end + if !prereq_ok + failed_prereqs.push(prereq_key) end end - - # Check custom rules - (flag[:rules] || []).each do |rule| - return variation_for_user(rule, user, flag) if rule_match_user(rule, user, store) - end - # Check the fallthrough rule - if !flag[:fallthrough].nil? - return variation_for_user(flag[:fallthrough], user, flag) + if failed_prereqs.empty? + return nil end - - # Not even the fallthrough matched-- return the off variation or default - nil + { kind: 'PREREQUISITES_FAILED', prerequisiteKeys: failed_prereqs } end def get_variation(flag, index) @@ -257,9 +290,9 @@ def clause_match_user_no_segments(clause, user) maybe_negate(clause, match_any(op, val, clause[:values])) end - def variation_for_user(rule, user, flag) + def variation_index_for_user(flag, rule, user) if !rule[:variation].nil? # fixed variation - return { variation: rule[:variation], value: get_variation(flag, rule[:variation]) } + return rule[:variation] elsif !rule[:rollout].nil? # percentage rollout rollout = rule[:rollout] bucket_by = rollout[:bucketBy].nil? ? "key" : rollout[:bucketBy] @@ -268,12 +301,12 @@ def variation_for_user(rule, user, flag) rollout[:variations].each do |variate| sum += variate[:weight].to_f / 100000.0 if bucket < sum - return { variation: variate[:variation], value: get_variation(flag, variate[:variation]) } + return variate[:variation] end end nil else # the rule isn't well-formed - raise EvaluationError, "Rule does not define a variation or rollout" + nil end end @@ -350,5 +383,31 @@ def match_any(op, value, values) end return false end + + :private + + def get_variation(flag, index, reason) + if index < 0 || index >= flag[:variations].length + logger.error("[LDClient] Data inconsistency in feature flag \"#{flag[:key]}\": invalid variation index") + return error_result('MALFORMED_FLAG') + end + EvaluationDetail.new(flag[:variations][index], index, reason) + end + + def get_off_value(flag, reason) + if flag[:offVariation].nil? # off variation unspecified - return default value + return EvaluationDetail.new(nil, nil, reason) + end + get_variation(flag, flag[:offVariation], reason) + end + + def get_value_for_variation_or_rollout(flag, vr, user, reason, logger) + index = variation_index_for_user(flag, vr, user) + if index.nil? + logger.error("[LDClient] Data inconsistency in feature flag \"#{flag[:key]}\": variation/rollout object with no variation or rollout") + return error_result('MALFORMED_FLAG') + end + return get_variation(flag, index, reason) + end end end diff --git a/lib/ldclient-rb/events.rb b/lib/ldclient-rb/events.rb index 202fc235..e19d6b02 100644 --- a/lib/ldclient-rb/events.rb +++ b/lib/ldclient-rb/events.rb @@ -363,6 +363,7 @@ def make_output_event(event) else out[:userKey] = event[:user].nil? ? nil : event[:user][:key] end + out[:reason] = event[:reason] if !event[:reason].nil? out when "identify" { diff --git a/lib/ldclient-rb/flags_state.rb b/lib/ldclient-rb/flags_state.rb index 09f88975..05079920 100644 --- a/lib/ldclient-rb/flags_state.rb +++ b/lib/ldclient-rb/flags_state.rb @@ -15,12 +15,13 @@ def initialize(valid) end # Used internally to build the state map. - def add_flag(flag, value, variation) + def add_flag(flag, value, variation, reason = nil) key = flag[:key] @flag_values[key] = value meta = { version: flag[:version], trackEvents: flag[:trackEvents] } meta[:variation] = variation if !variation.nil? meta[:debugEventsUntilDate] = flag[:debugEventsUntilDate] if flag[:debugEventsUntilDate] + meta[:reason] = reason if !reason.nil? @flag_metadata[key] = meta end diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index e9873679..8efd422a 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -120,52 +120,11 @@ def initialized? # @return the variation to show the user, or the # default value if there's an an error def variation(key, user, default) - return default if @config.offline? - - if !initialized? - if @store.initialized? - @config.logger.warn { "[LDClient] Client has not finished initializing; using last known values from feature store" } - else - @config.logger.error { "[LDClient] Client has not finished initializing; feature store unavailable, returning default value" } - @event_processor.add_event(kind: "feature", key: key, value: default, default: default, user: user) - return default - end - end - - sanitize_user(user) if !user.nil? - feature = @store.get(FEATURES, key) - - if feature.nil? - @config.logger.info { "[LDClient] Unknown feature flag #{key}. Returning default value" } - @event_processor.add_event(kind: "feature", key: key, value: default, default: default, user: user) - return default - end - - unless user - @config.logger.error { "[LDClient] Must specify user" } - @event_processor.add_event(make_feature_event(feature, user, nil, default, default)) - return default - end + evaluate_internal(key, user, default, false).value + end - begin - res = evaluate(feature, user, @store, @config.logger) - if !res[:events].nil? - res[:events].each do |event| - @event_processor.add_event(event) - end - end - value = res[:value] - if value.nil? - @config.logger.debug { "[LDClient] Result value is null in toggle" } - value = default - end - @event_processor.add_event(make_feature_event(feature, user, res[:variation], value, default)) - return value - rescue => exn - Util.log_exception(@config.logger, "Error evaluating feature flag", exn) - @event_processor.add_event(make_feature_event(feature, user, nil, default, default)) - return default - end + def variation_detail(key, user, default) + evaluate_internal(key, user, default, true) end # @@ -213,6 +172,8 @@ def all_flags(user) # @param options={} [Hash] Optional parameters to control how the state is generated # @option options [Boolean] :client_side_only (false) True if only flags marked for use with the # client-side SDK should be included in the state. By default, all flags are included. + # @option options [Boolean] :with_reasons (false) True if evaluation reasons should be included + # in the state. By default, they are not included. # @return [FeatureFlagsState] a FeatureFlagsState object which can be serialized to JSON # def all_flags_state(user, options={}) @@ -234,16 +195,17 @@ def all_flags_state(user, options={}) state = FeatureFlagsState.new(true) client_only = options[:client_side_only] || false + with_reasons = options[:with_reasons] || false features.each do |k, f| if client_only && !f[:clientSide] next end begin result = evaluate(f, user, @store, @config.logger) - state.add_flag(f, result[:value], result[:variation]) + state.add_flag(f, result.detail.value, result.detail.variation, with_reasons ? result.detail.reason : nil) rescue => exn Util.log_exception(@config.logger, "Error evaluating flag \"#{k}\" in all_flags_state", exn) - state.add_flag(f, nil, nil) + state.add_flag(f, nil, nil, with_reasons ? { kind: 'ERROR', errorKind: 'EXCEPTION' } : nil) end end @@ -261,27 +223,83 @@ def close @store.stop end + :private + + # @return [EvaluationDetail] + def evaluate_internal(key, user, default, include_reasons_in_events) + if @config.offline? + return error_result('CLIENT_NOT_READY', default) + end + + if !initialized? + if @store.initialized? + @config.logger.warn { "[LDClient] Client has not finished initializing; using last known values from feature store" } + else + @config.logger.error { "[LDClient] Client has not finished initializing; feature store unavailable, returning default value" } + @event_processor.add_event(kind: "feature", key: key, value: default, default: default, user: user) + return error_result('CLIENT_NOT_READY', default) + end + end + + sanitize_user(user) if !user.nil? + feature = @store.get(FEATURES, key) + + if feature.nil? + @config.logger.info { "[LDClient] Unknown feature flag \"#{key}\". Returning default value" } + detail = error_result('FLAG_NOT_FOUND', default) + @event_processor.add_event(kind: "feature", key: key, value: default, default: default, user: user, + reason: include_reasons_in_events ? detail.reason : nil) + return detail + end + + unless user + @config.logger.error { "[LDClient] Must specify user" } + detail = error_result('USER_NOT_SPECIFIED', default) + @event_processor.add_event(make_feature_event(feature, user, detail, default, include_reasons_in_events)) + return detail + end + + begin + res = evaluate(feature, user, @store, @config.logger) + if !res.events.nil? + res.events.each do |event| + @event_processor.add_event(event) + end + end + detail = res.detail + if detail.variation.nil? + detail = EvaluationDetail.new(default, nil, detail.reason) + end + @event_processor.add_event(make_feature_event(feature, user, detail, default, include_reasons_in_events)) + return detail + rescue => exn + Util.log_exception(@config.logger, "Error evaluating feature flag \"#{key}\"", exn) + detail = error_result('EXCEPTION', default) + @event_processor.add_event(make_feature_event(feature, user, detail, default, include_reasons_in_events)) + return detail + end + end + def sanitize_user(user) if user[:key] user[:key] = user[:key].to_s end end - def make_feature_event(flag, user, variation, value, default) + def make_feature_event(flag, user, detail, default, with_reasons) { kind: "feature", key: flag[:key], user: user, - variation: variation, - value: value, + variation: detail.variation, + value: detail.value, default: default, version: flag[:version], trackEvents: flag[:trackEvents], - debugEventsUntilDate: flag[:debugEventsUntilDate] + debugEventsUntilDate: flag[:debugEventsUntilDate], + reason: with_reasons ? detail.reason : nil } end - - private :evaluate, :sanitize_user, :make_feature_event end # diff --git a/spec/evaluation_spec.rb b/spec/evaluation_spec.rb index a8d980ae..d5ee1097 100644 --- a/spec/evaluation_spec.rb +++ b/spec/evaluation_spec.rb @@ -2,6 +2,9 @@ describe LaunchDarkly::Evaluation do subject { LaunchDarkly::Evaluation } + + include LaunchDarkly::Evaluation + let(:features) { LaunchDarkly::InMemoryFeatureStore.new } let(:user) { @@ -14,7 +17,13 @@ let(:logger) { LaunchDarkly::Config.default_logger } - include LaunchDarkly::Evaluation + def boolean_flag_with_rules(rules) + { key: 'feature', on: true, rules: rules, fallthrough: { variation: 0 }, variations: [ false, true ] } + end + + def boolean_flag_with_clauses(clauses) + boolean_flag_with_rules([{ id: 'ruleid', clauses: clauses, variation: 1 }]) + end describe "evaluate" do it "returns off variation if flag is off" do @@ -26,7 +35,10 @@ variations: ['a', 'b', 'c'] } user = { key: 'x' } - expect(evaluate(flag, user, features, logger)).to eq({variation: 1, value: 'b', events: []}) + detail = LaunchDarkly::EvaluationDetail.new('b', 1, { kind: 'OFF' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) end it "returns nil if flag is off and off variation is unspecified" do @@ -37,7 +49,10 @@ variations: ['a', 'b', 'c'] } user = { key: 'x' } - expect(evaluate(flag, user, features, logger)).to eq({variation: nil, value: nil, events: []}) + detail = LaunchDarkly::EvaluationDetail.new(nil, nil, { kind: 'OFF' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) end it "returns off variation if prerequisite is not found" do @@ -50,7 +65,11 @@ variations: ['a', 'b', 'c'] } user = { key: 'x' } - expect(evaluate(flag, user, features, logger)).to eq({variation: 1, value: 'b', events: []}) + detail = LaunchDarkly::EvaluationDetail.new('b', 1, + { kind: 'PREREQUISITES_FAILED', prerequisiteKeys: ['badfeature'] }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) end it "returns off variation and event if prerequisite of a prerequisite is not found" do @@ -73,11 +92,15 @@ } features.upsert(LaunchDarkly::FEATURES, flag1) user = { key: 'x' } + detail = LaunchDarkly::EvaluationDetail.new('b', 1, + { kind: 'PREREQUISITES_FAILED', prerequisiteKeys: ['feature1'] }) events_should_be = [{ kind: 'feature', key: 'feature1', variation: nil, value: nil, version: 2, prereqOf: 'feature0', trackEvents: nil, debugEventsUntilDate: nil }] - expect(evaluate(flag, user, features, logger)).to eq({variation: 1, value: 'b', events: events_should_be}) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq(events_should_be) end it "returns off variation and event if prerequisite is not met" do @@ -99,11 +122,15 @@ } features.upsert(LaunchDarkly::FEATURES, flag1) user = { key: 'x' } + detail = LaunchDarkly::EvaluationDetail.new('b', 1, + { kind: 'PREREQUISITES_FAILED', prerequisiteKeys: ['feature1'] }) events_should_be = [{ kind: 'feature', key: 'feature1', variation: 0, value: 'd', version: 2, prereqOf: 'feature0', trackEvents: nil, debugEventsUntilDate: nil }] - expect(evaluate(flag, user, features, logger)).to eq({variation: 1, value: 'b', events: events_should_be}) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq(events_should_be) end it "returns fallthrough variation and event if prerequisite is met and there are no rules" do @@ -125,11 +152,14 @@ } features.upsert(LaunchDarkly::FEATURES, flag1) user = { key: 'x' } + detail = LaunchDarkly::EvaluationDetail.new('a', 0, { kind: 'FALLTHROUGH' }) events_should_be = [{ kind: 'feature', key: 'feature1', variation: 1, value: 'e', version: 2, prereqOf: 'feature0', trackEvents: nil, debugEventsUntilDate: nil }] - expect(evaluate(flag, user, features, logger)).to eq({variation: 0, value: 'a', events: events_should_be}) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq(events_should_be) end it "matches user from targets" do @@ -144,57 +174,96 @@ variations: ['a', 'b', 'c'] } user = { key: 'userkey' } - expect(evaluate(flag, user, features, logger)).to eq({variation: 2, value: 'c', events: []}) + detail = LaunchDarkly::EvaluationDetail.new('c', 2, { kind: 'TARGET_MATCH' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) end it "matches user from rules" do - flag = { - key: 'feature0', - on: true, - rules: [ - { - clauses: [ - { - attribute: 'key', - op: 'in', - values: [ 'userkey' ] - } - ], - variation: 2 - } - ], - fallthrough: { variation: 0 }, - offVariation: 1, - variations: ['a', 'b', 'c'] - } + rule = { id: 'ruleid', clauses: [{ attribute: 'key', op: 'in', values: ['userkey'] }], variation: 1 } + flag = boolean_flag_with_rules([rule]) user = { key: 'userkey' } - expect(evaluate(flag, user, features, logger)).to eq({variation: 2, value: 'c', events: []}) + detail = LaunchDarkly::EvaluationDetail.new(true, 1, + { kind: 'RULE_MATCH', ruleIndex: 0, ruleId: 'ruleid' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) + end + + it "returns an error if rule variation is too high" do + rule = { id: 'ruleid', clauses: [{ attribute: 'key', op: 'in', values: ['userkey'] }], variation: 999 } + flag = boolean_flag_with_rules([rule]) + user = { key: 'userkey' } + detail = LaunchDarkly::EvaluationDetail.new(nil, nil, + { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) + end + + it "returns an error if rule variation is negative" do + rule = { id: 'ruleid', clauses: [{ attribute: 'key', op: 'in', values: ['userkey'] }], variation: -1 } + flag = boolean_flag_with_rules([rule]) + user = { key: 'userkey' } + detail = LaunchDarkly::EvaluationDetail.new(nil, nil, + { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) + end + + it "returns an error if rule has neither variation nor rollout" do + rule = { id: 'ruleid', clauses: [{ attribute: 'key', op: 'in', values: ['userkey'] }] } + flag = boolean_flag_with_rules([rule]) + user = { key: 'userkey' } + detail = LaunchDarkly::EvaluationDetail.new(nil, nil, + { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) + end + + it "returns an error if rule has a rollout with no variations" do + rule = { id: 'ruleid', clauses: [{ attribute: 'key', op: 'in', values: ['userkey'] }], + rollout: { variations: [] } } + flag = boolean_flag_with_rules([rule]) + user = { key: 'userkey' } + detail = LaunchDarkly::EvaluationDetail.new(nil, nil, + { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) end end - describe "clause_match_user" do + describe "clause" do it "can match built-in attribute" do user = { key: 'x', name: 'Bob' } clause = { attribute: 'name', op: 'in', values: ['Bob'] } - expect(clause_match_user(clause, user, features)).to be true + flag = boolean_flag_with_clauses([clause]) + expect(evaluate(flag, user, features, logger).detail.value).to be true end it "can match custom attribute" do user = { key: 'x', name: 'Bob', custom: { legs: 4 } } clause = { attribute: 'legs', op: 'in', values: [4] } - expect(clause_match_user(clause, user, features)).to be true + flag = boolean_flag_with_clauses([clause]) + expect(evaluate(flag, user, features, logger).detail.value).to be true end it "returns false for missing attribute" do user = { key: 'x', name: 'Bob' } clause = { attribute: 'legs', op: 'in', values: [4] } - expect(clause_match_user(clause, user, features)).to be false + flag = boolean_flag_with_clauses([clause]) + expect(evaluate(flag, user, features, logger).detail.value).to be false end it "can be negated" do user = { key: 'x', name: 'Bob' } clause = { attribute: 'name', op: 'in', values: ['Bob'], negate: true } - expect(clause_match_user(clause, user, features)).to be false + flag = boolean_flag_with_clauses([clause]) + expect(evaluate(flag, user, features, logger).detail.value).to be false end it "retrieves segment from segment store for segmentMatch operator" do @@ -208,23 +277,24 @@ user = { key: 'userkey' } clause = { attribute: '', op: 'segmentMatch', values: ['segkey'] } - - expect(clause_match_user(clause, user, features)).to be true + flag = boolean_flag_with_clauses([clause]) + expect(evaluate(flag, user, features, logger).detail.value).to be true end it "falls through with no errors if referenced segment is not found" do user = { key: 'userkey' } clause = { attribute: '', op: 'segmentMatch', values: ['segkey'] } - - expect(clause_match_user(clause, user, features)).to be false + flag = boolean_flag_with_clauses([clause]) + expect(evaluate(flag, user, features, logger).detail.value).to be false end it "can be negated" do user = { key: 'x', name: 'Bob' } clause = { attribute: 'name', op: 'in', values: ['Bob'] } + flag = boolean_flag_with_clauses([clause]) expect { clause[:negate] = true - }.to change {clause_match_user(clause, user, features)}.from(true).to(false) + }.to change {evaluate(flag, user, features, logger).detail.value}.from(true).to(false) end end @@ -326,7 +396,8 @@ it "should return #{shouldBe} for #{value1} #{op} #{value2}" do user = { key: 'x', custom: { foo: value1 } } clause = { attribute: 'foo', op: op, values: [value2] } - expect(clause_match_user(clause, user, features)).to be shouldBe + flag = boolean_flag_with_clauses([clause]) + expect(evaluate(flag, user, features, logger).detail.value).to be shouldBe end end end @@ -385,17 +456,6 @@ end end - def make_flag(key) - { - key: key, - rules: [], - variations: [ false, true ], - on: true, - fallthrough: { variation: 0 }, - version: 1 - } - end - def make_segment(key) { key: key, @@ -424,35 +484,30 @@ def make_user_matching_clause(user, attr) end describe 'segment matching' do - it 'explicitly includes user' do - segment = make_segment('segkey') - segment[:included] = [ user[:key] ] + def test_segment_match(segment) features.upsert(LaunchDarkly::SEGMENTS, segment) clause = make_segment_match_clause(segment) + flag = boolean_flag_with_clauses([clause]) + evaluate(flag, user, features, logger).detail.value + end - result = clause_match_user(clause, user, features) - expect(result).to be true + it 'explicitly includes user' do + segment = make_segment('segkey') + segment[:included] = [ user[:key] ] + expect(test_segment_match(segment)).to be true end it 'explicitly excludes user' do segment = make_segment('segkey') segment[:excluded] = [ user[:key] ] - features.upsert(LaunchDarkly::SEGMENTS, segment) - clause = make_segment_match_clause(segment) - - result = clause_match_user(clause, user, features) - expect(result).to be false + expect(test_segment_match(segment)).to be false end it 'both includes and excludes user; include takes priority' do segment = make_segment('segkey') segment[:included] = [ user[:key] ] segment[:excluded] = [ user[:key] ] - features.upsert(LaunchDarkly::SEGMENTS, segment) - clause = make_segment_match_clause(segment) - - result = clause_match_user(clause, user, features) - expect(result).to be true + expect(test_segment_match(segment)).to be true end it 'matches user by rule when weight is absent' do @@ -462,11 +517,7 @@ def make_user_matching_clause(user, attr) } segment = make_segment('segkey') segment[:rules] = [ segRule ] - features.upsert(LaunchDarkly::SEGMENTS, segment) - clause = make_segment_match_clause(segment) - - result = clause_match_user(clause, user, features) - expect(result).to be true + expect(test_segment_match(segment)).to be true end it 'matches user by rule when weight is nil' do @@ -477,11 +528,7 @@ def make_user_matching_clause(user, attr) } segment = make_segment('segkey') segment[:rules] = [ segRule ] - features.upsert(LaunchDarkly::SEGMENTS, segment) - clause = make_segment_match_clause(segment) - - result = clause_match_user(clause, user, features) - expect(result).to be true + expect(test_segment_match(segment)).to be true end it 'matches user with full rollout' do @@ -492,11 +539,7 @@ def make_user_matching_clause(user, attr) } segment = make_segment('segkey') segment[:rules] = [ segRule ] - features.upsert(LaunchDarkly::SEGMENTS, segment) - clause = make_segment_match_clause(segment) - - result = clause_match_user(clause, user, features) - expect(result).to be true + expect(test_segment_match(segment)).to be true end it "doesn't match user with zero rollout" do @@ -507,11 +550,7 @@ def make_user_matching_clause(user, attr) } segment = make_segment('segkey') segment[:rules] = [ segRule ] - features.upsert(LaunchDarkly::SEGMENTS, segment) - clause = make_segment_match_clause(segment) - - result = clause_match_user(clause, user, features) - expect(result).to be false + expect(test_segment_match(segment)).to be false end it "matches user with multiple clauses" do @@ -522,11 +561,7 @@ def make_user_matching_clause(user, attr) } segment = make_segment('segkey') segment[:rules] = [ segRule ] - features.upsert(LaunchDarkly::SEGMENTS, segment) - clause = make_segment_match_clause(segment) - - result = clause_match_user(clause, user, features) - expect(result).to be true + expect(test_segment_match(segment)).to be true end it "doesn't match user with multiple clauses if a clause doesn't match" do @@ -538,11 +573,7 @@ def make_user_matching_clause(user, attr) } segment = make_segment('segkey') segment[:rules] = [ segRule ] - features.upsert(LaunchDarkly::SEGMENTS, segment) - clause = make_segment_match_clause(segment) - - result = clause_match_user(clause, user, features) - expect(result).to be false + expect(test_segment_match(segment)).to be false end end end diff --git a/spec/ldclient_spec.rb b/spec/ldclient_spec.rb index ae76a678..efaa1438 100644 --- a/spec/ldclient_spec.rb +++ b/spec/ldclient_spec.rb @@ -34,11 +34,18 @@ def event_processor end describe '#variation' do - it "will return the default value if the client is offline" do + feature_with_value = { key: "key", on: false, offVariation: 0, variations: ["value"], version: 100, + trackEvents: true, debugEventsUntilDate: 1000 } + + it "returns the default value if the client is offline" do result = offline_client.variation("doesntmatter", user, "default") expect(result).to eq "default" end + it "returns the default value for an unknown feature" do + expect(client.variation("badkey", user, "default")).to eq "default" + end + it "queues a feature request event for an unknown feature" do expect(event_processor).to receive(:add_event).with(hash_including( kind: "feature", key: "badkey", user: user, value: "default", default: "default" @@ -46,56 +53,113 @@ def event_processor client.variation("badkey", user, "default") end + it "returns the value for an existing feature" do + config.feature_store.init({ LaunchDarkly::FEATURES => {} }) + config.feature_store.upsert(LaunchDarkly::FEATURES, feature_with_value) + expect(client.variation("key", user, "default")).to eq "value" + end + it "queues a feature request event for an existing feature" do config.feature_store.init({ LaunchDarkly::FEATURES => {} }) - config.feature_store.upsert(LaunchDarkly::FEATURES, feature) + config.feature_store.upsert(LaunchDarkly::FEATURES, feature_with_value) expect(event_processor).to receive(:add_event).with(hash_including( kind: "feature", - key: feature[:key], - version: feature[:version], + key: "key", + version: 100, user: user, variation: 0, - value: true, + value: "value", default: "default", trackEvents: true, - debugEventsUntilDate: nil + debugEventsUntilDate: 1000 )) - client.variation(feature[:key], user, "default") + client.variation("key", user, "default") end it "queues a feature event for an existing feature when user is nil" do config.feature_store.init({ LaunchDarkly::FEATURES => {} }) - config.feature_store.upsert(LaunchDarkly::FEATURES, feature) + config.feature_store.upsert(LaunchDarkly::FEATURES, feature_with_value) expect(event_processor).to receive(:add_event).with(hash_including( kind: "feature", - key: feature[:key], - version: feature[:version], + key: "key", + version: 100, user: nil, variation: nil, value: "default", default: "default", trackEvents: true, - debugEventsUntilDate: nil + debugEventsUntilDate: 1000 )) - client.variation(feature[:key], nil, "default") + client.variation("key", nil, "default") end it "queues a feature event for an existing feature when user key is nil" do config.feature_store.init({ LaunchDarkly::FEATURES => {} }) - config.feature_store.upsert(LaunchDarkly::FEATURES, feature) + config.feature_store.upsert(LaunchDarkly::FEATURES, feature_with_value) bad_user = { name: "Bob" } expect(event_processor).to receive(:add_event).with(hash_including( kind: "feature", - key: feature[:key], - version: feature[:version], + key: "key", + version: 100, user: bad_user, variation: nil, value: "default", default: "default", trackEvents: true, - debugEventsUntilDate: nil + debugEventsUntilDate: 1000 + )) + client.variation("key", bad_user, "default") + end + end + + describe '#variation_detail' do + feature_with_value = { key: "key", on: false, offVariation: 0, variations: ["value"], version: 100, + trackEvents: true, debugEventsUntilDate: 1000 } + + it "returns the default value if the client is offline" do + result = offline_client.variation_detail("doesntmatter", user, "default") + expected = LaunchDarkly::EvaluationDetail.new("default", nil, { kind: 'ERROR', errorKind: 'CLIENT_NOT_READY' }) + expect(result).to eq expected + end + + it "returns the default value for an unknown feature" do + result = client.variation_detail("badkey", user, "default") + expected = LaunchDarkly::EvaluationDetail.new("default", nil, { kind: 'ERROR', errorKind: 'FLAG_NOT_FOUND'}) + expect(result).to eq expected + end + + it "queues a feature request event for an unknown feature" do + expect(event_processor).to receive(:add_event).with(hash_including( + kind: "feature", key: "badkey", user: user, value: "default", default: "default", + reason: { kind: 'ERROR', errorKind: 'FLAG_NOT_FOUND' } + )) + client.variation_detail("badkey", user, "default") + end + + it "returns a value for an existing feature" do + config.feature_store.init({ LaunchDarkly::FEATURES => {} }) + config.feature_store.upsert(LaunchDarkly::FEATURES, feature_with_value) + result = client.variation_detail("key", user, "default") + expected = LaunchDarkly::EvaluationDetail.new("value", 0, { kind: 'OFF' }) + expect(result).to eq expected + end + + it "queues a feature request event for an existing feature" do + config.feature_store.init({ LaunchDarkly::FEATURES => {} }) + config.feature_store.upsert(LaunchDarkly::FEATURES, feature_with_value) + expect(event_processor).to receive(:add_event).with(hash_including( + kind: "feature", + key: "key", + version: 100, + user: user, + variation: 0, + value: "value", + default: "default", + trackEvents: true, + debugEventsUntilDate: 1000, + reason: { kind: "OFF" } )) - client.variation(feature[:key], bad_user, "default") + client.variation_detail("key", user, "default") end end From d2c2ab81abd6e19934a2e444993cef1e1285e069 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 22 Aug 2018 20:03:36 -0700 Subject: [PATCH 12/21] misc cleanup --- lib/ldclient-rb/evaluation.rb | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/lib/ldclient-rb/evaluation.rb b/lib/ldclient-rb/evaluation.rb index b803f4a2..7a316aca 100644 --- a/lib/ldclient-rb/evaluation.rb +++ b/lib/ldclient-rb/evaluation.rb @@ -156,21 +156,21 @@ def evaluate(flag, user, store, logger) return EvalResult.new(detail, events) end - return EvalResult.new(get_off_value(flag, { kind: 'OFF' }), events) + return EvalResult.new(get_off_value(flag, { kind: 'OFF' }, logger), events) end def eval_internal(flag, user, store, events, logger) prereq_failure_reason = check_prerequisites(flag, user, store, events, logger) if !prereq_failure_reason.nil? - return get_off_value(flag, prereq_failure_reason) + return get_off_value(flag, prereq_failure_reason, logger) end # Check user target matches (flag[:targets] || []).each do |target| (target[:values] || []).each do |value| if value == user[:key] - return get_variation(flag, target[:variation], { kind: 'TARGET_MATCH' }) + return get_variation(flag, target[:variation], { kind: 'TARGET_MATCH' }, logger) end end end @@ -240,13 +240,6 @@ def check_prerequisites(flag, user, store, events, logger) { kind: 'PREREQUISITES_FAILED', prerequisiteKeys: failed_prereqs } end - def get_variation(flag, index) - if index >= flag[:variations].length - raise EvaluationError, "Invalid variation index" - end - flag[:variations][index] - end - def rule_match_user(rule, user, store) return false if !rule[:clauses] @@ -386,7 +379,7 @@ def match_any(op, value, values) :private - def get_variation(flag, index, reason) + def get_variation(flag, index, reason, logger) if index < 0 || index >= flag[:variations].length logger.error("[LDClient] Data inconsistency in feature flag \"#{flag[:key]}\": invalid variation index") return error_result('MALFORMED_FLAG') @@ -394,11 +387,11 @@ def get_variation(flag, index, reason) EvaluationDetail.new(flag[:variations][index], index, reason) end - def get_off_value(flag, reason) + def get_off_value(flag, reason, logger) if flag[:offVariation].nil? # off variation unspecified - return default value return EvaluationDetail.new(nil, nil, reason) end - get_variation(flag, flag[:offVariation], reason) + get_variation(flag, flag[:offVariation], reason, logger) end def get_value_for_variation_or_rollout(flag, vr, user, reason, logger) @@ -407,7 +400,7 @@ def get_value_for_variation_or_rollout(flag, vr, user, reason, logger) logger.error("[LDClient] Data inconsistency in feature flag \"#{flag[:key]}\": variation/rollout object with no variation or rollout") return error_result('MALFORMED_FLAG') end - return get_variation(flag, index, reason) + return get_variation(flag, index, reason, logger) end end end From 64a00a1a9388e85cb26e5650da97fa2029198d64 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 22 Aug 2018 20:14:37 -0700 Subject: [PATCH 13/21] misc cleanup, more error checking --- lib/ldclient-rb/evaluation.rb | 6 +- spec/evaluation_spec.rb | 111 +++++++++++++++++++++++++++++++++- 2 files changed, 111 insertions(+), 6 deletions(-) diff --git a/lib/ldclient-rb/evaluation.rb b/lib/ldclient-rb/evaluation.rb index 7a316aca..7dfbc3db 100644 --- a/lib/ldclient-rb/evaluation.rb +++ b/lib/ldclient-rb/evaluation.rb @@ -132,9 +132,6 @@ def self.comparator(converter) end } - class EvaluationError < StandardError - end - # Used internally to hold an evaluation result and the events that were generated from prerequisites. EvalResult = Struct.new(:detail, :events) @@ -268,9 +265,8 @@ def clause_match_user_no_segments(clause, user) return false if val.nil? op = OPERATORS[clause[:op].to_sym] - if op.nil? - raise EvaluationError, "Unsupported operator #{clause[:op]} in evaluation" + return false end if val.is_a? Enumerable diff --git a/spec/evaluation_spec.rb b/spec/evaluation_spec.rb index d5ee1097..9cb148ff 100644 --- a/spec/evaluation_spec.rb +++ b/spec/evaluation_spec.rb @@ -55,6 +55,38 @@ def boolean_flag_with_clauses(clauses) expect(result.events).to eq([]) end + it "returns an error if off variation is too high" do + flag = { + key: 'feature', + on: false, + offVariation: 999, + fallthrough: { variation: 0 }, + variations: ['a', 'b', 'c'] + } + user = { key: 'x' } + detail = LaunchDarkly::EvaluationDetail.new(nil, nil, + { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) + end + + it "returns an error if off variation is negative" do + flag = { + key: 'feature', + on: false, + offVariation: -1, + fallthrough: { variation: 0 }, + variations: ['a', 'b', 'c'] + } + user = { key: 'x' } + detail = LaunchDarkly::EvaluationDetail.new(nil, nil, + { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) + end + it "returns off variation if prerequisite is not found" do flag = { key: 'feature0', @@ -162,9 +194,69 @@ def boolean_flag_with_clauses(clauses) expect(result.events).to eq(events_should_be) end + it "returns an error if fallthrough variation is too high" do + flag = { + key: 'feature', + on: true, + fallthrough: { variation: 999 }, + offVariation: 1, + variations: ['a', 'b', 'c'] + } + user = { key: 'userkey' } + detail = LaunchDarkly::EvaluationDetail.new(nil, nil, { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) + end + + it "returns an error if fallthrough variation is negative" do + flag = { + key: 'feature', + on: true, + fallthrough: { variation: -1 }, + offVariation: 1, + variations: ['a', 'b', 'c'] + } + user = { key: 'userkey' } + detail = LaunchDarkly::EvaluationDetail.new(nil, nil, { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) + end + + it "returns an error if fallthrough has no variation or rollout" do + flag = { + key: 'feature', + on: true, + fallthrough: { }, + offVariation: 1, + variations: ['a', 'b', 'c'] + } + user = { key: 'userkey' } + detail = LaunchDarkly::EvaluationDetail.new(nil, nil, { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) + end + + it "returns an error if fallthrough has a rollout with no variations" do + flag = { + key: 'feature', + on: true, + fallthrough: { rollout: { variations: [] } }, + offVariation: 1, + variations: ['a', 'b', 'c'] + } + user = { key: 'userkey' } + detail = LaunchDarkly::EvaluationDetail.new(nil, nil, { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }) + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq([]) + end + it "matches user from targets" do flag = { - key: 'feature0', + key: 'feature', on: true, targets: [ { values: [ 'whoever', 'userkey' ], variation: 2 } @@ -259,6 +351,23 @@ def boolean_flag_with_clauses(clauses) expect(evaluate(flag, user, features, logger).detail.value).to be false end + it "returns false for unknown operator" do + user = { key: 'x', name: 'Bob' } + clause = { attribute: 'name', op: 'unknown', values: [4] } + flag = boolean_flag_with_clauses([clause]) + expect(evaluate(flag, user, features, logger).detail.value).to be false + end + + it "does not stop evaluating rules after clause with unknown operator" do + user = { key: 'x', name: 'Bob' } + clause0 = { attribute: 'name', op: 'unknown', values: [4] } + rule0 = { clauses: [ clause0 ], variation: 1 } + clause1 = { attribute: 'name', op: 'in', values: ['Bob'] } + rule1 = { clauses: [ clause1 ], variation: 1 } + flag = boolean_flag_with_rules([rule0, rule1]) + expect(evaluate(flag, user, features, logger).detail.value).to be true + end + it "can be negated" do user = { key: 'x', name: 'Bob' } clause = { attribute: 'name', op: 'in', values: ['Bob'], negate: true } From 46b642b0c0498bfba69577a544226a33f9095cd6 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 22 Aug 2018 20:49:58 -0700 Subject: [PATCH 14/21] don't keep evaluating prerequisites if one fails --- lib/ldclient-rb/evaluation.rb | 10 ++-------- spec/evaluation_spec.rb | 6 +++--- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/lib/ldclient-rb/evaluation.rb b/lib/ldclient-rb/evaluation.rb index 7dfbc3db..51cf3c66 100644 --- a/lib/ldclient-rb/evaluation.rb +++ b/lib/ldclient-rb/evaluation.rb @@ -192,8 +192,6 @@ def eval_internal(flag, user, store, events, logger) end def check_prerequisites(flag, user, store, events, logger) - failed_prereqs = [] - (flag[:prerequisites] || []).each do |prerequisite| prereq_ok = true prereq_key = prerequisite[:key] @@ -227,14 +225,10 @@ def check_prerequisites(flag, user, store, events, logger) end end if !prereq_ok - failed_prereqs.push(prereq_key) + return { kind: 'PREREQUISITE_FAILED', prerequisiteKey: prereq_key } end end - - if failed_prereqs.empty? - return nil - end - { kind: 'PREREQUISITES_FAILED', prerequisiteKeys: failed_prereqs } + nil end def rule_match_user(rule, user, store) diff --git a/spec/evaluation_spec.rb b/spec/evaluation_spec.rb index 9cb148ff..7f0c82b4 100644 --- a/spec/evaluation_spec.rb +++ b/spec/evaluation_spec.rb @@ -98,7 +98,7 @@ def boolean_flag_with_clauses(clauses) } user = { key: 'x' } detail = LaunchDarkly::EvaluationDetail.new('b', 1, - { kind: 'PREREQUISITES_FAILED', prerequisiteKeys: ['badfeature'] }) + { kind: 'PREREQUISITE_FAILED', prerequisiteKey: 'badfeature' }) result = evaluate(flag, user, features, logger) expect(result.detail).to eq(detail) expect(result.events).to eq([]) @@ -125,7 +125,7 @@ def boolean_flag_with_clauses(clauses) features.upsert(LaunchDarkly::FEATURES, flag1) user = { key: 'x' } detail = LaunchDarkly::EvaluationDetail.new('b', 1, - { kind: 'PREREQUISITES_FAILED', prerequisiteKeys: ['feature1'] }) + { kind: 'PREREQUISITE_FAILED', prerequisiteKey: 'feature1' }) events_should_be = [{ kind: 'feature', key: 'feature1', variation: nil, value: nil, version: 2, prereqOf: 'feature0', trackEvents: nil, debugEventsUntilDate: nil @@ -155,7 +155,7 @@ def boolean_flag_with_clauses(clauses) features.upsert(LaunchDarkly::FEATURES, flag1) user = { key: 'x' } detail = LaunchDarkly::EvaluationDetail.new('b', 1, - { kind: 'PREREQUISITES_FAILED', prerequisiteKeys: ['feature1'] }) + { kind: 'PREREQUISITE_FAILED', prerequisiteKey: 'feature1' }) events_should_be = [{ kind: 'feature', key: 'feature1', variation: 0, value: 'd', version: 2, prereqOf: 'feature0', trackEvents: nil, debugEventsUntilDate: nil From 855c4e2be634b475957d46cda6870d1c52b326ed Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 22 Aug 2018 22:28:28 -0700 Subject: [PATCH 15/21] doc comment --- lib/ldclient-rb/ldclient.rb | 40 ++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index 8efd422a..1d5c23a1 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -115,7 +115,7 @@ def initialized? # @param key [String] the unique feature key for the feature flag, as shown # on the LaunchDarkly dashboard # @param user [Hash] a hash containing parameters for the end user requesting the flag - # @param default=false the default value of the flag + # @param default the default value of the flag # # @return the variation to show the user, or the # default value if there's an an error @@ -123,6 +123,44 @@ def variation(key, user, default) evaluate_internal(key, user, default, false).value end + # + # Determines the variation of a feature flag for a user, like `variation`, but also + # provides additional information about how this value was calculated. + # + # The return value of `variation_detail` is an `EvaluationDetail` object, which has + # three properties: + # + # `value`: the value that was calculated for this user (same as the return value + # of `variation`) + # + # `variation`: the positional index of this value in the flag, e.g. 0 for the first + # variation - or `nil` if it is the default value + # + # `reason`: a hash describing the main reason why this value was selected. Its `:kind` + # property will be one of the following: + # + # * `'OFF'`: the flag was off and therefore returned its configured off value + # * `'FALLTHROUGH'`: the flag was on but the user did not match any targets or rules + # * `'TARGET_MATCH'`: the user key was specifically targeted for this flag + # * `'RULE_MATCH'`: the user matched one of the flag's rules; the `:ruleIndex` and + # `:ruleId` properties indicate the positional index and unique identifier of the rule + # * `'PREREQUISITE_FAILED`': the flag was considered off because it had at least one + # prerequisite flag that either was off or did not return the desired variation; the + # `:prerequisiteKey` property indicates the key of the prerequisite that failed + # * `'ERROR'`: the flag could not be evaluated, e.g. because it does not exist or due + # to an unexpected error, and therefore returned the default value; the `:errorKind` + # property describes the nature of the error, such as `'FLAG_NOT_FOUND'` + # + # The `reason` will also be included in analytics events, if you are capturing + # detailed event data for this flag. + # + # @param key [String] the unique feature key for the feature flag, as shown + # on the LaunchDarkly dashboard + # @param user [Hash] a hash containing parameters for the end user requesting the flag + # @param default the default value of the flag + # + # @return an `EvaluationDetail` object describing the result + # def variation_detail(key, user, default) evaluate_internal(key, user, default, true) end From a0f002f3c1e1cdb8313b5f116d9ba909e4d0e17d Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 22 Aug 2018 22:34:25 -0700 Subject: [PATCH 16/21] rename variation to variation_index --- lib/ldclient-rb/evaluation.rb | 12 ++++++------ lib/ldclient-rb/ldclient.rb | 10 +++++----- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/ldclient-rb/evaluation.rb b/lib/ldclient-rb/evaluation.rb index 51cf3c66..bd4544dc 100644 --- a/lib/ldclient-rb/evaluation.rb +++ b/lib/ldclient-rb/evaluation.rb @@ -5,9 +5,9 @@ module LaunchDarkly # An object returned by `LDClient.variation_detail`, combining the result of a flag evaluation with # an explanation of how it was calculated. class EvaluationDetail - def initialize(value, variation, reason) + def initialize(value, variation_index, reason) @value = value - @variation = variation + @variation_index = variation_index @reason = reason end @@ -17,13 +17,13 @@ def initialize(value, variation, reason) # @return [int|nil] The index of the returned value within the flag's list of variations, e.g. # 0 for the first variation - or `nil` if the default value was returned. - attr_reader :variation + attr_reader :variation_index # @return [Hash] An object describing the main factor that influenced the flag evaluation value. attr_reader :reason def ==(other) - @value == other.value && @variation == other.variation && @reason == other.reason + @value == other.value && @variation_index == other.variation_index && @reason == other.reason end end @@ -208,7 +208,7 @@ def check_prerequisites(flag, user, store, events, logger) event = { kind: "feature", key: prereq_key, - variation: prereq_res.variation, + variation: prereq_res.variation_index, value: prereq_res.value, version: prereq_flag[:version], prereqOf: flag[:key], @@ -216,7 +216,7 @@ def check_prerequisites(flag, user, store, events, logger) debugEventsUntilDate: prereq_flag[:debugEventsUntilDate] } events.push(event) - if prereq_res.variation != prerequisite[:variation] + if prereq_res.variation_index != prerequisite[:variation] prereq_ok = false end rescue => exn diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index 1d5c23a1..177b91a2 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -133,8 +133,8 @@ def variation(key, user, default) # `value`: the value that was calculated for this user (same as the return value # of `variation`) # - # `variation`: the positional index of this value in the flag, e.g. 0 for the first - # variation - or `nil` if it is the default value + # `variation_index`: the positional index of this value in the flag, e.g. 0 for the + # first variation - or `nil` if the default value was returned # # `reason`: a hash describing the main reason why this value was selected. Its `:kind` # property will be one of the following: @@ -240,7 +240,7 @@ def all_flags_state(user, options={}) end begin result = evaluate(f, user, @store, @config.logger) - state.add_flag(f, result.detail.value, result.detail.variation, with_reasons ? result.detail.reason : nil) + state.add_flag(f, result.detail.value, result.detail.variation_index, with_reasons ? result.detail.reason : nil) rescue => exn Util.log_exception(@config.logger, "Error evaluating flag \"#{k}\" in all_flags_state", exn) state.add_flag(f, nil, nil, with_reasons ? { kind: 'ERROR', errorKind: 'EXCEPTION' } : nil) @@ -305,7 +305,7 @@ def evaluate_internal(key, user, default, include_reasons_in_events) end end detail = res.detail - if detail.variation.nil? + if detail.variation_index.nil? detail = EvaluationDetail.new(default, nil, detail.reason) end @event_processor.add_event(make_feature_event(feature, user, detail, default, include_reasons_in_events)) @@ -329,7 +329,7 @@ def make_feature_event(flag, user, detail, default, with_reasons) kind: "feature", key: flag[:key], user: user, - variation: detail.variation, + variation: detail.variation_index, value: detail.value, default: default, version: flag[:version], From 4ec43db7e4b7d58ad04bf5f9dde015f0eed0a816 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 22 Aug 2018 22:44:24 -0700 Subject: [PATCH 17/21] comment --- lib/ldclient-rb/ldclient.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index 177b91a2..1c2d2257 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -211,7 +211,7 @@ def all_flags(user) # @option options [Boolean] :client_side_only (false) True if only flags marked for use with the # client-side SDK should be included in the state. By default, all flags are included. # @option options [Boolean] :with_reasons (false) True if evaluation reasons should be included - # in the state. By default, they are not included. + # in the state (see `variation_detail`). By default, they are not included. # @return [FeatureFlagsState] a FeatureFlagsState object which can be serialized to JSON # def all_flags_state(user, options={}) From 9622e0116f5b4a513e705630a19603842d07cd75 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Thu, 23 Aug 2018 17:11:29 -0700 Subject: [PATCH 18/21] more test coverage, convenience method --- lib/ldclient-rb/evaluation.rb | 6 ++++++ lib/ldclient-rb/ldclient.rb | 2 +- spec/ldclient_spec.rb | 17 +++++++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/ldclient-rb/evaluation.rb b/lib/ldclient-rb/evaluation.rb index bd4544dc..4f6cbb0e 100644 --- a/lib/ldclient-rb/evaluation.rb +++ b/lib/ldclient-rb/evaluation.rb @@ -22,6 +22,12 @@ def initialize(value, variation_index, reason) # @return [Hash] An object describing the main factor that influenced the flag evaluation value. attr_reader :reason + # @return [boolean] True if the flag evaluated to the default value rather than to one of its + # variations. + def default_value? + variation_index.nil? + end + def ==(other) @value == other.value && @variation_index == other.variation_index && @reason == other.reason end diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index 1c2d2257..a87344ed 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -305,7 +305,7 @@ def evaluate_internal(key, user, default, include_reasons_in_events) end end detail = res.detail - if detail.variation_index.nil? + if detail.default_value? detail = EvaluationDetail.new(default, nil, detail.reason) end @event_processor.add_event(make_feature_event(feature, user, detail, default, include_reasons_in_events)) diff --git a/spec/ldclient_spec.rb b/spec/ldclient_spec.rb index efaa1438..d76f7834 100644 --- a/spec/ldclient_spec.rb +++ b/spec/ldclient_spec.rb @@ -59,6 +59,13 @@ def event_processor expect(client.variation("key", user, "default")).to eq "value" end + it "returns the default value if a feature evaluates to nil" do + empty_feature = { key: "key", on: false, offVariation: nil } + config.feature_store.init({ LaunchDarkly::FEATURES => {} }) + config.feature_store.upsert(LaunchDarkly::FEATURES, empty_feature) + expect(client.variation("key", user, "default")).to eq "default" + end + it "queues a feature request event for an existing feature" do config.feature_store.init({ LaunchDarkly::FEATURES => {} }) config.feature_store.upsert(LaunchDarkly::FEATURES, feature_with_value) @@ -144,6 +151,16 @@ def event_processor expect(result).to eq expected end + it "returns the default value if a feature evaluates to nil" do + empty_feature = { key: "key", on: false, offVariation: nil } + config.feature_store.init({ LaunchDarkly::FEATURES => {} }) + config.feature_store.upsert(LaunchDarkly::FEATURES, empty_feature) + result = client.variation_detail("key", user, "default") + expected = LaunchDarkly::EvaluationDetail.new("default", nil, { kind: 'OFF' }) + expect(result).to eq expected + expect(result.default_value?).to be true + end + it "queues a feature request event for an existing feature" do config.feature_store.init({ LaunchDarkly::FEATURES => {} }) config.feature_store.upsert(LaunchDarkly::FEATURES, feature_with_value) From 084d9eacf32a6cc36ff1a150dc3bef9190ba2b64 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 29 Aug 2018 11:25:31 -0700 Subject: [PATCH 19/21] fix event generation for a prerequisite that is off --- lib/ldclient-rb/evaluation.rb | 26 ++++++++++++-------------- spec/evaluation_spec.rb | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/lib/ldclient-rb/evaluation.rb b/lib/ldclient-rb/evaluation.rb index 4f6cbb0e..aa4eb20d 100644 --- a/lib/ldclient-rb/evaluation.rb +++ b/lib/ldclient-rb/evaluation.rb @@ -153,17 +153,15 @@ def evaluate(flag, user, store, logger) end events = [] - - if flag[:on] - detail = eval_internal(flag, user, store, events, logger) - return EvalResult.new(detail, events) - end - - return EvalResult.new(get_off_value(flag, { kind: 'OFF' }, logger), events) + detail = eval_internal(flag, user, store, events, logger) + return EvalResult.new(detail, events) end - def eval_internal(flag, user, store, events, logger) + if !flag[:on] + return get_off_value(flag, { kind: 'OFF' }, logger) + end + prereq_failure_reason = check_prerequisites(flag, user, store, events, logger) if !prereq_failure_reason.nil? return get_off_value(flag, prereq_failure_reason, logger) @@ -203,14 +201,17 @@ def check_prerequisites(flag, user, store, events, logger) prereq_key = prerequisite[:key] prereq_flag = store.get(FEATURES, prereq_key) - if prereq_flag.nil? || !prereq_flag[:on] + if prereq_flag.nil? logger.error { "[LDClient] Could not retrieve prerequisite flag \"#{prereq_key}\" when evaluating \"#{flag[:key]}\"" } prereq_ok = false - elsif !prereq_flag[:on] - prereq_ok = false else begin prereq_res = eval_internal(prereq_flag, user, store, events, logger) + # Note that if the prerequisite flag is off, we don't consider it a match no matter what its + # off variation was. But we still need to evaluate it in order to generate an event. + if !prereq_flag[:on] || prereq_res.variation_index != prerequisite[:variation] + prereq_ok = false + end event = { kind: "feature", key: prereq_key, @@ -222,9 +223,6 @@ def check_prerequisites(flag, user, store, events, logger) debugEventsUntilDate: prereq_flag[:debugEventsUntilDate] } events.push(event) - if prereq_res.variation_index != prerequisite[:variation] - prereq_ok = false - end rescue => exn Util.log_exception(logger, "Error evaluating prerequisite flag \"#{prereq_key}\" for flag \"{flag[:key]}\"", exn) prereq_ok = false diff --git a/spec/evaluation_spec.rb b/spec/evaluation_spec.rb index 7f0c82b4..3af960c6 100644 --- a/spec/evaluation_spec.rb +++ b/spec/evaluation_spec.rb @@ -135,6 +135,38 @@ def boolean_flag_with_clauses(clauses) expect(result.events).to eq(events_should_be) end + it "returns off variation and event if prerequisite is off" do + flag = { + key: 'feature0', + on: true, + prerequisites: [{key: 'feature1', variation: 1}], + fallthrough: { variation: 0 }, + offVariation: 1, + variations: ['a', 'b', 'c'], + version: 1 + } + flag1 = { + key: 'feature1', + on: false, + # note that even though it returns the desired variation, it is still off and therefore not a match + offVariation: 1, + fallthrough: { variation: 0 }, + variations: ['d', 'e'], + version: 2 + } + features.upsert(LaunchDarkly::FEATURES, flag1) + user = { key: 'x' } + detail = LaunchDarkly::EvaluationDetail.new('b', 1, + { kind: 'PREREQUISITE_FAILED', prerequisiteKey: 'feature1' }) + events_should_be = [{ + kind: 'feature', key: 'feature1', variation: 1, value: 'e', version: 2, prereqOf: 'feature0', + trackEvents: nil, debugEventsUntilDate: nil + }] + result = evaluate(flag, user, features, logger) + expect(result.detail).to eq(detail) + expect(result.events).to eq(events_should_be) + end + it "returns off variation and event if prerequisite is not met" do flag = { key: 'feature0', From 02b5712c434c7a4e6524d6e3752c09be4437feca Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 29 Aug 2018 11:27:26 -0700 Subject: [PATCH 20/21] fix private --- lib/ldclient-rb/evaluation.rb | 2 +- lib/ldclient-rb/ldclient.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ldclient-rb/evaluation.rb b/lib/ldclient-rb/evaluation.rb index 4f6cbb0e..a16d9adb 100644 --- a/lib/ldclient-rb/evaluation.rb +++ b/lib/ldclient-rb/evaluation.rb @@ -373,7 +373,7 @@ def match_any(op, value, values) return false end - :private + private def get_variation(flag, index, reason, logger) if index < 0 || index >= flag[:variations].length diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index a87344ed..7e86662b 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -261,7 +261,7 @@ def close @store.stop end - :private + private # @return [EvaluationDetail] def evaluate_internal(key, user, default, include_reasons_in_events) From 530ff9eb0934775120e4a549432557bb7e2c18f3 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 29 Aug 2018 17:15:21 -0700 Subject: [PATCH 21/21] version 5.2.0 --- CHANGELOG.md | 8 ++++++++ lib/ldclient-rb/version.rb | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b81056b..848abd1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ All notable changes to the LaunchDarkly Ruby SDK will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org). +## [5.2.0] - 2018-08-29 +### Added: +- The new `LDClient` method `variation_detail` allows you to evaluate a feature flag (using the same parameters as you would for `variation`) and receive more information about how the value was calculated. This information is returned in an `EvaluationDetail` object, which contains both the result value and a "reason" object which will tell you, for instance, if the user was individually targeted for the flag or was matched by one of the flag's rules, or if the flag returned the default value due to an error. + +### Fixed: +- Evaluating a prerequisite feature flag did not produce an analytics event if the prerequisite flag was off. + + ## [5.1.0] - 2018-08-27 ### Added: - The new `LDClient` method `all_flags_state()` should be used instead of `all_flags()` if you are passing flag data to the front end for use with the JavaScript SDK. It preserves some flag metadata that the front end requires in order to send analytics events correctly. Versions 2.5.0 and above of the JavaScript SDK are able to use this metadata, but the output of `all_flags_state()` will still work with older versions. diff --git a/lib/ldclient-rb/version.rb b/lib/ldclient-rb/version.rb index 1d54cc52..35f02783 100644 --- a/lib/ldclient-rb/version.rb +++ b/lib/ldclient-rb/version.rb @@ -1,3 +1,3 @@ module LaunchDarkly - VERSION = "5.1.0" + VERSION = "5.2.0" end