diff --git a/lib/experiment/evaluation/evaluation.rb b/lib/experiment/evaluation/evaluation.rb index a1c01cd..a06decb 100644 --- a/lib/experiment/evaluation/evaluation.rb +++ b/lib/experiment/evaluation/evaluation.rb @@ -66,24 +66,25 @@ def evaluate_conditions(target, conditions) def match_condition(target, condition) prop_value = Evaluation.select(target, condition.selector) - # Special matching for null properties and set type prop values and operators - if !prop_value - match_null(condition.op, condition.values) - elsif set_operator?(condition.op) - prop_value_string_list = coerce_string_array(prop_value) + return match_null(condition.op, condition.values) unless prop_value + + prop_value_string_list = coerce_string_array(prop_value) + if set_operator?(condition.op) return false unless prop_value_string_list match_set(prop_value_string_list, condition.op, condition.values) + elsif prop_value_string_list + match_strings_non_set(prop_value_string_list, condition.op, condition.values) else prop_value_string = coerce_string(prop_value) - if prop_value_string - match_string(prop_value_string, condition.op, condition.values) - else - false - end + prop_value_string ? match_string(prop_value_string, condition.op, condition.values) : false end end + def match_strings_non_set(prop_values, op, filter_values) + prop_values.any? { |v| match_string(v, op, filter_values) } + end + def get_hash(key) Murmur3.hash32x86(key) end @@ -263,23 +264,19 @@ def coerce_string(value) end def coerce_string_array(value) - if value.is_a?(Array) - value.map { |e| coerce_string(e) }.compact - else - string_value = value.to_s - begin - parsed_value = JSON.parse(string_value) - if parsed_value.is_a?(Array) - parsed_value.map { |e| coerce_string(e) }.compact - else - s = coerce_string(string_value) - s ? [s] : nil - end - rescue JSON::ParserError - s = coerce_string(string_value) - s ? [s] : nil - end + return value.map { |e| coerce_string(e) }.compact if value.is_a?(Array) + + string_value = value.to_s + return nil unless string_value.start_with?('[') + + begin + parsed_value = JSON.parse(string_value) + rescue JSON::ParserError + return nil end + return nil unless parsed_value.is_a?(Array) + + parsed_value.map { |e| coerce_string(e) }.compact end def set_operator?(op) diff --git a/spec/experiment/evaluation/evaluation_intgration_spec.rb b/spec/experiment/evaluation/evaluation_intgration_spec.rb index d4e68f9..c39db22 100644 --- a/spec/experiment/evaluation/evaluation_intgration_spec.rb +++ b/spec/experiment/evaluation/evaluation_intgration_spec.rb @@ -4,8 +4,12 @@ require 'json' module AmplitudeExperiment + # Cached once to work around a JRuby 10.x issue where repeated URI() parsing + # under sustained string allocation spuriously raises "URI must be ascii only". + FLAGS_URI = URI('https://api.lab.amplitude.com/sdk/v2/flags?eval_mode=remote').freeze + describe Evaluation::Engine do - let(:deployment_key) { 'server-NgJxxvg8OGwwBsWVXqyxQbdiflbhvugy' } + let(:deployment_key) { 'server-VVhLULXCxxY0xqmszXouXxiEzoeJWmSh' } let(:engine) { Evaluation::Engine.new } let(:flags) { get_flags(deployment_key) } @@ -421,6 +425,48 @@ module AmplitudeExperiment expect(result.key).to eq('on') end + it 'tests set is with json array string' do + user = user_context(nil, nil, nil, { 'key' => '["1", "2", "3"]' }) + result = engine.evaluate(user, flags)['test-set-is'] + expect(result.key).to eq('on') + end + + it 'tests is with array' do + user = user_context(nil, nil, nil, { 'key' => %w[value1 value2] }) + result = engine.evaluate(user, flags)['test-is-array'] + expect(result.key).to eq('on') + end + + it 'tests is with json array string' do + user = user_context(nil, nil, nil, { 'key' => '["value1", "value2"]' }) + result = engine.evaluate(user, flags)['test-is-array'] + expect(result.key).to eq('on') + end + + it 'tests is not with array' do + user = user_context(nil, nil, nil, { 'key' => %w[value3 value4] }) + result = engine.evaluate(user, flags)['test-is-not-array'] + expect(result.key).to eq('on') + end + + it 'tests contains with array' do + user = user_context(nil, nil, nil, { 'key' => %w[has-target-value has value] }) + result = engine.evaluate(user, flags)['test-contains-array'] + expect(result.key).to eq('on') + end + + it 'tests does not contain with array' do + user = user_context(nil, nil, nil, { 'key' => %w[has-value has value] }) + result = engine.evaluate(user, flags)['test-does-not-contain-array'] + expect(result.key).to eq('on') + end + + it 'tests does not contain with json array string' do + user = user_context(nil, nil, nil, { 'key' => '["has-value", "has", "value"]' }) + result = engine.evaluate(user, flags)['test-does-not-contain-array'] + expect(result.key).to eq('on') + end + it 'tests is with booleans' do # Test with uppercase TRUE/FALSE user = user_context(nil, nil, nil, { @@ -479,13 +525,10 @@ def group_context(group_type, group_name, group_properties = nil) end def get_flags(deployment_key) - server_url = 'https://api.lab.amplitude.com' - uri = URI("#{server_url}/sdk/v2/flags?eval_mode=remote") - - request = Net::HTTP::Get.new(uri) + request = Net::HTTP::Get.new(FLAGS_URI) request['Authorization'] = "Api-Key #{deployment_key}" - response = Net::HTTP.start(uri.hostname, uri.port, use_ssl: true) do |http| + response = Net::HTTP.start(FLAGS_URI.hostname, FLAGS_URI.port, use_ssl: true) do |http| http.request(request) end diff --git a/spec/experiment/evaluation/evaluation_spec.rb b/spec/experiment/evaluation/evaluation_spec.rb new file mode 100644 index 0000000..e6edcaf --- /dev/null +++ b/spec/experiment/evaluation/evaluation_spec.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +module AmplitudeExperiment + describe Evaluation::Engine do + let(:engine) { Evaluation::Engine.new } + + def flag_with_condition(op, values) + Evaluation::Flag.from_hash( + 'key' => 'test-flag', + 'variants' => { + 'on' => { 'key' => 'on', 'value' => 'on' } + }, + 'segments' => [ + { + 'conditions' => [[ + { + 'selector' => %w[context user user_properties test_prop], + 'op' => op, + 'values' => values + } + ]], + 'variant' => 'on' + } + ] + ) + end + + def context_with_prop(value) + { + 'user' => { + 'user_properties' => { 'test_prop' => value } + } + } + end + + def evaluate(prop_value, op, values) + flag = flag_with_condition(op, values) + context = context_with_prop(prop_value) + engine.evaluate(context, [flag])['test-flag'] + end + + def assert_match(prop_value, op, values) + result = evaluate(prop_value, op, values) + expect(result).not_to be_nil + expect(result.key).to eq('on') + end + + def assert_no_match(prop_value, op, values) + result = evaluate(prop_value, op, values) + expect(result).to be_nil + end + + describe 'non-set operator array matching' do + it 'matches scalar string IS' do + assert_match('hello', Evaluation::Operator::IS, ['hello']) + end + + it 'matches scalar string CONTAINS' do + assert_match('hello', Evaluation::Operator::CONTAINS, ['ell']) + end + + it 'matches scalar string GREATER_THAN' do + assert_match('2', Evaluation::Operator::GREATER_THAN, ['1']) + end + + it 'does not match scalar string IS when value differs' do + assert_no_match('world', Evaluation::Operator::IS, ['hello']) + end + + it 'matches non-string scalar GREATER_THAN' do + assert_match(42, Evaluation::Operator::GREATER_THAN, ['1']) + end + + it 'matches non-string scalar IS (boolean)' do + assert_match(true, Evaluation::Operator::IS, ['true']) + end + + it 'matches JSON array string with set operator' do + assert_match('["a","b"]', Evaluation::Operator::SET_CONTAINS, ['a']) + end + + it 'matches JSON array string with non-set operator' do + assert_match('["a","b"]', Evaluation::Operator::IS, ['a']) + end + + it 'matches collection with set operator' do + assert_match(%w[a b], Evaluation::Operator::SET_CONTAINS, ['a']) + end + + it 'matches collection with non-set operator' do + assert_match(%w[a b], Evaluation::Operator::IS, ['a']) + end + + it 'falls through for malformed JSON array and matches as scalar' do + assert_match('[broken', Evaluation::Operator::IS, ['[broken']) + end + + it 'does not match empty JSON array for set operator' do + assert_no_match('[]', Evaluation::Operator::SET_CONTAINS, ['a']) + end + + it 'treats leading-whitespace string as scalar for non-set operator' do + assert_match(' ["a"]', Evaluation::Operator::IS, [' ["a"]']) + end + + it 'treats leading-whitespace string as scalar for set operator (no match)' do + assert_no_match(' ["a"]', Evaluation::Operator::SET_CONTAINS, ['a']) + end + end + end +end