From d90a18d94c312658d95911feb32783224c920052 Mon Sep 17 00:00:00 2001 From: Mauro Antonio Sanz Date: Wed, 17 Apr 2024 15:55:34 -0300 Subject: [PATCH 1/2] implementation --- .../cache/repositories/splits_repository.rb | 43 +++++++++- .../repositories/splits_repository_spec.rb | 82 +++++++++++++++---- spec/repository_helper.rb | 16 ++-- spec/telemetry/synchronizer_spec.rb | 6 +- 4 files changed, 118 insertions(+), 29 deletions(-) diff --git a/lib/splitclient-rb/cache/repositories/splits_repository.rb b/lib/splitclient-rb/cache/repositories/splits_repository.rb index 3f17a0c7..a023c75f 100644 --- a/lib/splitclient-rb/cache/repositories/splits_repository.rb +++ b/lib/splitclient-rb/cache/repositories/splits_repository.rb @@ -5,7 +5,32 @@ module Cache module Repositories class SplitsRepository < Repository attr_reader :adapter - + DEFAULT_CONDITIONS_TEMPLATE = { + conditionType: "ROLLOUT", + matcherGroup: { + combiner: "AND", + matchers: [ + { + keySelector: nil, + matcherType: "ALL_KEYS", + negate: false, + userDefinedSegmentMatcherData: nil, + whitelistMatcherData: nil, + unaryNumericMatcherData: nil, + betweenMatcherData: nil, + dependencyMatcherData: nil, + booleanMatcherData: nil, + stringMatcherData: nil + }] + }, + partitions: [ + { + treatment: "control", + size: 100 + } + ], + label: "unsupported matcher type" + } def initialize(config, flag_sets_repository, flag_set_filter) super(config) @tt_cache = {} @@ -155,6 +180,10 @@ def add_feature_flag(split) remove_from_flag_sets(existing_split) end + if check_undefined_matcher(split) + @config.logger.warn("Feature Flag #{split[:name]} has undefined matcher, setting conditions to default template.") + split[:conditions] = [SplitsRepository::DEFAULT_CONDITIONS_TEMPLATE] + end if !split[:sets].nil? for flag_set in split[:sets] if !@flag_sets.flag_set_exist?(flag_set) @@ -170,6 +199,18 @@ def add_feature_flag(split) @adapter.set_string(namespace_key(".split.#{split[:name]}"), split.to_json) end + def check_undefined_matcher(split) + for condition in split[:conditions] + for matcher in condition[:matcherGroup][:matchers] + if !SplitIoClient::Condition.instance_methods(false).map(&:to_s).include?("matcher_#{matcher[:matcherType].downcase}") + @config.logger.error("Detected undefined matcher #{matcher[:matcherType].downcase} in feature flag #{split[:name]}") + return true + end + end + end + return false + end + def remove_feature_flag(split) decrease_tt_name_count(split[:trafficTypeName]) remove_from_flag_sets(split) diff --git a/spec/cache/repositories/splits_repository_spec.rb b/spec/cache/repositories/splits_repository_spec.rb index 351ed30d..6d8ae3f7 100644 --- a/spec/cache/repositories/splits_repository_spec.rb +++ b/spec/cache/repositories/splits_repository_spec.rb @@ -17,9 +17,9 @@ before do # in memory setup - repository.update([{name: 'foo', trafficTypeName: 'tt_name_1'}, - {name: 'bar', trafficTypeName: 'tt_name_2'}, - {name: 'baz', trafficTypeName: 'tt_name_1'}], [], -1) + repository.update([{name: 'foo', trafficTypeName: 'tt_name_1', conditions: []}, + {name: 'bar', trafficTypeName: 'tt_name_2', conditions: []}, + {name: 'baz', trafficTypeName: 'tt_name_1', conditions: []}], [], -1) # redis setup repository.instance_variable_get(:@adapter).set_string( @@ -31,13 +31,13 @@ end after do - repository.update([], [{name: 'foo', trafficTypeName: 'tt_name_1'}, - {name: 'bar', trafficTypeName: 'tt_name_2'}, - {name: 'bar', trafficTypeName: 'tt_name_2'}, - {name: 'qux', trafficTypeName: 'tt_name_3'}, - {name: 'quux', trafficTypeName: 'tt_name_4'}, - {name: 'corge', trafficTypeName: 'tt_name_5'}, - {name: 'corge', trafficTypeName: 'tt_name_6'}], -1) + repository.update([], [{name: 'foo', trafficTypeName: 'tt_name_1', conditions: []}, + {name: 'bar', trafficTypeName: 'tt_name_2', conditions: []}, + {name: 'bar', trafficTypeName: 'tt_name_2', conditions: []}, + {name: 'qux', trafficTypeName: 'tt_name_3', conditions: []}, + {name: 'quux', trafficTypeName: 'tt_name_4', conditions: []}, + {name: 'corge', trafficTypeName: 'tt_name_5', conditions: []}, + {name: 'corge', trafficTypeName: 'tt_name_6', conditions: []}], -1) end it 'returns splits names' do @@ -52,7 +52,7 @@ expect(repository.traffic_type_exists('tt_name_1')).to be true expect(repository.traffic_type_exists('tt_name_2')).to be true - split = { name: 'qux', trafficTypeName: 'tt_name_3' } + split = { name: 'qux', trafficTypeName: 'tt_name_3', conditions: [] } repository.update([split], [], -1) repository.update([], [split], -1) @@ -61,7 +61,7 @@ end it 'does not increment traffic type count when adding same split twice' do - split = { name: 'quux', trafficTypeName: 'tt_name_4' } + split = { name: 'quux', trafficTypeName: 'tt_name_4', conditions: [] } repository.update([split, split], [], -1) repository.update([], [split], -1) @@ -70,7 +70,7 @@ end it 'updates traffic type count accordingly when split changes traffic type' do - split = { name: 'corge', trafficTypeName: 'tt_name_5' } + split = { name: 'corge', trafficTypeName: 'tt_name_5', conditions: [] } repository.update([split], [], -1) repository.instance_variable_get(:@adapter).set_string( @@ -79,7 +79,7 @@ expect(repository.traffic_type_exists('tt_name_5')).to be true - split = { name: 'corge', trafficTypeName: 'tt_name_6' } + split = { name: 'corge', trafficTypeName: 'tt_name_6', conditions: [] } repository.update([split], [], -1) @@ -97,11 +97,59 @@ it 'returns splits data' do expect(repository.splits).to eq( - 'foo' => { name: 'foo', trafficTypeName: 'tt_name_1' }, - 'bar' => { name: 'bar', trafficTypeName: 'tt_name_2' }, - 'baz' => { name: 'baz', trafficTypeName: 'tt_name_1' } + 'foo' => { name: 'foo', trafficTypeName: 'tt_name_1', conditions: [] }, + 'bar' => { name: 'bar', trafficTypeName: 'tt_name_2', conditions: [] }, + 'baz' => { name: 'baz', trafficTypeName: 'tt_name_1', conditions: [] } ) end + + it 'remove undefined matcher with template condition' do + split = { name: 'corge', trafficTypeName: 'tt_name_5', conditions: [ + { + partitions: [ + {treatment: 'on', size: 50}, + {treatment: 'off', size: 50} + ], + contitionType: 'WHITELIST', + label: 'some_label', + matcherGroup: { + matchers: [ + { + matcherType: 'UNDEFINED', + whitelistMatcherData: { + whitelist: ['k1', 'k2', 'k3'] + }, + negate: false, + } + ], + combiner: 'AND' + } + }] + } + repository.update([split], [], -1) + expect(repository.get_split('corge')[:conditions]).to eq [SplitIoClient::Cache::Repositories::SplitsRepository::DEFAULT_CONDITIONS_TEMPLATE] + + # test with multiple conditions + split[:conditions] .append({ + partitions: [ + {treatment: 'on', size: 25}, + {treatment: 'off', size: 75} + ], + contitionType: 'WHITELIST', + label: 'some_other_label', + matcherGroup: { + matchers: [ + { + matcherType: 'ALL_KEYS', + negate: false, + } + ], + combiner: 'AND' + } + }) + repository.update([split], [], -1) + expect(repository.get_split('corge')[:conditions]).to eq [SplitIoClient::Cache::Repositories::SplitsRepository::DEFAULT_CONDITIONS_TEMPLATE] + end end describe 'with Memory Adapter' do diff --git a/spec/repository_helper.rb b/spec/repository_helper.rb index 994c9ca9..f4a72ab6 100644 --- a/spec/repository_helper.rb +++ b/spec/repository_helper.rb @@ -13,16 +13,16 @@ flag_sets_repository, flag_set_filter) - SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split1', :status => 'ACTIVE', :sets => []}], -1, config) + SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split1', :status => 'ACTIVE', conditions: [], :sets => []}], -1, config) expect(feature_flag_repository.get_split('split1').nil?).to eq(true) - SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split1', :status => 'ACTIVE', :sets => ['set_3']}], -1, config) + SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split1', :status => 'ACTIVE', conditions: [], :sets => ['set_3']}], -1, config) expect(feature_flag_repository.get_split('split1').nil?).to eq(true) - SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split1', :status => 'ACTIVE', :sets => ['set_1']}], -1, config) + SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split1', :status => 'ACTIVE', conditions: [], :sets => ['set_1']}], -1, config) expect(feature_flag_repository.get_split('split1').nil?).to eq(false) - SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split1', :status => 'ARCHIVED', :sets => ['set_1']}], -1, config) + SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split1', :status => 'ARCHIVED', conditions: [], :sets => ['set_1']}], -1, config) expect(feature_flag_repository.get_split('split1').nil?).to eq(true) end @@ -35,16 +35,16 @@ flag_sets_repository, flag_set_filter) - SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split1', :status => 'ACTIVE', :sets => []}], -1, config) + SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split1', :status => 'ACTIVE', conditions: [], :sets => []}], -1, config) expect(feature_flag_repository.get_split('split1').nil?).to eq(false) - SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split2', :status => 'ACTIVE', :sets => ['set_3']}], -1, config) + SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split2', :status => 'ACTIVE', conditions: [], :sets => ['set_3']}], -1, config) expect(feature_flag_repository.get_split('split2').nil?).to eq(false) - SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split3', :status => 'ACTIVE', :sets => ['set_1']}], -1, config) + SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split3', :status => 'ACTIVE', conditions: [], :sets => ['set_1']}], -1, config) expect(feature_flag_repository.get_split('split1').nil?).to eq(false) - SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split1', :status => 'ARCHIVED', :sets => ['set_1']}], -1, config) + SplitIoClient::Helpers::RepositoryHelper.update_feature_flag_repository(feature_flag_repository, [{:name => 'split1', :status => 'ARCHIVED', conditions: [], :sets => ['set_1']}], -1, config) expect(feature_flag_repository.get_split('split1').nil?).to eq(true) end end diff --git a/spec/telemetry/synchronizer_spec.rb b/spec/telemetry/synchronizer_spec.rb index 18e67594..5448027c 100644 --- a/spec/telemetry/synchronizer_spec.rb +++ b/spec/telemetry/synchronizer_spec.rb @@ -65,9 +65,9 @@ end it 'with data' do - splits_repository.update([{name: 'foo', trafficTypeName: 'tt_name_1'}, - {name: 'bar', trafficTypeName: 'tt_name_2'}, - {name: 'baz', trafficTypeName: 'tt_name_1'}], [], -1) + splits_repository.update([{name: 'foo', trafficTypeName: 'tt_name_1', conditions: []}, + {name: 'bar', trafficTypeName: 'tt_name_2', conditions: []}, + {name: 'baz', trafficTypeName: 'tt_name_1', conditions: []}], [], -1) segments_repository.add_to_segment(name: 'foo-1', added: [1, 2, 3], removed: []) segments_repository.add_to_segment(name: 'foo-2', added: [1, 2, 3, 4], removed: []) From 774ce4c5e0d97423742fc771e16439bab30194ae Mon Sep 17 00:00:00 2001 From: Mauro Antonio Sanz Date: Thu, 18 Apr 2024 10:22:50 -0300 Subject: [PATCH 2/2] fixed spec --- .../cache/repositories/splits_repository.rb | 6 +- .../repositories/splits_repository_spec.rb | 57 +++++++++++++------ 2 files changed, 42 insertions(+), 21 deletions(-) diff --git a/lib/splitclient-rb/cache/repositories/splits_repository.rb b/lib/splitclient-rb/cache/repositories/splits_repository.rb index a023c75f..eab01183 100644 --- a/lib/splitclient-rb/cache/repositories/splits_repository.rb +++ b/lib/splitclient-rb/cache/repositories/splits_repository.rb @@ -5,7 +5,7 @@ module Cache module Repositories class SplitsRepository < Repository attr_reader :adapter - DEFAULT_CONDITIONS_TEMPLATE = { + DEFAULT_CONDITIONS_TEMPLATE = [{ conditionType: "ROLLOUT", matcherGroup: { combiner: "AND", @@ -30,7 +30,7 @@ class SplitsRepository < Repository } ], label: "unsupported matcher type" - } + }] def initialize(config, flag_sets_repository, flag_set_filter) super(config) @tt_cache = {} @@ -182,7 +182,7 @@ def add_feature_flag(split) if check_undefined_matcher(split) @config.logger.warn("Feature Flag #{split[:name]} has undefined matcher, setting conditions to default template.") - split[:conditions] = [SplitsRepository::DEFAULT_CONDITIONS_TEMPLATE] + split[:conditions] = SplitsRepository::DEFAULT_CONDITIONS_TEMPLATE end if !split[:sets].nil? for flag_set in split[:sets] diff --git a/spec/cache/repositories/splits_repository_spec.rb b/spec/cache/repositories/splits_repository_spec.rb index 6d8ae3f7..34c720e8 100644 --- a/spec/cache/repositories/splits_repository_spec.rb +++ b/spec/cache/repositories/splits_repository_spec.rb @@ -127,28 +127,49 @@ }] } repository.update([split], [], -1) - expect(repository.get_split('corge')[:conditions]).to eq [SplitIoClient::Cache::Repositories::SplitsRepository::DEFAULT_CONDITIONS_TEMPLATE] + expect(repository.get_split('corge')[:conditions]).to eq SplitIoClient::Cache::Repositories::SplitsRepository::DEFAULT_CONDITIONS_TEMPLATE # test with multiple conditions - split[:conditions] .append({ - partitions: [ - {treatment: 'on', size: 25}, - {treatment: 'off', size: 75} - ], - contitionType: 'WHITELIST', - label: 'some_other_label', - matcherGroup: { - matchers: [ - { - matcherType: 'ALL_KEYS', - negate: false, - } + split2 = { + name: 'corge2', + trafficTypeName: 'tt_name_5', + conditions: [ + { + partitions: [ + {treatment: 'on', size: 50}, + {treatment: 'off', size: 50} + ], + contitionType: 'WHITELIST', + label: 'some_label', + matcherGroup: { + matchers: [ + { + matcherType: 'UNDEFINED', + whitelistMatcherData: { + whitelist: ['k1', 'k2', 'k3'] + }, + negate: false, + } + ], + combiner: 'AND' + } + }, + { + partitions: [ + {treatment: 'on', size: 25}, + {treatment: 'off', size: 75} ], - combiner: 'AND' + contitionType: 'WHITELIST', + label: 'some_other_label', + matcherGroup: { + matchers: [{matcherType: 'ALL_KEYS', negate: false}], + combiner: 'AND' + } + }] } - }) - repository.update([split], [], -1) - expect(repository.get_split('corge')[:conditions]).to eq [SplitIoClient::Cache::Repositories::SplitsRepository::DEFAULT_CONDITIONS_TEMPLATE] + + repository.update([split2], [], -1) + expect(repository.get_split('corge2')[:conditions]).to eq SplitIoClient::Cache::Repositories::SplitsRepository::DEFAULT_CONDITIONS_TEMPLATE end end