[ARVADOS] updated: b48b26080b88f5ebf1caedb5e29bbd509d8427af
git at public.curoverse.com
git at public.curoverse.com
Mon Aug 18 13:09:19 EDT 2014
Summary of changes:
services/api/app/models/arvados_model.rb | 7 ++++++-
services/api/app/models/group.rb | 9 +++++++++
...125735_add_not_null_constraint_to_group_name.rb | 6 ++++++
services/api/db/structure.sql | 6 ++++--
services/api/test/fixtures/groups.yml | 11 +++++------
services/api/test/fixtures/links.yml | 4 ++--
.../arvados/v1/collections_controller_test.rb | 22 +++++++++++++++++++---
.../functional/arvados/v1/links_controller_test.rb | 15 ---------------
services/api/test/unit/group_test.rb | 14 ++++++--------
services/api/test/unit/owner_test.rb | 16 ++++++++--------
10 files changed, 65 insertions(+), 45 deletions(-)
create mode 100644 services/api/db/migrate/20140818125735_add_not_null_constraint_to_group_name.rb
via b48b26080b88f5ebf1caedb5e29bbd509d8427af (commit)
from 49ca83c5f0d6f17ca6fac5f6082fdcb4f0cec036 (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 b48b26080b88f5ebf1caedb5e29bbd509d8427af
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Mon Aug 18 13:09:14 2014 -0400
3036: API server unit tests pass again after adding uniqueness constraints.
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index c5a42ae..1067447 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -21,8 +21,8 @@ class ArvadosModel < ActiveRecord::Base
after_update :log_update
after_destroy :log_destroy
after_find :convert_serialized_symbols_to_strings
+ before_validation :normalize_collection_uuids
validate :ensure_serialized_attribute_type
- validate :normalize_collection_uuids
validate :ensure_valid_uuids
# Note: This only returns permission links. It does not account for
@@ -211,6 +211,11 @@ class ArvadosModel < ActiveRecord::Base
self.owner_uuid ||= current_user.uuid
end
+ if self.owner_uuid.nil?
+ errors.add :owner_uuid, "cannot be nil"
+ raise PermissionDeniedError
+ end
+
rsc_class = ArvadosModel::resource_class_for_uuid owner_uuid
unless rsc_class == User or rsc_class == Group
errors.add :owner_uuid, "can only be set to User or Group"
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index d380244..0e857ad 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -7,6 +7,7 @@ class Group < ArvadosModel
include CanBeAnOwner
after_create :invalidate_permissions_cache
after_update :maybe_invalidate_permissions_cache
+ before_create :assign_name
api_accessible :user, extend: :common do |t|
t.add :name
@@ -28,4 +29,12 @@ class Group < ArvadosModel
# immediately after being created.
User.invalidate_permissions_cache
end
+
+ def assign_name
+ if self.new_record? and (self.name.nil? or self.name.empty?)
+ self.name = self.uuid
+ end
+ true
+ end
+
end
diff --git a/services/api/db/migrate/20140818125735_add_not_null_constraint_to_group_name.rb b/services/api/db/migrate/20140818125735_add_not_null_constraint_to_group_name.rb
new file mode 100644
index 0000000..1b07470
--- /dev/null
+++ b/services/api/db/migrate/20140818125735_add_not_null_constraint_to_group_name.rb
@@ -0,0 +1,6 @@
+class AddNotNullConstraintToGroupName < ActiveRecord::Migration
+ def change
+ ActiveRecord::Base.connection.execute("update groups set name=uuid where name is null or name=''")
+ change_column_null :groups, :name, false
+ end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index a71bf8a..a00e6cd 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -271,7 +271,7 @@ CREATE TABLE groups (
modified_by_client_uuid character varying(255),
modified_by_user_uuid character varying(255),
modified_at timestamp without time zone,
- name character varying(255),
+ name character varying(255) NOT NULL,
description text,
updated_at timestamp without time zone NOT NULL,
group_class character varying(255)
@@ -2022,4 +2022,6 @@ INSERT INTO schema_migrations (version) VALUES ('20140811184643');
INSERT INTO schema_migrations (version) VALUES ('20140815171049');
-INSERT INTO schema_migrations (version) VALUES ('20140817035914');
\ No newline at end of file
+INSERT INTO schema_migrations (version) VALUES ('20140817035914');
+
+INSERT INTO schema_migrations (version) VALUES ('20140818125735');
\ No newline at end of file
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index c4ed133..34adfd5 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -16,12 +16,6 @@ private_and_can_read_foofile:
name: Private and Can Read Foofile
description: Another Private Group
-system_owned_group:
- uuid: zzzzz-j7d0g-8ulrifv67tve5sx
- owner_uuid: zzzzz-tpzed-000000000000000
- name: System Private
- description: System-owned Group
-
system_group:
uuid: zzzzz-j7d0g-000000000000000
owner_uuid: zzzzz-tpzed-000000000000000
@@ -111,3 +105,8 @@ anonymously_accessible_project:
name: Unrestricted public data
group_class: project
description: An anonymously accessible project
+
+active_user_has_can_manage:
+ uuid: zzzzz-j7d0g-ptt1ou6a9lxrv07
+ owner_uuid: zzzzz-tpzed-d9tiejq69daie8f
+ name: Active user has can_manage
diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index ea7527e..3a0b063 100644
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -40,7 +40,7 @@ active_user_member_of_all_users_group:
head_uuid: zzzzz-j7d0g-fffffffffffffff
properties: {}
-active_user_can_manage_system_owned_group:
+active_user_can_manage_group:
uuid: zzzzz-o0j2j-3sa30nd3bqn1msh
owner_uuid: zzzzz-tpzed-000000000000000
created_at: 2014-02-03 15:42:26 -0800
@@ -51,7 +51,7 @@ active_user_can_manage_system_owned_group:
tail_uuid: zzzzz-tpzed-xurymjxw79nv3jz
link_class: permission
name: can_manage
- head_uuid: zzzzz-j7d0g-8ulrifv67tve5sx
+ head_uuid: zzzzz-j7d0g-ptt1ou6a9lxrv07
properties: {}
user_agreement_signed_by_active:
diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb
index 30559a5..4f99dd2 100644
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -136,23 +136,38 @@ EOS
assert_equal 'zzzzz-j7d0g-rew6elm53kancon', resp['owner_uuid']
end
+ test "create fails with duplicate name" do
+ permit_unsigned_manifests
+ authorize_with :admin
+ manifest_text = ". d41d8cd98f00b204e9800998ecf8427e 0:0:foo.txt\n"
+ post :create, {
+ collection: {
+ owner_uuid: 'zzzzz-tpzed-000000000000000',
+ manifest_text: manifest_text,
+ portable_data_hash: "d30fe8ae534397864cb96c544f4cf102+47",
+ name: "foo_file"
+ }
+ }
+ assert_response 422
+ end
+
test "create with owner_uuid set to group i can_manage" do
permit_unsigned_manifests
authorize_with :active
manifest_text = ". d41d8cd98f00b204e9800998ecf8427e 0:0:foo.txt\n"
post :create, {
collection: {
- owner_uuid: groups(:system_owned_group).uuid,
+ owner_uuid: groups(:active_user_has_can_manage).uuid,
manifest_text: manifest_text,
portable_data_hash: "d30fe8ae534397864cb96c544f4cf102+47"
}
}
assert_response :success
resp = JSON.parse(@response.body)
- assert_equal 'zzzzz-j7d0g-8ulrifv67tve5sx', resp['owner_uuid']
+ assert_equal groups(:active_user_has_can_manage).uuid, resp['owner_uuid']
end
- test "create with owner_uuid fails on group with can_read permission" do
+ test "create with owner_uuid fails on group with only can_read permission" do
permit_unsigned_manifests
authorize_with :active
manifest_text = ". d41d8cd98f00b204e9800998ecf8427e 0:0:foo.txt\n"
@@ -531,4 +546,5 @@ EOS
assert_empty Collection.where('uuid like ?', manifest_uuid+'%'),
"Collection should not exist in database after failed create"
end
+
end
diff --git a/services/api/test/functional/arvados/v1/links_controller_test.rb b/services/api/test/functional/arvados/v1/links_controller_test.rb
index ac33776..faeaae0 100644
--- a/services/api/test/functional/arvados/v1/links_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/links_controller_test.rb
@@ -270,21 +270,6 @@ class Arvados::V1::LinksControllerTest < ActionController::TestCase
assert_response :success
end
- test "refuse duplicate name" do
- skip "Fix for uniqueness constraints"
- the_name = links(:job_name_in_aproject).name
- the_project = links(:job_name_in_aproject).tail_uuid
- authorize_with :active
- post :create, link: {
- tail_uuid: the_project,
- head_uuid: specimens(:owned_by_active_user).uuid,
- link_class: 'name',
- name: the_name,
- properties: {this_s: "a duplicate name"}
- }
- assert_response 422
- end
-
test "project owner can show a project permission" do
uuid = links(:project_viewer_can_read_project).uuid
authorize_with :active
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 97977a5..8c40d3a 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -14,7 +14,8 @@ class GroupTest < ActiveSupport::TestCase
# Use the group as the owner of a new object
s = Specimen.
create(owner_uuid: groups(:bad_group_has_ownership_cycle_b).uuid)
- assert s.valid?, "ownership should pass validation"
+ puts s.errors.messages
+ assert s.valid?, "ownership should pass validation #{s.errors.messages}"
assert_equal false, s.save, "should not save object with #{g.uuid} as owner"
# Use the group as the new owner of an existing object
@@ -27,11 +28,8 @@ class GroupTest < ActiveSupport::TestCase
test "cannot create a new ownership cycle" do
set_user_from_auth :active_trustedclient
- g_foo = Group.create(name: "foo")
- g_foo.save!
-
- g_bar = Group.create(name: "bar")
- g_bar.save!
+ g_foo = Group.create!(name: "foo")
+ g_bar = Group.create!(name: "bar")
g_foo.owner_uuid = g_bar.uuid
assert g_foo.save, lambda { g_foo.errors.messages }
@@ -44,11 +42,11 @@ class GroupTest < ActiveSupport::TestCase
test "cannot create a single-object ownership cycle" do
set_user_from_auth :active_trustedclient
- g_foo = Group.create(name: "foo")
+ g_foo = Group.create!(name: "foo")
assert g_foo.save
# Ensure I have permission to manage this group even when its owner changes
- perm_link = Link.create(tail_uuid: users(:active).uuid,
+ perm_link = Link.create!(tail_uuid: users(:active).uuid,
head_uuid: g_foo.uuid,
link_class: 'permission',
name: 'can_manage')
diff --git a/services/api/test/unit/owner_test.rb b/services/api/test/unit/owner_test.rb
index c177bc3..c7f9776 100644
--- a/services/api/test/unit/owner_test.rb
+++ b/services/api/test/unit/owner_test.rb
@@ -17,7 +17,7 @@ class OwnerTest < ActiveSupport::TestCase
Group.all
[User, Group].each do |o_class|
test "create object with legit #{o_class} owner" do
- o = o_class.create
+ o = o_class.create!
i = Specimen.create(owner_uuid: o.uuid)
assert i.valid?, "new item should pass validation"
assert i.uuid, "new item should have an ID"
@@ -40,9 +40,9 @@ class OwnerTest < ActiveSupport::TestCase
[User, Group].each do |new_o_class|
test "change owner from legit #{o_class} to legit #{new_o_class} owner" do
- o = o_class.create
- i = Specimen.create(owner_uuid: o.uuid)
- new_o = new_o_class.create
+ o = o_class.create!
+ i = Specimen.create!(owner_uuid: o.uuid)
+ new_o = new_o_class.create!
assert(Specimen.where(uuid: i.uuid).any?,
"new item should really be in DB")
assert(i.update_attributes(owner_uuid: new_o.uuid),
@@ -51,7 +51,7 @@ class OwnerTest < ActiveSupport::TestCase
end
test "delete #{o_class} that owns nothing" do
- o = o_class.create
+ o = o_class.create!
assert(o_class.where(uuid: o.uuid).any?,
"new #{o_class} should really be in DB")
assert(o.destroy, "should delete #{o_class} that owns nothing")
@@ -61,7 +61,7 @@ class OwnerTest < ActiveSupport::TestCase
test "change uuid of #{o_class} that owns nothing" do
# (we're relying on our admin credentials here)
- o = o_class.create
+ o = o_class.create!
assert(o_class.where(uuid: o.uuid).any?,
"new #{o_class} should really be in DB")
old_uuid = o.uuid
@@ -97,7 +97,7 @@ class OwnerTest < ActiveSupport::TestCase
end
test "delete User that owns self" do
- o = User.create
+ o = User.create!
assert User.where(uuid: o.uuid).any?, "new User should really be in DB"
assert_equal(true, o.update_attributes(owner_uuid: o.uuid),
"setting owner to self should work")
@@ -107,7 +107,7 @@ class OwnerTest < ActiveSupport::TestCase
end
test "change uuid of User that owns self" do
- o = User.create
+ o = User.create!
assert User.where(uuid: o.uuid).any?, "new User should really be in DB"
assert_equal(true, o.update_attributes(owner_uuid: o.uuid),
"setting owner to self should work")
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list