* Refactored to remove load_kind_params filter and instead override load_where_param...
authorPeter Amstutz <peter.amstutz@curoverse.com>
Mon, 14 Apr 2014 13:31:42 +0000 (09:31 -0400)
committerPeter Amstutz <peter.amstutz@curoverse.com>
Mon, 14 Apr 2014 13:31:42 +0000 (09:31 -0400)
* Removed 'rescue' clause.  I think I left that in from debugging.
* Fixed :log -> :logs
* filters: ['tail_kind','=','arvados#user'] works
* Added more tests.
* Added equivalent logic and tests for logs controller for object_kind

services/api/app/controllers/application_controller.rb
services/api/app/controllers/arvados/v1/links_controller.rb
services/api/app/controllers/arvados/v1/logs_controller.rb
services/api/app/models/arvados_model.rb
services/api/app/models/link.rb
services/api/app/models/log.rb
services/api/db/migrate/20140325175653_remove_kind_columns.rb
services/api/test/fixtures/logs.yml [new file with mode: 0644]
services/api/test/functional/arvados/v1/links_controller_test.rb
services/api/test/functional/arvados/v1/logs_controller_test.rb

index 9aad19507583c2e426a9a8070bbacbd3fbdc6c15..0039bb028812452c3fa9fb8a8eee53c963a39237 100644 (file)
@@ -118,11 +118,12 @@ class ApplicationController < ActionController::Base
   end
 
   def load_filters_param
+    @filters ||= []
     if params[:filters].is_a? Array
-      @filters = params[:filters]
+      @filters += params[:filters]
     elsif params[:filters].is_a? String and !params[:filters].empty?
       begin
-        @filters = Oj.load params[:filters]
+        @filters += Oj.load params[:filters]
         raise unless @filters.is_a? Array
       rescue
         raise ArgumentError.new("Could not parse \"filters\" param as an array")
index 1b79295af73524c607149d3aa849b9d2be4e9622..1b5bf78dbeb61821411e6cb754d6d977825bad58 100644 (file)
@@ -1,33 +1,49 @@
 class Arvados::V1::LinksController < ApplicationController
 
-  prepend_before_filter :load_kind_params, :only => :index
-
   def create
     resource_attrs.delete :head_kind
     resource_attrs.delete :tail_kind
     super
   end
 
-  def load_kind_params
-    if params[:tail_uuid]
-      params[:where] = Oj.load(params[:where]) if params[:where].is_a?(String)
-      @where ||= {}
-      @where[:tail_uuid] = params[:tail_uuid]
-    end
+  protected
 
-    if params[:where] and params[:where].is_a? Hash
-      if params[:where][:head_kind]
-        params[:filters] ||= []
-        params[:filters] << ['head_uuid', 'is_a', params[:where][:head_kind]]
-        params[:where].delete :head_kind
+  # Overrides ApplicationController load_where_param
+  def load_where_param
+    super
+
+    # head_kind and tail_kind columns are now virtual,
+    # equivilent functionality is now provided by
+    # 'is_a', so fix up any old-style 'where' clauses.
+    if @where
+      @filters ||= []
+      if @where[:head_kind]
+        @filters << ['head_uuid', 'is_a', @where[:head_kind]]
+        @where.delete :head_kind
       end
-      if params[:where][:tail_kind]
-        params[:filters] ||= []
-        params[:filters] << ['tail_uuid', 'is_a', params[:where][:tail_kind]]
-        params[:where].delete :tail_kind
+      if @where[:tail_kind]
+        @filters << ['tail_uuid', 'is_a', @where[:tail_kind]]
+        @where.delete :tail_kind
       end
     end
+  end
 
+  # Overrides ApplicationController load_filters_param
+  def load_filters_param
+    super
+
+    # head_kind and tail_kind columns are now virtual,
+    # equivilent functionality is now provided by
+    # 'is_a', so fix up any old-style 'filter' clauses.
+    @filters = @filters.map do |k|
+      if k[0] == 'head_kind' and k[1] == '='
+        ['head_uuid', 'is_a', k[2]]
+      elsif k[0] == 'tail_kind' and k[1] == '='
+        ['tail_uuid', 'is_a', k[2]]
+      else
+        k
+      end
+    end
   end
 
 end
