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 37969fb4..cf443896 100644 --- a/optimizely/optimizely_config.py +++ b/optimizely/optimizely_config.py @@ -19,6 +19,8 @@ from .helpers.types import VariationDict, ExperimentDict, RolloutDict, AttributeDict, EventDict from .project_config import ProjectConfig +from .logger import Logger + class OptimizelyConfig: def __init__( @@ -126,11 +128,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 +414,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..b6b60adf 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 - # 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. @@ -12,9 +11,11 @@ # limitations under the License. import json +from unittest.mock import patch from optimizely import optimizely, project_config from optimizely import optimizely_config +from optimizely import logger from . import base @@ -23,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) + self.opt_config_service = optimizely_config.OptimizelyConfigService(self.project_config, + logger=logger.SimpleLogger()) self.expected_config = { 'sdk_key': 'features-test', @@ -1452,7 +1454,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): @@ -1473,6 +1475,81 @@ def test__get_experiments_maps(self): self.assertEqual(expected_id_map, self.to_dict(actual_id_map)) + 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 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) + + 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() + + with patch('optimizely.logger.SimpleLogger.warning') as mock_logger: + 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 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" + + # 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. """ @@ -1674,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=logger.SimpleLogger()) for audience in config_service.audiences: self.assertIsInstance(audience, optimizely_config.OptimizelyAudience) @@ -1742,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) @@ -1760,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) @@ -1776,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()