[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