From cb3d664128c31625f28fa2cff6ca63a06cb196f6 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Tue, 5 Dec 2023 09:47:35 -0800 Subject: [PATCH 1/6] firt run to add guard againsts duplicate key --- optimizely/config_manager.py | 2 +- optimizely/optimizely.py | 2 +- optimizely/optimizely_config.py | 14 +++++- tests/test_optimizely_config.py | 79 +++++++++++++++++++++++++++++++-- 4 files changed, 90 insertions(+), 7 deletions(-) diff --git a/optimizely/config_manager.py b/optimizely/config_manager.py index 0e4008b7..032189e9 100644 --- a/optimizely/config_manager.py +++ b/optimizely/config_manager.py @@ -159,7 +159,7 @@ def _set_config(self, datafile: Optional[str | bytes]) -> None: self._config = config self._sdk_key = self._sdk_key or config.sdk_key - self.optimizely_config = OptimizelyConfigService(config).get_config() + self.optimizely_config = OptimizelyConfigService(config, self.logger).get_config() self.notification_center.send_notifications(enums.NotificationTypes.OPTIMIZELY_CONFIG_UPDATE) internal_notification_center = _NotificationCenterRegistry.get_notification_center( diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index 7904f551..c50bfcb3 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -1039,7 +1039,7 @@ def get_optimizely_config(self) -> Optional[OptimizelyConfig]: if hasattr(self.config_manager, 'optimizely_config'): return self.config_manager.optimizely_config - return OptimizelyConfigService(project_config).get_config() + return OptimizelyConfigService(project_config, self.logger).get_config() def create_user_context( self, user_id: str, attributes: Optional[UserAttributes] = None diff --git a/optimizely/optimizely_config.py b/optimizely/optimizely_config.py index c4f55d86..27b5477a 100644 --- a/optimizely/optimizely_config.py +++ b/optimizely/optimizely_config.py @@ -15,10 +15,13 @@ import copy from typing import Any, Optional +from . import logger from .helpers.condition import ConditionOperatorTypes from .helpers.types import VariationDict, ExperimentDict, RolloutDict, AttributeDict, EventDict from .project_config import ProjectConfig +from .logger import Logger + class OptimizelyConfig: def __init__( @@ -30,7 +33,7 @@ def __init__( environment_key: Optional[str] = None, attributes: Optional[list[OptimizelyAttribute]] = None, events: Optional[list[OptimizelyEvent]] = None, - audiences: Optional[list[OptimizelyAudience]] = None + audiences: Optional[list[OptimizelyAudience]] = None, ): self.revision = revision @@ -47,6 +50,7 @@ def __init__( self.attributes = attributes or [] self.events = events or [] self.audiences = audiences or [] + self.logger = logger def get_datafile(self) -> Optional[str]: """ Get the datafile associated with OptimizelyConfig. @@ -126,11 +130,12 @@ def __init__(self, id: Optional[str], name: Optional[str], conditions: Optional[ class OptimizelyConfigService: """ Class encapsulating methods to be used in creating instance of OptimizelyConfig. """ - def __init__(self, project_config: ProjectConfig): + def __init__(self, project_config: ProjectConfig, logger: Logger): """ Args: project_config ProjectConfig """ + self.logger = logger self.is_valid = True if not isinstance(project_config, ProjectConfig): @@ -411,7 +416,12 @@ def _get_experiments_maps(self) -> tuple[dict[str, OptimizelyExperiment], dict[s audiences_map[audience_id] = audience_name if audience_name is not None else '' all_experiments = self._get_all_experiments() + for exp in all_experiments: + # check if experiment key already exists + if exp["key"] in experiments_key_map: + self.logger.warning(f"Duplicate experiment keys found in datafile: {exp['key']}") + optly_exp = OptimizelyExperiment( exp['id'], exp['key'], self._get_variations_map(exp) ) diff --git a/tests/test_optimizely_config.py b/tests/test_optimizely_config.py index e33c1272..1b40dee1 100644 --- a/tests/test_optimizely_config.py +++ b/tests/test_optimizely_config.py @@ -4,7 +4,7 @@ # You may obtain a copy of the License at # # http://www.apache.org/licenses/LICENSE-2.0 - +import copy # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -15,6 +15,7 @@ from optimizely import optimizely, project_config from optimizely import optimizely_config +from optimizely import logger from . import base @@ -23,7 +24,7 @@ def setUp(self): base.BaseTest.setUp(self) opt_instance = optimizely.Optimizely(json.dumps(self.config_dict_with_features)) self.project_config = opt_instance.config_manager.get_config() - self.opt_config_service = optimizely_config.OptimizelyConfigService(self.project_config) + self.opt_config_service = optimizely_config.OptimizelyConfigService(self.project_config, logger.SimpleLogger()) # todo - added logger self.expected_config = { 'sdk_key': 'features-test', @@ -1235,7 +1236,7 @@ def setUp(self): } self.actual_config = self.opt_config_service.get_config() - self.actual_config_dict = self.to_dict(self.actual_config) + self.actual_config_dict = self.to_dict(self.actual_config) # TODO - fails here after I add logger, actual_config not a dict? self.typed_audiences_config = { 'version': '2', @@ -1473,6 +1474,78 @@ def test__get_experiments_maps(self): self.assertEqual(expected_id_map, self.to_dict(actual_id_map)) + # TODO - I ADDED + def test__duplicate_experiment_keys(self): + """ Test that multiple features don't have the same experiment key. """ + + # update the test datafile with an additional feature flag with the same experiment rule key + new_experiment = { + 'key': 'test_experiment', # added duplicate "test_experiment" + 'status': 'Running', + 'layerId': '8', + "audienceConditions": [ + "or", + "11160" + ], + 'audienceIds': ['11160'], + 'id': '111137', + 'forcedVariations': {}, + 'trafficAllocation': [ + {'entityId': '222242', 'endOfRange': 8000}, + {'entityId': '', 'endOfRange': 10000} + ], + 'variations': [ + { + 'id': '222242', + 'key': 'control', + 'variables': [], + } + ], + } + + new_feature = { + 'id': '91117', + 'key': 'new_feature', + 'experimentIds': ['111137'], + 'rolloutId': '', + 'variables': [ + {'id': '127', 'key': 'is_working', 'defaultValue': 'true', 'type': 'boolean'}, + {'id': '128', 'key': 'environment', 'defaultValue': 'devel', 'type': 'string'}, + {'id': '129', 'key': 'cost', 'defaultValue': '10.99', 'type': 'double'}, + {'id': '130', 'key': 'count', 'defaultValue': '999', 'type': 'integer'}, + {'id': '131', 'key': 'variable_without_usage', 'defaultValue': '45', 'type': 'integer'}, + {'id': '132', 'key': 'object', 'defaultValue': '{"test": 12}', 'type': 'string', + 'subType': 'json'}, + {'id': '133', 'key': 'true_object', 'defaultValue': '{"true_test": 23.54}', 'type': 'json'}, + ], + } + + # add new feature with the same rule key + self.config_dict_with_features['experiments'].append(new_experiment) + self.config_dict_with_features['featureFlags'].append(new_feature) + + config_with_duplicate_key = self.config_dict_with_features + opt_instance = optimizely.Optimizely(json.dumps(config_with_duplicate_key)) + self.project_config = opt_instance.config_manager.get_config() + self.opt_config_service = optimizely_config.OptimizelyConfigService(self.project_config, logger=logger.SimpleLogger()) + + actual_key_map, actual_id_map = self.opt_config_service._get_experiments_maps() + + self.assertIsInstance(actual_key_map, dict) + for exp in actual_key_map.values(): + self.assertIsInstance(exp, optimizely_config.OptimizelyExperiment) + + # assert on the log message + # TODO + + # assert we get ID of the duplicated experiment + assert actual_key_map.get('test_experiment').id == "111137" + + # assert we get one duplicated experiment + keys_list = list(actual_key_map.keys()) + assert "test_experiment" in keys_list, "Key 'test_experiment' not found in actual key map" + assert keys_list.count("test_experiment") == 1, "Key 'test_experiment' found more than once in actual key map" + def test__get_features_map(self): """ Test that get_features_map returns expected features map. """ From 25a46766198d7817e58c14a99f54a21891b9156b Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Tue, 5 Dec 2023 09:55:33 -0800 Subject: [PATCH 2/6] cleanup --- optimizely/optimizely_config.py | 2 +- tests/test_optimizely_config.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/optimizely/optimizely_config.py b/optimizely/optimizely_config.py index 27b5477a..1dd44781 100644 --- a/optimizely/optimizely_config.py +++ b/optimizely/optimizely_config.py @@ -33,7 +33,7 @@ def __init__( environment_key: Optional[str] = None, attributes: Optional[list[OptimizelyAttribute]] = None, events: Optional[list[OptimizelyEvent]] = None, - audiences: Optional[list[OptimizelyAudience]] = None, + audiences: Optional[list[OptimizelyAudience]] = None ): self.revision = revision diff --git a/tests/test_optimizely_config.py b/tests/test_optimizely_config.py index 1b40dee1..11886b48 100644 --- a/tests/test_optimizely_config.py +++ b/tests/test_optimizely_config.py @@ -4,7 +4,6 @@ # You may obtain a copy of the License at # # http://www.apache.org/licenses/LICENSE-2.0 -import copy # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. From 79a8cb70746e729de5fbcbbf45f7aca551d03bc2 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Tue, 5 Dec 2023 14:01:02 -0800 Subject: [PATCH 3/6] fix logger --- optimizely/optimizely_config.py | 2 -- tests/test_optimizely_config.py | 31 ++++++++++++++++++------------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/optimizely/optimizely_config.py b/optimizely/optimizely_config.py index 1dd44781..1720b6a0 100644 --- a/optimizely/optimizely_config.py +++ b/optimizely/optimizely_config.py @@ -15,7 +15,6 @@ import copy from typing import Any, Optional -from . import logger from .helpers.condition import ConditionOperatorTypes from .helpers.types import VariationDict, ExperimentDict, RolloutDict, AttributeDict, EventDict from .project_config import ProjectConfig @@ -50,7 +49,6 @@ def __init__( self.attributes = attributes or [] self.events = events or [] self.audiences = audiences or [] - self.logger = logger def get_datafile(self) -> Optional[str]: """ Get the datafile associated with OptimizelyConfig. diff --git a/tests/test_optimizely_config.py b/tests/test_optimizely_config.py index 11886b48..9628176d 100644 --- a/tests/test_optimizely_config.py +++ b/tests/test_optimizely_config.py @@ -11,6 +11,7 @@ # limitations under the License. import json +from unittest.mock import patch from optimizely import optimizely, project_config from optimizely import optimizely_config @@ -23,7 +24,7 @@ def setUp(self): base.BaseTest.setUp(self) opt_instance = optimizely.Optimizely(json.dumps(self.config_dict_with_features)) self.project_config = opt_instance.config_manager.get_config() - self.opt_config_service = optimizely_config.OptimizelyConfigService(self.project_config, logger.SimpleLogger()) # todo - added logger + self.opt_config_service = optimizely_config.OptimizelyConfigService(self.project_config, logger.SimpleLogger()) self.expected_config = { 'sdk_key': 'features-test', @@ -1452,7 +1453,7 @@ def test__get_config(self): def test__get_config__invalid_project_config(self): """ Test that get_config returns None when invalid project config supplied. """ - opt_service = optimizely_config.OptimizelyConfigService({"key": "invalid"}) + opt_service = optimizely_config.OptimizelyConfigService({"key": "invalid"}, None) self.assertIsNone(opt_service.get_config()) def test__get_experiments_maps(self): @@ -1526,16 +1527,20 @@ def test__duplicate_experiment_keys(self): config_with_duplicate_key = self.config_dict_with_features opt_instance = optimizely.Optimizely(json.dumps(config_with_duplicate_key)) self.project_config = opt_instance.config_manager.get_config() - self.opt_config_service = optimizely_config.OptimizelyConfigService(self.project_config, logger=logger.SimpleLogger()) - actual_key_map, actual_id_map = self.opt_config_service._get_experiments_maps() + with patch('optimizely.logger.SimpleLogger.warning') as mock_logger: + self.opt_config_service = optimizely_config.OptimizelyConfigService(self.project_config, + logger=logger.SimpleLogger()) - self.assertIsInstance(actual_key_map, dict) - for exp in actual_key_map.values(): - self.assertIsInstance(exp, optimizely_config.OptimizelyExperiment) + actual_key_map, actual_id_map = self.opt_config_service._get_experiments_maps() + + self.assertIsInstance(actual_key_map, dict) + for exp in actual_key_map.values(): + self.assertIsInstance(exp, optimizely_config.OptimizelyExperiment) - # assert on the log message - # TODO + # Assert that the warning method of the mock logger was called with the expected message + expected_warning_message = f"Duplicate experiment keys found in datafile: {new_experiment['key']}" + mock_logger.assert_called_with(expected_warning_message) # assert we get ID of the duplicated experiment assert actual_key_map.get('test_experiment').id == "111137" @@ -1746,7 +1751,7 @@ def test_get_audiences(self): error_handler=None ) - config_service = optimizely_config.OptimizelyConfigService(proj_conf) + config_service = optimizely_config.OptimizelyConfigService(proj_conf, logger) for audience in config_service.audiences: self.assertIsInstance(audience, optimizely_config.OptimizelyAudience) @@ -1814,7 +1819,7 @@ def test_stringify_audience_conditions_all_cases(self): '("us" OR ("female" AND "adult")) AND ("fr" AND ("male" OR "adult"))' ] - config_service = optimizely_config.OptimizelyConfigService(config) + config_service = optimizely_config.OptimizelyConfigService(config, None) for i in range(len(audiences_input)): result = config_service.stringify_conditions(audiences_input[i], audiences_map) @@ -1832,7 +1837,7 @@ def test_optimizely_audience_conversion(self): error_handler=None ) - config_service = optimizely_config.OptimizelyConfigService(proj_conf) + config_service = optimizely_config.OptimizelyConfigService(proj_conf, None) for audience in config_service.audiences: self.assertIsInstance(audience, optimizely_config.OptimizelyAudience) @@ -1848,7 +1853,7 @@ def test_get_variations_from_experiments_map(self): error_handler=None ) - config_service = optimizely_config.OptimizelyConfigService(proj_conf) + config_service = optimizely_config.OptimizelyConfigService(proj_conf, None) experiments_key_map, experiments_id_map = config_service._get_experiments_maps() From 85d671d9d7a2990fc38c418eef8adb57e1b843ee Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Tue, 5 Dec 2023 14:04:03 -0800 Subject: [PATCH 4/6] cleanup comments --- tests/test_optimizely_config.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/test_optimizely_config.py b/tests/test_optimizely_config.py index 9628176d..369860f0 100644 --- a/tests/test_optimizely_config.py +++ b/tests/test_optimizely_config.py @@ -1236,7 +1236,7 @@ def setUp(self): } self.actual_config = self.opt_config_service.get_config() - self.actual_config_dict = self.to_dict(self.actual_config) # TODO - fails here after I add logger, actual_config not a dict? + self.actual_config_dict = self.to_dict(self.actual_config) self.typed_audiences_config = { 'version': '2', @@ -1474,7 +1474,6 @@ def test__get_experiments_maps(self): self.assertEqual(expected_id_map, self.to_dict(actual_id_map)) - # TODO - I ADDED def test__duplicate_experiment_keys(self): """ Test that multiple features don't have the same experiment key. """ @@ -1520,7 +1519,7 @@ def test__duplicate_experiment_keys(self): ], } - # add new feature with the same rule key + # add new experiment rule with the same key and a new feature with the same rule key self.config_dict_with_features['experiments'].append(new_experiment) self.config_dict_with_features['featureFlags'].append(new_feature) From f7b507b51687bcba1cabcdbf9c83b4b57d671405 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Tue, 5 Dec 2023 14:10:32 -0800 Subject: [PATCH 5/6] linting --- optimizely/optimizely_config.py | 2 +- tests/test_optimizely_config.py | 72 ++++++++++++++++----------------- 2 files changed, 37 insertions(+), 37 deletions(-) diff --git a/optimizely/optimizely_config.py b/optimizely/optimizely_config.py index 1720b6a0..cf443896 100644 --- a/optimizely/optimizely_config.py +++ b/optimizely/optimizely_config.py @@ -246,7 +246,7 @@ def stringify_conditions(self, conditions: str | list[Any], audiences_map: dict[ operand = conditions[i].upper() else: # Check if element is a list or not - if type(conditions[i]) == list: + if isinstance(conditions[i], list): # Check if at the end or not to determine where to add the operand # Recursive call to call stringify on embedded list if i + 1 < length: diff --git a/tests/test_optimizely_config.py b/tests/test_optimizely_config.py index 369860f0..0f968010 100644 --- a/tests/test_optimizely_config.py +++ b/tests/test_optimizely_config.py @@ -1479,45 +1479,45 @@ def test__duplicate_experiment_keys(self): # update the test datafile with an additional feature flag with the same experiment rule key new_experiment = { - 'key': 'test_experiment', # added duplicate "test_experiment" - 'status': 'Running', - 'layerId': '8', - "audienceConditions": [ - "or", - "11160" - ], - 'audienceIds': ['11160'], - 'id': '111137', - 'forcedVariations': {}, - 'trafficAllocation': [ - {'entityId': '222242', 'endOfRange': 8000}, - {'entityId': '', 'endOfRange': 10000} - ], - 'variations': [ - { - 'id': '222242', - 'key': 'control', - 'variables': [], - } - ], + 'key': 'test_experiment', # added duplicate "test_experiment" + 'status': 'Running', + 'layerId': '8', + "audienceConditions": [ + "or", + "11160" + ], + 'audienceIds': ['11160'], + 'id': '111137', + 'forcedVariations': {}, + 'trafficAllocation': [ + {'entityId': '222242', 'endOfRange': 8000}, + {'entityId': '', 'endOfRange': 10000} + ], + 'variations': [ + { + 'id': '222242', + 'key': 'control', + 'variables': [], } + ], + } new_feature = { - 'id': '91117', - 'key': 'new_feature', - 'experimentIds': ['111137'], - 'rolloutId': '', - 'variables': [ - {'id': '127', 'key': 'is_working', 'defaultValue': 'true', 'type': 'boolean'}, - {'id': '128', 'key': 'environment', 'defaultValue': 'devel', 'type': 'string'}, - {'id': '129', 'key': 'cost', 'defaultValue': '10.99', 'type': 'double'}, - {'id': '130', 'key': 'count', 'defaultValue': '999', 'type': 'integer'}, - {'id': '131', 'key': 'variable_without_usage', 'defaultValue': '45', 'type': 'integer'}, - {'id': '132', 'key': 'object', 'defaultValue': '{"test": 12}', 'type': 'string', - 'subType': 'json'}, - {'id': '133', 'key': 'true_object', 'defaultValue': '{"true_test": 23.54}', 'type': 'json'}, - ], - } + 'id': '91117', + 'key': 'new_feature', + 'experimentIds': ['111137'], + 'rolloutId': '', + 'variables': [ + {'id': '127', 'key': 'is_working', 'defaultValue': 'true', 'type': 'boolean'}, + {'id': '128', 'key': 'environment', 'defaultValue': 'devel', 'type': 'string'}, + {'id': '129', 'key': 'cost', 'defaultValue': '10.99', 'type': 'double'}, + {'id': '130', 'key': 'count', 'defaultValue': '999', 'type': 'integer'}, + {'id': '131', 'key': 'variable_without_usage', 'defaultValue': '45', 'type': 'integer'}, + {'id': '132', 'key': 'object', 'defaultValue': '{"test": 12}', 'type': 'string', + 'subType': 'json'}, + {'id': '133', 'key': 'true_object', 'defaultValue': '{"true_test": 23.54}', 'type': 'json'}, + ], + } # add new experiment rule with the same key and a new feature with the same rule key self.config_dict_with_features['experiments'].append(new_experiment) From 149f0f01a2e7043e3aff046b8ae9eaa6e950a5f1 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Wed, 6 Dec 2023 14:26:33 -0800 Subject: [PATCH 6/6] fix logger --- tests/test_optimizely_config.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_optimizely_config.py b/tests/test_optimizely_config.py index 0f968010..b6b60adf 100644 --- a/tests/test_optimizely_config.py +++ b/tests/test_optimizely_config.py @@ -24,7 +24,8 @@ def setUp(self): base.BaseTest.setUp(self) opt_instance = optimizely.Optimizely(json.dumps(self.config_dict_with_features)) self.project_config = opt_instance.config_manager.get_config() - self.opt_config_service = optimizely_config.OptimizelyConfigService(self.project_config, logger.SimpleLogger()) + self.opt_config_service = optimizely_config.OptimizelyConfigService(self.project_config, + logger=logger.SimpleLogger()) self.expected_config = { 'sdk_key': 'features-test', @@ -1750,7 +1751,7 @@ def test_get_audiences(self): error_handler=None ) - config_service = optimizely_config.OptimizelyConfigService(proj_conf, logger) + config_service = optimizely_config.OptimizelyConfigService(proj_conf, logger=logger.SimpleLogger()) for audience in config_service.audiences: self.assertIsInstance(audience, optimizely_config.OptimizelyAudience)