fix: odp issues identified by FSC (#412)

* check if opti instance is valid on odp methods

* fix variable missing error

* fix extrenous identify calls

* change integrations to default to first with key

* fix cache_size bug

* add timeout to pollingconfig stop

* Update python.yml

* revert branch to master

* fix create_user_context

* remove unnecessary checks

Co-authored-by: Matjaz Pirnovar <[email protected]>

Author: andrewleap-optimizely

Diff for: .github/workflows/python.yml

@@ -5,7 +5,7 @@ name: build
 
 on:
   push:
-    branches: [ master ]
+    branches: [ master  ]
   pull_request:
     branches: [ master ]
 

Diff for: optimizely/config_manager.py

@@ -375,10 +375,11 @@ def is_running(self) -> bool:
         return self._polling_thread.is_alive()
 
     def stop(self) -> None:
-        """ Stop the polling thread and wait for it to exit. """
+        """ Stop the polling thread and briefly wait for it to exit. """
         if self.is_running:
             self.stopped.set()
-            self._polling_thread.join()
+            # no need to wait too long as this exists to avoid interfering with tests
+            self._polling_thread.join(timeout=0.2)
 
     def _run(self) -> None:
         """ Triggered as part of the thread which fetches the datafile and sleeps until next update interval. """

Diff for: optimizely/entities.py

@@ -188,7 +188,7 @@ def __str__(self) -> str:
 
 
 class Integration(BaseEntity):
-    def __init__(self, key: str, host: Optional[str] = None, publicKey: Optional[str] = None):
+    def __init__(self, key: str, host: Optional[str] = None, publicKey: Optional[str] = None, **kwargs: Any):
         self.key = key
         self.host = host
         self.publicKey = publicKey

Diff for: optimizely/optimizely.py

@@ -345,10 +345,8 @@ def _get_feature_variable_for_type(
         source_info = {}
         variable_value = variable.defaultValue
 
-        user_context = self.create_user_context(user_id, attributes)
-        # error is logged in create_user_context
-        if user_context is None:
-            return None
+        user_context = OptimizelyUserContext(self, self.logger, user_id, attributes, False)
+
         decision, _ = self.decision_service.get_variation_for_feature(project_config, feature_flag, user_context)
 
         if decision.variation:
@@ -434,10 +432,8 @@ def _get_all_feature_variables_for_type(
         feature_enabled = False
         source_info = {}
 
-        user_context = self.create_user_context(user_id, attributes)
-        # error is logged in create_user_context
-        if user_context is None:
-            return None
+        user_context = OptimizelyUserContext(self, self.logger, user_id, attributes, False)
+
         decision, _ = self.decision_service.get_variation_for_feature(project_config, feature_flag, user_context)
 
         if decision.variation:
@@ -643,10 +639,7 @@ def get_variation(
         if not self._validate_user_inputs(attributes):
             return None
 
-        user_context = self.create_user_context(user_id, attributes)
-        # error is logged in create_user_context
-        if not user_context:
-            return None
+        user_context = OptimizelyUserContext(self, self.logger, user_id, attributes, False)
 
         variation, _ = self.decision_service.get_variation(project_config, experiment, user_context)
         if variation:
@@ -705,10 +698,8 @@ def is_feature_enabled(self, feature_key: str, user_id: str, attributes: Optiona
 
         feature_enabled = False
         source_info = {}
-        user_context = self.create_user_context(user_id, attributes)
-        # error is logged in create_user_context
-        if not user_context:
-            return False
+
+        user_context = OptimizelyUserContext(self, self.logger, user_id, attributes, False)
 
         decision, _ = self.decision_service.get_variation_for_feature(project_config, feature, user_context)
         is_source_experiment = decision.source == enums.DecisionSources.FEATURE_TEST
@@ -1083,7 +1074,7 @@ def create_user_context(
             self.logger.error(enums.Errors.INVALID_INPUT.format('attributes'))
             return None
 
-        return OptimizelyUserContext(self, self.logger, user_id, attributes)
+        return OptimizelyUserContext(self, self.logger, user_id, attributes, True)
 
     def _decide(
         self, user_context: Optional[OptimizelyUserContext], key: str,
@@ -1330,8 +1321,8 @@ def setup_odp(self) -> None:
 
         if not self.sdk_settings.segments_cache:
             self.sdk_settings.segments_cache = LRUCache(
-                self.sdk_settings.segments_cache_size or enums.OdpSegmentsCacheConfig.DEFAULT_CAPACITY,
-                self.sdk_settings.segments_cache_timeout_in_secs or enums.OdpSegmentsCacheConfig.DEFAULT_TIMEOUT_SECS
+                self.sdk_settings.segments_cache_size,
+                self.sdk_settings.segments_cache_timeout_in_secs
             )
 
     def _update_odp_config_on_datafile_update(self) -> None:
@@ -1354,9 +1345,17 @@ def _update_odp_config_on_datafile_update(self) -> None:
         )
 
     def identify_user(self, user_id: str) -> None:
+        if not self.is_valid:
+            self.logger.error(enums.Errors.INVALID_OPTIMIZELY.format('identify_user'))
+            return
+
         self.odp_manager.identify_user(user_id)
 
     def fetch_qualified_segments(self, user_id: str, options: Optional[list[str]] = None) -> Optional[list[str]]:
+        if not self.is_valid:
+            self.logger.error(enums.Errors.INVALID_OPTIMIZELY.format('fetch_qualified_segments'))
+            return None
+
         return self.odp_manager.fetch_qualified_segments(user_id, options or [])
 
     def send_odp_event(
@@ -1376,11 +1375,16 @@ def send_odp_event(
             data: An optional dictionary for associated data. The default event data will be added to this data
             before sending to the ODP server.
         """
+        if not self.is_valid:
+            self.logger.error(enums.Errors.INVALID_OPTIMIZELY.format('send_odp_event'))
+            return
+
         self.odp_manager.send_event(type, action, identifiers or {}, data or {})
 
     def close(self) -> None:
         if callable(getattr(self.event_processor, 'stop', None)):
             self.event_processor.stop()  # type: ignore[attr-defined]
-        self.odp_manager.close()
+        if self.is_valid:
+            self.odp_manager.close()
         if callable(getattr(self.config_manager, 'stop', None)):
             self.config_manager.stop()  # type: ignore[attr-defined]

Diff for: optimizely/optimizely_config.py

@@ -343,9 +343,11 @@ def _get_variables_map(
 
             # set variation specific variable value if any
             if variation.get('featureEnabled'):
+                feature_variables_map = self.feature_key_variable_id_to_variable_map[feature_flag['key']]
                 for variable in variation.get('variables', []):
-                    feature_variable = self.feature_key_variable_id_to_variable_map[feature_flag['key']][variable['id']]
-                    variables_map[feature_variable.key].value = variable['value']
+                    feature_variable = feature_variables_map.get(variable['id'])
+                    if feature_variable:
+                        variables_map[feature_variable.key].value = variable['value']
 
         return variables_map
 

Diff for: optimizely/project_config.py

@@ -112,7 +112,9 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any):
                 self.experiment_id_map[experiment_dict['id']] = entities.Experiment(**experiment_dict)
 
         if self.integrations:
-            self.integration_key_map = self._generate_key_map(self.integrations, 'key', entities.Integration)
+            self.integration_key_map = self._generate_key_map(
+                self.integrations, 'key', entities.Integration, first_value=True
+            )
             odp_integration = self.integration_key_map.get('odp')
             if odp_integration:
                 self.public_key_for_odp = odp_integration.publicKey
@@ -191,21 +193,24 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any):
 
     @staticmethod
     def _generate_key_map(
-        entity_list: Iterable[Any], key: str, entity_class: Type[EntityClass]
+        entity_list: Iterable[Any], key: str, entity_class: Type[EntityClass], first_value: bool = False
     ) -> dict[str, EntityClass]:
         """ Helper method to generate map from key to entity object for given list of dicts.
 
         Args:
             entity_list: List consisting of dict.
             key: Key in each dict which will be key in the map.
             entity_class: Class representing the entity.
+            first_value: If True, only save the first value found for each key.
 
         Returns:
             Map mapping key to entity object.
         """
 
-        key_map = {}
+        key_map: dict[str, EntityClass] = {}
         for obj in entity_list:
+            if first_value and key_map.get(obj[key]):
+                continue
             key_map[obj[key]] = entity_class(**obj)
 
         return key_map