From: Lucas Di Pentima <lucas.dipentima@curii.com>
Date: Tue, 1 Mar 2022 23:10:14 +0000 (-0300)
Subject: 18574: Fixes value list edge cases (with tests). Also avoids duplicated code.
X-Git-Tag: 2.4.0~67^2~5
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/a480283b6fe33bd0df12d879c835427e1b71489a

18574: Fixes value list edge cases (with tests). Also avoids duplicated code.

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 a3344a17b9..ae03ef42db 100644
--- a/sdk/python/arvados/vocabulary.py
+++ b/sdk/python/arvados/vocabulary.py
@@ -38,49 +38,42 @@ class Vocabulary(object):
     def convert_to_identifiers(self, obj={}):
         """Translate key/value pairs to machine readable identifiers.
         """
-        if not isinstance(obj, dict):
-            raise ValueError("obj must be a dict")
-        r = {}
-        for k, v in obj.items():
-            k_id, v_id = k, v
-            try:
-                k_id = self[k].identifier
-                try:
-                    if isinstance(v, list):
-                        v_id = [self[k][x].identifier for x in v]
-                    else:
-                        v_id = self[k][v].identifier
-                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)
-            r[k_id] = v_id
-        return r
+        return self._convert_to_what(obj, 'identifier')
 
     def convert_to_labels(self, obj={}):
         """Translate key/value pairs to human readable labels.
         """
+        return self._convert_to_what(obj, 'preferred_label')
+
+    def _convert_to_what(self, obj={}, what=None):
         if not isinstance(obj, dict):
             raise ValueError("obj must be a dict")
+        if what not in ['preferred_label', 'identifier']:
+            raise ValueError("what attr must be 'preferred_label' or 'identifier'")
         r = {}
         for k, v in obj.items():
-            k_lbl, v_lbl = k, v
+            k_what, v_what = k, v
             try:
-                k_lbl = self[k].preferred_label
-                try:
-                    if isinstance(v, list):
-                        v_lbl = [self[k][x].preferred_label for x in v]
-                    else:
-                        v_lbl = self[k][v].preferred_label
-                except KeyError:
-                    if self[k].strict:
-                        raise ValueError("value '%s' not found for key '%s'" % (v, k))
+                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:
+                    try:
+                        v_what = getattr(self[k][v], 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)
-            r[k_lbl] = v_lbl
+            r[k_what] = v_what
         return r
 
 class VocabularyData(object):
diff --git a/sdk/python/tests/test_vocabulary.py b/sdk/python/tests/test_vocabulary.py
index 1d20777823..56647e8243 100644
--- a/sdk/python/tests/test_vocabulary.py
+++ b/sdk/python/tests/test_vocabulary.py
@@ -180,6 +180,18 @@ class VocabularyTest(unittest.TestCase):
         with self.assertRaises(ValueError):
             self.voc.convert_to_identifiers({'Priority': 'foo'})
 
+    def test_convert_to_identifiers_unknown_value_list(self):
+        # Non-strict key
+        self.assertEqual(self.voc['animal'].strict, False)
+        self.assertEqual(
+            self.voc.convert_to_identifiers({'Animal': ['foo', 'loxodonta']}),
+            {'IDTAGANIMALS': ['foo', 'IDVALANIMAL2']}
+        )
+        # Strict key
+        self.assertEqual(self.voc['priority'].strict, True)
+        with self.assertRaises(ValueError):
+            self.voc.convert_to_identifiers({'Priority': ['foo', 'bar']})
+
     def test_convert_to_labels(self):
         cases = [
             {'IDTAGIMPORTANCES': 'IDVALIMPORTANCE1'},
@@ -241,6 +253,18 @@ class VocabularyTest(unittest.TestCase):
         with self.assertRaises(ValueError):
             self.voc.convert_to_labels({'IDTAGIMPORTANCES': 'foo'})
 
+    def test_convert_to_labels_unknown_value_list(self):
+        # Non-strict key
+        self.assertEqual(self.voc['animal'].strict, False)
+        self.assertEqual(
+            self.voc.convert_to_labels({'IDTAGANIMALS': ['foo', 'IDVALANIMAL1']}),
+            {'Animal': ['foo', 'Human']}
+        )
+        # Strict key
+        self.assertEqual(self.voc['priority'].strict, True)
+        with self.assertRaises(ValueError):
+            self.voc.convert_to_labels({'IDTAGIMPORTANCES': ['foo', 'bar']})
+
     def test_convert_roundtrip(self):
         initial = {'IDTAGIMPORTANCES': 'IDVALIMPORTANCE1', 'IDTAGANIMALS': 'IDVALANIMAL1', 'IDTAGCOMMENTS': 'Very important person'}
         converted = self.voc.convert_to_labels(initial)