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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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/45] 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 88676380bed1f147d04c8852f58ddb4f294e0eb5 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Thu, 4 Oct 2018 19:04:17 -0700 Subject: [PATCH 21/45] add option to reduce front-end metadata for untracked flags --- lib/ldclient-rb/flags_state.rb | 10 ++++++--- lib/ldclient-rb/ldclient.rb | 10 +++++++-- spec/flags_state_spec.rb | 3 +-- spec/ldclient_spec.rb | 41 ++++++++++++++++++++++++++++++++-- 4 files changed, 55 insertions(+), 9 deletions(-) diff --git a/lib/ldclient-rb/flags_state.rb b/lib/ldclient-rb/flags_state.rb index 05079920..a6036bde 100644 --- a/lib/ldclient-rb/flags_state.rb +++ b/lib/ldclient-rb/flags_state.rb @@ -15,13 +15,17 @@ def initialize(valid) end # Used internally to build the state map. - def add_flag(flag, value, variation, reason = nil) + def add_flag(flag, value, variation, reason = nil, details_only_if_tracked = false) key = flag[:key] @flag_values[key] = value - meta = { version: flag[:version], trackEvents: flag[:trackEvents] } + meta = {} + if !details_only_if_tracked || flag[:trackEvents] || flag[:debugEventsUntilDate] + meta[:version] = flag[:version] + meta[:reason] = reason if !reason.nil? + end meta[:variation] = variation if !variation.nil? + meta[:trackEvents] = true if flag[:trackEvents] 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 7e86662b..4ad7928e 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -212,6 +212,10 @@ def all_flags(user) # 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 (see `variation_detail`). By default, they are not included. + # @option options [Boolean] :details_only_for_tracked_flags (false) True if any flag metadata that is + # normally only used for event generation - such as flag versions and evaluation reasons - should be + # omitted for any flag that does not have event tracking or debugging turned on. This reduces the size + # of the JSON data if you are passing the flag state to the front end. # @return [FeatureFlagsState] a FeatureFlagsState object which can be serialized to JSON # def all_flags_state(user, options={}) @@ -234,16 +238,18 @@ def all_flags_state(user, options={}) state = FeatureFlagsState.new(true) client_only = options[:client_side_only] || false with_reasons = options[:with_reasons] || false + details_only_if_tracked = options[:details_only_for_tracked_flags] || 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.detail.value, result.detail.variation_index, with_reasons ? result.detail.reason : nil) + state.add_flag(f, result.detail.value, result.detail.variation_index, with_reasons ? result.detail.reason : nil, + details_only_if_tracked) 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) + state.add_flag(f, nil, nil, with_reasons ? { kind: 'ERROR', errorKind: 'EXCEPTION' } : nil, details_only_if_tracked) end end diff --git a/spec/flags_state_spec.rb b/spec/flags_state_spec.rb index 3d21029b..bda55b11 100644 --- a/spec/flags_state_spec.rb +++ b/spec/flags_state_spec.rb @@ -42,8 +42,7 @@ '$flagsState' => { 'key1' => { :variation => 0, - :version => 100, - :trackEvents => false + :version => 100 }, 'key2' => { :variation => 1, diff --git a/spec/ldclient_spec.rb b/spec/ldclient_spec.rb index d76f7834..6b923775 100644 --- a/spec/ldclient_spec.rb +++ b/spec/ldclient_spec.rb @@ -233,8 +233,7 @@ def event_processor '$flagsState' => { 'key1' => { :variation => 0, - :version => 100, - :trackEvents => false + :version => 100 }, 'key2' => { :variation => 1, @@ -263,6 +262,44 @@ def event_processor expect(values).to eq({ 'client-side-1' => 'value1', 'client-side-2' => 'value2' }) end + it "can omit details for untracked flags" do + flag1 = { key: "key1", version: 100, offVariation: 0, variations: [ 'value1' ], trackEvents: false } + flag2 = { key: "key2", version: 200, offVariation: 1, variations: [ 'x', 'value2' ], trackEvents: true } + flag3 = { key: "key3", version: 300, offVariation: 1, variations: [ 'x', 'value3' ], debugEventsUntilDate: 1000 } + + config.feature_store.init({ LaunchDarkly::FEATURES => { 'key1' => flag1, 'key2' => flag2, 'key3' => flag3 } }) + + 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', 'key3' => 'value3' }) + + result = state.as_json + expect(result).to eq({ + 'key1' => 'value1', + 'key2' => 'value2', + 'key3' => 'value3', + '$flagsState' => { + 'key1' => { + :variation => 0, + :version => 100 + }, + 'key2' => { + :variation => 1, + :version => 200, + :trackEvents => true + }, + 'key3' => { + :variation => 1, + :version => 300, + :debugEventsUntilDate => 1000 + } + }, + '$valid' => true + }) + end + it "returns empty state for nil user" do config.feature_store.init({ LaunchDarkly::FEATURES => { 'key1' => flag1, 'key2' => flag2 } }) From 9ea43e022a331d7c5ad577aad0b6d68d59ca22bd Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Mon, 8 Oct 2018 16:42:06 -0700 Subject: [PATCH 22/45] fix logic for whether a flag is tracked in all_flags_state --- lib/ldclient-rb/flags_state.rb | 6 +++++- spec/ldclient_spec.rb | 10 +++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/ldclient-rb/flags_state.rb b/lib/ldclient-rb/flags_state.rb index a6036bde..b761149c 100644 --- a/lib/ldclient-rb/flags_state.rb +++ b/lib/ldclient-rb/flags_state.rb @@ -19,7 +19,11 @@ def add_flag(flag, value, variation, reason = nil, details_only_if_tracked = fal key = flag[:key] @flag_values[key] = value meta = {} - if !details_only_if_tracked || flag[:trackEvents] || flag[:debugEventsUntilDate] + with_details = !details_only_if_tracked || flag[:trackEvents] + if !with_details && flag[:debugEventsUntilDate] + with_details = flag[:debugEventsUntilDate] > (Time.now.to_f * 1000).to_i + end + if with_details meta[:version] = flag[:version] meta[:reason] = reason if !reason.nil? end diff --git a/spec/ldclient_spec.rb b/spec/ldclient_spec.rb index 6b923775..262f53f9 100644 --- a/spec/ldclient_spec.rb +++ b/spec/ldclient_spec.rb @@ -263,13 +263,14 @@ def event_processor end it "can omit details for untracked flags" do + future_time = (Time.now.to_f * 1000).to_i + 100000 flag1 = { key: "key1", version: 100, offVariation: 0, variations: [ 'value1' ], trackEvents: false } flag2 = { key: "key2", version: 200, offVariation: 1, variations: [ 'x', 'value2' ], trackEvents: true } - flag3 = { key: "key3", version: 300, offVariation: 1, variations: [ 'x', 'value3' ], debugEventsUntilDate: 1000 } + flag3 = { key: "key3", version: 300, offVariation: 1, variations: [ 'x', 'value3' ], debugEventsUntilDate: future_time } config.feature_store.init({ LaunchDarkly::FEATURES => { 'key1' => flag1, 'key2' => flag2, 'key3' => flag3 } }) - state = client.all_flags_state({ key: 'userkey' }) + state = client.all_flags_state({ key: 'userkey' }, { details_only_for_tracked_flags: true }) expect(state.valid?).to be true values = state.values_map @@ -282,8 +283,7 @@ def event_processor 'key3' => 'value3', '$flagsState' => { 'key1' => { - :variation => 0, - :version => 100 + :variation => 0 }, 'key2' => { :variation => 1, @@ -293,7 +293,7 @@ def event_processor 'key3' => { :variation => 1, :version => 300, - :debugEventsUntilDate => 1000 + :debugEventsUntilDate => future_time } }, '$valid' => true From cce8e84964835b8d6d02ddff612a1af1e179e1c9 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 31 Oct 2018 10:23:48 -0700 Subject: [PATCH 23/45] implement file data source --- ldclient-rb.gemspec | 1 + lib/ldclient-rb.rb | 1 + lib/ldclient-rb/config.rb | 10 +- lib/ldclient-rb/file_data_source.rb | 209 ++++++++++++++++++++++++++++ lib/ldclient-rb/ldclient.rb | 33 +++-- spec/file_data_source_spec.rb | 167 ++++++++++++++++++++++ 6 files changed, 404 insertions(+), 17 deletions(-) create mode 100644 lib/ldclient-rb/file_data_source.rb create mode 100644 spec/file_data_source_spec.rb diff --git a/ldclient-rb.gemspec b/ldclient-rb.gemspec index a9bbfb23..9e7d5d04 100644 --- a/ldclient-rb.gemspec +++ b/ldclient-rb.gemspec @@ -40,4 +40,5 @@ Gem::Specification.new do |spec| spec.add_runtime_dependency "hashdiff", "~> 0.2" spec.add_runtime_dependency "http_tools", '~> 0.4.5' spec.add_runtime_dependency "socketry", "~> 0.5.1" + spec.add_runtime_dependency "listen", "~> 3.0" end diff --git a/lib/ldclient-rb.rb b/lib/ldclient-rb.rb index 7264b220..d3ee6ffc 100644 --- a/lib/ldclient-rb.rb +++ b/lib/ldclient-rb.rb @@ -18,3 +18,4 @@ require "ldclient-rb/events" require "ldclient-rb/redis_store" require "ldclient-rb/requestor" +require "ldclient-rb/file_data_source" diff --git a/lib/ldclient-rb/config.rb b/lib/ldclient-rb/config.rb index 3b62b2a3..dc89d30a 100644 --- a/lib/ldclient-rb/config.rb +++ b/lib/ldclient-rb/config.rb @@ -61,8 +61,11 @@ class Config # @option opts [Boolean] :inline_users_in_events (false) Whether to include full user details in every # analytics event. By default, events will only include the user key, except for one "index" event # that provides the full details for the user. - # @option opts [Object] :update_processor An object that will receive feature flag data from LaunchDarkly. - # Defaults to either the streaming or the polling processor, can be customized for tests. + # @option opts [Object] :update_processor (DEPRECATED) An object that will receive feature flag data from + # LaunchDarkly. Defaults to either the streaming or the polling processor, can be customized for tests. + # @option opts [Object] :update_processor_factory A function that takes the SDK and configuration object + # as parameters, and returns an object that can obtain feature flag data and put it into the feature + # store. Defaults to creating either the streaming or the polling processor, can be customized for tests. # @return [type] [description] # rubocop:disable Metrics/AbcSize, Metrics/PerceivedComplexity def initialize(opts = {}) @@ -88,6 +91,7 @@ def initialize(opts = {}) @user_keys_flush_interval = opts[:user_keys_flush_interval] || Config.default_user_keys_flush_interval @inline_users_in_events = opts[:inline_users_in_events] || false @update_processor = opts[:update_processor] + @update_processor_factory = opts[:update_processor_factory] end # @@ -218,6 +222,8 @@ def offline? attr_reader :update_processor + attr_reader :update_processor_factory + # # The default LaunchDarkly client configuration. This configuration sets # reasonable defaults for most users. diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb new file mode 100644 index 00000000..65ba0735 --- /dev/null +++ b/lib/ldclient-rb/file_data_source.rb @@ -0,0 +1,209 @@ +require 'concurrent/atomics' +require 'json' +require 'yaml' +require 'listen' +require 'pathname' + +module LaunchDarkly + + # + # Provides a way to use local files as a source of feature flag state. This would typically be + # used in a test environment, to operate using a predetermined feature flag state without an + # actual LaunchDarkly connection. + # + # To use this component, call `FileDataSource.factory`, and store its return value in the + # `update_processor_factory` property of your LaunchDarkly client configuration. In the options + # to `factory`, set `paths` to the file path(s) of your data file(s): + # + # config.update_processor_factory = FileDataSource.factory(paths: [ myFilePath ]) + # + # This will cause the client not to connect to LaunchDarkly to get feature flags. The + # client may still make network connections to send analytics events, unless you have disabled + # this with Config.send_events or Config.offline. + # + # Flag data files can be either JSON or YAML. They contain an object with three possible + # properties: + # + # - "flags": Feature flag definitions. + # - "flagValues": Simplified feature flags that contain only a value. + # - "segments": User segment definitions. + # + # The format of the data in "flags" and "segments" is defined by the LaunchDarkly application + # and is subject to change. Rather than trying to construct these objects yourself, it is simpler + # to request existing flags directly from the LaunchDarkly server in JSON format, and use this + # output as the starting point for your file. In Linux you would do this: + # + # curl -H "Authorization: {your sdk key}" https://app.launchdarkly.com/sdk/latest-all + # + # The output will look something like this (but with many more properties): + # + # { + # "flags": { + # "flag-key-1": { + # "key": "flag-key-1", + # "on": true, + # "variations": [ "a", "b" ] + # } + # }, + # "segments": { + # "segment-key-1": { + # "key": "segment-key-1", + # "includes": [ "user-key-1" ] + # } + # } + # } + # + # Data in this format allows the SDK to exactly duplicate all the kinds of flag behavior supported + # by LaunchDarkly. However, in many cases you will not need this complexity, but will just want to + # set specific flag keys to specific values. For that, you can use a much simpler format: + # + # { + # "flagValues": { + # "my-string-flag-key": "value-1", + # "my-boolean-flag-key": true, + # "my-integer-flag-key": 3 + # } + # } + # + # Or, in YAML: + # + # flagValues: + # my-string-flag-key: "value-1" + # my-boolean-flag-key: true + # my-integer-flag-key: 1 + # + # It is also possible to specify both "flags" and "flagValues", if you want some flags + # to have simple values and others to have complex behavior. However, it is an error to use the + # same flag key or segment key more than once, either in a single file or across multiple files. + # + # If the data source encounters any error in any file-- malformed content, a missing file, or a + # duplicate key-- it will not load flags from any of the files. + # + class FileDataSource + def self.factory(options={}) + return Proc.new do |sdk_key, config| + FileDataSourceImpl.new(config.feature_store, config.logger, options) + end + end + end + + class FileDataSourceImpl + def initialize(feature_store, logger, options={}) + @feature_store = feature_store + @logger = logger + @paths = options[:paths] || [] + @auto_update = options[:auto_update] + @initialized = Concurrent::AtomicBoolean.new(false) + @ready = Concurrent::Event.new + end + + def initialized? + @initialized.value + end + + def start + ready = Concurrent::Event.new + + # We will return immediately regardless of whether the file load succeeded or failed - + # the difference can be detected by checking "initialized?" + ready.set + + load_all + + if @auto_update + # If we're going to watch files, then the start event will be set the first time we get + # a successful load. + @listener = start_listener + end + + ready + end + + def stop + @listener.stop if !@listener.nil? + end + + private + + def load_all + all_data = { + FEATURES => {}, + SEGMENTS => {} + } + @paths.each do |path| + begin + load_file(path, all_data) + rescue => exn + Util.log_exception(@logger, "Unable to load flag data from \"#{path}\"", exn) + return + end + end + @feature_store.init(all_data) + @initialized.make_true + end + + def load_file(path, all_data) + parsed = parse_content(IO.read(path)) + (parsed[:flags] || {}).each do |key, flag| + add_item(all_data, FEATURES, flag) + end + (parsed[:flagValues] || {}).each do |key, value| + add_item(all_data, FEATURES, make_flag_with_value(key.to_s, value)) + end + (parsed[:segments] || {}).each do |key, segment| + add_item(all_data, SEGMENTS, segment) + end + end + + def parse_content(content) + if content.strip.start_with?("{") + JSON.parse(content, symbolize_names: true) + else + symbolize_all_keys(YAML.load(content)) + end + end + + def symbolize_all_keys(value) + # This is necessary because YAML.load doesn't have an option for parsing keys as symbols, and + # the SDK expects all objects to be formatted that way. + if value.is_a?(Hash) + value.map{ |k, v| [k.to_sym, symbolize_all_keys(v)] }.to_h + elsif value.is_a?(Array) + value.map{ |v| symbolize_all_keys(v) } + else + value + end + end + + def add_item(all_data, kind, item) + items = all_data[kind] || {} + if !items[item[:key]].nil? + raise ArgumentError, "#{kind[:namespace]} key \"#{item[:key]}\" was used more than once" + end + items[item[:key]] = item + end + + def make_flag_with_value(key, value) + { + key: key, + on: true, + fallthrough: { variation: 0 }, + variations: [ value ] + } + end + + def start_listener + resolved_paths = @paths.map { |p| Pathname.new(File.absolute_path(p)).realpath.to_s } + path_set = resolved_paths.to_set + dir_paths = resolved_paths.map{ |p| File.dirname(p) }.uniq + l = Listen.to(*dir_paths) do |modified, added, removed| + paths = modified + added + removed + if paths.any? { |p| path_set.include?(p) } + load_all + end + end + l.start + l + end + end +end diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index 4ad7928e..94c24229 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -39,22 +39,11 @@ def initialize(sdk_key, config = Config.default, wait_for_sec = 5) return # requestor and update processor are not used in this mode end - requestor = Requestor.new(sdk_key, config) - - if @config.offline? - @update_processor = NullUpdateProcessor.new + if @config.update_processor + @update_processor = @config.update_processor else - if @config.update_processor.nil? - if @config.stream? - @update_processor = StreamProcessor.new(sdk_key, config, requestor) - else - @config.logger.info { "Disabling streaming API" } - @config.logger.warn { "You should only disable the streaming API if instructed to do so by LaunchDarkly support" } - @update_processor = PollingProcessor.new(config, requestor) - end - else - @update_processor = @config.update_processor - end + factory = @config.update_processor || self.method(:create_default_update_processor) + @update_processor = factory.call(sdk_key, config) end ready = @update_processor.start @@ -269,6 +258,20 @@ def close private + def create_default_update_processor(sdk_key, config) + if config.offline? + return NullUpdateProcessor.new + end + requestor = Requestor.new(sdk_key, config) + if config.stream? + StreamProcessor.new(sdk_key, config, requestor) + else + config.logger.info { "Disabling streaming API" } + config.logger.warn { "You should only disable the streaming API if instructed to do so by LaunchDarkly support" } + PollingProcessor.new(config, requestor) + end + end + # @return [EvaluationDetail] def evaluate_internal(key, user, default, include_reasons_in_events) if @config.offline? diff --git a/spec/file_data_source_spec.rb b/spec/file_data_source_spec.rb new file mode 100644 index 00000000..c163d385 --- /dev/null +++ b/spec/file_data_source_spec.rb @@ -0,0 +1,167 @@ +require "spec_helper" +require "tempfile" + +describe LaunchDarkly::FileDataSource do + let(:full_flag_1_key) { "flag1" } + let(:flag_value_1_key) { "flag2" } + let(:all_flag_keys) { [ full_flag_1_key, flag_value_1_key ] } + let(:full_segment_1_key) { "seg1" } + let(:all_segment_keys) { [ full_segment_1_key ] } + + let(:flag_only_json) { <<-EOF + { + "flags": { + "flag1": { + "key": "flag1", + "on": true + } + } + } +EOF +} + + let(:all_properties_json) { <<-EOF + { + "flags": { + "flag1": { + "key": "flag1", + "on": true + } + }, + "flagValues": { + "flag2": "value2" + }, + "segments": { + "seg1": { + "key": "seg1", + "include": ["user1"] + } + } + } +EOF + } + + let(:all_properties_yaml) { <<-EOF +--- +flags: + flag1: + key: flag1 + "on": true +flagValues: + flag2: value2 +segments: + seg1: + key: seg1 + include: ["user1"] +EOF + } + + let(:bad_file_path) { "no-such-file" } + + before do + @config = LaunchDarkly::Config.new + @store = @config.feature_store + end + + def make_temp_file(content) + file = Tempfile.new('flags') + IO.write(file, content) + file + end + + def with_data_source(options) + factory = LaunchDarkly::FileDataSource.factory(options) + ds = factory.call('', @config) + begin + yield ds + ensure + ds.stop + end + end + + it "doesn't load flags prior to start" do + file = make_temp_file('{"flagValues":{"key":"value"}}') + with_data_source({ paths: [ file.path ] }) do |ds| + expect(@store.initialized?).to eq(false) + expect(@store.all(LaunchDarkly::FEATURES)).to eq({}) + expect(@store.all(LaunchDarkly::SEGMENTS)).to eq({}) + end + end + + it "loads flags on start - from JSON" do + file = make_temp_file(all_properties_json) + with_data_source({ paths: [ file.path ] }) do |ds| + ds.start + expect(@store.initialized?).to eq(true) + expect(@store.all(LaunchDarkly::FEATURES).keys).to eq(all_flag_keys) + expect(@store.all(LaunchDarkly::SEGMENTS).keys).to eq(all_segment_keys) + end + end + + it "loads flags on start - from YAML" do + file = make_temp_file(all_properties_yaml) + with_data_source({ paths: [ file.path ] }) do |ds| + ds.start + expect(@store.initialized?).to eq(true) + expect(@store.all(LaunchDarkly::FEATURES).keys).to eq(all_flag_keys) + expect(@store.all(LaunchDarkly::SEGMENTS).keys).to eq(all_segment_keys) + end + end + + it "sets start event and initialized on successful load" do + file = make_temp_file(all_properties_json) + with_data_source({ paths: [ file.path ] }) do |ds| + event = ds.start + expect(event.set?).to eq(true) + expect(ds.initialized?).to eq(true) + end + end + + it "sets start event and does not set initialized on unsuccessful load" do + with_data_source({ paths: [ bad_file_path ] }) do |ds| + event = ds.start + expect(event.set?).to eq(true) + expect(ds.initialized?).to eq(false) + end + end + + it "does not reload modified file if auto-update is off" do + file = make_temp_file(flag_only_json) + + with_data_source({ paths: [ file.path ] }) do |ds| + event = ds.start + expect(event.set?).to eq(true) + expect(@store.all(LaunchDarkly::SEGMENTS).keys).to eq([]) + + IO.write(file, all_properties_json) + sleep(0.5) + expect(@store.all(LaunchDarkly::SEGMENTS).keys).to eq([]) + end + end + + it "reloads modified file if auto-update is on" do + file = make_temp_file(flag_only_json) + + with_data_source({ auto_update: true, paths: [ file.path ] }) do |ds| + event = ds.start + expect(event.set?).to eq(true) + expect(@store.all(LaunchDarkly::SEGMENTS).keys).to eq([]) + + sleep(1) + IO.write(file, all_properties_json) + + max_time = 10 + ok = wait_for_condition(10) { @store.all(LaunchDarkly::SEGMENTS).keys == all_segment_keys } + expect(ok).to eq(true), "Waited #{max_time}s after modifying file and it did not reload" + end + end + + def wait_for_condition(max_time) + deadline = Time.now + max_time + while Time.now < deadline + return true if yield + sleep(0.1) + end + false + end +end From 22ebdeddf21c3d7cf9602add1442e934ead6b43d Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 31 Oct 2018 11:03:21 -0700 Subject: [PATCH 24/45] add poll interval param, tolerate single file path string, add doc comments --- lib/ldclient-rb/file_data_source.rb | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index 65ba0735..c61ddcf9 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -80,6 +80,20 @@ module LaunchDarkly # duplicate key-- it will not load flags from any of the files. # class FileDataSource + # + # Returns a factory for the file data source component. + # + # @param options [Hash] the configuration options + # @option options [Array] :paths The paths of the source files for loading flag data. These + # may be absolute paths or relative to the current working directory. + # @option options [Boolean] :auto_update True if the data source should watch for changes to + # the source file(s) and reload flags whenever there is a change. Note that auto-updating + # will only work if all of the files you specified have valid directory paths at startup time. + # @option options [Float] :poll_interval The minimum interval, in seconds, between checks for + # file modifications - used only if auto_update is true. On Linux and Mac platforms, you do + # not need to set this as there is a native OS mechanism for detecting file changes; on other + # platforms, the default interval is one second. + # def self.factory(options={}) return Proc.new do |sdk_key, config| FileDataSourceImpl.new(config.feature_store, config.logger, options) @@ -92,7 +106,11 @@ def initialize(feature_store, logger, options={}) @feature_store = feature_store @logger = logger @paths = options[:paths] || [] + if @paths.is_a? String + @paths = [ @paths ] + end @auto_update = options[:auto_update] + @poll_interval = options[:poll_interval] @initialized = Concurrent::AtomicBoolean.new(false) @ready = Concurrent::Event.new end @@ -196,7 +214,11 @@ def start_listener resolved_paths = @paths.map { |p| Pathname.new(File.absolute_path(p)).realpath.to_s } path_set = resolved_paths.to_set dir_paths = resolved_paths.map{ |p| File.dirname(p) }.uniq - l = Listen.to(*dir_paths) do |modified, added, removed| + opts = {} + if !@poll_interval.nil? + opts[:latency] = @poll_interval + end + l = Listen.to(*dir_paths, opts) do |modified, added, removed| paths = modified + added + removed if paths.any? { |p| path_set.include?(p) } load_all From b864390a2079c6588e3fae0d8f8cfce359136cb6 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 31 Oct 2018 12:02:29 -0700 Subject: [PATCH 25/45] make listen dependency optional --- ldclient-rb.gemspec | 2 +- lib/ldclient-rb/file_data_source.rb | 23 ++++++++++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/ldclient-rb.gemspec b/ldclient-rb.gemspec index 9e7d5d04..0b8f4f9d 100644 --- a/ldclient-rb.gemspec +++ b/ldclient-rb.gemspec @@ -29,6 +29,7 @@ Gem::Specification.new do |spec| spec.add_development_dependency "rake", "~> 10.0" spec.add_development_dependency "rspec_junit_formatter", "~> 0.3.0" spec.add_development_dependency "timecop", "~> 0.9.1" + spec.add_development_dependency "listen", "~> 3.0" # see file_data_source.rb spec.add_runtime_dependency "json", [">= 1.8", "< 3"] spec.add_runtime_dependency "faraday", [">= 0.9", "< 2"] @@ -40,5 +41,4 @@ Gem::Specification.new do |spec| spec.add_runtime_dependency "hashdiff", "~> 0.2" spec.add_runtime_dependency "http_tools", '~> 0.4.5' spec.add_runtime_dependency "socketry", "~> 0.5.1" - spec.add_runtime_dependency "listen", "~> 3.0" end diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index c61ddcf9..833d6ec3 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -1,10 +1,21 @@ require 'concurrent/atomics' require 'json' require 'yaml' -require 'listen' require 'pathname' module LaunchDarkly + # To avoid pulling in 'listen' and its transitive dependencies for people who aren't using the + # file data source or who don't need auto-updating, we only enable auto-update if the 'listen' + # gem has been provided by the host app. + @@have_listen = false + begin + require 'listen' + @@have_listen = true + rescue + end + def self.can_watch_files? + @@have_listen + end # # Provides a way to use local files as a source of feature flag state. This would typically be @@ -87,8 +98,10 @@ class FileDataSource # @option options [Array] :paths The paths of the source files for loading flag data. These # may be absolute paths or relative to the current working directory. # @option options [Boolean] :auto_update True if the data source should watch for changes to - # the source file(s) and reload flags whenever there is a change. Note that auto-updating - # will only work if all of the files you specified have valid directory paths at startup time. + # the source file(s) and reload flags whenever there is a change. In order to use this + # feature, you must install the 'listen' gem - it is not included by default to avoid adding + # unwanted dependencies to the SDK. Note that auto-updating will only work if all of the files + # you specified have valid directory paths at startup time. # @option options [Float] :poll_interval The minimum interval, in seconds, between checks for # file modifications - used only if auto_update is true. On Linux and Mac platforms, you do # not need to set this as there is a native OS mechanism for detecting file changes; on other @@ -110,6 +123,10 @@ def initialize(feature_store, logger, options={}) @paths = [ @paths ] end @auto_update = options[:auto_update] + if @auto_update && !LaunchDarkly::can_watch_files? + @logger.error { "[LDClient] To use the auto_update option for FileDataSource, you must install the 'listen' gem." } + @auto_update = false + end @poll_interval = options[:poll_interval] @initialized = Concurrent::AtomicBoolean.new(false) @ready = Concurrent::Event.new From 789b5a4b54de8d84802af0579bacabbd07f92169 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 31 Oct 2018 12:04:07 -0700 Subject: [PATCH 26/45] readme --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 1790b2d4..ead2bb6b 100644 --- a/README.md +++ b/README.md @@ -121,6 +121,10 @@ else end ``` +Using flag data from a file +--------------------------- +For testing purposes, the SDK can be made to read feature flag state from a file or files instead of connecting to LaunchDarkly. See [`file_data_source.rb`](https://github.com/launchdarkly/ruby-client/blob/master/lib/ldclient-rb/file_data_source.rb) for more details. + Learn more ----------- From 31a62c59a8f2209dbd758ca27fe113825b2a2943 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Wed, 31 Oct 2018 13:20:02 -0700 Subject: [PATCH 27/45] fix key handling and client integration, add tests --- lib/ldclient-rb/file_data_source.rb | 2 +- lib/ldclient-rb/ldclient.rb | 2 +- spec/file_data_source_spec.rb | 46 ++++++++++++++++++++++++++--- 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index 833d6ec3..10588b5d 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -215,7 +215,7 @@ def add_item(all_data, kind, item) if !items[item[:key]].nil? raise ArgumentError, "#{kind[:namespace]} key \"#{item[:key]}\" was used more than once" end - items[item[:key]] = item + items[item[:key].to_sym] = item end def make_flag_with_value(key, value) diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index 94c24229..f8a75780 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -42,7 +42,7 @@ def initialize(sdk_key, config = Config.default, wait_for_sec = 5) if @config.update_processor @update_processor = @config.update_processor else - factory = @config.update_processor || self.method(:create_default_update_processor) + factory = @config.update_processor_factory || self.method(:create_default_update_processor) @update_processor = factory.call(sdk_key, config) end diff --git a/spec/file_data_source_spec.rb b/spec/file_data_source_spec.rb index c163d385..cf5d52ad 100644 --- a/spec/file_data_source_spec.rb +++ b/spec/file_data_source_spec.rb @@ -3,17 +3,23 @@ describe LaunchDarkly::FileDataSource do let(:full_flag_1_key) { "flag1" } + let(:full_flag_1_value) { "on" } let(:flag_value_1_key) { "flag2" } - let(:all_flag_keys) { [ full_flag_1_key, flag_value_1_key ] } + let(:flag_value_1) { "value2" } + let(:all_flag_keys) { [ full_flag_1_key.to_sym, flag_value_1_key.to_sym ] } let(:full_segment_1_key) { "seg1" } - let(:all_segment_keys) { [ full_segment_1_key ] } + let(:all_segment_keys) { [ full_segment_1_key.to_sym ] } let(:flag_only_json) { <<-EOF { "flags": { "flag1": { "key": "flag1", - "on": true + "on": true, + "fallthrough": { + "variation": 2 + }, + "variations": [ "fall", "off", "on" ] } } } @@ -25,7 +31,11 @@ "flags": { "flag1": { "key": "flag1", - "on": true + "on": true, + "fallthrough": { + "variation": 2 + }, + "variations": [ "fall", "off", "on" ] } }, "flagValues": { @@ -156,6 +166,34 @@ def with_data_source(options) end end + it "evaluates simplified flag with client as expected" do + file = make_temp_file(all_properties_json) + factory = LaunchDarkly::FileDataSource.factory({ paths: file.path }) + config = LaunchDarkly::Config.new(send_events: false, update_processor_factory: factory) + client = LaunchDarkly::LDClient.new('sdkKey', config) + + begin + value = client.variation(flag_value_1_key, { key: 'user' }, '') + expect(value).to eq(flag_value_1) + ensure + client.close + end + end + + it "evaluates full flag with client as expected" do + file = make_temp_file(all_properties_json) + factory = LaunchDarkly::FileDataSource.factory({ paths: file.path }) + config = LaunchDarkly::Config.new(send_events: false, update_processor_factory: factory) + client = LaunchDarkly::LDClient.new('sdkKey', config) + + begin + value = client.variation(full_flag_1_key, { key: 'user' }, '') + expect(value).to eq(full_flag_1_value) + ensure + client.close + end + end + def wait_for_condition(max_time) deadline = Time.now + max_time while Time.now < deadline From 778cb6dc5e4c2c367ccd2c1c7399a1338ec5196a Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Thu, 1 Nov 2018 12:08:46 -0700 Subject: [PATCH 28/45] debugging --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index df9dac51..58c754ba 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -22,7 +22,7 @@ ruby-docker-template: &ruby-docker-template - run: gem install bundler - run: bundle install - run: mkdir ./rspec - - run: bundle exec rspec --format progress --format RspecJunitFormatter -o ./rspec/rspec.xml spec + - run: LISTEN_GEM_DEBUGGING=2 bundle exec rspec --format progress --format RspecJunitFormatter -o ./rspec/rspec.xml spec - store_test_results: path: ./rspec - store_artifacts: From 20dbef28105da9a1eca453ee86f2ff90267f4793 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Thu, 1 Nov 2018 12:13:58 -0700 Subject: [PATCH 29/45] debugging --- .circleci/config.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 58c754ba..05bc4746 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -22,7 +22,7 @@ ruby-docker-template: &ruby-docker-template - run: gem install bundler - run: bundle install - run: mkdir ./rspec - - run: LISTEN_GEM_DEBUGGING=2 bundle exec rspec --format progress --format RspecJunitFormatter -o ./rspec/rspec.xml spec + - run: bundle exec rspec --format progress --format RspecJunitFormatter -o ./rspec/rspec.xml spec - store_test_results: path: ./rspec - store_artifacts: @@ -93,5 +93,5 @@ jobs: do rvm use $i; cp "Gemfile.lock.$i" Gemfile.lock; - bundle exec rspec spec; + LISTEN_GEM_DEBUGGING=2 bundle exec rspec spec; done From f1c00b1616a6767dd350c44497ba71d6b03e4bff Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Thu, 1 Nov 2018 15:47:10 -0700 Subject: [PATCH 30/45] add fallback polling logic, fix tests --- lib/ldclient-rb/file_data_source.rb | 85 ++++++++++++++++++++++------- spec/file_data_source_spec.rb | 23 +++++++- 2 files changed, 86 insertions(+), 22 deletions(-) diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index 10588b5d..ae19bea8 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -13,7 +13,7 @@ module LaunchDarkly @@have_listen = true rescue end - def self.can_watch_files? + def self.have_listen? @@have_listen end @@ -23,10 +23,10 @@ def self.can_watch_files? # actual LaunchDarkly connection. # # To use this component, call `FileDataSource.factory`, and store its return value in the - # `update_processor_factory` property of your LaunchDarkly client configuration. In the options + # `update_processor_class` property of your LaunchDarkly client configuration. In the options # to `factory`, set `paths` to the file path(s) of your data file(s): # - # config.update_processor_factory = FileDataSource.factory(paths: [ myFilePath ]) + # config.update_processor_class = FileDataSource.factory(paths: [ myFilePath ]) # # This will cause the client not to connect to LaunchDarkly to get feature flags. The # client may still make network connections to send analytics events, unless you have disabled @@ -98,14 +98,15 @@ class FileDataSource # @option options [Array] :paths The paths of the source files for loading flag data. These # may be absolute paths or relative to the current working directory. # @option options [Boolean] :auto_update True if the data source should watch for changes to - # the source file(s) and reload flags whenever there is a change. In order to use this - # feature, you must install the 'listen' gem - it is not included by default to avoid adding - # unwanted dependencies to the SDK. Note that auto-updating will only work if all of the files - # you specified have valid directory paths at startup time. + # the source file(s) and reload flags whenever there is a change. Note that the default + # implementation of this feature is based on polling the filesystem, which may not perform + # well. If you install the 'listen' gem (not included by default, to avoid adding unwanted + # dependencies to the SDK), its native file watching mechanism will be used instead. Note + # that auto-updating will only work if all of the files you specified have valid directory + # paths at startup time. # @option options [Float] :poll_interval The minimum interval, in seconds, between checks for - # file modifications - used only if auto_update is true. On Linux and Mac platforms, you do - # not need to set this as there is a native OS mechanism for detecting file changes; on other - # platforms, the default interval is one second. + # file modifications - used only if auto_update is true, and if the native file-watching + # mechanism from 'listen' is not being used. # def self.factory(options={}) return Proc.new do |sdk_key, config| @@ -123,11 +124,8 @@ def initialize(feature_store, logger, options={}) @paths = [ @paths ] end @auto_update = options[:auto_update] - if @auto_update && !LaunchDarkly::can_watch_files? - @logger.error { "[LDClient] To use the auto_update option for FileDataSource, you must install the 'listen' gem." } - @auto_update = false - end - @poll_interval = options[:poll_interval] + @use_listen = @auto_update && LaunchDarkly.have_listen? && !options[:force_polling] # force_polling is used only for tests + @poll_interval = options[:poll_interval] || 1 @initialized = Concurrent::AtomicBoolean.new(false) @ready = Concurrent::Event.new end @@ -229,12 +227,17 @@ def make_flag_with_value(key, value) def start_listener resolved_paths = @paths.map { |p| Pathname.new(File.absolute_path(p)).realpath.to_s } + if @use_listen + start_listener_with_listen_gem(resolved_paths) + else + FileDataSourcePoller.new(resolved_paths, @poll_interval, self.method(:load_all)) + end + end + + def start_listener_with_listen_gem(resolved_paths) path_set = resolved_paths.to_set dir_paths = resolved_paths.map{ |p| File.dirname(p) }.uniq - opts = {} - if !@poll_interval.nil? - opts[:latency] = @poll_interval - end + opts = { latency: @poll_interval } l = Listen.to(*dir_paths, opts) do |modified, added, removed| paths = modified + added + removed if paths.any? { |p| path_set.include?(p) } @@ -244,5 +247,49 @@ def start_listener l.start l end + + # + # Used internally by FileDataSource to track data file changes if the 'listen' gem is not available. + # + class FileDataSourcePoller + def initialize(resolved_paths, interval, reloader) + @stopped = Concurrent::AtomicBoolean.new(false) + get_file_times = Proc.new do + ret = {} + resolved_paths.each do |path| + begin + ret[path] = File.mtime(path) + rescue + ret[path] = nil + end + end + ret + end + last_times = get_file_times.call + @thread = Thread.new do + while true + sleep interval + break if @stopped.value + new_times = get_file_times.call + changed = false + last_times.each do |path, old_time| + new_time = new_times[path] + if !new_time.nil? && new_time != old_time + changed = true + break + end + end + if changed + reloader.call + end + end + end + end + + def stop + @stopped.make_true + @thread.run # wakes it up if it's sleeping + end + end end end diff --git a/spec/file_data_source_spec.rb b/spec/file_data_source_spec.rb index cf5d52ad..5267a5f2 100644 --- a/spec/file_data_source_spec.rb +++ b/spec/file_data_source_spec.rb @@ -71,10 +71,18 @@ before do @config = LaunchDarkly::Config.new @store = @config.feature_store + @tmp_dir = Dir.mktmpdir + end + + after do + FileUtils.remove_dir(@tmp_dir) end def make_temp_file(content) - file = Tempfile.new('flags') + # Note that we don't create our files in the default temp file directory, but rather in an empty directory + # that we made. That's because (depending on the platform) the temp file directory may contain huge numbers + # of files, which can make the file watcher perform poorly enough to break the tests. + file = Tempfile.new('flags', @tmp_dir) IO.write(file, content) file end @@ -149,10 +157,11 @@ def with_data_source(options) end end - it "reloads modified file if auto-update is on" do + def test_auto_reload(options) file = make_temp_file(flag_only_json) + options[:paths] = [ file.path ] - with_data_source({ auto_update: true, paths: [ file.path ] }) do |ds| + with_data_source(options) do |ds| event = ds.start expect(event.set?).to eq(true) expect(@store.all(LaunchDarkly::SEGMENTS).keys).to eq([]) @@ -166,6 +175,14 @@ def with_data_source(options) end end + it "reloads modified file if auto-update is on" do + test_auto_reload({ auto_update: true }) + end + + it "reloads modified file in polling mode" do + test_auto_reload({ auto_update: true, force_polling: true, poll_interval: 0.1 }) + end + it "evaluates simplified flag with client as expected" do file = make_temp_file(all_properties_json) factory = LaunchDarkly::FileDataSource.factory({ paths: file.path }) From 198b843bba00fe92e9cfa9ef658c2649ce09be2f Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Thu, 1 Nov 2018 16:02:22 -0700 Subject: [PATCH 31/45] rm debugging --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 05bc4746..df9dac51 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -93,5 +93,5 @@ jobs: do rvm use $i; cp "Gemfile.lock.$i" Gemfile.lock; - LISTEN_GEM_DEBUGGING=2 bundle exec rspec spec; + bundle exec rspec spec; done From c5d1823372044bd067049fed90fb8e1f13428d94 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Thu, 1 Nov 2018 17:25:16 -0700 Subject: [PATCH 32/45] debugging --- spec/file_data_source_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/file_data_source_spec.rb b/spec/file_data_source_spec.rb index 5267a5f2..194ebc2c 100644 --- a/spec/file_data_source_spec.rb +++ b/spec/file_data_source_spec.rb @@ -68,6 +68,8 @@ let(:bad_file_path) { "no-such-file" } + Thread.report_on_exception = true + before do @config = LaunchDarkly::Config.new @store = @config.feature_store From 9baffe35cf84bbfdbf77f01989437620f4124bc7 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Thu, 1 Nov 2018 17:38:15 -0700 Subject: [PATCH 33/45] debugging --- .circleci/config.yml | 2 +- spec/file_data_source_spec.rb | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index df9dac51..05bc4746 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -93,5 +93,5 @@ jobs: do rvm use $i; cp "Gemfile.lock.$i" Gemfile.lock; - bundle exec rspec spec; + LISTEN_GEM_DEBUGGING=2 bundle exec rspec spec; done diff --git a/spec/file_data_source_spec.rb b/spec/file_data_source_spec.rb index 194ebc2c..5267a5f2 100644 --- a/spec/file_data_source_spec.rb +++ b/spec/file_data_source_spec.rb @@ -68,8 +68,6 @@ let(:bad_file_path) { "no-such-file" } - Thread.report_on_exception = true - before do @config = LaunchDarkly::Config.new @store = @config.feature_store From 4d8121592756df99aefbef4c0aeb78032f544046 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Thu, 1 Nov 2018 17:47:03 -0700 Subject: [PATCH 34/45] debugging --- lib/ldclient-rb/file_data_source.rb | 2 ++ spec/file_data_source_spec.rb | 1 + 2 files changed, 3 insertions(+) diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index ae19bea8..de8ef34e 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -238,8 +238,10 @@ def start_listener_with_listen_gem(resolved_paths) path_set = resolved_paths.to_set dir_paths = resolved_paths.map{ |p| File.dirname(p) }.uniq opts = { latency: @poll_interval } + puts('*** starting listener') l = Listen.to(*dir_paths, opts) do |modified, added, removed| paths = modified + added + removed + puts('*** got listener notification: #{paths}') if paths.any? { |p| path_set.include?(p) } load_all end diff --git a/spec/file_data_source_spec.rb b/spec/file_data_source_spec.rb index 5267a5f2..f06c19f9 100644 --- a/spec/file_data_source_spec.rb +++ b/spec/file_data_source_spec.rb @@ -168,6 +168,7 @@ def test_auto_reload(options) sleep(1) IO.write(file, all_properties_json) + puts('*** modified the file') max_time = 10 ok = wait_for_condition(10) { @store.all(LaunchDarkly::SEGMENTS).keys == all_segment_keys } From 30d0cd270acf6518555e126bad28c689177ebb1d Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Thu, 1 Nov 2018 17:48:09 -0700 Subject: [PATCH 35/45] debugging --- lib/ldclient-rb/file_data_source.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index de8ef34e..9a63e56b 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -241,7 +241,7 @@ def start_listener_with_listen_gem(resolved_paths) puts('*** starting listener') l = Listen.to(*dir_paths, opts) do |modified, added, removed| paths = modified + added + removed - puts('*** got listener notification: #{paths}') + puts("*** got listener notification: #{paths}") if paths.any? { |p| path_set.include?(p) } load_all end From 8cb2ed9adc1a7ac486f077eeb37d0100fa9d9bb5 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Thu, 1 Nov 2018 17:51:27 -0700 Subject: [PATCH 36/45] comment correction --- lib/ldclient-rb/file_data_source.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index 9a63e56b..71f3a8be 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -23,10 +23,11 @@ def self.have_listen? # actual LaunchDarkly connection. # # To use this component, call `FileDataSource.factory`, and store its return value in the - # `update_processor_class` property of your LaunchDarkly client configuration. In the options + # `update_processor_factory` property of your LaunchDarkly client configuration. In the options # to `factory`, set `paths` to the file path(s) of your data file(s): # - # config.update_processor_class = FileDataSource.factory(paths: [ myFilePath ]) + # factory = FileDataSource.factory(paths: [ myFilePath ]) + # config = LaunchDarkly::Config.new(update_processor_factory: factory) # # This will cause the client not to connect to LaunchDarkly to get feature flags. The # client may still make network connections to send analytics events, unless you have disabled From a10f973ad98f033bd480e2ca9568041e826cd02b Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 2 Nov 2018 11:15:29 -0700 Subject: [PATCH 37/45] documentation --- lib/ldclient-rb/file_data_source.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index 71f3a8be..721eff75 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -107,7 +107,7 @@ class FileDataSource # paths at startup time. # @option options [Float] :poll_interval The minimum interval, in seconds, between checks for # file modifications - used only if auto_update is true, and if the native file-watching - # mechanism from 'listen' is not being used. + # mechanism from 'listen' is not being used. The default value is 1 second. # def self.factory(options={}) return Proc.new do |sdk_key, config| From 16cf9c086c06344d352b6e85bb6e02449af44cc1 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 2 Nov 2018 11:15:54 -0700 Subject: [PATCH 38/45] always use YAML parser --- lib/ldclient-rb/file_data_source.rb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index 721eff75..a607923d 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -190,11 +190,10 @@ def load_file(path, all_data) end def parse_content(content) - if content.strip.start_with?("{") - JSON.parse(content, symbolize_names: true) - else - symbolize_all_keys(YAML.load(content)) - end + # We can use the Ruby YAML parser for both YAML and JSON (JSON is a subset of YAML and while + # not all YAML parsers handle it correctly, we have verified that the Ruby one does, at least + # for all the samples of actual flag data that we've tested). + symbolize_all_keys(YAML.load(content)) end def symbolize_all_keys(value) From 27d954e7f5f84ba4b87573ff80e9304a4eedab3b Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 2 Nov 2018 11:21:29 -0700 Subject: [PATCH 39/45] report internal error that shouldn't happen --- lib/ldclient-rb/file_data_source.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index a607923d..fae68123 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -209,7 +209,8 @@ def symbolize_all_keys(value) end def add_item(all_data, kind, item) - items = all_data[kind] || {} + items = all_data[kind] + raise ArgumentError, "Received unknown item kind #{kind} in add_data" if items.nil? # shouldn't be possible since we preinitialize the hash if !items[item[:key]].nil? raise ArgumentError, "#{kind[:namespace]} key \"#{item[:key]}\" was used more than once" end From fd308a9de3142b8fd493a995411d320a42664932 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 2 Nov 2018 11:24:28 -0700 Subject: [PATCH 40/45] add test for multiple files --- spec/file_data_source_spec.rb | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/spec/file_data_source_spec.rb b/spec/file_data_source_spec.rb index f06c19f9..c0af4c67 100644 --- a/spec/file_data_source_spec.rb +++ b/spec/file_data_source_spec.rb @@ -24,7 +24,19 @@ } } EOF -} + } + + let(:segment_only_json) { <<-EOF + { + "segments": { + "seg1": { + "key": "seg1", + "include": ["user1"] + } + } + } +EOF + } let(:all_properties_json) { <<-EOF { @@ -143,6 +155,16 @@ def with_data_source(options) end end + it "can load multiple files" do + file1 = make_temp_file(flag_only_json) + file2 = make_temp_file(segment_only_json) + with_data_source({ paths: [ file1.path, file2.path ] }) do |ds| + ds.start + expect(@store.all(LaunchDarkly::FEATURES).keys).to eq([ full_flag_1_key.to_sym ]) + expect(@store.all(LaunchDarkly::SEGMENTS).keys).to eq([ full_segment_1_key.to_sym ]) + end + end + it "does not reload modified file if auto-update is off" do file = make_temp_file(flag_only_json) From 1d016bfc9349000c8ddffce20b48634e1e20d6b3 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 2 Nov 2018 11:26:10 -0700 Subject: [PATCH 41/45] fix duplicate key checking (string vs. symbol problem) --- lib/ldclient-rb/file_data_source.rb | 5 +++-- spec/file_data_source_spec.rb | 11 +++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index fae68123..aebd9709 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -211,10 +211,11 @@ def symbolize_all_keys(value) def add_item(all_data, kind, item) items = all_data[kind] raise ArgumentError, "Received unknown item kind #{kind} in add_data" if items.nil? # shouldn't be possible since we preinitialize the hash - if !items[item[:key]].nil? + key = item[:key].to_sym + if !items[key].nil? raise ArgumentError, "#{kind[:namespace]} key \"#{item[:key]}\" was used more than once" end - items[item[:key].to_sym] = item + items[key] = item end def make_flag_with_value(key, value) diff --git a/spec/file_data_source_spec.rb b/spec/file_data_source_spec.rb index c0af4c67..10e49e3c 100644 --- a/spec/file_data_source_spec.rb +++ b/spec/file_data_source_spec.rb @@ -160,11 +160,22 @@ def with_data_source(options) file2 = make_temp_file(segment_only_json) with_data_source({ paths: [ file1.path, file2.path ] }) do |ds| ds.start + expect(@store.initialized?).to eq(true) expect(@store.all(LaunchDarkly::FEATURES).keys).to eq([ full_flag_1_key.to_sym ]) expect(@store.all(LaunchDarkly::SEGMENTS).keys).to eq([ full_segment_1_key.to_sym ]) end end + it "does not allow duplicate keys" do + file1 = make_temp_file(flag_only_json) + file2 = make_temp_file(flag_only_json) + with_data_source({ paths: [ file1.path, file2.path ] }) do |ds| + ds.start + expect(@store.initialized?).to eq(false) + expect(@store.all(LaunchDarkly::FEATURES).keys).to eq([]) + end + end + it "does not reload modified file if auto-update is off" do file = make_temp_file(flag_only_json) From c3e66d35c64909084d6d879fa485497fddf6c4a4 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 2 Nov 2018 11:44:09 -0700 Subject: [PATCH 42/45] Don't use 'listen' in JRuby 9.1 --- lib/ldclient-rb/file_data_source.rb | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index aebd9709..23834be4 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -99,12 +99,12 @@ class FileDataSource # @option options [Array] :paths The paths of the source files for loading flag data. These # may be absolute paths or relative to the current working directory. # @option options [Boolean] :auto_update True if the data source should watch for changes to - # the source file(s) and reload flags whenever there is a change. Note that the default - # implementation of this feature is based on polling the filesystem, which may not perform - # well. If you install the 'listen' gem (not included by default, to avoid adding unwanted - # dependencies to the SDK), its native file watching mechanism will be used instead. Note - # that auto-updating will only work if all of the files you specified have valid directory - # paths at startup time. + # the source file(s) and reload flags whenever there is a change. Auto-updating will only + # work if all of the files you specified have valid directory paths at startup time. + # Note that the default implementation of this feature is based on polling the filesystem, + # which may not perform well. If you install the 'listen' gem (not included by default, to + # avoid adding unwanted dependencies to the SDK), its native file watching mechanism will be + # used instead. However, 'listen' will not be used in JRuby 9.1 due to a known instability. # @option options [Float] :poll_interval The minimum interval, in seconds, between checks for # file modifications - used only if auto_update is true, and if the native file-watching # mechanism from 'listen' is not being used. The default value is 1 second. @@ -125,7 +125,15 @@ def initialize(feature_store, logger, options={}) @paths = [ @paths ] end @auto_update = options[:auto_update] - @use_listen = @auto_update && LaunchDarkly.have_listen? && !options[:force_polling] # force_polling is used only for tests + if @auto_update && LaunchDarkly.have_listen? && !options[:force_polling] # force_polling is used only for tests + # We have seen unreliable behavior in the 'listen' gem in JRuby 9.1 (https://github.com/guard/listen/issues/449). + # Therefore, on that platform we'll fall back to file polling instead. + if defined?(JRUBY_VERSION) && JRUBY_VERSION.start_with?("9.1.") + @use_listen = false + else + @use_listen = true + end + end @poll_interval = options[:poll_interval] || 1 @initialized = Concurrent::AtomicBoolean.new(false) @ready = Concurrent::Event.new From 1a36fd86ab5b867ad265e89f13d9c8e839278b39 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 2 Nov 2018 11:50:22 -0700 Subject: [PATCH 43/45] rm debugging --- .circleci/config.yml | 2 +- lib/ldclient-rb/file_data_source.rb | 2 -- spec/file_data_source_spec.rb | 1 - 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 05bc4746..df9dac51 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -93,5 +93,5 @@ jobs: do rvm use $i; cp "Gemfile.lock.$i" Gemfile.lock; - LISTEN_GEM_DEBUGGING=2 bundle exec rspec spec; + bundle exec rspec spec; done diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index 23834be4..1549f6ec 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -248,10 +248,8 @@ def start_listener_with_listen_gem(resolved_paths) path_set = resolved_paths.to_set dir_paths = resolved_paths.map{ |p| File.dirname(p) }.uniq opts = { latency: @poll_interval } - puts('*** starting listener') l = Listen.to(*dir_paths, opts) do |modified, added, removed| paths = modified + added + removed - puts("*** got listener notification: #{paths}") if paths.any? { |p| path_set.include?(p) } load_all end diff --git a/spec/file_data_source_spec.rb b/spec/file_data_source_spec.rb index 10e49e3c..60107e26 100644 --- a/spec/file_data_source_spec.rb +++ b/spec/file_data_source_spec.rb @@ -201,7 +201,6 @@ def test_auto_reload(options) sleep(1) IO.write(file, all_properties_json) - puts('*** modified the file') max_time = 10 ok = wait_for_condition(10) { @store.all(LaunchDarkly::SEGMENTS).keys == all_segment_keys } From 78ba8150b1a486b2a568ff7ac59f8b589fdfe98e Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 2 Nov 2018 12:02:32 -0700 Subject: [PATCH 44/45] better error handling in poll thread --- lib/ldclient-rb/file_data_source.rb | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/lib/ldclient-rb/file_data_source.rb b/lib/ldclient-rb/file_data_source.rb index 1549f6ec..c5207afb 100644 --- a/lib/ldclient-rb/file_data_source.rb +++ b/lib/ldclient-rb/file_data_source.rb @@ -240,7 +240,7 @@ def start_listener if @use_listen start_listener_with_listen_gem(resolved_paths) else - FileDataSourcePoller.new(resolved_paths, @poll_interval, self.method(:load_all)) + FileDataSourcePoller.new(resolved_paths, @poll_interval, self.method(:load_all), @logger) end end @@ -262,14 +262,14 @@ def start_listener_with_listen_gem(resolved_paths) # Used internally by FileDataSource to track data file changes if the 'listen' gem is not available. # class FileDataSourcePoller - def initialize(resolved_paths, interval, reloader) + def initialize(resolved_paths, interval, reloader, logger) @stopped = Concurrent::AtomicBoolean.new(false) get_file_times = Proc.new do ret = {} resolved_paths.each do |path| begin ret[path] = File.mtime(path) - rescue + rescue Errno::ENOENT ret[path] = nil end end @@ -280,17 +280,19 @@ def initialize(resolved_paths, interval, reloader) while true sleep interval break if @stopped.value - new_times = get_file_times.call - changed = false - last_times.each do |path, old_time| - new_time = new_times[path] - if !new_time.nil? && new_time != old_time - changed = true - break + begin + new_times = get_file_times.call + changed = false + last_times.each do |path, old_time| + new_time = new_times[path] + if !new_time.nil? && new_time != old_time + changed = true + break + end end - end - if changed - reloader.call + reloader.call if changed + rescue => exn + Util.log_exception(logger, "Unexpected exception in FileDataSourcePoller", exn) end end end From 38f534fd3b5968a7d6f75cf5f214be768f810f9f Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 2 Nov 2018 12:51:09 -0700 Subject: [PATCH 45/45] don't use Thread.raise to stop PollingProcessor thread; add test for PollingProcessor.stop --- lib/ldclient-rb/polling.rb | 3 +- spec/polling_spec.rb | 81 ++++++++++++++++++++++++++------------ 2 files changed, 58 insertions(+), 26 deletions(-) diff --git a/lib/ldclient-rb/polling.rb b/lib/ldclient-rb/polling.rb index 15965201..4ecd93f8 100644 --- a/lib/ldclient-rb/polling.rb +++ b/lib/ldclient-rb/polling.rb @@ -26,7 +26,8 @@ def start def stop if @stopped.make_true if @worker && @worker.alive? - @worker.raise "shutting down client" + @worker.run # causes the thread to wake up if it's currently in a sleep + @worker.join end @config.logger.info { "[LDClient] Polling connection stopped" } end diff --git a/spec/polling_spec.rb b/spec/polling_spec.rb index 8183b8c3..690147d0 100644 --- a/spec/polling_spec.rb +++ b/spec/polling_spec.rb @@ -3,10 +3,17 @@ describe LaunchDarkly::PollingProcessor do subject { LaunchDarkly::PollingProcessor } - let(:store) { LaunchDarkly::InMemoryFeatureStore.new } - let(:config) { LaunchDarkly::Config.new(feature_store: store) } let(:requestor) { double() } - let(:processor) { subject.new(config, requestor) } + + def with_processor(store) + config = LaunchDarkly::Config.new(feature_store: store) + processor = subject.new(config, requestor) + begin + yield processor + ensure + processor.stop + end + end describe 'successful request' do flag = { key: 'flagkey', version: 1 } @@ -22,47 +29,60 @@ it 'puts feature data in store' do allow(requestor).to receive(:request_all_data).and_return(all_data) - ready = processor.start - ready.wait - expect(store.get(LaunchDarkly::FEATURES, "flagkey")).to eq(flag) - expect(store.get(LaunchDarkly::SEGMENTS, "segkey")).to eq(segment) + store = LaunchDarkly::InMemoryFeatureStore.new + with_processor(store) do |processor| + ready = processor.start + ready.wait + expect(store.get(LaunchDarkly::FEATURES, "flagkey")).to eq(flag) + expect(store.get(LaunchDarkly::SEGMENTS, "segkey")).to eq(segment) + end end it 'sets initialized to true' do allow(requestor).to receive(:request_all_data).and_return(all_data) - ready = processor.start - ready.wait - expect(processor.initialized?).to be true - expect(store.initialized?).to be true + store = LaunchDarkly::InMemoryFeatureStore.new + with_processor(store) do |processor| + ready = processor.start + ready.wait + expect(processor.initialized?).to be true + expect(store.initialized?).to be true + end end end describe 'connection error' do it 'does not cause immediate failure, does not set initialized' do allow(requestor).to receive(:request_all_data).and_raise(StandardError.new("test error")) - ready = processor.start - finished = ready.wait(0.2) - expect(finished).to be false - expect(processor.initialized?).to be false - expect(store.initialized?).to be false + store = LaunchDarkly::InMemoryFeatureStore.new + with_processor(store) do |processor| + ready = processor.start + finished = ready.wait(0.2) + expect(finished).to be false + expect(processor.initialized?).to be false + expect(store.initialized?).to be false + end end end describe 'HTTP errors' do def verify_unrecoverable_http_error(status) allow(requestor).to receive(:request_all_data).and_raise(LaunchDarkly::UnexpectedResponseError.new(status)) - ready = processor.start - finished = ready.wait(0.2) - expect(finished).to be true - expect(processor.initialized?).to be false + with_processor(LaunchDarkly::InMemoryFeatureStore.new) do |processor| + ready = processor.start + finished = ready.wait(0.2) + expect(finished).to be true + expect(processor.initialized?).to be false + end end def verify_recoverable_http_error(status) allow(requestor).to receive(:request_all_data).and_raise(LaunchDarkly::UnexpectedResponseError.new(status)) - ready = processor.start - finished = ready.wait(0.2) - expect(finished).to be false - expect(processor.initialized?).to be false + with_processor(LaunchDarkly::InMemoryFeatureStore.new) do |processor| + ready = processor.start + finished = ready.wait(0.2) + expect(finished).to be false + expect(processor.initialized?).to be false + end end it 'stops immediately for error 401' do @@ -85,5 +105,16 @@ def verify_recoverable_http_error(status) verify_recoverable_http_error(503) end end -end + describe 'stop' do + it 'stops promptly rather than continuing to wait for poll interval' do + with_processor(LaunchDarkly::InMemoryFeatureStore.new) do |processor| + sleep(1) # somewhat arbitrary, but should ensure that it has started polling + start_time = Time.now + processor.stop + end_time = Time.now + expect(end_time - start_time).to be <(LaunchDarkly::Config.default_poll_interval - 5) + end + end + end +end