[ARVADOS] created: 1.1.3-86-g281fe0c

Git user git at public.curoverse.com
Mon Feb 26 09:30:05 EST 2018


        at  281fe0c40c16aacd82cd1ca122e38aa3a3854f2e (commit)


commit 281fe0c40c16aacd82cd1ca122e38aa3a3854f2e
Author: Lucas Di Pentima <ldipentima at veritasgenetics.com>
Date:   Fri Feb 23 15:09:30 2018 -0300

    12707: Some fixes and additions on storage classes:
    
    * Remove the unnecessary default values from model.
    * Fixes and tests on storage classes columns.
    * Adds tests for 'exists' filter on jsonb array columns.
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <ldipentima at veritasgenetics.com>

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 6018a27..221176e 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -19,13 +19,13 @@ class Collection < ArvadosModel
   serialize :storage_classes_confirmed, Array
 
   before_validation :default_empty_manifest
-  before_validation :default_storage_classes, on: :create
   before_validation :check_encoding
   before_validation :check_manifest_validity
   before_validation :check_signatures
   before_validation :strip_signatures_and_update_replication_confirmed
   validate :ensure_pdh_matches_manifest_text
   validate :ensure_storage_classes_desired_is_not_empty
+  validate :ensure_storage_classes_contain_non_empty_strings
   before_save :set_file_names
 
   api_accessible :user, extend: :common do |t|
@@ -453,13 +453,6 @@ class Collection < ArvadosModel
 
   protected
 
-  def default_storage_classes
-    if self.storage_classes_desired.nil? || self.storage_classes_desired.empty?
-      self.storage_classes_desired = ["default"]
-    end
-    self.storage_classes_confirmed ||= []
-  end
-
   def portable_manifest_text
     self.class.munge_manifest_locators(manifest_text) do |match|
       if match[2] # size
@@ -502,7 +495,15 @@ class Collection < ArvadosModel
 
   def ensure_storage_classes_desired_is_not_empty
     if self.storage_classes_desired.empty?
-      raise ArvadosModel::PermissionDeniedError.new("storage_classes_desired shouldn't be empty")
+      raise ArvadosModel::InvalidStateTransitionError.new("storage_classes_desired shouldn't be empty")
+    end
+  end
+
+  def ensure_storage_classes_contain_non_empty_strings
+    (self.storage_classes_desired + self.storage_classes_confirmed).each do |c|
+      if !c.is_a?(String) || c == ''
+        raise ArvadosModel::InvalidStateTransitionError.new("storage classes should only be non-empty strings")
+      end
     end
   end
 end
diff --git a/services/api/test/fixtures/collections.yml b/services/api/test/fixtures/collections.yml
index 57a0381..807047e 100644
--- a/services/api/test/fixtures/collections.yml
+++ b/services/api/test/fixtures/collections.yml
@@ -564,6 +564,20 @@ storage_classes_desired_default_confirmed_default:
   manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:bar\n"
   name: storage classes want=[default] have=[default]
 
+storage_classes_desired_archive_confirmed_default:
+  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  created_at: 2015-02-07 00:21:35.050333515 Z
+  modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  modified_at: 2015-02-07 00:21:35.050189104 Z
+  portable_data_hash: fa7aeb5140e2848d39b416daeef4ffc5+45
+  storage_classes_desired: ["archive"]
+  storage_classes_confirmed_at: ~
+  storage_classes_confirmed: ["default"]
+  updated_at: 2015-02-07 00:21:35.050126576 Z
+  uuid: zzzzz-4zz18-3t236wr12769qqa
+  manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:bar\n"
+  name: storage classes want=[archive] have=[default]
+
 collection_with_empty_properties:
   uuid: zzzzz-4zz18-emptyproperties
   portable_data_hash: fa7aeb5140e2848d39b416daeef4ffc5+45
diff --git a/services/api/test/functional/arvados/v1/filters_test.rb b/services/api/test/functional/arvados/v1/filters_test.rb
index ef120b1..c76b94e 100644
--- a/services/api/test/functional/arvados/v1/filters_test.rb
+++ b/services/api/test/functional/arvados/v1/filters_test.rb
@@ -193,7 +193,7 @@ class Arvados::V1::FiltersTest < ActionController::TestCase
     end
   end
 
