3036: API server unit tests pass again after adding uniqueness constraints.
authorPeter Amstutz <peter.amstutz@curoverse.com>
Mon, 18 Aug 2014 17:09:14 +0000 (13:09 -0400)
committerPeter Amstutz <peter.amstutz@curoverse.com>
Mon, 18 Aug 2014 17:09:14 +0000 (13:09 -0400)
services/api/app/models/arvados_model.rb
services/api/app/models/group.rb
services/api/db/migrate/20140818125735_add_not_null_constraint_to_group_name.rb [new file with mode: 0644]
services/api/db/structure.sql
services/api/test/fixtures/groups.yml
services/api/test/fixtures/links.yml
services/api/test/functional/arvados/v1/collections_controller_test.rb
services/api/test/functional/arvados/v1/links_controller_test.rb
services/api/test/unit/group_test.rb
services/api/test/unit/owner_test.rb

index c5a42ae7f628722492ca13f371afa811e95f0eba..10674477f881a0f1bedce5b8bc6fe46a378330b1 100644 (file)
@@ -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"
index d3802441317f21f5be00ccc194d16c2c7338037f..0e857ad15c22101d0d93e4b497479f3e14a6055d 100644 (file)
@@ -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 (file)
index 0000000..1b07470
--- /dev/null
@@ -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
index a71bf8ac17cc89b19e98d91a1c594ee254d0378e..a00e6cd6f2dc5d8199708e2559cc5a55284bf5cc 100644 (file)
@@ -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
index c4ed1335ea4fb243d6e40cbccd3e1bbad204837f..34adfd500d2e97aed47a5b2a80cf17cf853a6ae5 100644 (file)
@@ -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
index ea7527e940fcdfd6e8d497d906d88bda39f1abf0..3a0b0635b0f6ce1e2187d7a7d6837943bc248e78 100644 (file)
@@ -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:
index 30559a5094a2226a6f6fcb671ae451cba660a59d..4f99dd2fcc834404643631954fb248d5cb98d81d 100644 (file)
@@ -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
index ac337760e2034c46fdc4a90879c81d0d4229aa38..faeaae00a488b9d823b9884b07b0ac28803362c1 100644 (file)
@@ -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
index 97977a5d56be7ba87bd8c2c93938b41ec36902b2..8c40d3aa476b3327a3abf5235e6982ceaa5a1df0 100644 (file)
@@ -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')
index c177bc3901cdc74db85d448b32222eb586bedcb1..c7f9776ac6a98c36f8ab8a3e002773c03603ea6a 100644 (file)
@@ -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")