prepare 5.5.5 release (#132)

Author: eli-darkly

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
 

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

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",

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)

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

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 }

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)