-  test "jsonb 'exists' and '!=' filter" do
+  test "jsonb hash 'exists' and '!=' filter" do
     @controller = Arvados::V1::CollectionsController.new
     authorize_with :admin
     get :index, {
@@ -208,7 +208,24 @@ class Arvados::V1::FiltersTest < ActionController::TestCase
     assert_includes(found, collections(:collection_with_prop1_other1).uuid)
   end
 
-  test "jsonb alternate form 'exists' and '!=' filter" do
+  test "jsonb array 'exists'" do
+    @controller = Arvados::V1::CollectionsController.new
+    authorize_with :admin
+    get :index, {
+      filters: [ ['storage_classes_confirmed.default', 'exists', true] ]
+    }
+    assert_response :success
+    found = assigns(:objects).collect(&:uuid)
+    assert_equal 2, found.length
+    assert_not_includes(found,
+      collections(:storage_classes_desired_default_unconfirmed).uuid)
+    assert_includes(found,
+      collections(:storage_classes_desired_default_confirmed_default).uuid)
+    assert_includes(found,
+      collections(:storage_classes_desired_archive_confirmed_default).uuid)
+  end
+
+  test "jsonb hash alternate form 'exists' and '!=' filter" do
     @controller = Arvados::V1::CollectionsController.new
     authorize_with :admin
     get :index, {
@@ -223,6 +240,23 @@ class Arvados::V1::FiltersTest < ActionController::TestCase
     assert_includes(found, collections(:collection_with_prop1_other1).uuid)
   end
 
+  test "jsonb array alternate form 'exists' filter" do
+    @controller = Arvados::V1::CollectionsController.new
+    authorize_with :admin
+    get :index, {
+      filters: [ ['storage_classes_confirmed', 'exists', 'default'] ]
+    }
+    assert_response :success
+    found = assigns(:objects).collect(&:uuid)
+    assert_equal 2, found.length
+    assert_not_includes(found,
+      collections(:storage_classes_desired_default_unconfirmed).uuid)
+    assert_includes(found,
+      collections(:storage_classes_desired_default_confirmed_default).uuid)
+    assert_includes(found,
+      collections(:storage_classes_desired_archive_confirmed_default).uuid)
+  end
+
   test "jsonb 'exists' must be boolean" do
     @controller = Arvados::V1::CollectionsController.new
     authorize_with :admin
diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index 9c4c758..d425bc6 100644
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -226,12 +226,30 @@ class CollectionTest < ActiveSupport::TestCase
       c = collections(:collection_owned_by_active)
       c.update_attributes storage_classes_desired: ["hot"]
       assert_equal ["hot"], c.storage_classes_desired
-      assert_raise ArvadosModel::PermissionDeniedError do
+      assert_raise ArvadosModel::InvalidStateTransitionError do
         c.update_attributes storage_classes_desired: []
       end
     end
   end
 
+  test "storage classes lists should only contain non-empty strings" do
+    c = collections(:storage_classes_desired_default_unconfirmed)
+    act_as_user users(:admin) do
+      assert c.update_attributes(storage_classes_desired: ["default", "a_string"],
+                                 storage_classes_confirmed: ["another_string"])
+      [
+        ["storage_classes_desired", ["default", 42]],
+        ["storage_classes_confirmed", [{the_answer: 42}]],
+        ["storage_classes_desired", ["default", ""]],
+        ["storage_classes_confirmed", [""]],
+      ].each do |attr, val|
+        assert_raise ArvadosModel::InvalidStateTransitionError do
+          assert c.update_attributes({attr => val})
+        end
+      end
+    end
+  end
+
   test "storage_classes_confirmed* can be set by admin user" do
     c = collections(:storage_classes_desired_default_unconfirmed)
     act_as_user users(:admin) do

commit b0d7f37097e8b5c56bfbfa79a134a8074789ebb4
Author: Lucas Di Pentima <ldipentima at veritasgenetics.com>
Date:   Fri Feb 16 20:47:49 2018 -0300

    12707: Add storage classes attributes to collections.
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <ldipentima at veritasgenetics.com>

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index c5fd96e..6018a27 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -15,13 +15,17 @@ class Collection < ArvadosModel
   include Trashable
 
   serialize :properties, Hash
+  serialize :storage_classes_desired, Array
+  serialize :storage_classes_confirmed, Array
 
   before_validation :default_empty_manifest
+  before_validation :default_storage_classes, on: :create
   before_validation :check_encoding
   before_validation :check_manifest_validity
   before_validation :check_signatures
   before_validation :strip_signatures_and_update_replication_confirmed
   validate :ensure_pdh_matches_manifest_text
+  validate :ensure_storage_classes_desired_is_not_empty
   before_save :set_file_names
 
   api_accessible :user, extend: :common do |t|
@@ -34,6 +38,9 @@ class Collection < ArvadosModel
     t.add :replication_desired
     t.add :replication_confirmed
     t.add :replication_confirmed_at
+    t.add :storage_classes_desired
+    t.add :storage_classes_confirmed
+    t.add :storage_classes_confirmed_at
     t.add :delete_at
     t.add :trash_at
     t.add :is_trashed
@@ -436,7 +443,7 @@ class Collection < ArvadosModel
   end
 
   def self.full_text_searchable_columns
-    super - ["manifest_text"]
+    super - ["manifest_text", "storage_classes_desired", "storage_classes_confirmed"]
   end
 
   def self.where *args
@@ -445,6 +452,14 @@ class Collection < ArvadosModel
   end
 
   protected
+
+  def default_storage_classes
+    if self.storage_classes_desired.nil? || self.storage_classes_desired.empty?
+      self.storage_classes_desired = ["default"]
+    end
+    self.storage_classes_confirmed ||= []
+  end
+
   def portable_manifest_text
     self.class.munge_manifest_locators(manifest_text) do |match|
       if match[2] # size
@@ -472,12 +487,22 @@ class Collection < ArvadosModel
   end
 
   def ensure_permission_to_save
-    if (not current_user.andand.is_admin and
-        (replication_confirmed_at_changed? or replication_confirmed_changed?) and
-        not (replication_confirmed_at.nil? and replication_confirmed.nil?))
-      raise ArvadosModel::PermissionDeniedError.new("replication_confirmed and replication_confirmed_at attributes cannot be changed, except by setting both to nil")
+    if (not current_user.andand.is_admin)
+      if (replication_confirmed_at_changed? or replication_confirmed_changed?) and
+        not (replication_confirmed_at.nil? and replication_confirmed.nil?)
+        raise ArvadosModel::PermissionDeniedError.new("replication_confirmed and replication_confirmed_at attributes cannot be changed, except by setting both to nil")
+      end
+      if (storage_classes_confirmed_changed? or storage_classes_confirmed_at_changed?) and
+        not (storage_classes_confirmed == [] and storage_classes_confirmed_at.nil?)
+        raise ArvadosModel::PermissionDeniedError.new("storage_classes_confirmed and storage_classes_confirmed_at attributes cannot be changed, except by setting them to [] and nil respectively")
+      end
     end
     super
   end
 
+  def ensure_storage_classes_desired_is_not_empty
+    if self.storage_classes_desired.empty?
+      raise ArvadosModel::PermissionDeniedError.new("storage_classes_desired shouldn't be empty")
+    end
+  end
 end
diff --git a/services/api/db/migrate/20180216203422_add_storage_classes_to_collections.rb b/services/api/db/migrate/20180216203422_add_storage_classes_to_collections.rb
new file mode 100644
index 0000000..112c2ba
--- /dev/null
+++ b/services/api/db/migrate/20180216203422_add_storage_classes_to_collections.rb
@@ -0,0 +1,17 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+class AddStorageClassesToCollections < ActiveRecord::Migration
+  def up
+    add_column :collections, :storage_classes_desired, :jsonb, :default => ["default"]
+    add_column :collections, :storage_classes_confirmed, :jsonb, :default => []
+    add_column :collections, :storage_classes_confirmed_at, :datetime, :default => nil, :null => true
+  end
+
+  def down
+    remove_column :collections, :storage_classes_desired
+    remove_column :collections, :storage_classes_confirmed
+    remove_column :collections, :storage_classes_confirmed_at
+  end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 14729d3..357e95c 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -170,7 +170,10 @@ CREATE TABLE collections (
     delete_at timestamp without time zone,
     file_names character varying(8192),
     trash_at timestamp without time zone,
-    is_trashed boolean DEFAULT false NOT NULL
+    is_trashed boolean DEFAULT false NOT NULL,
+    storage_classes_desired jsonb DEFAULT '["default"]'::jsonb,
+    storage_classes_confirmed jsonb DEFAULT '[]'::jsonb,
+    storage_classes_confirmed_at timestamp without time zone
 );
 
 
@@ -3049,3 +3052,5 @@ INSERT INTO schema_migrations (version) VALUES ('20171208203841');
 
 INSERT INTO schema_migrations (version) VALUES ('20171212153352');
 
+INSERT INTO schema_migrations (version) VALUES ('20180216203422');
+
diff --git a/services/api/test/fixtures/collections.yml b/services/api/test/fixtures/collections.yml
index ea87cca..57a0381 100644
--- a/services/api/test/fixtures/collections.yml
+++ b/services/api/test/fixtures/collections.yml
@@ -536,6 +536,34 @@ replication_desired_2_confirmed_2:
   manifest_text: ". acbd18db4cc2f85cedef654fccc4a4d8+3 37b51d194a7513e45b56f6524f2d51f2+3 0:3:foo 3:6:bar\n"
   name: replication want=2 have=2
 
+storage_classes_desired_default_unconfirmed:
+  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  created_at: 2015-02-07 00:21:35.050333515 Z
+  modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  modified_at: 2015-02-07 00:21:35.050189104 Z
+  portable_data_hash: fa7aeb5140e2848d39b416daeef4ffc5+45
+  storage_classes_desired: ["default"]
+  storage_classes_confirmed_at: ~
+  storage_classes_confirmed: ~
+  updated_at: 2015-02-07 00:21:35.050126576 Z
+  uuid: zzzzz-4zz18-3t236wrz4769tga
+  manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:bar\n"
+  name: storage classes want=[default] have=[]
+
+storage_classes_desired_default_confirmed_default:
+  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  created_at: 2015-02-07 00:21:35.050333515 Z
+  modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  modified_at: 2015-02-07 00:21:35.050189104 Z
+  portable_data_hash: fa7aeb5140e2848d39b416daeef4ffc5+45
+  storage_classes_desired: ["default"]
+  storage_classes_confirmed_at: 2015-02-07 00:21:35.050126576 Z
+  storage_classes_confirmed: ["default"]
+  updated_at: 2015-02-07 00:21:35.050126576 Z
+  uuid: zzzzz-4zz18-3t236wr12769tga
+  manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:bar\n"
+  name: storage classes want=[default] have=[default]
+
 collection_with_empty_properties:
   uuid: zzzzz-4zz18-emptyproperties
   portable_data_hash: fa7aeb5140e2848d39b416daeef4ffc5+45
diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index 62e3755..9c4c758 100644
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -221,6 +221,63 @@ class CollectionTest < ActiveSupport::TestCase
     end
   end
 
+  test "storage_classes_desired cannot be empty" do
+    act_as_user users(:active) do
+      c = collections(:collection_owned_by_active)
+      c.update_attributes storage_classes_desired: ["hot"]
+      assert_equal ["hot"], c.storage_classes_desired
+      assert_raise ArvadosModel::PermissionDeniedError do
+        c.update_attributes storage_classes_desired: []
+      end
+    end
+  end
+
+  test "storage_classes_confirmed* can be set by admin user" do
+    c = collections(:storage_classes_desired_default_unconfirmed)
+    act_as_user users(:admin) do
+      assert c.update_attributes(storage_classes_confirmed: ["default"],
+                                 storage_classes_confirmed_at: Time.now)
+    end
+  end
+
+  test "storage_classes_confirmed* cannot be set by non-admin user" do
+    act_as_user users(:active) do
+      c = collections(:storage_classes_desired_default_unconfirmed)
+      # Cannot set just one at a time.
+      assert_raise ArvadosModel::PermissionDeniedError do
+        c.update_attributes storage_classes_confirmed: ["default"]
+      end
+      c.reload
+      assert_raise ArvadosModel::PermissionDeniedError do
+        c.update_attributes storage_classes_confirmed_at: Time.now
+      end
+      # Cannot set bot at once, either.
+      c.reload
+      assert_raise ArvadosModel::PermissionDeniedError do
+        assert c.update_attributes(storage_classes_confirmed: ["default"],
+                                   storage_classes_confirmed_at: Time.now)
+      end
+    end
+  end
+
+  test "storage_classes_confirmed* can be cleared (but only together) by non-admin user" do
+    act_as_user users(:active) do
+      c = collections(:storage_classes_desired_default_confirmed_default)
+      # Cannot clear just one at a time.
+      assert_raise ArvadosModel::PermissionDeniedError do
+        c.update_attributes storage_classes_confirmed: []
+      end
+      c.reload
+      assert_raise ArvadosModel::PermissionDeniedError do
+        c.update_attributes storage_classes_confirmed_at: nil
+      end
+      # Can clear both at once.
+      c.reload
+      assert c.update_attributes(storage_classes_confirmed: [],
+                                 storage_classes_confirmed_at: nil)
+    end
+  end
+
   [0, 2, 4, nil].each do |ask|
     test "set replication_desired to #{ask.inspect}" do
       Rails.configuration.default_collection_replication = 2

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


hooks/post-receive
-- 




More information about the arvados-commits mailing list