18574: Adds key & value type checking. Improves code layout for readability.

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima@curii.com>

diff --git a/sdk/python/arvados/vocabulary.py b/sdk/python/arvados/vocabulary.py
index 89698e20afc49c4c058d4858e418320dcb189080..3bb87c48dc8217ffe2f07bc21c362e179af1a656 100644 (file)
--- a/sdk/python/arvados/vocabulary.py
+++ b/sdk/python/arvados/vocabulary.py
@@ -15,6 +15,17 @@ def load_vocabulary(api_client=None):
         api_client = api('v1')
     return Vocabulary(api_client.vocabulary())
 
+class VocabularyError(Exception):
+    """Base class for all vocabulary errors.
+    """
+    pass
+
+class VocabularyKeyError(VocabularyError):
+    pass
+
+class VocabularyValueError(VocabularyError):
+    pass
+
 class Vocabulary(object):
     def __init__(self, voc_definition={}):
         self.strict_keys = voc_definition.get('strict_tags', False)
@@ -52,27 +63,39 @@ class Vocabulary(object):
             raise ValueError("what attr must be 'preferred_label' or 'identifier'")
         r = {}
         for k, v in obj.items():
+            # Key validation & lookup
+            key_found = False
+            if not isinstance(k, str):
+                raise VocabularyKeyError("key '{}' must be a string".format(k))
             k_what, v_what = k, v
             try:
                 k_what = getattr(self[k], what)
-                if isinstance(v, list):
-                    v_what = []
-                    for x in v:
-                        try:
-                            v_what.append(getattr(self[k][x], what))
-                        except KeyError:
-                            if self[k].strict:
-                                raise ValueError("value '%s' not found for key '%s'" % (x, k))
-                            v_what.append(x)
-                else:
+                key_found = True
+            except KeyError:
+                if self.strict_keys:
+                    raise VocabularyKeyError("key '{}' not found in vocabulary".format(k))
+
+            # Value validation & lookup
+            if isinstance(v, list):
+                v_what = []
+                for x in v:
+                    if not isinstance(x, str):
+                        raise VocabularyValueError("value '{}' for key '{}' must be a string".format(x, k))
                     try:
-                        v_what = getattr(self[k][v], what)
+                        v_what.append(getattr(self[k][x], what))
                     except KeyError:
                         if self[k].strict:
-                            raise ValueError("value '%s' not found for key '%s'" % (v, k))
-            except KeyError:
-                if self.strict_keys:
-                    raise KeyError("key '%s' not found" % k)
+                            raise VocabularyValueError("value '{}' not found for key '{}'".format(x, k))
+                        v_what.append(x)
+            else:
+                if not isinstance(v, str):
+                    raise VocabularyValueError("{} value '{}' for key '{}' must be a string".format(type(v).__name__, v, k))
+                try:
+                    v_what = getattr(self[k][v], what)
+                except KeyError:
+                    if key_found and self[k].strict:
+                        raise VocabularyValueError("value '{}' not found for key '{}'".format(v, k))
+
             r[k_what] = v_what
         return r
 

diff --git a/sdk/python/tests/test_vocabulary.py b/sdk/python/tests/test_vocabulary.py
index cce2ec62338f28f5bdfa6128e7a9da8f392ace97..1d3d7b62723f39d4bb959f487293e508a4828265 100644 (file)
--- a/sdk/python/tests/test_vocabulary.py
+++ b/sdk/python/tests/test_vocabulary.py
@@ -169,18 +169,26 @@ class VocabularyTest(unittest.TestCase):
         # Strict vocabulary
         strict_voc = arvados.vocabulary.Vocabulary(self.EXAMPLE_VOC)
         strict_voc.strict_keys = True
-        with self.assertRaises(KeyError):
+        with self.assertRaises(vocabulary.VocabularyKeyError):
             strict_voc.convert_to_identifiers({'foo': 'bar'})
 
+    def test_convert_to_identifiers_invalid_key(self):
+        with self.assertRaises(vocabulary.VocabularyKeyError):
+            self.voc.convert_to_identifiers({('f', 'o', 'o'): 'bar'})
+
     def test_convert_to_identifiers_unknown_value(self):
         # Non-strict key
         self.assertEqual(self.voc['animal'].strict, False)
         self.assertEqual(self.voc.convert_to_identifiers({'Animal': 'foo'}), {'IDTAGANIMALS': 'foo'})
         # Strict key
         self.assertEqual(self.voc['priority'].strict, True)
-        with self.assertRaises(ValueError):
+        with self.assertRaises(vocabulary.VocabularyValueError):
             self.voc.convert_to_identifiers({'Priority': 'foo'})
 
+    def test_convert_to_identifiers_invalid_value(self):
+        with self.assertRaises(vocabulary.VocabularyValueError):
+            self.voc.convert_to_identifiers({'Animal': 42})
+
     def test_convert_to_identifiers_unknown_value_list(self):
         # Non-strict key
         self.assertEqual(self.voc['animal'].strict, False)
@@ -190,7 +198,7 @@ class VocabularyTest(unittest.TestCase):
         )
         # Strict key
         self.assertEqual(self.voc['priority'].strict, True)
-        with self.assertRaises(ValueError):
+        with self.assertRaises(vocabulary.VocabularyValueError):
             self.voc.convert_to_identifiers({'Priority': ['foo', 'bar']})
 
     def test_convert_to_labels(self):
@@ -242,18 +250,26 @@ class VocabularyTest(unittest.TestCase):
         # Strict vocabulary
         strict_voc = arvados.vocabulary.Vocabulary(self.EXAMPLE_VOC)
         strict_voc.strict_keys = True
-        with self.assertRaises(KeyError):
+        with self.assertRaises(vocabulary.VocabularyKeyError):
             strict_voc.convert_to_labels({'foo': 'bar'})
 
+    def test_convert_to_labels_invalid_key(self):
+        with self.assertRaises(vocabulary.VocabularyKeyError):
+            self.voc.convert_to_labels({42: 'bar'})
+
     def test_convert_to_labels_unknown_value(self):
         # Non-strict key
         self.assertEqual(self.voc['animal'].strict, False)
         self.assertEqual(self.voc.convert_to_labels({'IDTAGANIMALS': 'foo'}), {'Animal': 'foo'})
         # Strict key
         self.assertEqual(self.voc['priority'].strict, True)
-        with self.assertRaises(ValueError):
+        with self.assertRaises(vocabulary.VocabularyValueError):
             self.voc.convert_to_labels({'IDTAGIMPORTANCES': 'foo'})
 
+    def test_convert_to_labels_invalid_value(self):
+        with self.assertRaises(vocabulary.VocabularyValueError):
+            self.voc.convert_to_labels({'IDTAGIMPORTANCES': {'high': True}})
+
     def test_convert_to_labels_unknown_value_list(self):
         # Non-strict key
         self.assertEqual(self.voc['animal'].strict, False)
@@ -263,7 +279,7 @@ class VocabularyTest(unittest.TestCase):
         )
         # Strict key
         self.assertEqual(self.voc['priority'].strict, True)
-        with self.assertRaises(ValueError):
+        with self.assertRaises(vocabulary.VocabularyValueError):
             self.voc.convert_to_labels({'IDTAGIMPORTANCES': ['foo', 'bar']})
 
     def test_convert_roundtrip(self):