Merge pull request #99 from splitio/improvement/inputSanitization

[SDKS: 29]: Python Input Validation

Author: mmelograno

.github/pull_request_template.md

@@ -0,0 +1,15 @@
+# Python SDK
+
+## Tickets covered:
+* [SDKS-{TICKET}](https://splitio.atlassian.net/browse/SDKS-{TICKET})
+
+## What did you accomplish?
+* Bullet 1
+* Bullet 2
+
+## How to test new changes?
+*
+
+## Extra Notes
+* Bullet 1
+* Bullet 2

splitio/__init__.py

@@ -2,7 +2,7 @@
     unicode_literals
 
 from .factories import get_factory # noqa
-from .clients import Key # noqa
+from .key import Key # noqa
 from .version import __version__  # noqa
 
 __all__ = ('api', 'brokers', 'cache', 'clients', 'matchers', 'segments',

splitio/clients.py

@@ -1,4 +1,4 @@
-"""A module for Split.io SDK API clients"""
+"""A module for Split.io SDK API clients."""
 from __future__ import absolute_import, division, print_function, \
     unicode_literals
 
@@ -10,19 +10,27 @@
 from splitio.metrics import SDK_GET_TREATMENT
 from splitio.splits import ConditionType
 from splitio.events import Event
-
-
-class Key(object):
-    def __init__(self, matching_key, bucketing_key):
-        """Bucketing Key implementation"""
-        self.matching_key = matching_key
-        self.bucketing_key = bucketing_key
+from . import input_validator
+from splitio.key import Key
 
 
 class Client(object):
+    """Client class that uses a broker for storage."""
+
     def __init__(self, broker, labels_enabled=True, impression_listener=None):
-        """Basic interface of a Client. Specific implementations need to override the
-        get_split_fetcher method (and optionally the get_splitter method).
+        """
+        Construct a Client instance.
+
+        :param broker: Broker that accepts/retrieves splits, segments, events, metrics & impressions
+        :type broker: BaseBroker
+
+        :param labels_enabled: Whether to store labels on impressions
+        :type labels_enabled: bool
+
+        :param impression_listener: impression listener implementation
+        :type impression_listener: ImpressionListener
+
+        :rtype: Client
         """
         self._logger = logging.getLogger(self.__class__.__name__)
         self._splitter = Splitter()
@@ -31,21 +39,10 @@ def __init__(self, broker, labels_enabled=True, impression_listener=None):
         self._destroyed = False
         self._impression_listener = impression_listener
 
-    @staticmethod
-    def _get_keys(key):
-        """
-        """
-        if isinstance(key, Key):
-            matching_key = key.matching_key
-            bucketing_key = key.bucketing_key
-        else:
-            matching_key = str(key)
-            bucketing_key = None
-        return matching_key, bucketing_key
-
     def destroy(self):
         """
         Disable the split-client and free all allocated resources.
+
         Only applicable when using in-memory operation mode.
         """
         self._destroyed = True
@@ -64,9 +61,11 @@ def _handle_custom_impression(self, impression, attributes):
 
     def get_treatment(self, key, feature, attributes=None):
         """
-        Get the treatment for a feature and key, with an optional dictionary of attributes. This
-        method never raises an exception. If there's a problem, the appropriate log message will
-        be generated and the method will return the CONTROL treatment.
+        Get the treatment for a feature and key, with an optional dictionary of attributes.
+
+        This method never raises an exception. If there's a problem, the appropriate log message
+        will be generated and the method will return the CONTROL treatment.
+
         :param key: The key for which to get the treatment
         :type key: str
         :param feature: The name of the feature for which to get the treatment
@@ -77,14 +76,19 @@ def get_treatment(self, key, feature, attributes=None):
         :rtype: str
         """
         if self._destroyed:
-            return CONTROL
-
-        if key is None or feature is None:
+            self._logger.warning("Client has already been destroyed, returning CONTROL")
             return CONTROL
 
         start = int(round(time.time() * 1000))
 
-        matching_key, bucketing_key = self._get_keys(key)
+        matching_key, bucketing_key = input_validator.validate_key(key)
+        feature = input_validator.validate_feature_name(feature)
+
+        if (matching_key is None and bucketing_key is None) or feature is None:
+            impression = self._build_impression(matching_key, feature, CONTROL, Label.EXCEPTION,
+                                                0, bucketing_key, start)
+            self._record_stats(impression, start, SDK_GET_TREATMENT)
+            return CONTROL
 
         try:
             label = ''
@@ -123,49 +127,79 @@ def get_treatment(self, key, feature, attributes=None):
             self._handle_custom_impression(impression, attributes)
 
             return _treatment
-        except:
+        except Exception:  # pylint: disable=broad-except
             self._logger.exception('Exception caught getting treatment for feature')
 
             try:
-                impression = self._build_impression(matching_key, feature, CONTROL, Label.EXCEPTION,
-                                                    self._broker.get_change_number(), bucketing_key, start)
+                impression = self._build_impression(
+                    matching_key,
+                    feature,
+                    CONTROL,
+                    Label.EXCEPTION,
+                    self._broker.get_change_number(), bucketing_key, start
+                )
                 self._record_stats(impression, start, SDK_GET_TREATMENT)
+            except Exception:  # pylint: disable=broad-except
+                self._logger.exception(
+                    'Exception reporting impression into get_treatment exception block'
+                )
 
                 self._handle_custom_impression(impression, attributes)
-            except:
-                self._logger.exception('Exception reporting impression into get_treatment exception block')
 
             return CONTROL
 
-    def _build_impression(self, matching_key, feature_name, treatment, label, change_number, bucketing_key, time):
+    def _build_impression(
+            self, matching_key, feature_name, treatment, label,
+            change_number, bucketing_key, imp_time
+    ):
+        """
+        Build an impression.
 
+        TODO: REFACTOR THIS!
+        """
         if not self._labels_enabled:
             label = None
 
         return Impression(
             matching_key=matching_key, feature_name=feature_name,
             treatment=treatment, label=label, change_number=change_number,
-            bucketing_key=bucketing_key, time=time
+            bucketing_key=bucketing_key, time=imp_time
         )
 
     def _record_stats(self, impression, start, operation):
+        """
+        Record impression and metrics.
+
+        :param impression: Generated impression
+        :type impression: Impression
+
+        :param start: timestamp when get_treatment was called
+        :type start: int
+
+        :param operation: operation performed.
+        :type operation: string
+        """
         try:
             end = int(round(time.time() * 1000))
             self._broker.log_impression(impression)
             self._broker.log_operation_time(operation, end - start)
-        except:
+        except Exception:  # pylint: disable=broad-except
             self._logger.exception('Exception caught recording impressions and metrics')
 
     def _get_treatment_for_split(self, split, matching_key, bucketing_key, attributes=None):
         """
-        Internal method to get the treatment for a given Split and optional attributes. This
-        method might raise exceptions and should never be used directly.
+        Evaluate the user submitted data againt a feature and return the resulting treatment.
+
+        This method might raise exceptions and should never be used directly.
         :param split: The split for which to get the treatment
         :type split: Split
+
         :param key: The key for which to get the treatment
         :type key: str
+
         :param attributes: An optional dictionary of attributes
         :type attributes: dict
+
         :return: The treatment for the key and split
         :rtype: str
         """
@@ -207,32 +241,85 @@ def _get_treatment_for_split(self, split, matching_key, bucketing_key, attribute
 
     def track(self, key, traffic_type, event_type, value=None):
         """
-        Track an event
+        Track an event.
+
+        :param key: user key associated to the event
+        :type key: string
+
+        :param traffic_type: traffic type name
+        :type traffic_type: string
+
+        :param event_type: event type name
+        :type event_type: string
+
+        :param value: (Optional) value associated to the event
+        :type value: Number
+
+        :rtype: bool
         """
-        e = Event(
+        key = input_validator.validate_track_key(key)
+        event_type = input_validator.validate_event_type(event_type)
+        traffic_type = input_validator.validate_traffic_type(traffic_type)
+        value = input_validator.validate_value(value)
+
+        if key is None or event_type is None or traffic_type is None or value is None:
+            return False
+
+        event = Event(
             key=key,
             trafficTypeName=traffic_type,
             eventTypeId=event_type,
             value=value,
             timestamp=int(time.time()*1000)
         )
-        return self._broker.get_events_log().log_event(e)
+        return self._broker.get_events_log().log_event(event)
 
 
 class MatcherClient(Client):
     """
+    Client to be used by matchers such as "Dependency Matcher".
+
+    TODO: Refactor This!
     """
-    def __init__(self, broker, splitter, logger):
+
+    def __init__(self, broker, splitter, logger):  # pylint: disable=super-init-not-called
+        """
+        Construct a MatcherClient instance.
+
+        :param broker: Broker where splits & segments will be fetched.
+        :type broker: BaseBroker
+
+        :param splitter: splitter
+        :type splitter: Splitter
+
+        :param logger: logger object
+        :type logger: logging.Logger
+        """
         self._broker = broker
         self._splitter = splitter
         self._logger = logger
 
     def get_treatment(self, key, feature, attributes=None):
         """
+        Evaluate a feature and return the appropriate traetment.
+
+        Will not generate impressions nor metrics
+
+        :param key: user key
+        :type key: mixed
+
+        :param feature: feature name
+        :type feature: str
+
+        :param attributes: (Optional) attributes associated with the user key
+        :type attributes: dict
         """
-        if key is None or feature is None: return CONTROL
+        matching_key, bucketing_key = input_validator.validate_key(key)
+        feature = input_validator.validate_feature_name(feature)
+
+        if (matching_key is None and bucketing_key is None) or feature is None:
+            return CONTROL
 
-        matching_key, bucketing_key = self._get_keys(key)
         try:
             # Fetching Split definition
             split = self._broker.fetch_feature(feature)
@@ -244,7 +331,8 @@ def get_treatment(self, key, feature, attributes=None):
                 )
                 return CONTROL
 
-            if split.killed: return split.default_treatment
+            if split.killed:
+                return split.default_treatment
 
             treatment, _ = self._get_treatment_for_split(
                 split,
@@ -253,8 +341,12 @@ def get_treatment(self, key, feature, attributes=None):
                 attributes
             )
 
-            if treatment is None: return split.default_treatment
+            if treatment is None:
+                return split.default_treatment
+
             return treatment
-        except:
-            self._logger.exception('Exception caught retrieving dependent feature. Returning CONTROL')
+        except Exception:  # pylint: disable=broad-except
+            self._logger.exception(
+                'Exception caught retrieving dependent feature. Returning CONTROL'
+            )
             return CONTROL