2800: Improve spec conformance of Python SDK KeepLocator.

* Require size to immediately follow digest.
* Accept all valid hints.

diff --git a/sdk/python/arvados/keep.py b/sdk/python/arvados/keep.py
index bffb02a147b926d258d65387342dc718eb934f51..36678291d877c91559835524f410dd27ca711186 100644 (file)
--- a/sdk/python/arvados/keep.py
+++ b/sdk/python/arvados/keep.py
@@ -31,28 +31,30 @@ import arvados.util
 
 class KeepLocator(object):
     EPOCH_DATETIME = datetime.datetime.utcfromtimestamp(0)
+    HINT_RE = re.compile(r'^[A-Z][A-Za-z0-9@_-]+$')
 
     def __init__(self, locator_str):
-        self.size = None
-        self.loc_hint = None
+        self.hints = []
         self._perm_sig = None
         self._perm_expiry = None
         pieces = iter(locator_str.split('+'))
         self.md5sum = next(pieces)
+        try:
+            self.size = int(next(pieces))
+        except StopIteration:
+            self.size = None
         for hint in pieces:
-            if hint.startswith('A'):
+            if self.HINT_RE.match(hint) is None:
+                raise ValueError("unrecognized hint data {}".format(hint))
+            elif hint.startswith('A'):
                 self.parse_permission_hint(hint)
-            elif hint.startswith('K'):
-                self.loc_hint = hint  # FIXME
-            elif hint.isdigit():
-                self.size = int(hint)
             else:
-                raise ValueError("unrecognized hint data {}".format(hint))
+                self.hints.append(hint)
 
     def __str__(self):
         return '+'.join(
-            str(s) for s in [self.md5sum, self.size, self.loc_hint,
-                             self.permission_hint()]
+            str(s) for s in [self.md5sum, self.size,
+                             self.permission_hint()] + self.hints
             if s is not None)
 
     def _make_hex_prop(name, length):

diff --git a/sdk/python/tests/test_collections.py b/sdk/python/tests/test_collections.py
index 5354f3a9f7bf64042ab7f635119b05d112ee99a8..b1af723635f46def25abb8516d631de5362e6329 100644 (file)
--- a/sdk/python/tests/test_collections.py
+++ b/sdk/python/tests/test_collections.py
@@ -586,7 +586,7 @@ class ArvadosCollectionsTest(ArvadosKeepLocalStoreTestCase):
         state = cwriter.current_state()
         # Add an expired locator to the state.
         state['_current_stream_locators'].append(''.join([
-                    'a' * 32, '+A', 'b' * 40, '@', '10000000']))
+                    'a' * 32, '+1+A', 'b' * 40, '@', '10000000']))
         self.assertRaises(arvados.errors.StaleWriterStateError,
                           TestResumableWriter.from_state, state)
 

diff --git a/sdk/python/tests/test_keep_locator.py b/sdk/python/tests/test_keep_locator.py
index e9d635673c97c743cc0e752b72d9f9d1c6b3cc00..a7e9cb1bc344bcbee62f2e3c2fe4ae17d9a861e2 100644 (file)
--- a/sdk/python/tests/test_keep_locator.py
+++ b/sdk/python/tests/test_keep_locator.py
@@ -8,7 +8,7 @@ import unittest
 
 from arvados.keep import KeepLocator
 
-class ArvadosPutResumeCacheTest(unittest.TestCase):
+class ArvadosKeepLocatorTest(unittest.TestCase):
     DEFAULT_TEST_COUNT = 10
 
     def numstrs(fmtstr, base, exponent):
@@ -22,13 +22,17 @@ class ArvadosPutResumeCacheTest(unittest.TestCase):
     signatures = numstrs('{:040x}', 16, 40)
     timestamps = numstrs('{:08x}', 16, 8)
 
+    def base_locators(self, count=DEFAULT_TEST_COUNT):
+        return ('+'.join(pair) for pair in
+                itertools.izip(self.checksums(count), self.sizes(count)))
+
     def perm_hints(self, count=DEFAULT_TEST_COUNT):
         for sig, ts in itertools.izip(self.signatures(count),
                                       self.timestamps(count)):
             yield 'A{}@{}'.format(sig, ts)
 
     def test_good_locators_returned(self):
-        for hint_gens in [(), (self.sizes(),), (self.perm_hints(),),
+        for hint_gens in [(), (self.sizes(),),
                           (self.sizes(), self.perm_hints())]:
             for loc_data in itertools.izip(self.checksums(), *hint_gens):
                 locator = '+'.join(loc_data)
@@ -40,24 +44,36 @@ class ArvadosPutResumeCacheTest(unittest.TestCase):
                        '3+8f9e68d957b504a29ba76c526c3145d9']:
             self.assertRaises(ValueError, KeepLocator, badstr)
 
+    def test_unknown_hints_accepted(self):
+        base = next(self.base_locators(1))
+        for weirdhint in ['Zfoo', 'Ybar234', 'Xa@b_c-372', 'W99']:
+            locator = '+'.join([base, weirdhint])
+            self.assertEquals(locator, str(KeepLocator(locator)))
+
     def test_bad_hints_rejected(self):
-        checksum = next(self.checksums(1))
-        for badhint in ['', 'nonsense', '+32', checksum]:
+        base = next(self.base_locators(1))
+        for badhint in ['', 'A', 'lowercase', '+32']:
             self.assertRaises(ValueError, KeepLocator,
-                              '+'.join([checksum, badhint]))
+                              '+'.join([base, badhint]))
+
+    def test_multiple_locator_hints_accepted(self):
+        base = next(self.base_locators(1))
+        for loc_hints in itertools.permutations(['Kab1cd', 'Kef2gh', 'Kij3kl']):
+            locator = '+'.join((base,) + loc_hints)
+            self.assertEquals(locator, str(KeepLocator(locator)))
 
     def test_expiry_passed(self):
-        checksum = next(self.checksums(1))
+        base = next(self.base_locators(1))
         signature = next(self.signatures(1))
         dt1980 = datetime.datetime(1980, 1, 1)
         dt2000 = datetime.datetime(2000, 2, 2)
         dt2080 = datetime.datetime(2080, 3, 3)
-        locator = KeepLocator(checksum)
+        locator = KeepLocator(base)
         self.assertFalse(locator.permission_expired())
         self.assertFalse(locator.permission_expired(dt1980))
         self.assertFalse(locator.permission_expired(dt2080))
         # Timestamped to 1987-01-05 18:48:32.
-        locator = KeepLocator('{}+A{}@20000000'.format(checksum, signature))
+        locator = KeepLocator('{}+A{}@20000000'.format(base, signature))
         self.assertTrue(locator.permission_expired())
         self.assertTrue(locator.permission_expired(dt2000))
         self.assertFalse(locator.permission_expired(dt1980))