index dffe662e7f1819c762eaca1a42bd408584f5e0bb..925eee523ed616adc69d7a0cb3354d536ef9cf23 100644 (file)
@@ -1,2 +1,34 @@
 class Arvados::V1::LogsController < ApplicationController
+  # Overrides ApplicationController load_where_param
+  def load_where_param
+    super
+
+    # object_kind and column is now virtual,
+    # equivilent functionality is now provided by
+    # 'is_a', so fix up any old-style 'where' clauses.
+    if @where
+      @filters ||= []
+      if @where[:object_kind]
+        @filters << ['object_uuid', 'is_a', @where[:object_kind]]
+        @where.delete :object_kind
+      end
+    end
+  end
+
+  # Overrides ApplicationController load_filters_param
+  def load_filters_param
+    super
+
+    # object_kind and column is now virtual,
+    # equivilent functionality is now provided by
+    # 'is_a', so fix up any old-style 'filter' clauses.
+    @filters = @filters.map do |k|
+      if k[0] == 'object_kind' and k[1] == '='
+        ['object_uuid', 'is_a', k[2]]
+      else
+        k
+      end
+    end
+  end
+
 end
index 08bb5ea96d06d5b5de4c84b4ac9c1400e189b4d2..dbb807c0b6b4005193babfef0a93481f1f75ef56 100644 (file)
@@ -222,18 +222,13 @@ class ArvadosModel < ActiveRecord::Base
     specials = [system_user_uuid, 'd41d8cd98f00b204e9800998ecf8427e+0']
 
     foreign_key_attributes.each do |attr|
-      begin
-        if new_record? or send (attr + "_changed?")
-          attr_value = send attr
-          r = ArvadosModel::resource_class_for_uuid attr_value if attr_value
-          r = r.readable_by(current_user) if r and not skip_uuid_read_permission_check.include? attr
-          if r and r.where(uuid: attr_value).count == 0 and not specials.include? attr_value
-            errors.add(attr, "'#{attr_value}' not found")
-          end
+      if new_record? or send (attr + "_changed?")
+        attr_value = send attr
+        r = ArvadosModel::resource_class_for_uuid attr_value if attr_value
+        r = r.readable_by(current_user) if r and not skip_uuid_read_permission_check.include? attr
+        if r and r.where(uuid: attr_value).count == 0 and not specials.include? attr_value
+          errors.add(attr, "'#{attr_value}' not found")
         end
-      rescue Exception => e
-        bt = e.backtrace.join("\n")
-        errors.add(attr, "'#{attr_value}' error '#{e}'\n#{bt}\n")
       end
     end
   end
index cf4ffce8ed8b1e0a7894742a7c9993955a46ebce..26e7183be385dba38da43b85eec02d6649311c6d 100644 (file)
@@ -8,6 +8,7 @@ class Link < ArvadosModel
   after_update :maybe_invalidate_permissions_cache
   after_create :maybe_invalidate_permissions_cache
   after_destroy :maybe_invalidate_permissions_cache
+  attr_accessor :head_kind, :tail_kind
 
   api_accessible :user, extend: :common do |t|
     t.add :tail_uuid
index 923a681b0d3a03dc55f671b82063abc286a2601a..f8e337b201018a7f2638fa551e1b9304e6334211 100644 (file)
@@ -4,17 +4,24 @@ class Log < ArvadosModel
   include CommonApiTemplate
   serialize :properties, Hash
   before_validation :set_default_event_at
-  attr_accessor :object
+  attr_accessor :object, :object_kind
 
   api_accessible :user, extend: :common do |t|
     t.add :object_uuid
     t.add :object, :if => :object
+    t.add :object_kind
     t.add :event_at
     t.add :event_type
     t.add :summary
     t.add :properties
   end
 
+  def object_kind
+    if k = ArvadosModel::resource_class_for_uuid(object_uuid)
+      k.kind
+    end
+  end
+
   def fill_object(thing)
     self.object_uuid ||= thing.uuid
     self.summary ||= "#{self.event_type} of #{thing.uuid}"
