diff --git a/lib/ldclient-rb/evaluation.rb b/lib/ldclient-rb/evaluation.rb index 1b5bbdca..7edef6b2 100644 --- a/lib/ldclient-rb/evaluation.rb +++ b/lib/ldclient-rb/evaluation.rb @@ -189,6 +189,10 @@ def self.comparator(converter) # Used internally to hold an evaluation result and the events that were generated from prerequisites. EvalResult = Struct.new(:detail, :events) + USER_ATTRS_TO_STRINGIFY_FOR_EVALUATION = [ :key, :secondary ] + # Currently we are not stringifying the rest of the built-in attributes prior to evaluation, only for events. + # This is because it could affect evaluation results for existing users (ch35206). + def error_result(errorKind, value = nil) EvaluationDetail.new(value, nil, { kind: 'ERROR', errorKind: errorKind }) end @@ -200,8 +204,10 @@ def evaluate(flag, user, store, logger) return EvalResult.new(error_result('USER_NOT_SPECIFIED'), []) end + sanitized_user = Util.stringify_attrs(user, USER_ATTRS_TO_STRINGIFY_FOR_EVALUATION) + events = [] - detail = eval_internal(flag, user, store, events, logger) + detail = eval_internal(flag, sanitized_user, store, events, logger) return EvalResult.new(detail, events) end diff --git a/lib/ldclient-rb/events.rb b/lib/ldclient-rb/events.rb index c45a9da2..69563572 100644 --- a/lib/ldclient-rb/events.rb +++ b/lib/ldclient-rb/events.rb @@ -7,9 +7,12 @@ module LaunchDarkly MAX_FLUSH_WORKERS = 5 CURRENT_SCHEMA_VERSION = 3 + USER_ATTRS_TO_STRINGIFY_FOR_EVENTS = [ :key, :secondary, :ip, :country, :email, :firstName, :lastName, + :avatar, :name ] private_constant :MAX_FLUSH_WORKERS private_constant :CURRENT_SCHEMA_VERSION + private_constant :USER_ATTRS_TO_STRINGIFY_FOR_EVENTS # @private class NullEventProcessor @@ -219,7 +222,7 @@ def notice_user(user) if user.nil? || !user.has_key?(:key) true else - @user_keys.add(user[:key]) + @user_keys.add(user[:key].to_s) end end @@ -371,6 +374,11 @@ def make_output_events(events, summary) private + def process_user(event) + filtered = @user_filter.transform_user_props(event[:user]) + Util.stringify_attrs(filtered, USER_ATTRS_TO_STRINGIFY_FOR_EVENTS) + end + def make_output_event(event) case event[:kind] when "feature" @@ -386,7 +394,7 @@ def make_output_event(event) out[:version] = event[:version] if event.has_key?(:version) out[:prereqOf] = event[:prereqOf] if event.has_key?(:prereqOf) if @inline_users || is_debug - out[:user] = @user_filter.transform_user_props(event[:user]) + out[:user] = process_user(event) else out[:userKey] = event[:user].nil? ? nil : event[:user][:key] end @@ -396,8 +404,8 @@ def make_output_event(event) { kind: "identify", creationDate: event[:creationDate], - key: event[:user].nil? ? nil : event[:user][:key], - user: @user_filter.transform_user_props(event[:user]) + key: event[:user].nil? ? nil : event[:user][:key].to_s, + user: process_user(event) } when "custom" out = { @@ -407,7 +415,7 @@ def make_output_event(event) } out[:data] = event[:data] if event.has_key?(:data) if @inline_users - out[:user] = @user_filter.transform_user_props(event[:user]) + out[:user] = process_user(event) else out[:userKey] = event[:user].nil? ? nil : event[:user][:key] end @@ -416,7 +424,7 @@ def make_output_event(event) { kind: "index", creationDate: event[:creationDate], - user: @user_filter.transform_user_props(event[:user]) + user: process_user(event) } else event diff --git a/lib/ldclient-rb/ldclient.rb b/lib/ldclient-rb/ldclient.rb index 28c21869..3680619a 100644 --- a/lib/ldclient-rb/ldclient.rb +++ b/lib/ldclient-rb/ldclient.rb @@ -215,7 +215,6 @@ def identify(user) @config.logger.warn("Identify called with nil user or nil user key!") return end - sanitize_user(user) @event_processor.add_event(kind: "identify", key: user[:key], user: user) end @@ -237,7 +236,6 @@ def track(event_name, user, data) @config.logger.warn("Track called with nil user or nil user key!") return end - sanitize_user(user) @event_processor.add_event(kind: "custom", key: event_name, user: user, data: data) end @@ -280,8 +278,6 @@ def all_flags_state(user, options={}) return FeatureFlagsState.new(false) end - sanitize_user(user) - begin features = @store.all(FEATURES) rescue => exn @@ -353,7 +349,6 @@ def evaluate_internal(key, user, default, include_reasons_in_events) end end - sanitize_user(user) if !user.nil? feature = @store.get(FEATURES, key) if feature.nil? @@ -367,12 +362,12 @@ def evaluate_internal(key, user, default, include_reasons_in_events) 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)) + @event_processor.add_event(make_feature_event(feature, nil, detail, default, include_reasons_in_events)) return detail end begin - res = evaluate(feature, user, @store, @config.logger) + res = evaluate(feature, user, @store, @config.logger) # note, evaluate will do its own sanitization if !res.events.nil? res.events.each do |event| @event_processor.add_event(event) @@ -392,12 +387,6 @@ def evaluate_internal(key, user, default, include_reasons_in_events) end end - def sanitize_user(user) - if user[:key] - user[:key] = user[:key].to_s - end - end - def make_feature_event(flag, user, detail, default, with_reasons) { kind: "feature", diff --git a/lib/ldclient-rb/util.rb b/lib/ldclient-rb/util.rb index 396a5171..e129c279 100644 --- a/lib/ldclient-rb/util.rb +++ b/lib/ldclient-rb/util.rb @@ -4,6 +4,21 @@ module LaunchDarkly # @private module Util + def self.stringify_attrs(hash, attrs) + return hash if hash.nil? + ret = hash + changed = false + attrs.each do |attr| + value = hash[attr] + if !value.nil? && !value.is_a?(String) + ret = hash.clone if !changed + ret[attr] = value.to_s + changed = true + end + end + ret + end + def self.new_http_client(uri_s, config) uri = URI(uri_s) client = Net::HTTP.new(uri.hostname, uri.port) diff --git a/spec/evaluation_spec.rb b/spec/evaluation_spec.rb index 68824ebd..52a617b6 100644 --- a/spec/evaluation_spec.rb +++ b/spec/evaluation_spec.rb @@ -359,6 +359,25 @@ def boolean_flag_with_clauses(clauses) expect(result.detail).to eq(detail) expect(result.events).to eq([]) end + + it "coerces user key to a string for evaluation" do + clause = { attribute: 'key', op: 'in', values: ['999'] } + flag = boolean_flag_with_clauses([clause]) + user = { key: 999 } + result = evaluate(flag, user, features, logger) + expect(result.detail.value).to eq(true) + end + + it "coerces secondary key to a string for evaluation" do + # We can't really verify that the rollout calculation works correctly, but we can at least + # make sure it doesn't error out if there's a non-string secondary value (ch35189) + rule = { id: 'ruleid', clauses: [{ attribute: 'key', op: 'in', values: ['userkey'] }], + rollout: { salt: '', variations: [ { weight: 100000, variation: 1 } ] } } + flag = boolean_flag_with_rules([rule]) + user = { key: "userkey", secondary: 999 } + result = evaluate(flag, user, features, logger) + expect(result.detail.reason).to eq({ kind: 'RULE_MATCH', ruleIndex: 0, ruleId: 'ruleid'}) + end end describe "clause" do diff --git a/spec/events_spec.rb b/spec/events_spec.rb index 90b91ec9..557c3594 100644 --- a/spec/events_spec.rb +++ b/spec/events_spec.rb @@ -9,6 +9,10 @@ let(:hc) { FakeHttpClient.new } let(:user) { { key: "userkey", name: "Red" } } let(:filtered_user) { { key: "userkey", privateAttrs: [ "name" ] } } + let(:numeric_user) { { key: 1, secondary: 2, ip: 3, country: 4, email: 5, firstName: 6, lastName: 7, + avatar: 8, name: 9, anonymous: false, custom: { age: 99 } } } + let(:stringified_numeric_user) { { key: '1', secondary: '2', ip: '3', country: '4', email: '5', firstName: '6', + lastName: '7', avatar: '8', name: '9', anonymous: false, custom: { age: 99 } } } after(:each) do if !@ep.nil? @@ -40,6 +44,21 @@ }) end + it "stringifies built-in user attributes in identify event" do + @ep = subject.new("sdk_key", default_config, hc) + flag = { key: "flagkey", version: 11 } + e = { kind: "identify", key: numeric_user[:key], user: numeric_user } + @ep.add_event(e) + + output = flush_and_get_events + expect(output).to contain_exactly( + kind: "identify", + key: numeric_user[:key].to_s, + creationDate: e[:creationDate], + user: stringified_numeric_user + ) + end + it "queues individual feature event with index event" do @ep = subject.new("sdk_key", default_config, hc) flag = { key: "flagkey", version: 11 } @@ -75,6 +94,23 @@ ) end + it "stringifies built-in user attributes in index event" do + @ep = subject.new("sdk_key", default_config, hc) + flag = { key: "flagkey", version: 11 } + fe = { + kind: "feature", key: "flagkey", version: 11, user: numeric_user, + variation: 1, value: "value", trackEvents: true + } + @ep.add_event(fe) + + output = flush_and_get_events + expect(output).to contain_exactly( + eq(index_event(fe, stringified_numeric_user)), + eq(feature_event(fe, flag, false, nil)), + include(:kind => "summary") + ) + end + it "can include inline user in feature event" do config = LaunchDarkly::Config.new(inline_users_in_events: true) @ep = subject.new("sdk_key", config, hc) @@ -92,6 +128,23 @@ ) end + it "stringifies built-in user attributes in feature event" do + config = LaunchDarkly::Config.new(inline_users_in_events: true) + @ep = subject.new("sdk_key", config, hc) + flag = { key: "flagkey", version: 11 } + fe = { + kind: "feature", key: "flagkey", version: 11, user: numeric_user, + variation: 1, value: "value", trackEvents: true + } + @ep.add_event(fe) + + output = flush_and_get_events + expect(output).to contain_exactly( + eq(feature_event(fe, flag, false, stringified_numeric_user)), + include(:kind => "summary") + ) + end + it "filters user in feature event" do config = LaunchDarkly::Config.new(all_attributes_private: true, inline_users_in_events: true) @ep = subject.new("sdk_key", config, hc) @@ -323,6 +376,18 @@ ) end + it "stringifies built-in user attributes in custom event" do + config = LaunchDarkly::Config.new(inline_users_in_events: true) + @ep = subject.new("sdk_key", config, hc) + e = { kind: "custom", key: "eventkey", user: numeric_user } + @ep.add_event(e) + + output = flush_and_get_events + expect(output).to contain_exactly( + eq(custom_event(e, stringified_numeric_user)) + ) + end + it "does a final flush when shutting down" do @ep = subject.new("sdk_key", default_config, hc) e = { kind: "identify", key: user[:key], user: user } diff --git a/spec/ldclient_spec.rb b/spec/ldclient_spec.rb index 6f530610..86cb5be5 100644 --- a/spec/ldclient_spec.rb +++ b/spec/ldclient_spec.rb @@ -25,22 +25,6 @@ } } end - let(:numeric_key_user) do - { - key: 33, - custom: { - groups: [ "microsoft", "google" ] - } - } - end - let(:sanitized_numeric_key_user) do - { - key: "33", - custom: { - groups: [ "microsoft", "google" ] - } - } - end let(:user_without_key) do { name: "Keyless Joe" } end @@ -354,11 +338,6 @@ def event_processor client.track("custom_event_name", user, 42) end - it "sanitizes the user in the event" do - expect(event_processor).to receive(:add_event).with(hash_including(user: sanitized_numeric_key_user)) - client.track("custom_event_name", numeric_key_user, nil) - end - it "does not send an event, and logs a warning, if user is nil" do expect(event_processor).not_to receive(:add_event) expect(logger).to receive(:warn) @@ -378,11 +357,6 @@ def event_processor client.identify(user) end - it "sanitizes the user in the event" do - expect(event_processor).to receive(:add_event).with(hash_including(user: sanitized_numeric_key_user)) - client.identify(numeric_key_user) - end - it "does not send an event, and logs a warning, if user is nil" do expect(event_processor).not_to receive(:add_event) expect(logger).to receive(:warn)