[ARVADOS] updated: d78a90009ca25da4f8cfa75672df0b5bd77cabfe

[git at public.curoverse.com]

Summary of changes:
 services/api/lib/salvage_collection.rb            |  3 +-
 services/api/test/unit/salvage_collection_test.rb | 62 +++++++++++++++++++----
 2 files changed, 52 insertions(+), 13 deletions(-)

       via  d78a90009ca25da4f8cfa75672df0b5bd77cabfe (commit)
      from  54819dd75fb929acfab581890dd1c8ee17cccf3e (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit d78a90009ca25da4f8cfa75672df0b5bd77cabfe
Author: radhika <radhika at curoverse.com>
Date:   Tue Aug 25 15:01:40 2015 -0400

    6859: add test that creates and salvages a collection with invalid manifest text.

diff --git a/services/api/lib/salvage_collection.rb b/services/api/lib/salvage_collection.rb
index 3c9b6df..2ffe0f4 100755
--- a/services/api/lib/salvage_collection.rb
+++ b/services/api/lib/salvage_collection.rb
@@ -19,7 +19,7 @@ module SalvageCollection
     if $?.success?
       new_manifest
     else
-      raise "Error during arv-put."
+      raise "Error during arv-put: #{$?} (cmd was #{cmd.inspect})"
     end
   end
 
@@ -84,7 +84,6 @@ module SalvageCollection
       new_collection = Collection.new
       new_collection.name = "salvaged from #{src_collection.uuid}, #{src_collection.portable_data_hash}"
       new_collection.manifest_text = new_manifest
-      new_collection.portable_data_hash = Digest::MD5.hexdigest(new_collection.manifest_text)
 
       created = new_collection.save!
       raise "New collection creation failed." if !created
diff --git a/services/api/test/unit/salvage_collection_test.rb b/services/api/test/unit/salvage_collection_test.rb
index 4782d01..9a52bb6 100644
--- a/services/api/test/unit/salvage_collection_test.rb
+++ b/services/api/test/unit/salvage_collection_test.rb
@@ -1,8 +1,18 @@
 require 'test_helper'
 require 'salvage_collection'
 
+# Valid manifest_text
 TEST_MANIFEST = ". 341dabea2bd78ad0d6fc3f5b926b450e+85626+Ad391622a17f61e4a254eda85d1ca751c4f368da9 at 55e076ce 0:85626:brca2-hg19.fa\n. d7321a918923627c972d8f8080c07d29+82570+A22e0a1d9b9bc85c848379d98bedc64238b0b1532 at 55e076ce 0:82570:brca1-hg19.fa\n"
 
+# This invalid manifest_text has the following flaws:
+#   Missing stream name with locator in it's place
+#   Two invalid locators:
+#     341dabea2bd78ad0d6fc3f5b926b450e+abc
+#     341dabea2bd78ad0d6fc3f5b926b450e
+# Expectation: These locators are preserved in salvaged_data
+BAD_MANIFEST = "341dabea2bd78ad0d6fc3f5b926b450e+abc 341dabea2bd78ad0d6fc3f5b926abcdf 0:85626:brca2-hg19.fa\n. abcdabea2bd78ad0d6fc3f5b926b450e+1000 0:1000:brca-hg19.fa\n . d7321a918923627c972d8f8080c07d29+2000+A22e0a1d9b9bc85c848379d98bedc64238b0b1532 at 55e076ce 0:2000:brca1-hg19.fa\n"
+
+# Mock arv_put
 module SalvageCollection
   def self.salvage_collection_arv_put(cmd)
     file_contents = File.new(cmd.split[-1], "r").gets
@@ -12,13 +22,13 @@ module SalvageCollection
       raise("Error during arv-put")
     else
       ". " +
-      Digest::MD5.hexdigest(TEST_MANIFEST) +
+      Digest::MD5.hexdigest(TEST_MANIFEST) + "+" + TEST_MANIFEST.length.to_s +
       " 0:" + TEST_MANIFEST.length.to_s + ":invalid_manifest_text.txt\n"
     end
   end
 end
 
-class SalvageCollectionMockTest < ActiveSupport::TestCase
+class SalvageCollectionTest < ActiveSupport::TestCase
   include SalvageCollection
 
   setup do
@@ -33,7 +43,7 @@ class SalvageCollectionMockTest < ActiveSupport::TestCase
     ENV['ARVADOS_API_TOKEN'] = ''
   end
 
-  test "salvage test collection" do
+  test "salvage test collection with valid manifest text" do
     # create a collection to test salvaging
     src_collection = Collection.new name: "test collection", manifest_text: TEST_MANIFEST
     src_collection.save!
@@ -93,21 +103,51 @@ class SalvageCollectionMockTest < ActiveSupport::TestCase
     assert_equal "Error during arv-put", e.message
   end
 
-  # This test has two invalid locators:
+  # This test uses BAD_MANIFEST, which has the following flaws:
+  #   Missing stream name with locator in it's place
+  #   Two invalid locators:
   #     341dabea2bd78ad0d6fc3f5b926b450e+abc
   #     341dabea2bd78ad0d6fc3f5b926b450e
-  # These locators should be preserved in salvaged_data
-  test "invalid locators dropped during salvaging" do
-    manifest = ". 341dabea2bd78ad0d6fc3f5b926b450e+abc 341dabea2bd78ad0d6fc3f5b926abcdf 0:85626:brca2-hg19.fa\n. 341dabea2bd78ad0d6fc3f5b926b450e+1000 0:1000:brca-hg19.fa\n . d7321a918923627c972d8f8080c07d29+2000+A22e0a1d9b9bc85c848379d98bedc64238b0b1532 at 55e076ce 0:2000:brca1-hg19.fa\n"
-
-    # salvage this collection
-    locator_data = SalvageCollection.salvage_collection_locator_data manifest
+  # Expectation: These locators are preserved in salvaged_data
+  test "invalid locators preserved during salvaging" do
+    locator_data = SalvageCollection.salvage_collection_locator_data BAD_MANIFEST
     assert_equal true, locator_data[0].size.eql?(4)
     assert_equal false, locator_data[0].include?("341dabea2bd78ad0d6fc3f5b926b450e+abc")
     assert_equal true, locator_data[0].include?("341dabea2bd78ad0d6fc3f5b926b450e")
     assert_equal true, locator_data[0].include?("341dabea2bd78ad0d6fc3f5b926abcdf")
-    assert_equal true, locator_data[0].include?("341dabea2bd78ad0d6fc3f5b926b450e+1000")
+    assert_equal true, locator_data[0].include?("abcdabea2bd78ad0d6fc3f5b926b450e+1000")
     assert_equal true, locator_data[0].include?("d7321a918923627c972d8f8080c07d29+2000")
     assert_equal true, locator_data[1].eql?(1000 + 2000)   # size
   end
+
+  test "salvage a collection with invalid manifest text" do
+    # create a collection to test salvaging
+    src_collection = Collection.new name: "test collection", manifest_text: BAD_MANIFEST, owner_uuid: 'zzzzz-tpzed-000000000000000'
+    src_collection.save!(validate: false)
+
+    # salvage this collection
+    SalvageCollection.salvage_collection src_collection.uuid, 'test salvage collection - see #6277, #6859'
+
+    # verify the updated src_collection data
+    updated_src_collection = Collection.find_by_uuid src_collection.uuid
+    updated_name = updated_src_collection.name
+    assert_equal true, updated_name.include?(src_collection.name)
+
+    match = updated_name.match /^test collection.*salvaged data at (.*)\)$/
+    assert_not_nil match
+    assert_not_nil match[1]
+    assert_empty updated_src_collection.manifest_text
+
+    # match[1] is the uuid of the new collection created from src_collection's salvaged data
+    # use this to get the new collection and verify
+    new_collection = Collection.find_by_uuid match[1]
+    match = new_collection.name.match /^salvaged from (.*),.*/
+    assert_not_nil match
+    assert_equal src_collection.uuid, match[1]
+
+    # verify the new collection's manifest includes the bad locators
+    match = new_collection.manifest_text.match /^. (.*) (.*):invalid_manifest_text.txt\n. (.*) (.*):salvaged_data/
+    assert_not_nil match
+    assert_includes(new_collection.manifest_text, ". 341dabea2bd78ad0d6fc3f5b926b450e 341dabea2bd78ad0d6fc3f5b926abcdf abcdabea2bd78ad0d6fc3f5b926b450e+1000 d7321a918923627c972d8f8080c07d29+2000 0:3000:salvaged_data")
+  end
 end

-----------------------------------------------------------------------


hooks/post-receive
--