index d77130d5c2cf3ae477bc729a7fb9ff15de045a2e..eae2a2c87e40141ffac327e6c79c32950e48837b 100644 (file)
@@ -4,13 +4,13 @@ class RemoveKindColumns < ActiveRecord::Migration
   def up
     remove_column :links, :head_kind
     remove_column :links, :tail_kind
-    remove_column :log, :object_kind
+    remove_column :logs, :object_kind
   end
 
   def down
     add_column :links, :head_kind, :string
     add_column :links, :tail_kind, :string
-    add_column :log, :object_kind, :string
+    add_column :logs, :object_kind, :string
 
     act_as_system_user do
       Link.all.each do |l|
diff --git a/services/api/test/fixtures/logs.yml b/services/api/test/fixtures/logs.yml
new file mode 100644 (file)
index 0000000..d805439
--- /dev/null
@@ -0,0 +1,3 @@
+log1:
+  uuid: zzzzz-xxxxx-pshmckwoma9plh7
+  object_uuid: zzzzz-tpzed-l1s2piq4t4mps8r
\ No newline at end of file
index 4c0c8dc825fc41907d3e31fccb0952777a98c56c..09dd1621d681fbecc6480b78e369a61e57241065 100644 (file)
@@ -44,7 +44,7 @@ class Arvados::V1::LinksControllerTest < ActionController::TestCase
     assert_response 422
   end
 
-  test "head and tail exist" do
+  test "head and tail exist, head_kind and tail_kind are returned" do
     link = {
       link_class: 'test',
       name: 'stuff',
@@ -153,5 +153,27 @@ class Arvados::V1::LinksControllerTest < ActionController::TestCase
     assert_equal found.count, (found.select { |f| f.head_uuid.match /[a-z0-9]{5}-tpzed-[a-z0-9]{15}/}).count
   end
 
+  test "test can still use filter tail_kind" do
+    authorize_with :admin
+    get :index, {
+      filters: [ ['tail_kind', '=', 'arvados#user'] ]
+    }
+    assert_response :success
+    found = assigns(:objects)
+    assert_not_equal 0, found.count
+    assert_equal found.count, (found.select { |f| f.tail_uuid.match /[a-z0-9]{5}-tpzed-[a-z0-9]{15}/}).count
+  end
+
+  test "test can still use filter head_kind" do
+    authorize_with :admin
+    get :index, {
+      filters: [ ['head_kind', '=', 'arvados#user'] ]
+    }
+    assert_response :success
+    found = assigns(:objects)
+    assert_not_equal 0, found.count
+    assert_equal found.count, (found.select { |f| f.head_uuid.match /[a-z0-9]{5}-tpzed-[a-z0-9]{15}/}).count
+  end
+
 
 end
index 9c410996b84509861f85f6fd8fcc1c9557cb5b4a..a224e2573f4a30ccd49eb7c098f9acf186a42c18 100644 (file)
@@ -1,6 +1,8 @@
 require 'test_helper'
 
 class Arvados::V1::LogsControllerTest < ActionController::TestCase
+  fixtures :logs
+
   test "non-admins can read their own logs" do
     authorize_with :active
     post :create, log: {summary: "test log"}
@@ -12,4 +14,29 @@ class Arvados::V1::LogsControllerTest < ActionController::TestCase
     assert_equal("test log", assigns(:object).summary,
                  "loaded wrong log after creation")
   end
+
+  test "test can still use where object_kind" do
+    authorize_with :admin
+    get :index, {
+      where: { object_kind: 'arvados#user' }
+    }
+    assert_response :success
+    found = assigns(:objects)
+    assert_not_equal 0, found.count
+    assert_equal found.count, (found.select { |f| f.object_uuid.match /[a-z0-9]{5}-tpzed-[a-z0-9]{15}/}).count
+    l = JSON.parse(@response.body)
+    assert_equal 'arvados#user', l['items'][0]['object_kind']
+  end
+
+  test "test can still use filter object_kind" do
+    authorize_with :admin
+    get :index, {
+      filters: [ ['object_kind', '=', 'arvados#user'] ]
+    }
+    assert_response :success
+    found = assigns(:objects)
+    assert_not_equal 0, found.count
+    assert_equal found.count, (found.select { |f| f.object_uuid.match /[a-z0-9]{5}-tpzed-[a-z0-9]{15}/}).count
+  end
+
 end