From a617106e4baa22910292a47a1cab38b7cb7ea739 Mon Sep 17 00:00:00 2001 From: Lucas Di Pentima Date: Thu, 3 Mar 2022 11:06:13 -0300 Subject: [PATCH] 18574: Adds key & value type checking. Improves code layout for readability. Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima --- sdk/python/arvados/vocabulary.py | 53 +++++++++++++++++++++-------- sdk/python/tests/test_vocabulary.py | 28 +++++++++++---- 2 files changed, 60 insertions(+), 21 deletions(-) diff --git a/sdk/python/arvados/vocabulary.py b/sdk/python/arvados/vocabulary.py index 89698e20af..3bb87c48dc 100644 --- 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 cce2ec6233..1d3d7b6272 100644 --- 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): -- 2.30.2