6277: Add more restrictions to manifest format such as cannot start with '/' and...

as well as stream names and files names should not contain '//'.
Added the tests provided by Tom during review.

diff --git a/sdk/ruby/lib/arvados/keep.rb b/sdk/ruby/lib/arvados/keep.rb
index 041058968e04fbb95593901f299469617116a8c3..bcab5fc69909623c33eebdbb6ca9ae333ed724e0 100644 (file)
--- a/sdk/ruby/lib/arvados/keep.rb
+++ b/sdk/ruby/lib/arvados/keep.rb
@@ -97,8 +97,8 @@ module Keep
   end
 
   class Manifest
-    STREAM_REGEXP = /(\.)(\/+.*)*$/
-    FILE_REGEXP = /^[[:digit:]]+:[[:digit:]]+:/
+    STREAM_REGEXP = /(\.)((\/+.*[^\/])*)$/
+    FILE_REGEXP = /^[[:digit:]]+:[[:digit:]]+:(?!\/).*[^\/]$/
 
     # Class to parse a manifest text and provide common views of that data.
     def initialize(manifest_text)
@@ -237,12 +237,14 @@ module Keep
       manifest.each_line do |line|
         line_count += 1
 
-        words = line.split
+        words = line.split(/[[:space:]]/)
+        raise ArgumentError.new "Manifest invalid for stream #{line_count}: missing stream name" if words.empty?
 
         count = 0
+
         word = words.shift
-        count += 1 if word =~ STREAM_REGEXP
-        raise ArgumentError.new "Manifest invalid for stream #{line_count}: missing or invalid stream name #{word.inspect}" if count != 1
+        count += 1 if word =~ STREAM_REGEXP and !word.include? '//'
+        raise ArgumentError.new "Manifest invalid for stream #{line_count}: missing or invalid stream name #{word.inspect if word}" if count != 1
 
         count = 0
         word = words.shift
@@ -250,10 +252,10 @@ module Keep
           word = words.shift
           count += 1
         end
-        raise ArgumentError.new "Manifest invalid for stream #{line_count}: missing or invalid locator #{word.inspect}" if count == 0
+        raise ArgumentError.new "Manifest invalid for stream #{line_count}: missing or invalid locator #{word.inspect if word}" if count == 0
 
         count = 0
-        while word =~ FILE_REGEXP
+        while(word =~ FILE_REGEXP and !word.include? '//')
           word = words.shift
           count += 1
         end

