Remove support for secondary attribute (#213)

As decided in the [spec], we are removing the special behavior of the
secondary attribute. Going forward, secondary will be treated like any
other attribute, and will no longer be included when determining the
bucket for a context.

[spec]:
https://launchdarkly.atlassian.net/wiki/spaces/ENG/pages/2165212563/Consistent+and+Transparent+Rollout+Behavior+Unifying+Percent+Rollout+and+Traffic+Allocation

Author: keelerm84

Makefile

@@ -7,7 +7,6 @@ TEMP_TEST_OUTPUT=/tmp/contract-test-service.log
 # - "events": These test suites will be unavailable until more of the U2C implementation is done.
 TEST_HARNESS_PARAMS := $(TEST_HARNESS_PARAMS) \
 	-skip 'evaluation/bucketing/bucket by non-key attribute/in rollouts/string value/complex attribute reference' \
-	-skip 'evaluation/bucketing/secondary' \
 	-skip 'evaluation/parameterized/attribute references' \
 	-skip 'evaluation/parameterized/bad attribute reference errors' \
 	-skip 'evaluation/parameterized/prerequisites' \

lib/ldclient-rb/context.rb

@@ -36,18 +36,16 @@ class LDContext
     # @param kind [String, nil]
     # @param name [String, nil]
     # @param anonymous [Boolean, nil]
-    # @param secondary [String, nil]
     # @param attributes [Hash, nil]
     # @param private_attributes [Array<String>, nil]
     # @param error [String, nil]
     # @param contexts [Array<LDContext>, nil]
     #
-    def initialize(key, kind, name = nil, anonymous = nil, secondary = nil, attributes = nil, private_attributes = nil, error = nil, contexts = nil)
+    def initialize(key, kind, name = nil, anonymous = nil, attributes = nil, private_attributes = nil, error = nil, contexts = nil)
       @key = key
       @kind = kind
       @name = name
       @anonymous = anonymous || false
-      @secondary = secondary
       @attributes = attributes
       @private_attributes = private_attributes
       @error = error
@@ -235,8 +233,6 @@ def individual_context(kind)
         @name
       when :anonymous
         @anonymous
-      when :secondary
-        @secondary
       else
         @attributes&.fetch(name, nil)
       end
@@ -322,15 +318,15 @@ def self.create_multi(contexts)
 
       return contexts[0] if contexts.length == 1
 
-      new(nil, "multi", nil, false, nil, nil, nil, nil, contexts)
+      new(nil, "multi", nil, false, nil, nil, nil, contexts)
     end
 
     #
     # @param error [String]
     # @return [LDContext]
     #
     private_class_method def self.create_invalid_context(error)
-      new(nil, nil, nil, false, nil, nil, nil, error)
+      new(nil, nil, nil, false, nil, nil, error)
     end
 
     #
@@ -376,7 +372,7 @@ def self.create_multi(contexts)
         return create_invalid_context("The provided private attributes are not an array")
       end
 
-      new(key.to_s, KIND_DEFAULT, name, anonymous, data[:secondary], attributes, private_attributes)
+      new(key.to_s, KIND_DEFAULT, name, anonymous, attributes, private_attributes)
     end
 
     #
@@ -416,21 +412,18 @@ def self.create_multi(contexts)
 
       # We only need to create an attribute hash if there are keys set outside
       # of the ones we store in dedicated instance variables.
-      #
-      # :secondary is not a supported top level key in the new schema.
-      # However, someone could still include it so we need to ignore it.
       attributes = nil
       data.each do |k, v|
         case k
-        when :kind, :key, :name, :anonymous, :secondary, :_meta
+        when :kind, :key, :name, :anonymous, :_meta
           next
         else
           attributes ||= {}
           attributes[k] = v.clone
         end
       end
 
-      new(key.to_s, kind, name, anonymous, meta[:secondary], attributes, private_attributes)
+      new(key.to_s, kind, name, anonymous, attributes, private_attributes)
     end
   end
 end

lib/ldclient-rb/events.rb

@@ -62,7 +62,7 @@ def stop
   end
 
   MAX_FLUSH_WORKERS = 5
-  USER_ATTRS_TO_STRINGIFY_FOR_EVENTS = [ :key, :secondary, :ip, :country, :email, :firstName, :lastName,
+  USER_ATTRS_TO_STRINGIFY_FOR_EVENTS = [ :key, :ip, :country, :email, :firstName, :lastName,
     :avatar, :name ]
 
   private_constant :MAX_FLUSH_WORKERS

lib/ldclient-rb/impl/evaluator_bucketing.rb

@@ -60,10 +60,6 @@ def self.bucket_context(context, context_kind, key, bucket_by, salt, seed)
         id_hash = bucketable_string_value(context_value)
         return 0.0 if id_hash.nil?
 
-        if matched_context.get_value(:secondary)
-          id_hash += "." + matched_context.get_value(:secondary).to_s
-        end
-
         if seed
           hash_key = "%d.%s" % [seed, id_hash]
         else

spec/context_spec.rb

@@ -67,16 +67,16 @@
       it "overwrite custom properties with built-ins when collisons occur" do
         context = {
           key: "user-key",
-          secondary: "secondary",
+          ip: "192.168.1.1",
           avatar: "avatar",
           custom: {
-            secondary: "custom secondary",
+            ip: "127.0.0.1",
             avatar: "custom avatar",
           },
         }
 
         result = subject.create(context)
-        expect(result.get_value(:secondary)).to eq("secondary")
+        expect(result.get_value(:ip)).to eq("192.168.1.1")
         expect(result.get_value(:avatar)).to eq("avatar")
       end
     end
@@ -136,20 +136,6 @@
         }
         expect(subject.create(context).valid?).to be false
       end
-
-      it "overwrite secondary property if also specified at top level" do
-        context = {
-          key: "user-key",
-          kind: "user",
-          secondary: "invalid secondary",
-          _meta: {
-            secondary: "real secondary",
-          },
-        }
-
-        result = subject.create(context)
-        expect(result.get_value(:secondary)).to eq("real secondary")
-      end
     end
 
     describe "multi-kind contexts" do

spec/events_spec.rb

@@ -13,9 +13,9 @@
   let(:default_config) { LaunchDarkly::Config.new(default_config_opts) }
   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,
+  let(:numeric_user) { { key: 1, 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',
+  let(:stringified_numeric_user) { { key: '1', ip: '3', country: '4', email: '5', firstName: '6',
     lastName: '7', avatar: '8', name: '9', anonymous: false, custom: { age: 99 } } }
 
   def with_processor_and_sender(config)

spec/impl/evaluator_rule_spec.rb

@@ -81,17 +81,6 @@ module Impl
           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 = factory.boolean_flag_with_rules([rule])
-          context = LDContext.create({ key: "userkey", secondary: 999 })
-          result = basic_evaluator.evaluate(flag, context)
-          expect(result.detail.reason).to eq(EvaluationReason::rule_match(0, 'ruleid'))
-        end
-
         describe "rule experiment/rollout behavior" do
           it "evaluates rollout for rule" do
             rule = { id: 'ruleid', clauses: [{ attribute: 'key', op: 'in', values: ['userkey'] }],
@@ -122,7 +111,7 @@ module Impl
             rule = { id: 'ruleid', clauses: [{ attribute: 'key', op: 'in', values: ['userkey'] }],
               rollout: { kind: 'experiment', variations: [ { weight: 100000, variation: 1, untracked: false } ] } }
             flag = factory.boolean_flag_with_rules([rule])
-            context = LDContext.create({ key: "userkey", secondary: 999 })
+            context = LDContext.create({ key: "userkey" })
             result = basic_evaluator.evaluate(flag, context)
             expect(result.detail.reason.to_json).to include('"inExperiment":true')
             expect(result.detail.reason.in_experiment).to eq(true)
@@ -132,7 +121,7 @@ module Impl
             rule = { id: 'ruleid', clauses: [{ attribute: 'key', op: 'in', values: ['userkey'] }],
               rollout: { kind: 'rollout', variations: [ { weight: 100000, variation: 1, untracked: false } ] } }
             flag = factory.boolean_flag_with_rules([rule])
-            context = LDContext.create({ key: "userkey", secondary: 999 })
+            context = LDContext.create({ key: "userkey" })
             result = basic_evaluator.evaluate(flag, context)
             expect(result.detail.reason.to_json).to_not include('"inExperiment":true')
             expect(result.detail.reason.in_experiment).to eq(nil)
@@ -142,7 +131,7 @@ module Impl
             rule = { id: 'ruleid', clauses: [{ attribute: 'key', op: 'in', values: ['userkey'] }],
               rollout: { kind: 'experiment', variations: [ { weight: 100000, variation: 1, untracked: true } ] } }
             flag = factory.boolean_flag_with_rules([rule])
-            context = LDContext.create({ key: "userkey", secondary: 999 })
+            context = LDContext.create({ key: "userkey" })
             result = basic_evaluator.evaluate(flag, context)
             expect(result.detail.reason.to_json).to_not include('"inExperiment":true')
             expect(result.detail.reason.in_experiment).to eq(nil)