Bug 1349452 - Improve type checking code in parse_events.py. r=dexter
authorMayank Madan <maddiemadan@gmail.com>
Tue, 11 Apr 2017 18:20:15 +0530
changeset 352597 35a62ecf4475
parent 352596 0c63f689efc7
child 352598 d0f1f01d3696
push id31642
push userkwierso@gmail.com
push dateWed, 12 Apr 2017 21:39:19 +0000
treeherdermozilla-central@fac2c174087f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdexter
bugs1349452
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1349452 - Improve type checking code in parse_events.py. r=dexter
toolkit/components/telemetry/parse_events.py
--- a/toolkit/components/telemetry/parse_events.py
+++ b/toolkit/components/telemetry/parse_events.py
@@ -32,91 +32,102 @@ def convert_to_cpp_identifier(s, sep):
 class OneOf:
     """This is a placeholder type for the TypeChecker below.
     It signals that the checked value should match one of the following arguments
     passed to the TypeChecker constructor.
     """
     pass
 
 
-class TypeChecker:
-    """This implements a convenience type TypeChecker to make the validation code more readable."""
-    def __init__(self, kind, *args):
-        """This takes 1-3 arguments, specifying the value type to check for.
-        It supports:
-        - atomic values, e.g.: TypeChecker(int)
-        - list values, e.g.: TypeChecker(list, basestring)
-        - dict values, e.g.: TypeChecker(dict, basestring, int)
-        - atomic values that can have different types, e.g.: TypeChecker(OneOf, int, date)"""
-        self._kind = kind
-        self._args = args
+class AtomicTypeChecker:
+    """Validate a simple value against a given type"""
+    def __init__(self, instance_type):
+        self.instance_type = instance_type
+
+    def check(self, identifier, key, value):
+        if not isinstance(value, self.instance_type):
+            raise ValueError("%s: failed type check for %s - expected %s, got %s" %
+                             (identifier, key, nice_type_name(self.instance_type),
+                              nice_type_name(type(value))))
+
+
+class MultiTypeChecker:
+    """Validate a simple value against a list of possible types"""
+    def __init__(self, *instance_types):
+        if not instance_types:
+            raise ValueError("At least one instance type is required")
+        self.instance_types = instance_types
+
+    def check(self, identifier, key, value):
+
+        if not any(isinstance(value, i) for i in self.instance_types):
+            raise ValueError("%s: failed type check for %s - got %s, expected one of: %s," %
+                             (identifier, key,
+                              nice_type_name(type(value)),
+                              " or ".join(map(nice_type_name, self.instance_types))))
+
+
+class ListTypeChecker:
+    """Validate a list of values against a given type"""
+    def __init__(self, instance_type):
+        self.instance_type = instance_type
 
     def check(self, identifier, key, value):
-        # Check fields that can be one of two different types.
-        if self._kind is OneOf:
-            if not isinstance(value, self._args[0]) and not isinstance(value, self._args[1]):
-                raise ValueError("%s: failed type check for %s - expected %s or %s, got %s" %
-                                 (identifier, key,
-                                  nice_type_name(self._args[0]),
-                                  nice_type_name(self._args[1]),
-                                  nice_type_name(type(value))))
-            return
+        if len(value) < 1:
+            raise ValueError("%s: failed check for %s - list should not be empty" %
+                             (identifier, key))
 
-        # Check basic type of value.
-        if not isinstance(value, self._kind):
-            raise ValueError("%s: failed type check for %s - expected %s, got %s" %
-                             (identifier, key,
-                              nice_type_name(self._kind),
-                              nice_type_name(type(value))))
+        for x in value:
+            if not isinstance(x, self.instance_type):
+                raise ValueError("%s: failed type check for %s - expected list value type %s, got"
+                                 " %s" % (identifier, key, nice_type_name(self.instance_type),
+                                          nice_type_name(type(x))))
+
+
+class DictTypeChecker:
+    """Validate keys and values of a dict against a given type"""
+    def __init__(self, keys_instance_type, values_instance_type):
+        self.keys_instance_type = keys_instance_type
+        self.values_instance_type = values_instance_type
 