diff --git a/sdk/ruby/test/test_keep_manifest.rb b/sdk/ruby/test/test_keep_manifest.rb
index 3de34f03c21067d5900c600b648c91fb77a91885..18a2b0688f5589733e26d0ac1366d97c502890fa 100644 (file)
--- a/sdk/ruby/test/test_keep_manifest.rb
+++ b/sdk/ruby/test/test_keep_manifest.rb
@@ -281,24 +281,79 @@ class ManifestTest < Minitest::Test
   end
 
   [
-   [false, '. d41d8cd98f00b204e9800998ecf8427e 0:0:abc.txt', "Invalid manifest: does not end with new line"],
-   [false, "abc d41d8cd98f00b204e9800998ecf8427e 0:0:abc.txt\n", "invalid stream name \"abc\""],
-   [false, "d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n", "invalid stream name \"d41d8cd98f00b204e9800998ecf8427e+0\""],
-   [false, ". d41d8cd98f00b204e9800998ecf8427 0:0:abc.txt\n", "invalid locator \"d41d8cd98f00b204e9800998ecf8427\""],
-   [false, ". d41d8cd98f00b204e9800998ecf8427e\n", "Manifest invalid for stream 1: no file tokens"],
-   [true, ". d41d8cd98f00b204e9800998ecf8427e 0:0:abc.txt\n"],
-   [true, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n"],
-   [true, ". d41d8cd98f00b204e9800998ecf8427e a41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n"], # 2 locators
-   [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n/dir1 d41d8cd98f00b204e9800998ecf842 0:0:abc.txt\n",
-    "Manifest invalid for stream 2: missing or invalid stream name \"/dir1\""],
-   [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n./dir1 d41d8cd98f00b204e9800998ecf842 0:0:abc.txt\n",
-    "Manifest invalid for stream 2: missing or invalid locator \"d41d8cd98f00b204e9800998ecf842\""],
-   [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n./dir1 a41d8cd98f00b204e9800998ecf8427e+0 abc.txt\n",
-    "Manifest invalid for stream 2: invalid file token \"abc.txt\""],
-   [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n./dir1 a41d8cd98f00b204e9800998ecf8427e+0 0:abc.txt\n",
-    "Manifest invalid for stream 2: invalid file token \"0:abc.txt\""],
-   [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n./dir1 a41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt xyz.txt\n",
-    "Manifest invalid for stream 2: invalid file token \"xyz.txt\""],
+    [false, '. d41d8cd98f00b204e9800998ecf8427e 0:0:abc.txt',
+      "Invalid manifest: does not end with new line"],
+    [false, "abc d41d8cd98f00b204e9800998ecf8427e 0:0:abc.txt\n",
+      "invalid stream name \"abc\""],
+    [false, "d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n",
+      "invalid stream name \"d41d8cd98f00b204e9800998ecf8427e+0\""],
+    [false, ". d41d8cd98f00b204e9800998ecf8427 0:0:abc.txt\n",
+      "invalid locator \"d41d8cd98f00b204e9800998ecf8427\""],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e\n",
+      "Manifest invalid for stream 1: no file tokens"],
+    [true, ". d41d8cd98f00b204e9800998ecf8427e 0:0:abc.txt\n"],
+    [true, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n"],
+    [true, ". d41d8cd98f00b204e9800998ecf8427e a41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n"], # 2 locators
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n/dir1 d41d8cd98f00b204e9800998ecf842 0:0:abc.txt\n",
+      "Manifest invalid for stream 2: missing or invalid stream name \"/dir1\""],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n./dir1 d41d8cd98f00b204e9800998ecf842 0:0:abc.txt\n",
+      "Manifest invalid for stream 2: missing or invalid locator \"d41d8cd98f00b204e9800998ecf842\""],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n./dir1 a41d8cd98f00b204e9800998ecf8427e+0 abc.txt\n",
+      "Manifest invalid for stream 2: invalid file token \"abc.txt\""],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n./dir1 a41d8cd98f00b204e9800998ecf8427e+0 0:abc.txt\n",
+      "Manifest invalid for stream 2: invalid file token \"0:abc.txt\""],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt\n./dir1 a41d8cd98f00b204e9800998ecf8427e+0 0:0:abc.txt xyz.txt\n",
+      "Manifest invalid for stream 2: invalid file token \"xyz.txt\""],
+    [true, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo/bar.txt\n"],
+    [true, ". d41d8cd98f00b204e9800998ecf8427e+0 000000000000000000000000000000:0777:foo.txt\n"],
+    [true, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:0:0\n"],
+    [true, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:\\040\n"],
+    [true, ". 00000000000000000000000000000000+0 0:0:0\n"],
+    [true, ". 00000000000000000000000000000000+0 0:0:d41d8cd98f00b204e9800998ecf8427e+0+Ad41d8cd98f00b204e9800998ecf8427e00000000@ffffffff\n"],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt d41d8cd98f00b204e9800998ecf8427e+0\n",
+      "Manifest invalid for stream 1: invalid file token \"d41d8cd98f00b204e9800998ecf8427e+0\""],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:\n",
+      "Manifest invalid for stream 1: invalid file token \"0:0:\""],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0\n",
+      "Manifest invalid for stream 1: no file tokens"],
+    [false, ". 0:0:foo.txt d41d8cd98f00b204e9800998ecf8427e+0\n",
+      "Manifest invalid for stream 1: missing or invalid locator \"0:0:foo.txt\""],
+    [false, ". 0:0:foo.txt\n", "Manifest invalid for stream 1: missing or invalid locator \"0:0:foo.txt\""],
+    [false, ".\n", "Manifest invalid for stream 1: missing or invalid locator"],
+    [false, ".", "Invalid manifest: does not end with new line"],
+    [false, ". \n", "Manifest invalid for stream 1: missing or invalid locator"],
+    [false, ".  \n", "Manifest invalid for stream 1: missing or invalid locator"],
+# This does not fail since \t is interpreted as one white space character and hence is split upon
+#    [false, ".\td41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n", "Manifest invalid for stream 1: no file tokens"],
+    [false, " . d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n", "Manifest invalid for stream 1: missing or invalid stream name"],
+# This does not fail since the white space after foo.txt is used to split upon
+#    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt \n", "Manifest invalid for stream 1: no file tokens"],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0  0:0:foo.txt\n",
+      "Manifest invalid for stream 1: invalid file token"],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n \n",
+      "Manifest invalid for stream 2: missing stream name"],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n\n",
+      "Manifest invalid for stream 2: missing stream name"],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n ",
+      "Invalid manifest: does not end with new line"],
+    [false, "\n. d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n",
+      "Manifest invalid for stream 1: missing stream name"],
+    [false, " \n. d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n",
+      "Manifest invalid for stream 1: missing stream name"],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:/foo.txt\n",
+      "Manifest invalid for stream 1: invalid file token \"0:0:/foo.txt\""],
+    [false, "./ d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n",
+      "Manifest invalid for stream 1: missing or invalid stream name \"./\""],
+    [false, ".//foo d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n",
+      "Manifest invalid for stream 1: missing or invalid stream name \".//foo\""],
+    [false, "./foo/ d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n",
+      "Manifest invalid for stream 1: missing or invalid stream name \"./foo/\""],
+    [false, "./foo//bar d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n",
+      "Manifest invalid for stream 1: missing or invalid stream name \"./foo//bar\""],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo//bar.txt\n",
+      "Manifest invalid for stream 1: invalid file token \"0:0:foo//bar.txt\""],
+    [false, ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo/\n",
+      "Manifest invalid for stream 1: invalid file token \"0:0:foo/\""],
   ].each do |ok, manifest, expected_error=nil|
     define_method "test_manifest_valid_#{ok}_#{manifest}_and_expect_error_#{expected_error}" do
       if ok
@@ -310,7 +365,7 @@ class ManifestTest < Minitest::Test
           msg = e.message
         end
         refute_nil msg, "Expected ArgumentError"
-        assert msg.include?(expected_error), "Did not find error message #{expected_error} in #{msg}"
+        assert msg.include?(expected_error), "Did not find expected error message. Expected: #{expected_error}; Actual: #{msg}"
       end
     end
   end