-        # Check types of values in lists.
-        if self._kind is list:
-            if len(value) < 1:
-                raise ValueError("%s: failed check for %s - list should not be empty" % (identifier, key))
-            for x in value:
-                if not isinstance(x, self._args[0]):
-                    raise ValueError("%s: failed type check for %s - expected list value type %s, got %s" %
-                                     (identifier, key,
-                                      nice_type_name(self._args[0]),
-                                      nice_type_name(type(x))))
-
-        # Check types of keys and values in dictionaries.
-        elif self._kind is dict:
-            if len(value.keys()) < 1:
-                    raise ValueError("%s: failed check for %s - dict should not be empty" % (identifier, key))
-            for x in value.iterkeys():
-                if not isinstance(x, self._args[0]):
-                    raise ValueError("%s: failed dict type check for %s - expected key type %s, got %s" %
-                                     (identifier, key,
-                                      nice_type_name(self._args[0]),
-                                      nice_type_name(type(x))))
-            for k, v in value.iteritems():
-                if not isinstance(x, self._args[1]):
-                    raise ValueError("%s: failed dict type check for %s - expected value type %s for key %s, got %s" %
-                                     (identifier, key,
-                                      nice_type_name(self._args[1]),
-                                      k,
-                                      nice_type_name(type(x))))
+    def check(self, identifier, key, value):
+        if len(value.keys()) < 1:
+            raise ValueError("%s: failed check for %s - dict should not be empty" %
+                             (identifier, key))
+        for x in value.iterkeys():
+            if not isinstance(x, self.keys_instance_type):
+                raise ValueError("%s: failed dict type check for %s - expected key type %s, got "
+                                 "%s" %
+                                 (identifier, key,
+                                  nice_type_name(self.keys_instance_type),
+                                  nice_type_name(type(x))))
+        for k, v in value.iteritems():
+            if not isinstance(v, self.values_instance_type):
+                raise ValueError("%s: failed dict type check for %s - "
+                                 "expected value type %s for key %s, got %s" %
+                                 (identifier, key,
+                                  nice_type_name(self.values_instance_type),
+                                  k, nice_type_name(type(v))))
 
 
 def type_check_event_fields(identifier, name, definition):
     """Perform a type/schema check on the event definition."""
     REQUIRED_FIELDS = {
-        'objects': TypeChecker(list, basestring),
-        'bug_numbers': TypeChecker(list, int),
-        'notification_emails': TypeChecker(list, basestring),
-        'record_in_processes': TypeChecker(list, basestring),
-        'description': TypeChecker(basestring),
+        'objects': ListTypeChecker(basestring),
+        'bug_numbers': ListTypeChecker(int),
+        'notification_emails': ListTypeChecker(basestring),
+        'record_in_processes': ListTypeChecker(basestring),
+        'description': AtomicTypeChecker(basestring),
     }
     OPTIONAL_FIELDS = {
-        'methods': TypeChecker(list, basestring),
-        'release_channel_collection': TypeChecker(basestring),
-        'expiry_date': TypeChecker(OneOf, basestring, datetime.date),
-        'expiry_version': TypeChecker(basestring),
-        'extra_keys': TypeChecker(dict, basestring, basestring),
+        'methods': ListTypeChecker(basestring),
+        'release_channel_collection': AtomicTypeChecker(basestring),
+        'expiry_date': MultiTypeChecker(basestring, datetime.date),
+        'expiry_version': AtomicTypeChecker(basestring),
+        'extra_keys': DictTypeChecker(basestring, basestring),
     }
     ALL_FIELDS = REQUIRED_FIELDS.copy()
     ALL_FIELDS.update(OPTIONAL_FIELDS)
 
     # Check that all the required fields are available.
     missing_fields = [f for f in REQUIRED_FIELDS.keys() if f not in definition]
     if len(missing_fields) > 0:
         raise KeyError(identifier + ' - missing required fields: ' + ', '.join(missing_fields))